[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 #20 from LINBIT <partner at linbit.com>  2009-10-14 08:21:09 EDT ---
Updated spec and source RPM are here:

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

(In reply to comment #17)

Just replying to the NOK items here, and those where Fabio saw room for
improvement.

>   * Licence tag needs fixing (as reported by rpmlint too)

Fixed to GPLv2+.

>   * It contains unnecessary BuildRequires

Udev is a valid build dependency (we check the udev version number via udevadm
or udevinfo, depending on what's available, and install different udev rules).
So is flex. Should we just remove gcc?

>   * Vendor tag should not be used

Dropped.

>   * Source tag should contain full URL to the source

Fixed.

>   * Requiring Base Package should use a fully versioned dependency

Fixed.

> - The License field in the package spec file must match the actual license:
> NOK. Source contains GPL2 and spec file GPL.

Fixed (see above).

> - If (and only if) the source package includes the text of the license(s) in
> its own file, then that file, containing the text of the license(s) for the
> package must be included in %doc: NOK: all subpackages should contain the
> COPYING file too.

Fixed.

> - The sources used to build the package must match the upstream source, as
> provided in the spec URL: NOK, cannot verify. URL doesn't match.

Full URL is now in the spec. Non-matching upstream source is a chicken-and-egg
problem; we are currently redoing our build setup significantly to comply with
this very review process.

> - A package must own all directories that it creates...: %{_prefix}/lib/%{name}
> seems to be un-owned.

Fixed.

> SHOULD items:
> - If the source package does not include license text(s) as..: source contains
> COPYING file with licence. OK

Done.

> fully versioned dependency: NOK, already noted above.

Done.

> Notes:
> 
> - drdb placeholder package is uninstallable if the default feature set is
> reduced.

Done, Requires tags for the meta package are now generated according to --with
options.

> - BuildRoot entry, while valid, is not in the preferred format. Packaging guide
> lines recommend: %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)

Done.

> - macros need to be used more consistently. (/etc/rc.d/init.d -> %{_initrddir})

Fixed. The configure script also autodecects a correct init script based on the
build platform, as %{_initrddir} is RPM specific and does not have an autoconf
equivalent.

> - %build doesn't respect smpflags

Done for now, requires a bit more testing on our part though.

> - udev rule is installed 755 ? 644 should be enough.

Done.

> - scriptlets:
>   %post utils package performs mknod operations. When udev is available, is
> that operation required at all?

Now dependent on --without udev.

>   %preun utils attempts to invoke rcdrbd stop. I don't think that's required in
> Fedora and a safer way to write the %preun is:
> if [ "$1" = 0 ]; then
>         /sbin/service foo stop >/dev/null 2>&1
>         /sbin/chkconfig --del foo
> fi

Done, except that I chose "%{_initrddir}/%{name} stop" which is more distro
agnostic.

> About the general packaing:
> 
> - most of the packages could be architecture generic. drdb placeholder, the
> several scripts integration stuff too.

I've added a provision for this, but the noarch sub-package build appears to be
broken for me right now. Will post koji build logs in a separate comment.

> - the -rgmanager variant conflicts with resource-agents. We will discuss this
> specific detail between upstreams tho. It's only partially relevant for this
> review as the package itself is and we will address it in cooperation with
> cluster/resource-agents people.

Building drbd-rgmanager is now disabled by default. When a user chooses to
enable this while re-packaging, it generates a conflict against rgmanager >=
3.0.1 (which was the rgmanager release during which the drbd agent got merged).
I repeat, that Conflict tag does not apply when built with a default
configuration.

This is the abbreviated changelog since the last update (most recent commit
first):

2de29b9... drbd.spec.in: bump release number
0247344... drbd.spec.in: remove Vendor tag
6e75748... drbd.spec.in: remove references to old kernel module package names
9ac1858... drbd.spec.in: move rmmod invocation to %preun km where it belongs
e53f1ae... drbd.spec.in: sanitize %preun for utils package
8b9d026... configure.ac, drbd.spec.in: conflict with rgmanager >= 3.0.1
472f59a... drbd.spec.in: simplify %files section in utils package
a049193... configure.ac, drbd.spec.in: allow RPM sub-packages to be configured
with BuildArch: noarch
cf704e9... configure.ac, drbd.spec.in: ditch file.list, add INITSCRIPT_SYMLINK
macro
e9177d3... drbd.spec.in: drbd-utils package must own /usr/lib/drbd
f7ab46e... drbd.spec.in: make utils %post mknod invocation depend on --without
udev
3a208ae... drbd.spec.in: fix %defattr on udev rules
38086fc... drbd.spec.in: add SMP flags on make in %build
0b3028a... drbd.spec.in: fix defattr on /var/lib/drbd
898bb37... drbd.spec.in: fix Requires for conditional builds
922111d... drbd.spec.in: drbd-km should depend on drbd-utils, not drbd
6583895... drbd.spec.in: Add explicit version dependency on drbd-utils for
integration sub-packages
9e5e911... drbd.spec.in: Include full URL in Source tag
8595e41... drbd.spec.in: include COPYING file in all subpackages
2eeb5df... drbd.spec.in: change License tag to GPLv2+
5521c28... drbd.spec.in: change BuildRoot to preferred format

-- 
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.