Fedora Merge Review: lvm2 http://cvs.fedora.redhat.com/viewcvs/devel/lvm2/ Initial Owner: lvm-team
Taking review.
rpmlint output: device-mapper-devel.x86_64: W: no-documentation device-mapper-libs.x86_64: W: no-documentation device-mapper-libs.x86_64: W: obsolete-not-provided device-mapper lvm2.src:31: W: unversioned-explicit-obsoletes lvm lvm2.src:52: W: rpm-buildroot-usage %build make DESTDIR=$RPM_BUILD_ROOT lvm2.x86_64: E: non-standard-executable-perm /sbin/lvm 0555 lvm2.x86_64: E: non-standard-dir-perm /var/lock/lvm 0700 lvm2.x86_64: E: non-standard-executable-perm /sbin/lvmdump 0555 lvm2.x86_64: E: non-standard-executable-perm /sbin/fsadm 0555 lvm2.x86_64: E: non-standard-dir-perm /etc/lvm/backup 0700 lvm2.x86_64: W: hidden-file-or-dir /etc/lvm/cache/.cache lvm2.x86_64: E: non-readable /etc/lvm/cache/.cache 0600 lvm2.x86_64: E: non-standard-dir-perm /etc/lvm/archive 0700 lvm2.x86_64: E: non-standard-dir-perm /etc/lvm/cache 0700 lvm2.x86_64: W: obsolete-not-provided lvm lvm2-cluster.x86_64: E: missing-mandatory-lsb-keyword Description in /etc/rc.d/init.d/clvmd lvm2-cluster.x86_64: E: missing-mandatory-lsb-keyword Short-Description in /etc/rc.d/init.d/clvmd lvm2-cluster.x86_64: W: no-reload-entry /etc/rc.d/init.d/clvmd lvm2-cluster.x86_64: W: incoherent-init-script-name clvmd 7 packages and 0 specfiles checked; 10 errors, 9 warnings. - Any reason why SMP make is not used? You should remove the DESTDIR argument from the build phase. - Modify init script to respect guidelines: https://fedoraproject.org/wiki/Packaging/SysVInitScript MUST: The spec file for the package is legible and macros are used consistently. OK MUST: The package must be named according to the Package Naming Guidelines. OK MUST: The spec file name must match the base package %{name}. OK MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines. OK MUST: The License field in the package spec file must match the actual license. NEEDSFIX - Source code files do not contain license headers. Please have them added. MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. NEEDSFIX - Source url is missing, but source matches upstream. Must add source url. MUST: The package MUST successfully compile and build into binary rpms. OK MUST: The spec file MUST handle locales properly. OK MUST: Optflags are used and time stamps preserved. OK MUST: Packages containing shared library files must call ldconfig. OK MUST: A package must own all directories that it creates or require the package that owns the directory. OK MUST: Files only listed once in %files listings. OK MUST: Permissions on files must be set properly. OK MUST: Clean section exists. OK MUST: Large documentation files must go in a -doc subpackage. OK MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. OK MUST: Header files must be in a -devel package. OK MUST: Static libraries must be in a -static package. OK MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'. OK MUST: If a package contains library files with a suffix then library files ending in .so must go in a -devel package. OK MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency. OK MUST: Packages does not contain any .la libtool archives. OK MUST: Desktop files are installed properly. OK MUST: No file conflicts with other packages and no general names. OK MUST: Buildroot cleaned before install. OK SHOULD: %{?dist} tag is used in release. OK SHOULD: If the package does not include license text(s) as separate files from upstream, the packager should query upstream to include it. OK SHOULD: The package builds in mock. OK
rpmlint is a moving target and generates almost all false positives there. But please propose some (separate) patches (against upstream or the spec file as appropriate) for the few things listed that should be changed!
So someone needs to do: clvmd init script extension DESTDIR/build cleanup - someone already tried looking at that, but hasn't sent patches yet - I don't know how far they got. Which files did you find were missing licence details? Source URL info is already in the repo - so that sounds like a scripting problem if 'make newbase' which we use is not doing the desired thing with it.
Created attachment 337157 [details] Output of Debian's licensecheck.pl
Created attachment 337158 [details] Review fixes to spec file
After application of patch (#6) the rpmlint output drops down to device-mapper-devel.x86_64: W: no-documentation device-mapper-libs.x86_64: W: no-documentation device-mapper-libs.x86_64: W: obsolete-not-provided device-mapper lvm2.x86_64: E: non-standard-executable-perm /sbin/lvm 0555 lvm2.x86_64: E: non-standard-dir-perm /var/lock/lvm 0700 lvm2.x86_64: E: non-standard-executable-perm /sbin/lvmdump 0555 lvm2.x86_64: E: non-standard-executable-perm /sbin/fsadm 0555 lvm2.x86_64: E: non-standard-dir-perm /etc/lvm/backup 0700 lvm2.x86_64: W: hidden-file-or-dir /etc/lvm/cache/.cache lvm2.x86_64: E: non-readable /etc/lvm/cache/.cache 0600 lvm2.x86_64: E: non-standard-dir-perm /etc/lvm/archive 0700 lvm2.x86_64: E: non-standard-dir-perm /etc/lvm/cache 0700 lvm2-cluster.x86_64: E: missing-mandatory-lsb-keyword Description in /etc/rc.d/init.d/clvmd lvm2-cluster.x86_64: E: missing-mandatory-lsb-keyword Short-Description in /etc/rc.d/init.d/clvmd lvm2-cluster.x86_64: W: no-reload-entry /etc/rc.d/init.d/clvmd lvm2-cluster.x86_64: W: incoherent-init-script-name clvmd 7 packages and 0 specfiles checked; 10 errors, 6 warnings. What still has to be fixed is the initrd script. PS. The patch has SMP make enabled, but the build doesn't work with it (at least not on my Quad Core desktop). Works fine using plain 'make', though. Maybe you can fix the missing dependencies in the makefile to get SMP make to work.
That "licensecheck" script is not very helpful! It should be more like rpmlint and say *why* it doesn't like the licence statements! Typos or obsolete versions perhaps?
Source0 with full URL: How does that work? How does rpmbuild use the URL? What if the person running it has no internet connectivity at the time? Does 'make newbase' use this instead of the 'mirrors' file now? If not, is there an enhancement bugzilla filed to integrate the two so the same info is not stored in two places?
We do not 'provide' lvm. lvm is the old obsolete package which does not form part of Fedora 10, but might be present on a system if someone is installing the rpm on an old system and which has to be removed. 'lvm2' version numbers are independent of 'lvm' version numbers.
What does '%{?_smp_mflags}' expand to? Then yes, someone needs to tackle fixing the build to work with it.
(In reply to comment #9) > Source0 with full URL: How does that work? How does rpmbuild use the URL? > What if the person running it has no internet connectivity at the time? Does > 'make newbase' use this instead of the 'mirrors' file now? If not, is there an > enhancement bugzilla filed to integrate the two so the same info is not stored > in two places? IIUC rpmbuild doesn't use the URL per se, but you can get the sources with spectool -g specfile.spec This way you don't have to download the sources by hand whenever a new version is released. (In reply to comment #10) > We do not 'provide' lvm. > lvm is the old obsolete package which does not form part of Fedora 10, but > might be present on a system if someone is installing the rpm on an old system > and which has to be removed. 'lvm2' version numbers are independent of 'lvm' > version numbers. Okay, it seems I have misunderstood the need for Provides. If lvm2 is not compatible with lvm, then the provides line must not be there. (In reply to comment #11) > What does '%{?_smp_mflags}' expand to? Then yes, someone needs to tackle > fixing the build to work with it. It's expanded as -j (number of cores on system), as specified in /usr/lib/rpm/redhat/macros: %_smp_mflags %([ -z "$RPM_BUILD_NCPUS" ] \\\ && RPM_BUILD_NCPUS="`/usr/bin/getconf _NPROCESSORS_ONLN`"; \\\ [ "$RPM_BUILD_NCPUS" -gt 1 ] && echo "-j$RPM_BUILD_NCPUS")
We used to use -j2 by default upstream, then removed it to leave it to the discretion of the build utility, so something must have got broken...
If rpmbuild ignores all except the last path component then that should be OK for us.
Source0 change applied and replacement 'make' line noted in a comment. lvm2-2.02.45-3.fc11
We've removed the DESTDIR build-time usage and fixed a missing build dependency that broke parallel make upstream, so those items can be fixed when we release 2.02.46.
DESTDIR and parallel make fixed in lvm2-2_02_47-1_fc12
Okay. Now: - Time stamps aren't preserved in the install phase, adding INSTALL="install -p" as argument to make install should fix that. Also, use the -p argument when installing the scripts and config files to preserve the time stamp. - Are you sure you want to use %define's instead of %global's? - You can drop the license header on subpackages that have the same license as the main package (cluster and device-mapper). - You shouldn't need %attr(755,root,root) /%{_lib}/libdevmapper.so a plain /%{_lib}/libdevmapper.so should do. If not, it's better to change the permissions in the install phase. rpmlint output: device-mapper-devel.x86_64: W: no-documentation device-mapper-libs.x86_64: W: obsolete-not-provided device-mapper device-mapper-libs.x86_64: W: no-documentation lvm2.src:31: W: unversioned-explicit-obsoletes lvm lvm2.x86_64: W: obsolete-not-provided lvm lvm2.x86_64: E: non-standard-executable-perm /sbin/lvm 0555 lvm2.x86_64: E: non-standard-dir-perm /var/lock/lvm 0700 lvm2.x86_64: E: non-standard-executable-perm /sbin/lvmdump 0555 lvm2.x86_64: E: non-standard-executable-perm /sbin/vgimportclone 0555 lvm2.x86_64: E: non-standard-executable-perm /sbin/fsadm 0555 lvm2.x86_64: E: non-standard-dir-perm /etc/lvm/backup 0700 lvm2.x86_64: W: hidden-file-or-dir /etc/lvm/cache/.cache lvm2.x86_64: E: non-readable /etc/lvm/cache/.cache 0600 lvm2.x86_64: E: non-standard-dir-perm /etc/lvm/archive 0700 lvm2.x86_64: E: non-standard-dir-perm /etc/lvm/cache 0700 lvm2-cluster.x86_64: W: missing-lsb-keyword Required-Start in /etc/rc.d/init.d/clvmd lvm2-cluster.x86_64: W: missing-lsb-keyword Required-Stop in /etc/rc.d/init.d/clvmd lvm2-cluster.x86_64: W: missing-lsb-keyword Default-Stop in /etc/rc.d/init.d/clvmd lvm2-cluster.x86_64: W: missing-lsb-keyword Short-Description in /etc/rc.d/init.d/clvmd lvm2-cluster.x86_64: W: no-reload-entry /etc/rc.d/init.d/clvmd lvm2-cluster.x86_64: W: incoherent-init-script-name clvmd 7 packages and 0 specfiles checked; 9 errors, 12 warnings. Out of these, I think you should fix: - The permissions of the binaries in /sbin, they should probably be 755. - The init file of clvmd must respect https://fedoraproject.org/wiki/Packaging/SysVInitScript
I think install -p is something else we should consider upstream rather than in the spec file. I added the licence headers to subpackages as they seemed to be missing from the RPMs in old builds. But it looks like inheritance works now. (Commenting them out then rebuilding still shows them with rpm -qp RPMS/x86_64/lvm2-cluster-2.02.47-1.fc10.x86_64.rpm --qf '%{LICENSE}') mode 555 vs 755: I've always taken the view that the default permissions for every file on the system should be 000 and then only bits that are absolutely necessary should be added to that. So even 555 is a compromise for me - 011 would often suffice! The proponents of 755 would argue that the '7' is documenting the fact that the kernel permits root to read/write/execute. But I counter that by arguing that root is not generally meant to write to those files - that's solely the job of the package manager software (rpm) for very limited periods of time - so the write bit should not be set. This has the advantage that some binaries (e.g. vim) will issue a warning message ("is read-only (add ! to override)") if root attempts to change these files. In other words not setting the owner write bit makes the system more resilient against sysadmin accidents. I'll chase up the person I asked to deal with the init script and see how they're getting on with it:-)
ping?
ping again?
I think most of that got done by now.
Currently reviewing lvm2-2.02.69-1.fc14.src.rpm. rpmlint output now stands at cmirror.x86_64: E: non-standard-executable-perm /etc/rc.d/init.d/cmirrord 0555L cmirror.x86_64: W: missing-lsb-keyword Default-Stop in /etc/rc.d/init.d/cmirrord cmirror.x86_64: W: no-reload-entry /etc/rc.d/init.d/cmirrord device-mapper.x86_64: E: explicit-lib-dependency libudev device-mapper.x86_64: W: spelling-error %description -l en_US userspace -> user space, user-space, users pace device-mapper-devel.x86_64: W: no-documentation device-mapper-event.x86_64: E: non-standard-executable-perm /sbin/dmeventd 0555L device-mapper-event-devel.x86_64: W: no-documentation device-mapper-event-libs.x86_64: W: spelling-error %description -l en_US libdevmapper -> clapperboard, understrapper, backslapper device-mapper-event-libs.x86_64: W: no-documentation device-mapper-libs.x86_64: W: spelling-error %description -l en_US libdevmapper -> clapperboard, understrapper, backslapper device-mapper-libs.x86_64: W: no-documentation lvm2.src: W: spelling-error Summary(en_US) Userland -> User land, User-land, Sunderland lvm2.src: W: spelling-error %description -l en_US mdadd -> daddy, Madden, Maddox lvm2.src: W: spelling-error %description -l en_US losetup -> lo setup, lo-setup, loser lvm2.src: W: no-cleaning-of-buildroot %install lvm2.src: W: no-buildroot-tag lvm2.x86_64: W: spelling-error Summary(en_US) Userland -> User land, User-land, Sunderland lvm2.x86_64: W: spelling-error %description -l en_US mdadd -> daddy, Madden, Maddox lvm2.x86_64: W: spelling-error %description -l en_US losetup -> lo setup, lo-setup, loser lvm2.x86_64: E: non-standard-executable-perm /sbin/lvmconf 0555L lvm2.x86_64: E: non-standard-executable-perm /sbin/lvm 0555L lvm2.x86_64: E: non-standard-dir-perm /var/lock/lvm 0700L lvm2.x86_64: E: non-standard-executable-perm /etc/rc.d/init.d/lvm2-monitor 0555L lvm2.x86_64: E: non-standard-executable-perm /sbin/lvmdump 0555L lvm2.x86_64: E: non-standard-executable-perm /sbin/vgimportclone 0555L lvm2.x86_64: E: non-standard-executable-perm /sbin/fsadm 0555L lvm2.x86_64: E: non-standard-dir-perm /etc/lvm/backup 0700L lvm2.x86_64: E: non-standard-dir-perm /etc/lvm 0700L lvm2.x86_64: W: hidden-file-or-dir /etc/lvm/cache/.cache lvm2.x86_64: E: non-readable /etc/lvm/cache/.cache 0600L lvm2.x86_64: E: non-standard-dir-perm /etc/lvm/archive 0700L lvm2.x86_64: E: non-standard-dir-perm /etc/lvm/cache 0700L lvm2.x86_64: W: service-default-enabled /etc/rc.d/init.d/lvm2-monitor lvm2.x86_64: W: service-default-enabled /etc/rc.d/init.d/lvm2-monitor lvm2.x86_64: W: no-reload-entry /etc/rc.d/init.d/lvm2-monitor lvm2.x86_64: W: incoherent-init-script-name lvm2-monitor ('lvm2', 'lvm2d') lvm2-cluster.x86_64: W: spelling-error Summary(en_US) userland -> user land, user-land, Sunderland lvm2-cluster.x86_64: E: non-standard-executable-perm /etc/rc.d/init.d/clvmd 0555L lvm2-cluster.x86_64: W: missing-lsb-keyword Default-Stop in /etc/rc.d/init.d/clvmd lvm2-cluster.x86_64: W: incoherent-init-script-name clvmd ('lvm2-cluster', 'lvm2-clusterd') lvm2-devel.x86_64: W: no-documentation lvm2-libs.x86_64: W: no-documentation lvm2-libs.x86_64: E: non-standard-executable-perm /lib64/device-mapper/libdevmapper-event-lvm2snapshot.so 0555L lvm2-libs.x86_64: E: non-standard-executable-perm /lib64/device-mapper/libdevmapper-event-lvm2mirror.so 0555L 13 packages and 0 specfiles checked; 18 errors, 27 warnings. - The explicit libudev require is OK. - The no-buildroot and no-cleaning-of-buildroot warnings are OK, since this version is in rawhide where RPM handles this stuff automatically. (Wouldn't hurt having them, though.) - Although normally services aren't usually enabled by default, I'm sure LVM as a core package is a sane exception to the rule. What I think should still be fixed: - The perms are usually 755 for libraries and executables. Please fix the install commands in the Makefile, or run chmod in %install. - Add the reload entries and the Default-Stop lines to the init scripts. https://fedoraproject.org/wiki/Packaging/SysVInitScript#Required_Actions https://fedoraproject.org/wiki/Packaging/SysVInitScript#.23_Default-Stop:_line Otherwise all of the issues of comment #2 have been addressed. PS. Shouldn't the URL be http://sourceware.org/lvm2/ ?
Oh, I also see that there are some errors in the build caused by missing headers, e.g. lvm_vg.c:15:17: error: lib.h: No such file or directory In file included from lvm_vg.c:16: lvm2app.h:17:26: error: libdevmapper.h: No such file or directory lvm_vg.c:17:25: error: toolcontext.h: No such file or directory lvm_vg.c:18:31: error: metadata-exported.h: No such file or directory lvm_vg.c:19:22: error: archiver.h: No such file or directory lvm_vg.c:20:21: error: locking.h: No such file or directory lvm_vg.c:21:24: error: lvm-string.h: No such file or directory lvm_vg.c:22:22: error: lvmcache.h: No such file or directory lvm_vg.c:23:22: error: metadata.h: No such file or directory Maybe you'll want to have a look at this, as well..
Using program and file names in descriptions is a "spelling-error"??? Another incompletely-implemented feature... The buildroot check message it gives is also inverted for Fedora: we were explicitly instructed to remove it from the package in the past. (This is because they respecified a better default and that meant changing every spec file that had the old one in. Far better to leave it out so the default would only need changing in one place in future.) Perms as I said above I won't be changing unless there is a demonstrable problem with any of them. The URLs point to the same place. The service thing was a bit awkward and it would be nice to work out a better way to handle it, but under some machine configurations it is required to be running (or the machine may freeze) so the script has to run at startup but it only leaves a daemon running on the system if one is actually needed (which addresses the main objection people have to "enabled by default" I think). Default-stop & a dummy reload (we can't reload this daemon without unmounting filesystems using it) need adding. Build errors I hadn't noticed. (Unless it's from the debuginfo generation scripts which have been broken for years.)
Mass reassigning all merge reviews to their component. For more details, see this FESCO ticket: https://fedorahosted.org/fesco/ticket/1269 If you don't know what merge reviews are about, please see: https://fedoraproject.org/wiki/Merge_Reviews How to handle this bug is left to the discretion of the package maintainer.
(In reply to Cole Robinson from comment #26) > How to handle this bug is left to the discretion of the package maintainer. No activity on this report for almost 5 years. Whenever there's a bug in the spec file or packages directly, there's an individual report for that which we are fixing then - it's much better that way than reporting everything in one bug. We can track bugs better by individual reports. I don't see a use case for this compound report any more - closing...
I am not going to report any other bug but I can see this package needs improvement in spec file to also follow current Fedora packaging guidelines. It is sad that FESCo decided to leave these reviews to package maintainer and some package maintainers are not finding this important to fix. Btw, this is merge-review and you can find its information https://fedoraproject.org/wiki/Merge_Reviews