Fedora Merge Review: mdadm http://cvs.fedora.redhat.com/viewcvs/devel/mdadm/ Initial Owner: dledford
Quick pre-review of release 2.6.1-2.fc7 * RPM name is OK * Source mdadm-2.6.1.tgz is the same as upstream * This is the latest version * Builds fine in mock * License GPL, is OK Needs work: * BuildRoot should be %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n) (wiki: Packaging/Guidelines#BuildRoot) * Missing SMP flags. If it doesn't build with it, please add a comment (wiki: Packaging/Guidelines#parallelmake) * Spec file: there is a "/sbin" in make install which should be replaced with RPM macros (%_sbindir) (wiki: Packaging/Guidelines#macros) * The BuildRoot must be cleaned at the beginning of %install * make install (static ?) -- compiles stuff but does not use rpm_opt_flags; why is it not compiled in %build ? * Timestamps are not preserved for man pages, mdadm.conf-example and init.d/mdmonitor (you should probably use install -p) * Missing dependancies on chkconfig and service for %post / %preun / %postun * mdadm service is enabled by default in /etc/rc.d/init.d/mdmonitor. Fedora policy recommends services to be off by default and allow the admin to enable them at will * Most of the ANNOUNCE files could be dropped. The useful information is available in the Changelog file * In %Changelog, the line with 2.5.0-6 (which should have been 1.5.0-6) contains an unescaped macro (%postun) * Should require /usr/sbin/sendmail or maybe MTA, not smtpdaemon. See also DSendmail="/usr/sbin/sendmail -t" in the Makefile
BuildRoot fixed Missing SMP flags fixed /sbin != %{_sbindir}, and this package *really* needs to be in /sbin, not /usr/sbin Cleaning of buildroot done The make during build was due to building things out of order and the Makefile was rebuilding mdadm because the object files were newer than the mdadm binary. Make mdadm the last thing to be built during %build solves the issue. Timestamps for man pages and mdmonitor fixed, for all the others they are done as part of the %doc macro, so if that gets them wrong then there's not much to do (although viewing the output shows %doc is using -p, so I suspect their timestamps are actually correct). Dependencies done. mdadm is a reasonable exception to the default off policy. It exits immediately if there is nothing for it to monitor, and if there is something for it to monitor, it's responsible for notifying the admin of failure events in their hardware that might compromise their raid subsystem integrity. Announce files dropped. Changelog fixed As for sendmail, don't all the various MTA alternatives that provide smtpdaemon also provide a sendmail compatibility link via alternatives?
WRT smtpdaemon: please read this thread https://www.redhat.com/archives/fedora-devel-list/2007-April/msg00377.html The problem is not that all MTA alternatives that provide smtpdaemon also provide a sendmail compatibility link but that mdadm really needs JUST that compatibility link. We could say that by mere coincidence some packages which provide /usr/sbin/sendmail also provide smtpdaemon. In my case, I have added "Provides: smtpdaemon" to ssmtp ONLY to satisfy mdadm and mailx who do not really need it [*] /sbin: You are right, I apologize. I was thinking that %{_sbindir}= /sbin Full review coming soon. First glance almost everything seems OK. [*] Once this review is done I'll file bugs against all other versions of mdadm (RHEL 3/4/5, FC5/6) to have Requires:smtpdaemon replaced.
rpmlint checks: [wolfy@wolfy64 mdadm]$ rpmlint mdadm-2.6.1-3.fc7.src.rpm W: mdadm summary-not-capitalized mdadm controls Linux md devices (software RAID arrays) -> ignorable W: mdadm strange-permission mdmonitor.init 0755 -> including the file as 644 and setting it via install -p at 0755 would probably make rpmlint happier. not a big deal anyway W: mdadm unversioned-explicit-obsoletes mdctl W: mdadm unversioned-explicit-obsoletes raidtools -> As far as I have understood, these tools are gone for good, so this could be OK If very very sure that none of these will ever come back, it's OK, but most probably a versioning measure should be put in place W: mdadm mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 7) -> cosmetic, please fix before uploading to CVS [wolfy@wolfy64 mdadm]$ rpmlint mdadm-2.6.1-3.fc7.x86_64.rpm W: mdadm summary-not-capitalized mdadm controls Linux md devices (software RAID arrays) ->ignorable E: mdadm obsolete-not-provided mdctl E: mdadm obsolete-not-provided raidtools -> see above E: mdadm non-standard-dir-perm /var/run/mdadm 0700 -> security measure? W: mdadm service-default-enabled /etc/rc.d/init.d/mdmonitor -> ignorable per comment #2 W: mdadm incoherent-init-script-name mdmonitor -> given my mdadm/mdmonitor change. ignorable GOOD - package meets naming guidelines - package meets packaging guidelines [*] - license (GPL ) OK, text in %doc, matches source - spec file legible, in am. english - source matches upstream, sha1sum c0f523853a9c816986c24c699fa80b2c02336f3d mdadm/devel/mdadm-2.6.1.tgz - no locales - not relocatable - owns all files/directories that it creates, does not take ownership of foreign files/dirs - no duplicate files - permissions ok - %clean ok - macro use consistent - code, not content - no need for separate package for docs - nothing in %doc affects runtime - not a GUI, so no need for .desktop file - no .pc, .la files - sciptlets are sane, including removal of previously existing but no longer needed mdmpd - final provides are sane: [wolfy@wolfy64]$ rpm -q --provides mdadm mdadm = 2.6.1-3.fc7 - final requires are mostly sane: [wolfy@wolfy64]$ rpm -q --requires mdadm|sort -u /bin/bash /bin/sh libc.so.6()(64bit) libc.so.6(GLIBC_2.2.5)(64bit) libc.so.6(GLIBC_2.3.3)(64bit) libc.so.6(GLIBC_2.3.4)(64bit) libc.so.6(GLIBC_2.3)(64bit) libc.so.6(GLIBC_2.4)(64bit) rpmlib(CompressedFileNames) <= 3.0.4-1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1 rtld(GNU_HASH) /sbin/chkconfig /sbin/service smtpdaemon [*] There is one static compiled binary but it is needed so this should be considered an exception to normal policy of a separate -static package. SHOULD - builds in mock/devel/x86_64 - works as advertised Recommended fixes: a) timestamp is not preserved for /usr/share/doc/mdadm-2.6.1/mdadm.conf-example: -rw-r--r-- 1 root root 2180 Apr 11 19:16 one can see that this is the build time, not the file time in the tar ( -rw-r--r-- 1 wolfy wolfy 2194 Jun 20 2006) The difference comes from mdadm-1.5.0-email.patch; I suggest "touch -r" using one of the other doc files as reference b) %defattr(-,root,root,-) instead of %defattr(-,root,root) Only remaining issue is the "smtpdaemon" Requires which definitely should be replaced either by the virtual provide MTA or (better yet) by "/usr/sbin/sendmail"
sorry for typo, "given my mdadm/mdmonitor change" should have been "given by ... "
W: mdadm unversioned-explicit-obsoletes mdctl W: mdadm unversioned-explicit-obsoletes raidtools -> As far as I have understood, these tools are gone for good, so this could be OK If very very sure that none of these will ever come back, it's OK, but most probably a versioning measure should be put in place These are most definitely gone forever. The ioctls that the raidtools use are being phased out of the kernel entirely, and mdctl was basically the first version of mdadm and the name was changed later. However, just in the last month I had to do a new update to raidtools for a bug fix for a commercial package that depended on raidtools, so I'm leary of hard coding the values since they *might* change on something like an older RHEL but we still want them to be obsoleted on upgrade to later RHEL or Fedora. Tab/spaces fixed. Changed summary and description so they start capitalized. In regards to /var/run/mdadm (I didn't make the change and don't fully know why it was necessary), here's the changelog: * Fri Jul 30 2004 Dan Walsh <dwalsh> 1.5.0-11 - Create a directory /var/run/mdadm to contain mdadm.pid - This cleans up SELinux problem I'm guessing that the permissions on the dir are to avoid possible cross context contamination since mdadm has it's own context in SELinux policy. As for the mdadm.conf-example timestamp, you're right that it's because of the email patch. But, that then raises the question of whether or not the timestamp should be preserved since the file no longer matches the original tarball file. The patch was originally done because the default install of mdadm copied the mdadm.conf-example to /etc and we wanted a working MAILADDR. However, now a days, Anaconda custom writes the mdadm.conf on install, so we don't even package an /etc/mdadm.conf file in the rpm. As a result, it's safe to drop the email patch entirely, so we will no longer be modifying the file, and the timestamp should come out correct now. [*] There is one static compiled binary but it is needed so this should be considered an exception to normal policy of a separate -static package. Actually, there are two: mdadm.static and mdassemble.static (we don't even include the shared version of mdassemble). However, these are both intended for use by mkinitrd to create initrd images and therefore *need* to be static, so I do think they both warrant the exception status. I changed the permission on the repo copy of the init script. It was already being installed with -m 0755 in the %install, the odd permissions were just a relic. Changed Requires: smptdaemon to /usr/sbin/sendmail New build currently in process. 2.6.1-4.fc7 is the tag.
All problems fixed, explanations are logical. Package is APPROVED. Thank you, Doug, for changing the Provides.
Thank you for reviewing. I'll go ahead and build this same package for FC5/FC6.
Doug, could you please close this ticket when you have 30 secs?
Sure, didn't know I was supposed to ;-)