Bug 527488 (drbd)

The DRBD source tarball is at http://oss.linbit.com/drbd/8.3/drbd-8.3.4.tar.gz.

Please also provide a link to the SRPM. But before doing so, you should revise the spec file in order to meet the Fedora packaging guidelines (see http://fedoraproject.org/wiki/Packaging:Guidelines). 

I also can't find you in the account system. If this is your first submission, you need a sponsor. To find one, add FE-NEEDSPONSOR to the Blocks field above. For further information see https://fedoraproject.org/wiki/PackageMaintainers/Join.

It seems, that DRBD is depending on specific kernel module, which I cannot find in the default Fedora kernel.

Unfortunately, if it's true, then this would be a blocker, since we don't allow packages with standalone kernel modules.

(In reply to comment #3)
> It seems, that DRBD is depending on specific kernel module, which I cannot find
> in the default Fedora kernel.
> 
> Unfortunately, if it's true, then this would be a blocker, since we don't allow
> packages with standalone kernel modules.  

there are no drdb kernel modules in fedora, but I think It can be built.

for example, we have user-mode-linux userland here, but no user-mode-linux modules enabled in fedora kernel

(In reply to comment #3)
> It seems, that DRBD is depending on specific kernel module, which I cannot find
> in the default Fedora kernel.
> 
> Unfortunately, if it's true, then this would be a blocker, since we don't allow
> packages with standalone kernel modules.  

DRDB kernel module is being included upstream. There is no need to build the kmod in fedora, and I really see no issue at all to make userland ready for inclusion.
Also other packages (as noted) that would require an external kernel module, are being shipped without it.

While there is absolutely no disagreement that external kmods are bad, there is also nothing wrong to allow users to have userland there ready for them, if they wish to build/install their kmod.

I explicitly asked to start the review process earlier because I did expect a lot of people to jump to the package throat :)

(In reply to comment #3)

Peter Lemenkov
is ok for you, ready to start review process ?



(In reply to comment #5)

for me is ok, I think you can review it, if you have sponsor power.

Philipp Reisner

is this your first package ?

do you have a fedora account ?


https://fedoraproject.org/wiki/PackageMaintainers/Join#Get_a_Fedora_Account

I don't have sponsor power, but I'll do my review too since it integrates with several other packages I maintain or co-maintain and I do have test environments for it.

A new spec file and an SRPM are now available from:

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

This reflects our latest drbd upstream changes to the spec, aimed at better alignment with the Fedora packaging guidelines. I will keep this updated as we make more changes to the build system.

A summary of changes since the spec was first posted is below, most recent first:

aa7609c... drbd.spec.in: fix %setup so it works correctly during rpmbuild --rebuild
2b03ec4... drbd.spec.in: trivial whitespace fixes
390a6d0... drbd.spec.in: make the "drbd" package depend on all the spun-off packages
698a4f1... drbd.spec.in: move %files sections around
468a18e... drbd.spec.in: introduce the drbd-utils package
13579b4... drbd.spec.in: remove the EXTRA_DRBD_KO hack from userland %post
cd79620... drbd.spec.in: unclutter spec, move all build/install stages to one place
01fd191... drbd.spec.in: remove Packager tag
5ff472e... drbd.spec.in: fix inconsistent whitespace
0823116... drbd.spec.in: move buildroot cleanup to %install
7992e71... drbd.spec.in: add -q flag to %setup
47fccfc... drbd.spec.in: package split

Please note: if you want to build DRBD from the SRPM, for a kernel other than the one whose headers are found in /lib/modules/`uname -r`/build, please invoke rpmbuild as follows:

rpmbuild --rebuild --define "kdir /lib/modules/<VERSION>/build" --define "kernelversion <VERSION>" <SRPM>

Yes, you do have to define both kdir and kernelversion. We are working on improving this. If however you _are_ building for the currently running kernel, and you have its headers installed, then you may omit these defines.

Those interested in the full commit history are invited to take a look at http://git.drbd.org/?p=drbd-8.3.git;a=history;f=drbd.spec.in

(In reply to comment #9)
ok, and about your fedora account, do you have one ?

what are your FAS username ?

(In reply to comment #7)
> Philipp Reisner
> 
> is this your first package ?
> 
> do you have a fedora account ?
> 
> 
> https://fedoraproject.org/wiki/PackageMaintainers/Join#Get_a_Fedora_Account  

Yes, this is the first package.

I just created my account. It is: reisner

I don't think if a generic rpm will work (one rpm for all distros)

I think you will need to create a fedora specific spec file.


---> look 

https://fedoraproject.org/wiki/Packaging:DistTag

https://fedoraproject.org/wiki/Packaging:Guidelines#.25global_preferred_over_.25define


https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag

https://fedoraproject.org/wiki/Packaging:RPMMacros




--->  also try to build your package with koji 

https://fedoraproject.org/wiki/PackageMaintainers/Join#Install_the_Client_Tools_.28Koji.29


--->  and join packager group in fedora accounts website.


ps: I am not a package reviewer/sponsor and I don't have power to anything, I am only sending some tips to speed up the process.

(In reply to comment #12)
> I don't think if a generic rpm will work (one rpm for all distros)
> 
> I think you will need to create a fedora specific spec file.

You opinion is noted. Please let us know which parts of the spec you find objectionable in that regard, and we will see if and how we can change things.

> https://fedoraproject.org/wiki/Packaging:DistTag

It states there that using the dist tag isn't mandatory; we wouldn't mind leaving it out. Objections?

> https://fedoraproject.org/wiki/Packaging:Guidelines#.25global_preferred_over_.25define

OK, point taken.

> https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag

Again, point taken. I'll add %{release}.

> https://fedoraproject.org/wiki/Packaging:RPMMacros

Can you be a bit more specific, i.e. outline how our spec violates this)

> --->  also try to build your package with koji 
> 
> https://fedoraproject.org/wiki/PackageMaintainers/Join#Install_the_Client_Tools_.28Koji.29

Will do once we get the spec in better shape.

Thanks!

> https://fedoraproject.org/wiki/Packaging:DistTag

>It states there that using the dist tag isn't mandatory; we wouldn't mind
>leaving it out. Objections?

yes,  you will have the same package for fedora 10, 11, 12 etc..., and without dist-tag your rpm will have the same name, when someone upgrades from f10 to f11 then your package won't get upgraded because it's have the same name, and using dist-tag you will have 

foo.fc10
foo.fc11  


foo.fc11 >  foo.fc10


> > https://fedoraproject.org/wiki/Packaging:RPMMacros
> 
> Can you be a bit more specific, i.e. outline how our spec violates this)
> 

for example

/usr/sbin/drbd-overview

becomes

%{_sbindir}/drbd-overview

A fresh update of the spec and SRPM are available here:

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

Note that this is a Fedora/RHEL/CentOS specific spec which we generate from a common template.

Here are the requested changes:

- Added %{?dist} to the Release tag (3b7550d)
- Replaced %define with %global where applicable (f9ac9b5)
- Included %{release} in BuildRoot (53a60a8)
- Used proper macros instead of hardcoded paths (96aa83f, 9259a3c)

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

0b063b1... drbd.spec.in: bump release number
9259a3c... drbd.spec.in: honor _datadir macro
8838f3b... drbd.spec.in: Create build dependencies from configure
828f15f... drbd.spec.in: Create conflicts from configure
36bc2c1... Move crm-fence-peer.sh to the drbd-pacemaker package
0c0e32d... drbd.spec.in: remove "make clean" from %build stage
919d4fb... documentation/Makefile.in: obey --with-<feature> options
f0795b9... drbd.spec.in: correct --with utils and --with km behavior
286aab8... drbd.spec.in: change %build for --with km
807427a... drbd.spec.in: move "make clean" after "configure"
023126c... drbd.spec.in: make the sanity check in %prep depend on "--with km"
3b7550d... configure.ac, drbd.spec.in: add @RPM_DIST_TAG@
e53e3e7... configure.ac: add --enable-spec flag
0cde8a5... drbd.spec.in: expect a sane %{buildroot} on clean
d0d79ac... drbd.spec.in: remove obsolete comment
bdc2abd... drbd.spec.in: remove clumsy way of finding installed init script
1e93fdf... drbd.spec.in: use DESTDIR, not PREFIX
edb960a... drbd.spec.in: clean up %build
6b79119... drbd.spec.in: add configure invocation
a3b74af... autoconf: add configure.ac, generate drbd.spec through configure
5e3b9e6... drbd.spec.in: introduce conditional builds
96aa83f... drbd.spec.in: replace hardcoded paths with RPM macros
f9ac9b5... drbd.spec.in: change %define to %global
53a60a8... drbd.spec.in: add %{release} to BuildRoot
aa7609c... drbd.spec.in: fix %setup so it works correctly during rpmbuild --rebuild

Yet another update after Fabio's extremely helpful comments:

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

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

ae4d483... drbd.spec.in: remove mkdir -p %{buildroot} during %build
931270d... drbd.spec.in: add defattr for %files section
20b1aa8... drbd.spec.in: bump release number
999bbfc... drbd.spec.in: add conditional build dependency on udev
a43b004... drbd.spec.in: move %configure invocation to %prep stage
789f4e6... configure.ac: set KVER and KDIR only with --with-km is set
89991a5... configure.ac, drbd.spec.in: change default to "--without km"

I have also started doing koji scratch builds for f12 and f13, see task logs here:

http://koji.fedoraproject.org/koji/taskinfo?taskID=1744253
http://koji.fedoraproject.org/koji/taskinfo?taskID=1744268

FYI, rpmlint still reports these errors:

drbd.spec:81: E: hardcoded-library-path in %{_prefix}/lib/%{name}/outdate-peer.sh
drbd.spec:82: E: hardcoded-library-path in %{_prefix}/lib/%{name}/snapshot-resync-target-lvm.sh
drbd.spec:83: E: hardcoded-library-path in %{_prefix}/lib/%{name}/unsnapshot-resync-target-lvm.sh
drbd.spec:84: E: hardcoded-library-path in %{_prefix}/lib/%{name}/notify-out-of-sync.sh
drbd.spec:85: E: hardcoded-library-path in %{_prefix}/lib/%{name}/notify-split-brain.sh
drbd.spec:86: E: hardcoded-library-path in %{_prefix}/lib/%{name}/notify-emergency-reboot.sh
drbd.spec:87: E: hardcoded-library-path in %{_prefix}/lib/%{name}/notify-emergency-shutdown.sh
drbd.spec:88: E: hardcoded-library-path in %{_prefix}/lib/%{name}/notify-io-error.sh
drbd.spec:89: E: hardcoded-library-path in %{_prefix}/lib/%{name}/notify-pri-lost-after-sb.sh
drbd.spec:90: E: hardcoded-library-path in %{_prefix}/lib/%{name}/notify-pri-lost.sh
drbd.spec:91: E: hardcoded-library-path in %{_prefix}/lib/%{name}/notify-pri-on-incon-degr.sh
drbd.spec:92: E: hardcoded-library-path in %{_prefix}/lib/%{name}/notify.sh
drbd.spec:183: E: hardcoded-library-path in %{_prefix}/lib/%{name}/crm-fence-peer.sh
drbd.spec:184: E: hardcoded-library-path in %{_prefix}/lib/%{name}/crm-unfence-peer.sh

I request an exception for these on the grounds that all of these are simply integration shell scripts that are exactly identical on 32 and 64 bit platforms, and it would make positively no sense to install these to either /usr/lib or /usr/lib64; that would just royally confuse users.

drbd.spec:185: E: hardcoded-library-path in %{_prefix}/lib/ocf/resource.d/linbit/drbd

I request an exception for this on the grounds that the %{_prefix}/lib/ocf/resource.d/<provider>/<agent> path is prescribed by the Open Cluster Framework (OCF) standard.

[fabbione@daitarn-fedora rpmbuild]$ rpmlint SPECS/drbd.spec SRPMS/drbd-8.3.4-5.fc12.src.rpm RPMS/*/*
SPECS/drbd.spec:82: E: hardcoded-library-path in %{_prefix}/lib/%{name}/outdate-peer.sh
SPECS/drbd.spec:83: E: hardcoded-library-path in %{_prefix}/lib/%{name}/snapshot-resync-target-lvm.sh
SPECS/drbd.spec:84: E: hardcoded-library-path in %{_prefix}/lib/%{name}/unsnapshot-resync-target-lvm.sh
SPECS/drbd.spec:85: E: hardcoded-library-path in %{_prefix}/lib/%{name}/notify-out-of-sync.sh
SPECS/drbd.spec:86: E: hardcoded-library-path in %{_prefix}/lib/%{name}/notify-split-brain.sh
SPECS/drbd.spec:87: E: hardcoded-library-path in %{_prefix}/lib/%{name}/notify-emergency-reboot.sh
SPECS/drbd.spec:88: E: hardcoded-library-path in %{_prefix}/lib/%{name}/notify-emergency-shutdown.sh
SPECS/drbd.spec:89: E: hardcoded-library-path in %{_prefix}/lib/%{name}/notify-io-error.sh
SPECS/drbd.spec:90: E: hardcoded-library-path in %{_prefix}/lib/%{name}/notify-pri-lost-after-sb.sh
SPECS/drbd.spec:91: E: hardcoded-library-path in %{_prefix}/lib/%{name}/notify-pri-lost.sh
SPECS/drbd.spec:92: E: hardcoded-library-path in %{_prefix}/lib/%{name}/notify-pri-on-incon-degr.sh
SPECS/drbd.spec:93: E: hardcoded-library-path in %{_prefix}/lib/%{name}/notify.sh
SPECS/drbd.spec:181: E: hardcoded-library-path in %{_prefix}/lib/%{name}/crm-fence-peer.sh
SPECS/drbd.spec:182: E: hardcoded-library-path in %{_prefix}/lib/%{name}/crm-unfence-peer.sh
SPECS/drbd.spec:183: E: hardcoded-library-path in %{_prefix}/lib/ocf/resource.d/linbit/drbd
drbd.src: W: name-repeated-in-summary DRBD
drbd.src: W: no-version-in-last-changelog
drbd.src: W: invalid-license GPL
drbd.src: W: strange-permission drbd.spec 0600
drbd.src:82: E: hardcoded-library-path in %{_prefix}/lib/%{name}/outdate-peer.sh
drbd.src:83: E: hardcoded-library-path in %{_prefix}/lib/%{name}/snapshot-resync-target-lvm.sh
drbd.src:84: E: hardcoded-library-path in %{_prefix}/lib/%{name}/unsnapshot-resync-target-lvm.sh
drbd.src:85: E: hardcoded-library-path in %{_prefix}/lib/%{name}/notify-out-of-sync.sh
drbd.src:86: E: hardcoded-library-path in %{_prefix}/lib/%{name}/notify-split-brain.sh
drbd.src:87: E: hardcoded-library-path in %{_prefix}/lib/%{name}/notify-emergency-reboot.sh
drbd.src:88: E: hardcoded-library-path in %{_prefix}/lib/%{name}/notify-emergency-shutdown.sh
drbd.src:89: E: hardcoded-library-path in %{_prefix}/lib/%{name}/notify-io-error.sh
drbd.src:90: E: hardcoded-library-path in %{_prefix}/lib/%{name}/notify-pri-lost-after-sb.sh
drbd.src:91: E: hardcoded-library-path in %{_prefix}/lib/%{name}/notify-pri-lost.sh
drbd.src:92: E: hardcoded-library-path in %{_prefix}/lib/%{name}/notify-pri-on-incon-degr.sh
drbd.src:93: E: hardcoded-library-path in %{_prefix}/lib/%{name}/notify.sh
drbd.src:181: E: hardcoded-library-path in %{_prefix}/lib/%{name}/crm-fence-peer.sh
drbd.src:182: E: hardcoded-library-path in %{_prefix}/lib/%{name}/crm-unfence-peer.sh
drbd.src:183: E: hardcoded-library-path in %{_prefix}/lib/ocf/resource.d/linbit/drbd
drbd.x86_64: W: name-repeated-in-summary DRBD
drbd.x86_64: W: no-version-in-last-changelog
drbd.x86_64: W: invalid-license GPL
drbd.x86_64: E: no-binary
drbd-bash-completion.x86_64: W: no-version-in-last-changelog
drbd-bash-completion.x86_64: W: invalid-license GPL
drbd-bash-completion.x86_64: W: no-documentation
drbd-bash-completion.x86_64: W: non-conffile-in-etc /etc/bash_completion.d/drbdadm
drbd-debuginfo.x86_64: W: no-version-in-last-changelog
drbd-debuginfo.x86_64: W: invalid-license GPL
drbd-heartbeat.x86_64: W: no-version-in-last-changelog
drbd-heartbeat.x86_64: W: invalid-license GPL
drbd-heartbeat.x86_64: W: spurious-executable-perm /usr/share/man/man8/drbddisk.8.gz
drbd-pacemaker.x86_64: W: no-version-in-last-changelog
drbd-pacemaker.x86_64: W: invalid-license GPL
drbd-pacemaker.x86_64: W: only-non-binary-in-usr-lib
drbd-pacemaker.x86_64: W: no-documentation
drbd-rgmanager.x86_64: W: no-version-in-last-changelog
drbd-rgmanager.x86_64: W: invalid-license GPL
drbd-rgmanager.x86_64: W: no-documentation
drbd-udev.x86_64: W: summary-not-capitalized udev integration scripts for DRBD
drbd-udev.x86_64: W: no-version-in-last-changelog
drbd-udev.x86_64: W: invalid-license GPL
drbd-udev.x86_64: W: no-documentation
drbd-udev.x86_64: E: script-without-shebang /etc/udev/rules.d/65-drbd.rules
drbd-utils.x86_64: W: no-version-in-last-changelog
drbd-utils.x86_64: W: invalid-license GPL
drbd-utils.x86_64: W: unstripped-binary-or-object /sbin/drbdsetup
drbd-utils.x86_64: W: unstripped-binary-or-object /sbin/drbdmeta
drbd-utils.x86_64: W: only-non-binary-in-usr-lib
drbd-utils.x86_64: E: non-standard-dir-perm /var/lib/drbd 0644
drbd-utils.x86_64: W: service-default-enabled /etc/rc.d/init.d/drbd
drbd-utils.x86_64: W: service-default-enabled /etc/rc.d/init.d/drbd
drbd-utils.x86_64: W: incoherent-init-script-name drbd ('drbd-utils', 'drbd-utilsd')
drbd-xen.x86_64: W: no-version-in-last-changelog
drbd-xen.x86_64: W: invalid-license GPL
drbd-xen.x86_64: W: no-documentation
10 packages and 1 specfiles checked; 33 errors, 38 warnings.

Most of those errors/warnings need to be fixed.

Regarding the hardcoded-library-path I understand the issue and it's ok to have exceptions. This is required for several other cluster packages in order to respect OCF and other RAs standards.

MUST items:
- The package must be named according to the  Package Naming Guidelines: OK
- The spec file name must match the base package: OK
- The package must meet the  Packaging Guidelines:
  * Licence tag needs fixing (as reported by rpmlint too)
  * It contains unnecessary BuildRequires
  * Changelog format doesn't comply with standards
  * Vendor tag should not be used
  * Source tag should contain full URL to the source
  * Requiring Base Package should use a fully versioned dependency
- The package must be licensed..: as above.
- The License field in the package spec file must match the actual license: NOK. Source contains GPL2 and spec file GPL.
- 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.
- The spec file must be written in American English: OK
- The spec file for the package MUST be legible: OK, see my notes below for minor improvements.
- 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.
- The package MUST successfully compile and build into binary rpms on at least one primary architecture: OK, tested in koji.
- All build dependencies must be listed in BuildRequires: OK. As noted above, some buildrequired don't need to be there.
- The spec file MUST handle locales properly: does not apply.
- Every binary RPM package (or subpackage) which stores shared library files: does not apply
- Packages must NOT bundle copies of system libraries: OK
- If the package is designed to be relocatable..: does not apply
- A package must own all directories that it creates...: %{_prefix}/lib/%{name} seems to be un-owned.
- A Fedora package must not list a file more than once in the spec file's %files listing: OK
- Permissions on files must be set properly...: OK, see minor note below.
- Each package must have a %clean section, which contains rm -rf %{buildroot}: OK
- Each package must consistently use macros: OK
- The package must contain code, or permissable content: OK
- Large documentation files must go in a -doc subpackage: does not apply
- If a package includes something as %doc..: OK
- Header files must be in a -devel package..: does not apply.
- Static libraries must be in a -static package..: does not apply.
- Packages containing pkgconfig(.pc) files must..: does not apply.
- If a package contains library...: does not apply
- In the vast majority of cases, devel packages...: does not apply.
- Packages must NOT contain any .la libtool archives..: does not apply.
- Packages containing GUI applications must include a %{name}.desktop file..: does not apply.
- Packages must not own files or directories already owned by other packages..: OK
- At the beginning of %install, each package MUST run rm -rf %{buildroot}.. : OK
- All filenames in rpm packages must be valid UTF-8: OK

SHOULD items:
- If the source package does not include license text(s) as..: source contains COPYING file with licence. OK
- The description and summary sections in the package spec file should contain translations..: does not apply
- The reviewer should test that the package builds in mock: OK. Tested with:
 koji build --scratch dist-f13 drbd-8.3.4-5.fc12.src.rpm
 http://koji.fedoraproject.org/koji/taskinfo?taskID=1745665
1745665 build (dist-f13, drbd-8.3.4-5.fc12.src.rpm) completed successfully
- The package should compile and build into binary rpms on all supported architectures: OK
- The reviewer should test that the package functions as described: will be done once we address the packaging issues.
- If scriptlets are used, those scriptlets must be sane..: see notes below.
- Usually, subpackages other than devel should require the base package using a fully versioned dependency: NOK, already noted above.
- The placement of pkgconfig(.pc) files depends..: does not apply
- If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin..: does not apply

Notes:

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

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

- spec file readability: need a bit of sorting. I personally prefer to sort entries in the spec file with build/setup/install bits at the top, and then all packages afterwards. All the different sub packages are splitted across 2 sections that's not really required and adds a lot of conditionals, while others are all packed in one (easier to read).

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

- %build doesn't respect smpflags

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

- scriptlets:
  %post utils package performs mknod operations. When udev is available, is that operation required at all?
  %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


About the general packaing:

- most of the packages could be architecture generic. drdb placeholder, the several scripts integration stuff too.

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

Let's start addressing those problems, then we will do another full review to see if I missed something.

Fabio

Again about kernel module - since you don't build module by default, then you should rely on specific kernel version. I advice you to add the following line in case %{km} is not set:

Requires: kernel >= 2.6.<add proper numbers here>

(In reply to comment #18)
> Again about kernel module - since you don't build module by default, then you
> should rely on specific kernel version. I advice you to add the following line
> in case %{km} is not set:
> 
> Requires: kernel >= 2.6.<add proper numbers here>  

It's a good advice but can't apply as long as the kmod is not upstream. This is indeed something to keep in mind once Fedora will ship the kmod via kernel upstream.

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

Going back to the noarch subpackages issue, here's a successful koji build with no BuildArch set on any subpackage:

http://koji.fedoraproject.org/koji/taskinfo?taskID=1745851

Conversely, when adding "BuildArch: noarch" on all subpackages _except_ km and utils (which are definitely arch-specific), then the koji build fails like this (apparently it's trying to build the utils package with noarch, which is  unexpected and bound to fail):

http://koji.fedoraproject.org/koji/taskinfo?taskID=1745862

Finally, adding --arch-override=x86_64 un-breaks the build again, and I am getting x86_64 drbd, drbd-utils, and drbd-debuginfo packages, and noarch builds for everything else.

http://koji.fedoraproject.org/koji/taskinfo?taskID=1745866

Seems to be a koji, mock, or whatever issue -- it looks like it's beyond our reach. :)

(In reply to comment #21)
> Going back to the noarch subpackages issue, here's a successful koji build with
> no BuildArch set on any subpackage:
> 
> http://koji.fedoraproject.org/koji/taskinfo?taskID=1745851
> 
> Conversely, when adding "BuildArch: noarch" on all subpackages _except_ km and
> utils (which are definitely arch-specific), then the koji build fails like this
> (apparently it's trying to build the utils package with noarch, which is 
> unexpected and bound to fail):
> 
> http://koji.fedoraproject.org/koji/taskinfo?taskID=1745862
> 
> Finally, adding --arch-override=x86_64 un-breaks the build again, and I am
> getting x86_64 drbd, drbd-utils, and drbd-debuginfo packages, and noarch builds
> for everything else.
> 
> http://koji.fedoraproject.org/koji/taskinfo?taskID=1745866
> 
> Seems to be a koji, mock, or whatever issue -- it looks like it's beyond our
> reach. :)  

Ok, let's leave the packages Arch any for now while we will investigate the reason of the failure. It can always be fixed at a later stage.

(In reply to comment #20)
> 
> >   * 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?

You can either leave it or remove it. gcc is guaranteed to be installed in the build environment. It's not a blocker either way.

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

Ok, it is nice to see such commitment from upstream. Once this review is completed and spec file approved, please post an extra final srpm to match the tarball release.

> > - %build doesn't respect smpflags
> 
> Done for now, requires a bit more testing on our part though.

Remember this is not mandatory. If upstream does not support smpflags, just note it in the spec file.

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

Ok perfect...

the spec file is still a bit difficult to read, but this is just from my PoV,
otherwise it looks sane and adhere with Policy.

2 small notes on the packages that are not built by default (so no blockers, just cleanup):

%package km-%{krelver}
Requires: %{name}-utils = %{version}, /sbin/depmod

file dependencies are generally to be avoided.

Conflicts: rgmanager >= 3.0.1

You want to this to be resource-agents. rgmanager package doesn't ship agents anymore in Fedora.

I am in the process of testing the packages now for runtime issues as required by the checklist. Unless there are issues here, the packaging looks OK.

Some more things...

building the km (for testing) fails in differnt ways.

Try builds with:

rpmbuild -ba --with km SPECS/drbd.spec

rpmbuild -ba --define 'kernelversion 2.6.31.1-56.fc12.x86_64' --define 'kdir /lib/modules/2.6.31.1-56.fc12.x86_64/build' --with km SPECS/drbd.spec

Then I had to add:

%configure \
    %{?_without_utils} \
    %{?_with_km} \
    KDIR=%{kdir} \
    %{?_without_udev} \

^^ KDIR in there otherwise the configure assumes uname -r that might not be true if I did override from command line.

Clearly the kernel module is NOT a blocker, but helps for testing of userland ;)

Something else triggered my attention:

[fabbione@daitarn-fedora rpmbuild]$ rpmlint SPECS/drbd.spec SRPMS/drbd-8.3.4-6.fc12.src.rpm RPMS/x86_64/*

drbd.src: W: name-repeated-in-summary DRBD
drbd.src: W: no-version-in-last-changelog
drbd.src: W: strange-permission drbd.spec 0600

drbd.x86_64: W: name-repeated-in-summary DRBD
drbd.x86_64: W: no-version-in-last-changelog
drbd.x86_64: E: no-binary
drbd-bash-completion.x86_64: W: no-version-in-last-changelog
drbd-bash-completion.x86_64: W: non-conffile-in-etc /etc/bash_completion.d/drbdadm
drbd-bash-completion.x86_64: E: non-standard-dir-perm /usr/share/doc/drbd-bash-completion-8.3.4 0644
drbd-debuginfo.x86_64: W: no-version-in-last-changelog
drbd-heartbeat.x86_64: W: no-version-in-last-changelog
drbd-heartbeat.x86_64: W: spurious-executable-perm /usr/share/doc/drbd-heartbeat-8.3.4/COPYING
drbd-heartbeat.x86_64: W: spurious-executable-perm /usr/share/man/man8/drbddisk.8.gz
drbd-km-2.6.31.1_56.fc12.x86_64.x86_64: W: summary-ended-with-dot Kernel driver for DRBD.
drbd-km-2.6.31.1_56.fc12.x86_64.x86_64: W: no-version-in-last-changelog
drbd-km-2.6.31.1_56.fc12.x86_64.x86_64: W: unstripped-binary-or-object /lib/modules/2.6.31.1-56.fc12.x86_64/kernel/drivers/block/drbd.ko
drbd-km-2.6.31.1_56.fc12.x86_64.x86_64: W: dangerous-command-in-%post mv
drbd-pacemaker.x86_64: W: no-version-in-last-changelog
drbd-pacemaker.x86_64: W: only-non-binary-in-usr-lib
drbd-pacemaker.x86_64: W: spurious-executable-perm /usr/share/doc/drbd-pacemaker-8.3.4/COPYING
drbd-udev.x86_64: W: summary-not-capitalized udev integration scripts for DRBD
drbd-udev.x86_64: W: no-version-in-last-changelog
drbd-udev.x86_64: W: non-conffile-in-etc /etc/udev/rules.d/65-drbd.rules
drbd-utils.x86_64: W: no-version-in-last-changelog
drbd-utils.x86_64: W: unstripped-binary-or-object /sbin/drbdsetup
drbd-utils.x86_64: W: unstripped-binary-or-object /sbin/drbdmeta
drbd-utils.x86_64: W: only-non-binary-in-usr-lib
drbd-utils.x86_64: W: service-default-enabled /etc/rc.d/init.d/drbd
drbd-utils.x86_64: W: service-default-enabled /etc/rc.d/init.d/drbd
drbd-utils.x86_64: W: incoherent-init-script-name drbd ('drbd-utils', 'drbd-utilsd')
drbd-xen.x86_64: W: no-version-in-last-changelog
drbd-xen.x86_64: W: spurious-executable-perm /usr/share/doc/drbd-xen-8.3.4/COPYING
10 packages and 1 specfiles checked; 10 errors, 30 warnings.

Please address those... it appears the changelog is still generated incorrectly.

(In reply to comment #23)
> %package km-%{krelver}
> Requires: %{name}-utils = %{version}, /sbin/depmod
> 
> file dependencies are generally to be avoided.

Specifying the dependency directly on the depmod binary seemed like the sanest thing to do to be distro agnostic. I can have this replaced by the distro-specific package name, but that seems a bit awkward to me.

> Conflicts: rgmanager >= 3.0.1
> 
> You want to this to be resource-agents. rgmanager package doesn't ship agents
> anymore in Fedora.

So resource-agents owns the /usr/share/cluster directory? In that case I'll happily change that.

And then the dependency is primarily there for rgmanager 2.0 users (read: CentOS, EPEL for 5.x) where rgmanager does, I believe, ship the resource agents. So I actually believe that the Conflicts is right, and I'm inclined to think that the relevant Requires tag is correct as well. Please confirm or refute.

(In reply to comment #24)
> Some more things...
> 
> building the km (for testing) fails in differnt ways.
> 
> Try builds with:
> 
> rpmbuild -ba --with km SPECS/drbd.spec
> 
> rpmbuild -ba --define 'kernelversion 2.6.31.1-56.fc12.x86_64' --define 'kdir
> /lib/modules/2.6.31.1-56.fc12.x86_64/build' --with km SPECS/drbd.spec
> 
> Then I had to add:
> 
> %configure \
>     %{?_without_utils} \
>     %{?_with_km} \
>     KDIR=%{kdir} \
>     %{?_without_udev} \
> 
> ^^ KDIR in there otherwise the configure assumes uname -r that might not be
> true if I did override from command line.

Yes, but you'll notice that when setting KDIR from configure you only set the KDIR _default_ in the generated Makefiles, which are meant to be overridden from make if necessary (they're defined with "?="). The whole idea was to be able to build kernel modules for different flavors without rerunning configure (and thereby with better short-circuiting).

So the fact that in the spec KDIR is only passed in on make, not on configure, is intentional and it does work for me. If it doesn't for you, please send me your build logs by private email and I'll be happy to look into them.

(In reply to comment #25)

> Please address those... it appears the changelog is still generated
> incorrectly.  

Yes, I still need to iron out one changelog-mangling sanity check in our Makefiles so it can deal with the fixed changelog format. Will be fixed shortly.

(In reply to comment #26)
> (In reply to comment #23)
> > %package km-%{krelver}
> > Requires: %{name}-utils = %{version}, /sbin/depmod
> > 
> > file dependencies are generally to be avoided.
> 
> Specifying the dependency directly on the depmod binary seemed like the sanest
> thing to do to be distro agnostic. I can have this replaced by the
> distro-specific package name, but that seems a bit awkward to me.

So 2 things.. the sub-package is not built by default so it's not a big deal. While I understand the reason of being distro-agnostic, we are still looking to apply Fedora policy here :)

> 
> > Conflicts: rgmanager >= 3.0.1
> > 
> > You want to this to be resource-agents. rgmanager package doesn't ship agents
> > anymore in Fedora.
> 
> So resource-agents owns the /usr/share/cluster directory? In that case I'll
> happily change that.

Yes that's right.

Updated spec and source RPM are here:

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

(In reply to comment #29)
> So 2 things.. the sub-package is not built by default so it's not a big deal.
> While I understand the reason of being distro-agnostic, we are still looking to
> apply Fedora policy here :)

Left as is for the time being. If there are violent objections, I'll change that (the spec, not the objections :) ).

> > So resource-agents owns the /usr/share/cluster directory? In that case I'll
> > happily change that.
> 
> Yes that's right.

What I did after all was add a Conflicts tag with resource-agents (there is no resource-agents version prior to 3, and it has the DRBD resource agent), and a Requires that lists rgmanager (pre-Cluster 3, where the agents were part of the rgmanager package). Still, either way it's probably moot as I expect most people to build with the defaults, which disable this part of the build entirely.

Changelog has also been modified to follow the recommended format, and the upstream changelog file has been moved to %doc.

This is the abbreviated revision since the last update. I finally found out about git log --reverse, so this time is actually in the correct chronological order:

7ee968e... drbd.spec.in: fix dependencies for drbd-rgmanager
b6d1616... drbd.spec.in: fix a few default permission bits
ed8033c... drbd.spec.in: bump release number

As you can see, it's really quieting down. Koji builds for f12 and f13 are here:

http://koji.fedoraproject.org/koji/taskinfo?taskID=1747192
http://koji.fedoraproject.org/koji/taskinfo?taskID=1747197

Just for the record, there are still some rpmlint warnings/errors that need to be addressed.

We discussed those quickly on IRC.

Here is a listing of rpmlint warnings and errors that Fabio reported:

> drbd.src: W: name-repeated-in-summary DRBD

Well, it's the DRBD driver. What else is there to say? :)

> drbd.src: W: strange-permission drbd.spec 0600

No idea. It's 0640 here. 

> drbd.x86_64: E: no-binary

Yes the drbd metapackage could be flagged as noarch. No it currently can't be built that way. See comment #21.

drbd-bash-completion.x86_64: W: non-conffile-in-etc /etc/bash_completion.d/drbdadm

Flagged with %config now.

> drbd-pacemaker.x86_64: W: spurious-executable-perm /usr/share/doc/drbd-pacemaker-8.3.4/COPYING

Fixed.

> drbd-udev.x86_64: W: summary-not-capitalized udev integration scripts for DRBD

Even Wikipedia doesn't capitalize "udev" at the beginning of a sentence.

> drbd-udev.x86_64: W: non-conffile-in-etc /etc/udev/rules.d/65-drbd.rules

Flagged with %config now.

> drbd-utils.x86_64: W: unstripped-binary-or-object /sbin/drbdsetup
> drbd-utils.x86_64: W: unstripped-binary-or-object /sbin/drbdmeta

This one leaves me completely baffled.  AFAICS this is supposed to happen when building with debuginfo disabled, not building with gcc -g, or when installing .so files without the x bit set. None of that is the case. I have no idea how to fix this.

Building and installing, and then examining the installed files, yields this:

file /sbin/drbdmeta /sbin/drbdsetup
/sbin/drbdmeta:  ELF 64-bit LSB executable, AMD x86-64, version 1 (SYSV), for GNU/Linux 2.6.9, dynamically linked (uses shared libs), for GNU/Linux 2.6.9, stripped
/sbin/drbdsetup: ELF 64-bit LSB executable, AMD x86-64, version 1 (SYSV), for GNU/Linux 2.6.9, dynamically linked (uses shared libs), for GNU/Linux 2.6.9, stripped

So, definitely stripped.

And they're not missing from the debuginfo package either:
rpm -qlp drbd-debuginfo-8.3.4-7.x86_64.rpm
/usr/lib/debug
/usr/lib/debug/sbin
/usr/lib/debug/sbin/drbdadm.debug
/usr/lib/debug/sbin/drbdmeta.debug
/usr/lib/debug/sbin/drbdsetup.debug

Either this is an rpmlint bug, or I'm overlooking something obvious.


> drbd-utils.x86_64: W: only-non-binary-in-usr-lib

OK, this refers to helper scripts installing into /usr/lib/drbd. Discussed previously, it's my understanding that that is OK.

> drbd-utils.x86_64: W: service-default-enabled /etc/rc.d/init.d/drbd
> drbd-utils.x86_64: W: service-default-enabled /etc/rc.d/init.d/drbd

chkconfig headers in the init script have been fixed.

> drbd-xen.x86_64: W: spurious-executable-perm /usr/share/doc/drbd-xen-8.3.4/COPYING

Also fixed to default attributes.

Updated spec and SRPM:

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

Koji build tasks:

http://koji.fedoraproject.org/koji/taskinfo?taskID=1747615 (f12)
http://koji.fedoraproject.org/koji/taskinfo?taskID=1747612 (f13)

Changelog:

0f23a9a... drbd init script: install in no runlevels by default
aab884b... drbd.spec.in: fix file permissions
87cbbe4... drbd.spec.in: add %config where appropriate
792e28f... drbd.spec.in: bump release number

(In reply to comment #32)
> Here is a listing of rpmlint warnings and errors that Fabio reported:
> 
> > drbd.src: W: name-repeated-in-summary DRBD
> 
> Well, it's the DRBD driver. What else is there to say? :)
> 
> > drbd.src: W: strange-permission drbd.spec 0600
>
> No idea. It's 0640 here. 

Interesting enough, when I unpack your rpm, it's 0600 and rpmlint catches it. Of cource changing the permissions locally and then testing is ok.

> drbd-bash-completion.x86_64: W: non-conffile-in-etc
> /etc/bash_completion.d/drbdadm
> 
> Flagged with %config now.

hehe Ok.. I was waiting for this one.. you can say I am a bit bastard ;)

drbd-bash-completion.x86_64: W: conffile-without-noreplace-flag /etc/bash_completion.d/drbdadm

> > drbd-udev.x86_64: W: non-conffile-in-etc /etc/udev/rules.d/65-drbd.rules
> 
> Flagged with %config now.

drbd-udev.x86_64: W: conffile-without-noreplace-flag /etc/udev/rules.d/65-drbd.rules


> 
> > drbd-utils.x86_64: W: unstripped-binary-or-object /sbin/drbdsetup
> > drbd-utils.x86_64: W: unstripped-binary-or-object /sbin/drbdmeta
> 
> This one leaves me completely baffled. 

I checked koji binaries and they are fine and yours are too. I guess I'll need to check my environment and see why it's happening.

Don't worry about it, I am sure it's my problem. Sorry I haven't noticed it before.

> 
> 
> > drbd-utils.x86_64: W: only-non-binary-in-usr-lib
> 
> OK, this refers to helper scripts installing into /usr/lib/drbd. Discussed
> previously, it's my understanding that that is OK.

Yes it is.. as I mentioned, I did only copy/paste the full output to you.

(In reply to comment #34)
> > > drbd.src: W: strange-permission drbd.spec 0600
> >
> > No idea. It's 0640 here. 
> 
> Interesting enough, when I unpack your rpm, it's 0600 and rpmlint catches it.
> Of cource changing the permissions locally and then testing is ok.

OK, I'll look into that one more time.

> > drbd-bash-completion.x86_64: W: non-conffile-in-etc
> > /etc/bash_completion.d/drbdadm
> > 
> > Flagged with %config now.
> 
> hehe Ok.. I was waiting for this one.. you can say I am a bit bastard ;)
> 
> drbd-bash-completion.x86_64: W: conffile-without-noreplace-flag
> /etc/bash_completion.d/drbdadm

I *want* to replace this on updates. It's programmable bash completion for drbdadm. This is definitely something the packager knows better than the user.

> > > drbd-udev.x86_64: W: non-conffile-in-etc /etc/udev/rules.d/65-drbd.rules
> > 
> > Flagged with %config now.
> 
> drbd-udev.x86_64: W: conffile-without-noreplace-flag
> /etc/udev/rules.d/65-drbd.rules

As above.

As it stands, would you now consider the package mature enough for sponsor review?

(In reply to comment #35)
> 
> As it stands, would you now consider the package mature enough for sponsor
> review?  

Yes.. I was completing the runtime tests right now:

/dev/drbd0 on /mnt type gfs2 (rw,relatime,localflocks,localcaching)

cat /proc/drbd 
version: 8.3.4 (api:88/proto:86-91)
GIT-hash: 792e28fe8928ff72c706e0a8fdbbcc10e36bbf3c build by fabbione.fabbione.net, 2009-10-15 19:14:04
 0: cs:Connected ro:Primary/Primary ds:UpToDate/UpToDate C r----
    ns:0 nr:6553364 dw:6553364 dr:204 al:0 bm:400 lo:0 pe:0 ua:0 ap:0 ep:1 wo:b oos:0

One small note, doesn't require neither a new srpm or a new spec for this review, the default config file is a placeholder to point to the documentation. This is fine, but it points to the wrong directory. /usr/share/doc/drbd/drbd.conf vs /usr/share/doc/drbd-utils-%{version}/drbd.conf :)

IMHO it's good enough to go through a sponsor review.

(In reply to comment #35)
I think you can reduce your spec file.

%doc COPYING
%doc ChangeLog
%doc README


can be changed to

%doc COPYING ChangeLog README

(In reply to comment #37)
> (In reply to comment #35)
> I think you can reduce your spec file.
> 
> %doc COPYING
> %doc ChangeLog
> %doc README
> 
> 
> can be changed to
> 
> %doc COPYING ChangeLog README  

Point taken, but I tend to keep these things on separate lines as much as possible.This makes it a lot easier to keep track of across multiple git branches that occasionally get merged back and forth. :)

(In reply to comment #36)
> /dev/drbd0 on /mnt type gfs2 (rw,relatime,localflocks,localcaching)
> 
> cat /proc/drbd 
> version: 8.3.4 (api:88/proto:86-91)
> GIT-hash: 792e28fe8928ff72c706e0a8fdbbcc10e36bbf3c build by
> fabbione.fabbione.net, 2009-10-15 19:14:04
>  0: cs:Connected ro:Primary/Primary ds:UpToDate/UpToDate C r----
>     ns:0 nr:6553364 dw:6553364 dr:204 al:0 bm:400 lo:0 pe:0 ua:0 ap:0 ep:1 wo:b
> oos:0

Yep, that looks good. I take it the --with km build difficulties you mentioned earlier are resolved then.

> One small note, doesn't require neither a new srpm or a new spec for this
> review, the default config file is a placeholder to point to the documentation.
> This is fine, but it points to the wrong directory.
> /usr/share/doc/drbd/drbd.conf vs /usr/share/doc/drbd-utils-%{version}/drbd.conf
> :)

Yeah right, and when I change it to that you pounce on me for not using %{_docdir}/%{name}-utils-%{version}. Called your bluff. :)

Fixed locally. I'm recklessly deeming this too minor to warrant another upload.

> IMHO it's good enough to go through a sponsor review.  

Excellent, thanks.

As one of the sponsor members I want to ask some questions before
someone (including) me can start review:

- Would you explain why non-arch-independent files under /usr/lib/%{name}
  cannot be moved to %{_datadir}?

- Would you explain why you want to keep "%bcond_with km" part 
  on the spec file which seems completely unneeded on Fedora 
  ( according to your comments )?
  Removing parts which are not needed for Fedora will make the spec
  file more readable and preferred.
  ( And I think anyway this "%bcond_with km" part is completely
    broken because we don't ensure that the kernel version of
    the build server and of the host that the rebuilt binary rpm
    is to be used is the same. For example while F-12 kernel is
    now 2.6.31.1, the build server to build F-12 rpms uses
    2.6.18 kernel: see the build.log of your comment 33)

- Similarly, would you explain why you want to keep
  %if %{without udev} part on Fedora?

- Please remove duplicate file entries. Try:
  $ rpm -qlp *rpm | sort | uniq -d
  This will show that some files are included in multiple rpms.

(In reply to comment #40)
> As one of the sponsor members I want to ask some questions before
> someone (including) me can start review:
> 
> - Would you explain why non-arch-independent files under /usr/lib/%{name}
>   cannot be moved to %{_datadir}?

They are arch-independent. All that gets installed in that directory is a number of shell scripts that are provided for drbd's userland callouts (which it fires in a number of situations, such as detecting split brain or becoming a synchronization target). Fabio and I have discussed this issue here; please see comment #16.

> - Would you explain why you want to keep "%bcond_with km" part 
>   on the spec file which seems completely unneeded on Fedora 
>   ( according to your comments )?

Well for one thing it's positively needed for this package review, as the drbd backport is not in the Fedora kernel as yet. :) Fabio has pointed out (in comment #5 and comment #24, among others) that building the kernel module is irrelevant for Fedora -- but that other packages do contain userland that is expected to interface with a kernel feature that's not in Fedora.

The alternative would be to put the kernel module build setup in a separate spec, and making that available outside of Fedora -- IMHO that's clearly an inferior approach in terms of accessibility.

>   Removing parts which are not needed for Fedora will make the spec
>   file more readable and preferred.
>   ( And I think anyway this "%bcond_with km" part is completely
>     broken because we don't ensure that the kernel version of
>     the build server and of the host that the rebuilt binary rpm
>     is to be used is the same.

That's actually irrelevant; we can build the userland on any kernel, it doesn't need to match that of the kernel module build.

>   For example while F-12 kernel is
>     now 2.6.31.1, the build server to build F-12 rpms uses
>     2.6.18 kernel: see the build.log of your comment 33)

Yes. Again, irrelevant to the userland build. Users can always locally build the kernel module from the source rpm. And don't need to build anything else.

> - Similarly, would you explain why you want to keep
>   %if %{without udev} part on Fedora?

Because users who rebuild my choose not to use the drbd udev integration scripts at all? We default to what seems sensible (to us), that is, use udev, but there's no reason to force this upon users, so they can and will disable this if they see fit.

> - Please remove duplicate file entries. Try:
>   $ rpm -qlp *rpm | sort | uniq -d
>   This will show that some files are included in multiple rpms.  

Correct, those crm-fence-peer scripts slipped through. I'll fix that. Thanks!

(In reply to comment #41)
> (In reply to comment #40)
> > As one of the sponsor members I want to ask some questions before
> > someone (including) me can start review:
> > 
> > - Would you explain why non-arch-independent files under /usr/lib/%{name}
> >   cannot be moved to %{_datadir}?

- Oops... I meant why "arch-independent" files under /usr/lib/%{name}
  cannot be moved to %{_datadir}?
> 
> They are arch-independent. All that gets installed in that directory is a
> number of shell scripts that are provided for drbd's userland callouts (which
> it fires in a number of situations, such as detecting split brain or becoming a
> synchronization target). Fabio and I have discussed this issue here; please see
> comment #16.

- Your comment 16 does not answer my question. Usually arch-independent
  files are supposed to be installed under %{_datadir}.

> 
> > - Would you explain why you want to keep "%bcond_with km" part 
> >   on the spec file which seems completely unneeded on Fedora 
> >   ( according to your comments )?
> 
> Well for one thing it's positively needed for this package review, as the drbd
> backport is not in the Fedora kernel as yet. :) Fabio has pointed out (in
> comment #5 and comment #24, among others) that building the kernel module is
> irrelevant for Fedora -- but that other packages do contain userland that is
> expected to interface with a kernel feature that's not in Fedora.

- I am speaking of writing _kernel space_ related hacks on the spec
  file (and you say this is "irrelevant for Fedora", right?)

> The alternative would be to put the kernel module build setup in a separate
> spec, and making that available outside of Fedora -- IMHO that's clearly an
> inferior approach in terms of accessibility.

- IMO Fedora / rpmfusion packages acutally do this approach.

> 
> >   Removing parts which are not needed for Fedora will make the spec
> >   file more readable and preferred.
> >   ( And I think anyway this "%bcond_with km" part is completely
> >     broken because we don't ensure that the kernel version of
> >     the build server and of the host that the rebuilt binary rpm
> >     is to be used is the same.
> 
> That's actually irrelevant; we can build the userland on any kernel, it doesn't
> need to match that of the kernel module build.

- What I am speaking is when "--with km" is passed to your srpm
  and not speaking about only building userland part binary rpm.
  So if you're thinking of userland build, again "%bcond_with km"
  is not needed.

> 
> >   For example while F-12 kernel is
> >     now 2.6.31.1, the build server to build F-12 rpms uses
> >     2.6.18 kernel: see the build.log of your comment 33)
> 
> Yes. Again, irrelevant to the userland build. Users can always locally build
> the kernel module from the source rpm. And don't need to build anything else.

- So this (i.e. user _has to_ build locally kernel module) is not expected
  on Fedora."

> 
> > - Similarly, would you explain why you want to keep
> >   %if %{without udev} part on Fedora?
> 
> Because users who rebuild my choose not to use the drbd udev integration
> scripts at all? We default to what seems sensible (to us), that is, use udev,
> but there's no reason to force this upon users, so they can and will disable
> this if they see fit.

- So why is it needed _on Fedora_? (i.e. why do you expect that F-10/11/12/13
  user chooses not to use udev integration scripts although all of them
  have udev installed?)

(In reply to comment #42)
> > >   Removing parts which are not needed for Fedora will make the spec
> > >   file more readable and preferred.
> > >   ( And I think anyway this "%bcond_with km" part is completely
> > >     broken because we don't ensure that the kernel version of
> > >     the build server and of the host that the rebuilt binary rpm
> > >     is to be used is the same.
> > 
> > That's actually irrelevant; we can build the userland on any kernel, it doesn't
> > need to match that of the kernel module build.
> 
> - What I am speaking is when "--with km" is passed to your srpm
>   and not speaking about only building userland part binary rpm.
>   So if you're thinking of userland build, again "%bcond_with km"
>   is not needed.

To be clear, I am asking to remove all kernel module related part from your
spec file (i.e. removing all parts which are in effect only
when "--with km" is passed to your srpm)

(In reply to comment #39)
> (In reply to comment #36)
> > /dev/drbd0 on /mnt type gfs2 (rw,relatime,localflocks,localcaching)
> > 
> > cat /proc/drbd 
> > version: 8.3.4 (api:88/proto:86-91)
> > GIT-hash: 792e28fe8928ff72c706e0a8fdbbcc10e36bbf3c build by
> > fabbione.fabbione.net, 2009-10-15 19:14:04
> >  0: cs:Connected ro:Primary/Primary ds:UpToDate/UpToDate C r----
> >     ns:0 nr:6553364 dw:6553364 dr:204 al:0 bm:400 lo:0 pe:0 ua:0 ap:0 ep:1 wo:b
> > oos:0
> 
> Yep, that looks good. I take it the --with km build difficulties you mentioned
> earlier are resolved then.

No but I'll come back to you on those privately.

> 
> > One small note, doesn't require neither a new srpm or a new spec for this
> > review, the default config file is a placeholder to point to the documentation.
> > This is fine, but it points to the wrong directory.
> > /usr/share/doc/drbd/drbd.conf vs /usr/share/doc/drbd-utils-%{version}/drbd.conf
> > :)
> 
> Yeah right, and when I change it to that you pounce on me for not using
> %{_docdir}/%{name}-utils-%{version}. Called your bluff. :)

hahaha ok :)

> 
> Fixed locally. I'm recklessly deeming this too minor to warrant another upload.
> 

That's fine, it can wait for the final import.

(In reply to comment #42)
> (In reply to comment #41)
> > (In reply to comment #40)
> > > As one of the sponsor members I want to ask some questions before
> > > someone (including) me can start review:
> > > 
> > > - Would you explain why non-arch-independent files under /usr/lib/%{name}
> > >   cannot be moved to %{_datadir}?
> 
> - Oops... I meant why "arch-independent" files under /usr/lib/%{name}
>   cannot be moved to %{_datadir}?
> > 
> > They are arch-independent. All that gets installed in that directory is a
> > number of shell scripts that are provided for drbd's userland callouts (which
> > it fires in a number of situations, such as detecting split brain or becoming a
> > synchronization target). Fabio and I have discussed this issue here; please see
> > comment #16.
> 
> - Your comment 16 does not answer my question. Usually arch-independent
>   files are supposed to be installed under %{_datadir}.

It was comment #17:

> Regarding the hardcoded-library-path I understand the issue and it's ok to have
> exceptions. This is required for several other cluster packages in order to
> respect OCF and other RAs standards.

There are several problems moving those files in /usr/share. All cluster (or similar) related scripts and agents (or callbacks like they are called in drbd world) need to adhere to OCF and RAs standards. The OCF standards, when was written, didn't have knowledge of things like /usr/lib64 and callbacks/RAs and similar can also be binaries or a mixture of binaries and scripts.

In short, given the mixed nature and that those files are always explicitly invoked within configuration files they have to be consistent across architectures (hardcoded /usr/lib vs lib/lib64) and they can't be in %{_datadir} because there could be binaries in there (there are none in drbd package now, but users can build their own custom hooks in there).
The configuration needs to be exactly the same on all nodes (so even a mixture of x86 and x86_64 can't carry the lib/lib64 difference) and mostoften is syncronized automatically across nodes.

This same issues has been discussed already 2/3 times for other cluster related packages and we have to accept that those packages need this (and only this) specific exception to adhere with the standards and be portable across different architectures (keeping in mind that there was no lib64 when they were written).

> 
> > 
> > > - Would you explain why you want to keep "%bcond_with km" part 
> > >   on the spec file which seems completely unneeded on Fedora 
> > >   ( according to your comments )?
> > 
> > Well for one thing it's positively needed for this package review, as the drbd
> > backport is not in the Fedora kernel as yet. :) Fabio has pointed out (in
> > comment #5 and comment #24, among others) that building the kernel module is
> > irrelevant for Fedora -- but that other packages do contain userland that is
> > expected to interface with a kernel feature that's not in Fedora.
> 
> - I am speaking of writing _kernel space_ related hacks on the spec
>   file (and you say this is "irrelevant for Fedora", right?)

It is irrelevant because they are not used by default to comply with "no kmod" policy.

I agree that we could drop them at somepoint, but I found them nice to have them there and be able to build the mod and complete the runtime testing required by the review.

(In reply to comment #42)
> (In reply to comment #41)
> > (In reply to comment #40)
> > > As one of the sponsor members I want to ask some questions before
> > > someone (including) me can start review:
> > > 
> > > - Would you explain why non-arch-independent files under /usr/lib/%{name}
> > >   cannot be moved to %{_datadir}?
> 
> - Oops... I meant why "arch-independent" files under /usr/lib/%{name}
>   cannot be moved to %{_datadir}?

Addressed succinctly by Fabio in comment #45; I have nothing to add there. :)

> - I am speaking of writing _kernel space_ related hacks on the spec
>   file (and you say this is "irrelevant for Fedora", right?)
> 
> > The alternative would be to put the kernel module build setup in a separate
> > spec, and making that available outside of Fedora -- IMHO that's clearly an
> > inferior approach in terms of accessibility.
> 
> - IMO Fedora / rpmfusion packages acutally do this approach.

That's fine, and it's certainly possible for us to do so, it's just that it seems convenient for reviewers to be able to build both from one spec file. If I understand Fabio correctly, his comment #45 corroborates that.

> > >   Removing parts which are not needed for Fedora will make the spec
> > >   file more readable and preferred.
> > >   ( And I think anyway this "%bcond_with km" part is completely
> > >     broken because we don't ensure that the kernel version of
> > >     the build server and of the host that the rebuilt binary rpm
> > >     is to be used is the same.
> > 
> > That's actually irrelevant; we can build the userland on any kernel, it doesn't
> > need to match that of the kernel module build.
> 
> - What I am speaking is when "--with km" is passed to your srpm
>   and not speaking about only building userland part binary rpm.
>   So if you're thinking of userland build, again "%bcond_with km"
>   is not needed.

Which is why it's disabled by default. And: it will build correctly if it finds a kernel Makefile in /lib/modules/`uname -r`/build, which is what people get when they just install kernel-devel and then invoke rpmbuild. It will also build correctly in a chroot environment if passed the kernelversion and kdir defines.
 
> - So this (i.e. user _has to_ build locally kernel module) is not expected
>   on Fedora."

Fully agreed, as discussed in earlier comments; it is expected that people will use this in conjunction with a DRBD-enabled Fedora kernel. As long as DRBD is not available in the stock Fedora kernel, people can

a) install kernel-source, patch DRBD into their kernel source checkout, and build a kernel with a DRBD backports or

b) install drbd from a kmod build.

The drbd-km part merely provides b) as an added convenience.

> > > - Similarly, would you explain why you want to keep
> > >   %if %{without udev} part on Fedora?
> > 
> > Because users who rebuild my choose not to use the drbd udev integration
> > scripts at all? We default to what seems sensible (to us), that is, use udev,
> > but there's no reason to force this upon users, so they can and will disable
> > this if they see fit.
> 
> - So why is it needed _on Fedora_? (i.e. why do you expect that F-10/11/12/13
>   user chooses not to use udev integration scripts although all of them
>   have udev installed?)  

The udev integration scripts are not required for DRBD functionality. I.e. DRBD will work happily not only on a non udev enabled system (clearly not expected for Fedora), but it will also interface nicely with udev when the drbd udev integration scripts are not installed. All the udev scripts do is install symlinks to the DRBD devices, with user-friendly names.

So rather than unconditionally shove the udev scripts down the throat of people who may not want them, I consider it prudent to enable them by default and giving users the option of installing and uninstalling them separately.

New spec file (with all km bits removed) and SRPM are here:

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

For those interested in rolling the kernel module, use this spec:
http://people.linbit.com/~florian/drbd-km.spec

That -km spec is still included in the SRPM, and supports being built from the same tarball.

Koji build logs (drbd.spec build):

http://koji.fedoraproject.org/koji/taskinfo?taskID=1749451 (f12)
http://koji.fedoraproject.org/koji/taskinfo?taskID=1749458 (f13)

Changes:
b974fce... Revert "drbd.spec.in: simplify %files section in utils package"
8b556f2... drbd.spec.in: add missing %dir entries
3edb51c... Split kernel module build into separate spec file
dee3f92... drbd.spec.in: bump release number

Commit b974fce fixes the duplicate files issue.

Does this look in better shape now?

Well,

* %description
  - I don't think writing the authors of this software in
    %description is needed.

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

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

* 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

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

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

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

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

* 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)

* sysvinit script treatment
  - Some Requires(post) or so are missing.
  - Would you explain why condrestart is not needed at %postun (on upgrade)?
    https://fedoraproject.org/wiki/Packaging/SysVInitScript#InitscriptScriptlets

  - Please use %{_initddir}
    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.

(In reply to comment #48)
> Well,
> * 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.

So the problem here is not in drbd, but an implementation/integration bug in resource-agents/rgmanager.

To make the long story short, the 2 conflicting files will have to vanish from resource-agents/rgmanager but because of that error it will take a bit of time.

The authoritative package will be -rgmanager from drbd.

Florian was kind enough to give us (upstream for rgmanager/resource-agents) time to fix it properly and avoid an upstream/fedora packaging specific change.

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

(In reply to comment #50)
> http://people.linbit.com/~florian/drbd-8.3.4-10.src.rpm

- This is 404.

(In reply to comment #48)
> > * 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.

- In your sense not only gcc is affected (glibc-headers, make, tar, 
  redhat-rpm-config.... and so on is also needed) and we don't want
  to write such redundant things. 
  We regard gcc and so on are surely installed, and
  actually we recommend to use mock for clean build, which installs
  gcc automatically.
  Note that not a few users forget to install redhat-rpm-config more
  likely than gcc.

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

- It is preferable to write such explanation in the spec file
  and file a bug on RH bugzilla so that we can keep track of it.

> > * 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?

- You don't have to include a document file which is included in a rpm
  which is required by the rpm.

(In reply to comment #51)
> (In reply to comment #50)
> > http://people.linbit.com/~florian/drbd-8.3.4-10.src.rpm
> 
> - This is 404.

I'm an idiot. Sorry about that. Fixed now.

Will address the other comments in a new iteration, along with any other comments you might have about the updated spec.

For -10:

* Source tarball
  - in your srpm differs from what I could download from the URL
    written as Source in your spec:
------------------------------------------------------------
426483 2009-10-06 22:26 drbd-8.3.4.tar.gz
450560 2009-10-20 03:03 drbd-8.3.4-10.src/drbd-8.3.4.tar.gz
------------------------------------------------------------

* -utils <-> -udev dependency
  - utils subpackage contains %post scriptlets:
------------------------------------------------------------
%post utils
chkconfig --add drbd
%if %{without udev}
for i in `seq 0 15` ; do
    test -b /dev/drbd$i || mknod -m 0660 /dev/drbd$i b 147 $i;
done
%endif #without udev
------------------------------------------------------------
    I wonder what happens if udev option is enabled (which
    is the default of this srpm), however sysadmin only installs
    -utils subpackage but does not install -udev subpackage (which
    is possible, according to this spec file).
    In this case udev related script is not installed, and
    /dev/drbd* devices are not created either. Is this expected?

    And, if udev option is enabled (which is the default), does
    this mean that -utils subpackage requires -udev subpackage
    at %post (i.e. -utils subpackage should have
    "Requires(post): %{name}-udev" )?

* %prep <-> %build
  - Forgot to mention that we usually write %configure in
    %build, not in %prep.

* %files
-------------------------------------------------------------
%defattr(-,root,root,-)
%dir %{_var}/lib/%{name}
%config(noreplace) %{_sysconfdir}/drbd.conf

%defattr(-,root,root,-)
%{_mandir}/man8/drbd.8.*
-------------------------------------------------------------
  - The second %defattr is not needed.

* drbd service
-------------------------------------------------------------
[root@localhost Binary]# service drbd status

                --== This is a new installation of DRBD ==--
Please take part in the global DRBD usage count at http://usage.drbd.org.

The counter works anonymously. It creates a random number to identify
your machine and sends that random number, along with 
DRBD's version number, to usage.drbd.org.

The benefits for you are:
 * In response to your submission, the server (usage.drbd.org) will tell you
   how many users before you have installed this version (8.3.4).
 * With a high counter LINBIT has a strong motivation to
   continue funding DRBD's development.
...
...
-------------------------------------------------------------
  - What is this? "service status" should just return the status
    of the service and should not try to execute some other installation
    process.

    ! For reference:
      https://fedoraproject.org/wiki/Packaging/SysVInitScript#Exit_Codes_for_the_Status_Action
      This is just a reference, not following the above exit status
      is not a blocker for the current review, however anyway "service ... status"
      should just return the status.

    Similarly, "service drbd start" should just fail abnormally if some needed
    initialization has not been completed (i.e. "start" command should just
    try to start daemon), for example.

* %changelog
  - Adding %{?dist} at the end of EVR in %changelog is not needed.

(In reply to comment #53)
> For -10:
> 
> * Source tarball
>   - in your srpm differs from what I could download from the URL
>     written as Source in your spec:
> ------------------------------------------------------------
> 426483 2009-10-06 22:26 drbd-8.3.4.tar.gz
> 450560 2009-10-20 03:03 drbd-8.3.4-10.src/drbd-8.3.4.tar.gz
> ------------------------------------------------------------

this was discussed in comment #20 and #23.

Upstream is making a commitment to Fedora by fixing spec file generation to fulfil this review within upstream build and will release a 8.3.5 tarball once this process is completed.
This is a simple catch22.. process completes, upstream releases 8.3.5 final with the changes. Upload here a new srpm/spec/url to final tarball, md5sum will match.

> * %prep <-> %build
>   - Forgot to mention that we usually write %configure in
>     %build, not in %prep.

Question for me, not relevant to the review, is there a specific policy that enforce %configure to be invoked only at %build time?

(In reply to comment #53)
> 
> * -utils <-> -udev dependency
>   - utils subpackage contains %post scriptlets:
> ------------------------------------------------------------
> %post utils
> chkconfig --add drbd
> %if %{without udev}
> for i in `seq 0 15` ; do
>     test -b /dev/drbd$i || mknod -m 0660 /dev/drbd$i b 147 $i;
> done
> %endif #without udev
> ------------------------------------------------------------
>     I wonder what happens if udev option is enabled (which
>     is the default of this srpm), however sysadmin only installs
>     -utils subpackage but does not install -udev subpackage (which
>     is possible, according to this spec file).
>     In this case udev related script is not installed, and
>     /dev/drbd* devices are not created either. Is this expected?

Since you are evidently referring to a udev-enabled target system, then yes it is, and it's completely harmless. The /dev/drbdX device nodes would simply be created by udev on demand. It is merely the convenience symlinks (/dev/drbd/by-res/<resourcename>) that would not be created.


>     And, if udev option is enabled (which is the default), does
>     this mean that -utils subpackage requires -udev subpackage
>     at %post (i.e. -utils subpackage should have
>     "Requires(post): %{name}-udev" )?

No, for the same reason as stated above. I repeat, our udev integration scripts are not essential, in any way whatsoever, for DRBD's functionality. They are merely provided for the user's convenience.

This is actually a typical situation where it would be extremely handy to have universal Suggests or Recommends tags for the packages, as discussed in the recent RPM summit (if I'm not misinformed).


> * %prep <-> %build
>   - Forgot to mention that we usually write %configure in
>     %build, not in %prep.

I got the idea from the cluster-glue package that has just very recently passed package review and has been accepted into Fedora, which uses the exact approach of invoking %configure from %prep. Putting %configure into %prep seems more logical to me than %build.


> * %files
> -------------------------------------------------------------
> %defattr(-,root,root,-)
> %dir %{_var}/lib/%{name}
> %config(noreplace) %{_sysconfdir}/drbd.conf
> 
> %defattr(-,root,root,-)
> %{_mandir}/man8/drbd.8.*
> -------------------------------------------------------------
>   - The second %defattr is not needed.

OK, will be fixed.


> * drbd service
> -------------------------------------------------------------
> [root@localhost Binary]# service drbd status
> 
>                 --== This is a new installation of DRBD ==--
> Please take part in the global DRBD usage count at http://usage.drbd.org.
> 
> The counter works anonymously. It creates a random number to identify
> your machine and sends that random number, along with 
> DRBD's version number, to usage.drbd.org.
> 
> The benefits for you are:
>  * In response to your submission, the server (usage.drbd.org) will tell you
>    how many users before you have installed this version (8.3.4).
>  * With a high counter LINBIT has a strong motivation to
>    continue funding DRBD's development.
> ...
> ...
> -------------------------------------------------------------
>   - What is this? "service status" should just return the status
>     of the service and should not try to execute some other installation
>     process.

Hm. That usage-count thing should really be silenced for service status. Need to talk to Phil and Lars about that.


>     ! For reference:
>      
> https://fedoraproject.org/wiki/Packaging/SysVInitScript#Exit_Codes_for_the_Status_Action
>       This is just a reference, not following the above exit status
>       is not a blocker for the current review, however anyway "service ...
> status"
>       should just return the status.

Point taken.


>     Similarly, "service drbd start" should just fail abnormally if some needed
>     initialization has not been completed (i.e. "start" command should just
>     try to start daemon), for example.
> 
> * %changelog
>   - Adding %{?dist} at the end of EVR in %changelog is not needed.  

I see. Will be fixed.

A fresh iteration of the spec is here:

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

* The license file has been removed from the subpackages dependent on drbd-utils.

* BuildRequires now only contains flex (and udev, unless invoked with --without udev, see comment #20)

* Unnecessary %defattr has been removed.

* %{?dist} has been removed from E-V-R in %changelog.

The %configure invocation is still unchanged (left in %prep) pending your feedback to Fabio's and my comments.

The "service drbd status" issue is also unchanged for the time being, but will be fixed either in the init script itself, or in the binary that the init script invokes. Since you said this was not a blocker, I guess the review can continue in parallel to this being fixed.

Koji builds:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1758850 (f12)
http://koji.fedoraproject.org/koji/taskinfo?taskID=1758855 (f13)

Changelog:
859be71... drbd.spec.in: remove COPYING file from subpackages other than -utils
0d9258e... configure.ac, drbd.spec.in: generate BuildRequires from autoconf
14f3c3d... drbd.spec.in: remove superfluous %defattr
de045ed... Spec files: remove optional %{dist} tag from changelog
5efac56... RPM spec files: bump release number

The service drbd status issue has been fixed with this upstream commit:

http://git.drbd.org/?p=drbd-8.3.git;a=commit;h=4a3b94400f3afe0c864575baca5bfab3e146a2c8

Our usage count facility, regardless of whether it is globally enabled in the configuration file, is now suppressed when invoking "service drbd status". The corner case in which it would have been invoked on status previously is a very limited one; now this behavior is eliminated altogether.

(In reply to comment #55)

> > * %prep <-> %build
> >   - Forgot to mention that we usually write %configure in
> >     %build, not in %prep.
> 
> I got the idea from the cluster-glue package that has just very recently passed
> package review and has been accepted into Fedora, which uses the exact approach
> of invoking %configure from %prep. Putting %configure into %prep seems more
> logical to me than %build.

- Usually we think that
  - %prep should just unpack the tarball and apply some patches
    needed for build process (and maybe sed, iconv, and so on)

  - %build should do things related to building the source
    (usually configure -> make)
    Note that when you want to modify configure option for
    some reason, you have to unpack the source again if you
    write configure in prep. On the other hand when configure is
    executed in %build, you can omit unpacking source by
    $ rpmbuild -bc --short-circuit (by the way if patches are
    modified, usually we have to begin with unpacking source again...)

  - And %install should just install files (if possible, usually
    some modification is needed after installing files)
    Here when we just want to fix %files list or so, we can begin
    with $ rpmbuild -bi --short-circuit .

Now:

-------------------------------------------------------------
NOTE: Before being sponsored:

This package will be accepted with another few work. 
But before I accept this package, someone (I am a candidate) 
must sponsor you.

Once you are sponsored, you have the right to review other 
submitters' review requests and approve the packages formally. 
For this reason, the person who want to be sponsored (like you) 
are required to "show that you have an understanding 
of the process and of the packaging guidelines" as is described
on :
http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored

Usually there are two ways to show this.
A. submit other review requests with enough quality.
B. Do a "pre-review" of other person's review request
   (at the time you are not sponsored, you cannot do
   a formal review)

When you have submitted a new review request or have pre-reviewed other 
person's review request, please write the bug number on this bug report 
so that I can check your comments or review request.

Fedora package collection review requests which are waiting for someone to
review can be checked on my wiki page:
http://fedoraproject.org/wiki/User:Mtasaka#B._Review_request_tickets
(Check "No one is reviewing")

Review guidelines are described mainly on:
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
http://fedoraproject.org/wiki/Packaging/Guidelines
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
------------------------------------------------------------

Note that you seem to be the upstream developer(s) for this
package and you may want to touch this package only, however
still I want to see your pre-review or another review request.

By the way for confirmation would you write who will become
the maintainer of this package on Fedora?

you can do a pre-review in bucardo.

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

(In reply to comment #58)
> (In reply to comment #55)
> 
> > > * %prep <-> %build
> > >   - Forgot to mention that we usually write %configure in
> > >     %build, not in %prep.
> > 
> > I got the idea from the cluster-glue package that has just very recently passed
> > package review and has been accepted into Fedora, which uses the exact approach
> > of invoking %configure from %prep. Putting %configure into %prep seems more
> > logical to me than %build.
> 
> - Usually we think that
>   - %prep should just unpack the tarball and apply some patches
>     needed for build process (and maybe sed, iconv, and so on)
> 
>   - %build should do things related to building the source
>     (usually configure -> make)
>     Note that when you want to modify configure option for
>     some reason, you have to unpack the source again if you
>     write configure in prep. On the other hand when configure is
>     executed in %build, you can omit unpacking source by
>     $ rpmbuild -bc --short-circuit (by the way if patches are
>     modified, usually we have to begin with unpacking source again...)
> 
>   - And %install should just install files (if possible, usually
>     some modification is needed after installing files)
>     Here when we just want to fix %files list or so, we can begin
>     with $ rpmbuild -bi --short-circuit .  

OK. Fixed.

New spec and SRPM are here:
http://people.linbit.com/~florian/drbd.spec
http://people.linbit.com/~florian/drbd-8.3.4-12.src.rpm

Changelog:
b2d294a... RPM spec files: move %configure to %build stage
3ce6270... drbd.spec.in: simplify make invocation in %build
d2b266b... Packaging: bump release number

Koji builds for the -12 SRPM:

http://koji.fedoraproject.org/koji/taskinfo?taskID=1762387 (f12)
http://koji.fedoraproject.org/koji/taskinfo?taskID=1762394 (f13)

( Just to be clear that now I am waiting for your pre-review
  or another review request, and clarifying that who will be
  the maintainer of this package on Fedora )

(In reply to comment #63)
> ( Just to be clear that now I am waiting for your pre-review
>   or another review request, and clarifying that who will be
>   the maintainer of this package on Fedora )  

Very well aware of that, yes. I have offered to do a pre-review on bug 515752. The initial package maintainer will be Phil Reisner (reisner) who is also the original submitter of this review request.

Thanks, I will check the bug when the actual pre-review is done.

Bug 515752 updated with several pre-review comments.

(In reply to comment #66)
> Bug 515752 updated with several pre-review comments.  
Well, as I wrote on the bug, would you write the summary of your
pre-review (especially about what needs fixing)?

Bug 515752 updated with a summary of suggested changes.

Well, okay (however please check my comment)

------------------------------------------------------
  This package (drbd) is APPROVED by mtasaka
------------------------------------------------------

To Phil:
Please follow the procedure written on:
http://fedoraproject.org/wiki/PackageMaintainers/Join
from "Get a Fedora Account".
After you request for sponsorship a mail will be sent to sponsor 
members automatically (which is invisible for you) which notifies 
that you need a sponsor. After that, please also write on
this bug for confirmation that you requested for sponsorship and
your FAS (Fedora Account System) name. Then I will sponsor you.

If you want to import this package into Fedora 10/11/12, you also have
to look at
http://fedoraproject.org/wiki/Infrastructure/UpdatesSystem/Bodhi-info-DRAFT
(after once you rebuilt this package on koji Fedora rebuilding system).

If you have questions, please ask me.

And thanks to Fabio for pre-review.

(In reply to comment #69)

> 
> And thanks to Fabio for pre-review.  

It was a real pleasure to work with you and it´s been good to learn some more things.

(In reply to comment #69)
> Well, okay (however please check my comment)
> 
> ------------------------------------------------------
>   This package (drbd) is APPROVED by mtasaka
> ------------------------------------------------------
> 
> To Phil:
> Please follow the procedure written on:
> http://fedoraproject.org/wiki/PackageMaintainers/Join
> from "Get a Fedora Account".
> After you request for sponsorship a mail will be sent to sponsor 
> members automatically (which is invisible for you) which notifies 
> that you need a sponsor. After that, please also write on
> this bug for confirmation that you requested for sponsorship and
> your FAS (Fedora Account System) name. Then I will sponsor you.
> 

Ok, I assume that "the request for sponsorship" is the request
to join the packager group. I just issued that request.

My FAS name is "reisner".

Thanks!

Okay, now I am sponsoring you. Please follow "Join" wiki again.

New Package CVS Request
=======================
Package Name: drbd
Short Description: Userland tools and scripts for DRBD 
Owners: Philipp Reisner <philipp.reisner>
Branches: F-12
InitialCC:

(In reply to comment #73)
> New Package CVS Request

For Owners, please write FAS account name instead of
your mail address.

New Package CVS Request
=======================
Package Name: drbd
Short Description: Userland tools and scripts for DRBD 
Owners: reisner
Branches: F-12
InitialCC:

(In reply to comment #75)

why not EL-5 too ?

cvs done. 

(likely no EL-5 is due to the kernel there not having the drbd module... )

Please rebuild this also for F-13.
Also for F-12, please submit push request on bodhi:
https://admin.fedoraproject.org/updates/

drbd-8.3.6-1.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/drbd-8.3.6-1.fc12

Package Change Request
======================
Package Name: drbd
New Branches: F-13
Owners: reisner

(In reply to comment #80)
there are no F-13 branch, at this moment F-13 is the devel.

drbd-8.3.6-1.fc12 has been pushed to the Fedora 12 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update drbd'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F12/FEDORA-2009-11357

will you build the devel branch ?

(In reply to comment #83)
> will you build the devel branch ?  

Just building, I am still learning all those Fedora processes...

http://koji.fedoraproject.org/koji/taskinfo?taskID=1801358

BTW, did you set the status of the bug to "new" by intention ?
It was "on_qa"

Closing.

drbd-8.3.6-2.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/drbd-8.3.6-2.fc12

drbd-8.3.6-2.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.