Bug 226111 - Merge Review: lvm2
Merge Review: lvm2
Status: CLOSED NOTABUG
Product: Fedora
Classification: Fedora
Component: lvm2 (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Susi Lehtola
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 14:36 EST by Nobody's working on this, feel free to take it
Modified: 2015-02-13 00:28 EST (History)
14 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2015-02-12 06:10:25 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Output of Debian's licensecheck.pl (16.55 KB, application/octet-stream)
2009-03-29 06:10 EDT, Susi Lehtola
no flags Details
Review fixes to spec file (1.75 KB, patch)
2009-03-29 06:27 EDT, Susi Lehtola
no flags Details | Diff

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 14:36:01 EST
Fedora Merge Review: lvm2

http://cvs.fedora.redhat.com/viewcvs/devel/lvm2/
Initial Owner: lvm-team@redhat.com
Comment 1 Susi Lehtola 2009-03-28 14:04:46 EDT
Taking review.
Comment 2 Susi Lehtola 2009-03-28 14:24:36 EDT
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
Comment 3 Alasdair Kergon 2009-03-28 22:23:51 EDT
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!
Comment 4 Alasdair Kergon 2009-03-28 22:54:18 EDT
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.
Comment 5 Susi Lehtola 2009-03-29 06:10:15 EDT
Created attachment 337157 [details]
Output of Debian's licensecheck.pl
Comment 6 Susi Lehtola 2009-03-29 06:27:23 EDT
Created attachment 337158 [details]
Review fixes to spec file
Comment 7 Susi Lehtola 2009-03-29 06:30:43 EDT
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.
Comment 8 Alasdair Kergon 2009-03-30 08:25:30 EDT
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?
Comment 9 Alasdair Kergon 2009-03-30 08:30:03 EDT
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?
Comment 10 Alasdair Kergon 2009-03-30 08:34:07 EDT
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.
Comment 11 Alasdair Kergon 2009-03-30 08:35:32 EDT
What does '%{?_smp_mflags}' expand to?  Then yes, someone needs to tackle fixing the build to work with it.
Comment 12 Susi Lehtola 2009-03-30 10:03:02 EDT
(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")
Comment 13 Alasdair Kergon 2009-03-30 13:04:33 EDT
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...
Comment 14 Alasdair Kergon 2009-03-30 13:05:48 EDT
If rpmbuild ignores all except the last path component then that should be OK for us.
Comment 15 Alasdair Kergon 2009-03-30 18:21:39 EDT
Source0 change applied and replacement 'make' line noted in a comment.

lvm2-2.02.45-3.fc11
Comment 16 Alasdair Kergon 2009-04-08 14:17:36 EDT
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.
Comment 17 Alasdair Kergon 2009-05-22 14:04:05 EDT
DESTDIR and parallel make fixed in lvm2-2_02_47-1_fc12
Comment 18 Susi Lehtola 2009-05-22 16:23:10 EDT
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
Comment 19 Alasdair Kergon 2009-05-22 21:40:49 EDT
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:-)
Comment 20 Susi Lehtola 2009-08-05 07:23:16 EDT
ping?
Comment 21 Susi Lehtola 2010-01-01 17:46:14 EST
ping again?
Comment 22 Alasdair Kergon 2010-07-05 08:04:51 EDT
I think most of that got done by now.
Comment 23 Susi Lehtola 2010-07-05 13:04:40 EDT
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/ ?
Comment 24 Susi Lehtola 2010-07-05 13:10:46 EDT
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..
Comment 25 Alasdair Kergon 2010-07-05 15:14:58 EDT
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.)
Comment 26 Cole Robinson 2015-02-11 15:37:40 EST
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.
Comment 27 Peter Rajnoha 2015-02-12 06:10:25 EST
(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...
Comment 28 Parag AN(पराग) 2015-02-13 00:28:11 EST
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

Note You need to log in before you can comment on or make changes to this bug.