[Bug 527488] Review Request: drbd - drbd tools

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=527488





--- Comment #50 from LINBIT <partner at linbit.com>  2009-10-19 03:50:49 EDT ---
Spec file and SRPM:

http://people.linbit.com/~florian/drbd.spec
http://people.linbit.com/~florian/drbd-8.3.4-10.src.rpm

(In reply to comment #48)
> Well,
> 
> * %description
>   - I don't think writing the authors of this software in
>     %description is needed.

Gone.

>   - And the description "Please report bugs to drbd-dev at lists.linbit.com"
>     is in my opinion wrong (because we have our BTS)

Gone.

> * BuildRequires
>   - BR: gcc is redundant:
>     https://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2

I realize that; however I still prefer to list the full build dependencies. The
packaging guidelines state that the packages are considered "the minimum build
environment", but if a clueless user happens to run rpmbuild without gcc
installed, then I consider it proper to fail with an unsatisfied build
dependency, rather than with a relatively obscure error from configure, midway
during %prep.

> * Dependency between subpackages
>   - Usually the dependency between packages rebuilt from a srpm should
>     be strict EVR (not just version) specific
>     (i.e. usually the dependency should be like:
>       Requires: %{name}-utils = %{version}-%{release} )
>     https://fedoraproject.org/wiki/Packaging/Guidelines#Requiring_Base_Package

Fixed.

> * Parallel make
>   - Use %{?_smp_mflags} instead of %{_smp_mflags} because %{_smp_mflags} macro
>     may not be defined.

Fixed.

> * %bcond_without
>   - If you want to really use %bcond_without/%bcond_with, please support
>     all possible patterns carefully, or just drop to use %bcond_ method.
>     ! For example currently passing "--without utils" to your srpm fails
>       because all other subpackages depend on -utils subpackage but
>       with "--without utils" -utils subpackage won't be rebuilt.

I had considered it convenient to still be able to provide a shortcut for
rolling, for example, a drbd-udev package without having to compile utils.
Which would absolutely be possible as far as the configure/make/make install
procedure is concerned. Apparently this convenience is not needed, thus, the
"--with utils" conditional is gone.

> * %defattr
>   - Now we recommend to use %defattr(-,root,root,-)

Fixed.

> * About -rgmanager subpackage
>   - Is -rgmanager part really needed? From the description in the spec file
>     currently this subpackage can be rebuilt only for F-10, which is about to
>     be EOL and on F-11/12/13 this cannot be supported.

Addressed by Fabio -- see comment #49.

> * License tag
>   - scripts/drbd.ocf is under GPLv2 and installed as
>     /usr/lib/ocf/resource.d/linbit/drbd, -pacemaker subpackage should have
>     "GPLv2 and GPLv2+" license tag (or just GPLv2)

Good catch. Both the Linux-HA and Pacemaker projects seem, for the time being,
to be GPLv2 exclusive, so I fixed the License tag for both drbd-pacemaker and
drbd-heartbeat.

> * sysvinit script treatment
>   - Some Requires(post) or so are missing.

Fixed.

>   - Would you explain why condrestart is not needed at %postun (on upgrade)?

The drbd init script merely configures DRBD resources listed in drbd.conf. It
does not start or stop any daemon or similar. The only situation in which a
restart would be needed during a package upgrade would be if

a) new features (and hence, new drbd.conf keywords) would be introduced in the
new release, AND
b) a resource listed in the drbd.conf configuration file would be changed to
actually use one of those new features.

Since b) is impossible as the configuration file is listed as
%config(noreplace) and is hence not touched upon upgrade, a condrestart is not
required on upgrade.

> https://fedoraproject.org/wiki/Packaging/SysVInitScript#InitscriptScriptlets
> 
>   - Please use %{_initddir}

Fixed. I now use %{_initddir} and if that is not defined, I fall back to
%{_initrddir} -- the spec has to build on CentOS 5, which does not have
%{_initddir} defined.

> https://fedoraproject.org/wiki/Packaging/SysVInitScript#Initscripts_on_the_filesystem
> 
> * Duplicate files
>   - You don't have to include COPYING file as %doc other than -utils
>     subpackage because all other packages depends on -utils subpackage.  

Now I have two conflicting reviewer comments. Fabio (in comment #17) tells me
that all subpackages should contain the COPYING file. Mamoru (in comment #48)
tells me they shouldn't.
http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text is
inconclusive and doesn't mention sub-packages specifically. What should I do?

Build logs:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1753544 (f12)
http://koji.fedoraproject.org/koji/taskinfo?taskID=1753549 (f13)

Changelog:
093e841... drbd.spec.in: remove reference to authors and bug reports
38114ad... drbd.spec.in: fix dependencies to include release number
546e733... drbd.spec.in: do not require defined %{_smp_mflags} macro
9de2c41... drbd.spec.in: fix %defattr to include directory mode
29f1771... drbd.spec.in: remove --with utils
e40e9ee... drbd.spec.in: change License tag for drbd-heartbeat and
drbd-pacemaker
8ad0220... configure.ac, drbd.spec.in: add --with-initdir
20671ee... drbd.spec.in: add missing Requires tags
600c73b... RPM spec files: bump release number

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.