Bug 501130 - Review Request: drbdlinks - A program for managing links into a DRBD shared partition
Review Request: drbdlinks - A program for managing links into a DRBD shared p...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Jochen Schmitt
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-05-16 16:17 EDT by Robert Scheck
Modified: 2009-05-25 17:19 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-05-23 21:18:18 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
jochen: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Robert Scheck 2009-05-16 16:17:30 EDT
Spec URL: http://labs.linuxnetz.de/bugzilla/drbdlinks.spec
SRPM URL: http://labs.linuxnetz.de/bugzilla/drbdlinks-1.15-1.src.rpm
Description:
The drbdlinks program manages links into a DRBD partition which is shared
among several machines. A simple configuration file, "/etc/drbdlinks.conf",
specifies the links. This can be used to manage e.g. links for /etc/httpd,
/var/lib/pgsql and other system directories that need to appear as if they
are local to the system when running applications after the drbd shared
partition has been mounted.

When running drbdlinks with "start" as the mode, drbdlinks will rename the
existing files/directories and then make symbolic links into the DRBD
partition, "stop" does the reverse. By default, rename appends ".drbdlinks"
to the name, but this can be overridden.

An init script is included which runs "stop" before heartbeat starts, and
after heartbeat stops. This is done to try to ensure that when the shared
partition isn't mounted, the links are in their normal state.


The drbdlinks upstream package ships a GPLv2 file, but nowhere a licensing is
mentioned. According to what I know, Tom is treating such a behaviour normally
as GPL+ in the spec file.
Comment 1 Robert Scheck 2009-05-16 20:51:57 EDT
Upstream clarified licensing to GPLv2-only with the 1.16 release now:

Spec URL: http://labs.linuxnetz.de/bugzilla/drbdlinks.spec
SRPM URL: http://labs.linuxnetz.de/bugzilla/drbdlinks-1.16-1.src.rpm
Comment 2 Jochen Schmitt 2009-05-17 16:07:26 EDT
Good:
+ Basename of the SPEC file matches with package name.
+ Name of the package fullfill naming guildines.
+ Package has proper RPM-group
+ URL tag show on proper project homepage
+ Package contains most recent release of the software
+ Could download upstream tar ball via spectool -g
+ Package tar ball matches with upstream
(md5sum: c11b38dfa60d8d07b55746a957b48b19 )
* Package contains valid License tag
+ License tag state GPLv2 as an valid OSS license
+ Header of the source file has GPLv2 license note
+ Package has verbatin copy of the license text
+ Package has proper defintion of the Buildroot
+ Buildroot will be cleaned at the beginning of %clean and %install
+ %doc stanza is small, so we need no extra doc subpackage
+ Package contains proper Changelog
+ Local build works fine
+ Rpmlint is quiete on source rpm
+ Koji build works fine.

Bad:
- Initscripts should be install on %{_initrddir}
  (Unproper usage of rpm macros)
- don't put a file on %{_localstatedir}/run. 
  you should only create a directory on it
- Who should owned %{_sysconfdir}/ha.d ?
- Rpmlint complaints on binary rpm:
$ rpmlint drbdlinks-1.16-1.fc10.noarch.rpm
drbdlinks.noarch: W: missing-lsb-keyword Default-Stop in /etc/rc.d/init.d/drbdlinksclean
drbdlinks.noarch: E: subsys-not-used /etc/rc.d/init.d/drbdlinksclean
drbdlinks.noarch: W: incoherent-init-script-name drbdlinksclean
1 packages and 0 specfiles checked; 1 errors, 2 warnings.
Comment 3 Robert Scheck 2009-05-17 16:17:11 EDT
(In reply to comment #2)
> - Initscripts should be install on %{_initrddir}
>   (Unproper usage of rpm macros)

I want to package drbdlinks for all active branches and this is only possible
without macro usage currently, look for %{_initrddir} vs. %{_initdir} or so.

> - don't put a file on %{_localstatedir}/run. 
>   you should only create a directory on it

Upstream initscript doesn't do that as well. And drbdlinks isn't a daemon. It
also can be triggered by heartbeat. Thus /var/run doesn't make sense.

> - Who should owned %{_sysconfdir}/ha.d ?

Heartbeat owns that directory. But heartbeat is optional, drbdlinks can be
used with and without.

> drbdlinks.noarch: W: missing-lsb-keyword Default-Stop in
> /etc/rc.d/init.d/drbdlinksclean

If no Default-Start, no Default-Stop. The line has not to exist, if it is
not filled, IIRC.

> drbdlinks.noarch: E: subsys-not-used /etc/rc.d/init.d/drbdlinksclean
> drbdlinks.noarch: W: incoherent-init-script-name drbdlinksclean

As explained above.
Comment 4 Jochen Schmitt 2009-05-18 11:09:50 EDT
(In reply to comment #3)

> I want to package drbdlinks for all active branches and this is only possible
> without macro usage currently, look for %{_initrddir} vs. %{_initdir} or so.

I thought, all rpm macros defined by the packaging guildlines should be available on EPEL too.
 
> > - Who should owned %{_sysconfdir}/ha.d ?
> 
> Heartbeat owns that directory. But heartbeat is optional, drbdlinks can be
> used with and without.

Please ask maintainer to create a filesystem subpackage of heartbeat. This subpackage should contains all directories which are common for heartbeat and yours. Your package should depends on this package.
Comment 5 Robert Scheck 2009-05-18 11:16:25 EDT
(In reply to comment #4)
> Please ask maintainer to create a filesystem subpackage of heartbeat. This
> subpackage should contains all directories which are common for heartbeat and
> yours. Your package should depends on this package.  

Hum? IMHO that behaves same as for /etc/logrotate.d/* or /etc/cron.d/* files.
It's a dependency, that can be satisfied, but must not necessarily.
Comment 6 Jochen Schmitt 2009-05-18 11:59:33 EDT
(In reply to comment #5)

> Hum? IMHO that behaves same as for /etc/logrotate.d/* or /etc/cron.d/* files.
> It's a dependency, that can be satisfied, but must not necessarily.  

Don't tell me anything about old packages which we have perhaps since RH 7 or so. You should know, that this old packages didn't follow the current packaging guidelines. That is the reason why we have the so-called merge review.

For a new package I want, the each file/directory should owned by a package.
Comment 7 Jochen Schmitt 2009-05-18 12:56:52 EDT
(In reply to comment #2)

> - don't put a file on %{_localstatedir}/run. 
>   you should only create a directory on it

Please at a the line

%{_localstatedir}/run/%{name}/

on the files stanza, so that directory and all directory belongs will be 
owned by this package.
Comment 8 Robert Scheck 2009-05-18 13:48:18 EDT
Ah, I mis-got you there - sorry. It should be %{_localstatedir}/run/%{name}/
of course, my packaging mistake. Anything else except that and /etc/ha.d/-mess?

Regarding %{_initrddir} vs. %{_initdir}: %{_initrddir} lives on EL-4/5, while
%{_initdir} lives on F-10/F-11/Rawhide. Thus two different macros, but EPEL is
getting often ignored by packagers, so it doesn't matter for many. And if you
don't want to mess with macro stuff inside spec file or make individual changes
per branch, /etc/rc.d/init/ is now the only sane and really readable way inside
of a spec file.
Comment 9 Jochen Schmitt 2009-05-18 14:28:43 EDT
Regarding of https://fedoraproject.org/wiki/Packaging/RPMMacros only
%{_initrdddir} is valid, which shows to %{_sysconfdir}/rc.d/init.d/
Comment 10 Robert Scheck 2009-05-18 14:41:13 EDT
Well, /usr/lib/rpm/platform/i586-linux/macros from rpm-4.6+ shows following:

%_initddir              %{_sysconfdir}/rc.d/init.d
# Deprecated misspelling
%_initrddir             %{_initddir}
Comment 11 Jochen Schmitt 2009-05-18 15:15:41 EDT
That may be. If you want to change the packaging guildlines you may feell free to open a discussion of the fedora-packaging list.
Comment 13 Jochen Schmitt 2009-05-19 12:37:06 EDT
After a discusion on fedora-Packaging, We have dicide no to use _initrddir or initddir in the funtere.

In addition I have open BZ #501518 for creating a filesystem subpackage for heartbeate.

So, after we have clarified this two topics, I'm able to APPROVE your package.
Comment 14 Robert Scheck 2009-05-19 14:48:40 EDT
Does that now mean, that I can/should/must revert my %{_initrddir} usage?


New Package CVS Request
=======================
Package Name: drbdlinks
Short Description: A program for managing links into a DRBD shared partition
Owners: robert
Branches: EL-4 EL-5 F-10 F-11
InitialCC:
Comment 15 Jochen Schmitt 2009-05-19 15:01:25 EDT
You should convert to %{_sysconfdir}/rc.d/init.d in the futere.
Comment 16 Kevin Fenzi 2009-05-20 01:48:49 EDT
cvs done.
Comment 17 Robert Scheck 2009-05-23 21:18:18 EDT
Package: drbdlinks-1.18-1.fc12 Tag: dist-f12 Status: complete
Package: drbdlinks-1.18-1.fc11 Tag: dist-f11-updates-candidate Status: complete 
Package: drbdlinks-1.18-1.fc10 Tag: dist-f10-updates-candidate Status: complete

2372 (drbdlinks): Build on target fedora-5-epel succeeded.
2373 (drbdlinks): Build on target fedora-4-epel succeeded.
Comment 18 Fedora Update System 2009-05-23 21:21:00 EDT
drbdlinks-1.18-1.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/drbdlinks-1.18-1.fc11
Comment 19 Fedora Update System 2009-05-23 21:21:04 EDT
drbdlinks-1.17-1.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/drbdlinks-1.17-1.fc10
Comment 20 Fedora Update System 2009-05-25 17:14:43 EDT
drbdlinks-1.18-1.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 21 Fedora Update System 2009-05-25 17:19:34 EDT
drbdlinks-1.18-1.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

Note You need to log in before you can comment on or make changes to this bug.