Fedora Merge Review: nut http://cvs.fedora.redhat.com/viewcvs/devel/nut/ Initial Owner: than
(!!) MUST: rpmlint output: **** Review message: W: nut prereq-use fileutils W: nut prereq-use /sbin/chkconfig W: nut prereq-use /sbin/service W: nut buildprereq-use gd-devel W: nut buildprereq-use freetype-devel W: nut buildprereq-use netpbm-devel W: nut buildprereq-use libpng-devel W: nut buildprereq-use net-snmp-devel W: nut buildprereq-use elfutils-devel W: nut buildprereq-use libX11-devel W: nut buildprereq-use libXpm-devel W: nut buildprereq-use libjpeg-devel W: nut buildprereq-use fontconfig-devel W: nut buildprereq-use libusb-devel W: nut prereq-use chkconfig W: nut prereq-use /usr/sbin/useradd W: nut macro-in-%changelog configure - Line: 512 - add --with-statepath and --sysconfdir to %configure (thanks Michael) - Should be: %%configure W: nut patch-not-applied Patch3: nut-2.0.1-bad.patch ******************** (!!) MUST: The package must meet the Packaging Guidelines. **** Review message: - Packages should not use the PreReq and BuildPrereq tags. Use Requires (or Requires(pre),etc.) and BuildRequires instead. ********************
The Release tag should also include the %{?dist} tag. Release: 2%{?dist}
Adding the current maintainer to cc:
OK source files match upstream: $ sha256sum nut-2.6.0.tar.gz* febaa230b6b5f0ad27d780851047527d36c8c7a34e557b3832d6d55174d7a0d5 nut-2.6.0.tar.gz febaa230b6b5f0ad27d780851047527d36c8c7a34e557b3832d6d55174d7a0d5 nut-2.6.0.tar.gz.orig OK package meets naming and versioning guidelines. OK specfile is properly named, is cleanly written and uses macros consistently. The spec file look fine, just two comments: - Please use %global instead of %define, see: https://fedoraproject.org/wiki/Packaging:Guidelines#.25global_preferred_over_.25define - Buildroot tag is no longer required, see: https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag OK dist tag is present. BAD license field matches the actual license. Only GPLv2+ is in the spec file, but the nut-client subpackage holds GPLv3+ files also. I think nut-client should have own license field with dual licensing scenario, see: https://fedoraproject.org/wiki/Packaging:LicensingGuidelines BAD license is open source-compatible. License text included in package. License texts are not included, but are not included in upstream tarball too (although both are mentioned in COPYING - LICENSE-GPL2, LICENSE.GPL3). It would be nice to point upstream to ship them. OK latest version is being packaged. OK BuildRequires are proper. OK compiler flags are appropriate. OK package builds in mock (Rawhide/i686). OK debuginfo package looks complete. BAD rpmlint is silent. $ rpmlint nut.spec nut.spec:177: W: macro-in-comment %{buildroot} nut.spec:177: W: macro-in-comment %{_mandir} nut.spec:195: W: macro-in-comment %{buildroot} nut.spec:320: E: files-attr-not-set 0 packages and 1 specfiles checked; 1 errors, 3 warnings. Warnings are OK, error on line 320 can be easily fixed by moving %doc COPYING line after %defattr(-,root,root) line. $ rpmlint nut-2.6.0-3.fc16.x86_64.rpm nut.x86_64: W: wrong-file-end-of-line-encoding /usr/share/doc/nut-2.6.0/docs/website/css/ie-overrides.css nut.x86_64: W: wrong-file-end-of-line-encoding /usr/share/doc/nut-2.6.0/docs/nut-qa.txt nut.x86_64: E: non-readable /etc/ups/nut.conf 0640L nut.x86_64: E: non-readable /etc/ups/upsd.users 0640L nut.x86_64: W: wrong-file-end-of-line-encoding /usr/share/doc/nut-2.6.0/docs/website/scripts/filter_png.js nut.x86_64: E: non-readable /etc/ups/ups.conf 0640L nut.x86_64: E: non-readable /etc/ups/upsd.conf 0640L nut.x86_64: W: no-manual-page-for-binary blazer_usb nut.x86_64: W: no-manual-page-for-binary upslog nut.x86_64: W: no-manual-page-for-binary newmge-shut nut.x86_64: W: no-manual-page-for-binary skel nut.x86_64: W: no-manual-page-for-binary clone-outlet nut.x86_64: W: no-manual-page-for-binary blazer_ser 1 packages and 0 specfiles checked; 4 errors, 9 warnings. Some manual pages missing (please check it, probably minor utilities, where man page is not needed), some non-readable files for everybody (if it's expected, it should be possible to add these files to the exception list), some files with CRLF line terminators (should be fixed). $ rpmlint nut-client-2.6.0-3.fc16.x86_64.rpm nut-client.x86_64: W: shared-lib-calls-exit /usr/lib64/libupsclient.so.1.0.0 exit.5 nut-client.x86_64: E: non-readable /etc/ups/upssched.conf 0640L nut-client.x86_64: E: non-standard-dir-perm /var/lib/ups 0750L nut-client.x86_64: E: non-readable /etc/ups/upsmon.conf 0640L nut-client.x86_64: E: non-executable-script /usr/lib/python2.7/site-packages/PyNUT.py 0644L /usr/bin/env nut-client.x86_64: W: no-manual-page-for-binary upssched-cmd nut-client.x86_64: W: no-manual-page-for-binary nut-monitor nut-client.x86_64: W: dangerous-command-in-%post install nut-client.x86_64: W: dangerous-command-in-%preun rm nut-client.x86_64: W: incoherent-init-script-name ups ('nut-client', 'nut-clientd') 1 packages and 0 specfiles checked; 4 errors, 6 warnings. Same as above, plus directory and script permission - please take a look if it's correct (directory permission is probably desired - this permission is set in the spec file, I'm not sure about python script). $ rpmlint nut-cgi-2.6.0-3.fc16.x86_64.rpm nut-cgi.x86_64: E: non-readable /etc/ups/upsset.conf 0600L 1 packages and 0 specfiles checked; 1 errors, 0 warnings. $ rpmlint nut-hal-2.6.0-3.fc16.x86_64.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. $ rpmlint nut-devel-2.6.0-3.fc16.x86_64.rpm nut-devel.x86_64: W: no-dependency-on nut/nut-libs/libnut 1 packages and 0 specfiles checked; 0 errors, 1 warnings. $ rpmlint nut-xml-2.6.0-3.fc16.x86_64.rpm nut-xml.x86_64: W: spelling-error %description -l en_US netxml -> netball, nettle, nether 1 packages and 0 specfiles checked; 0 errors, 1 warnings. Almost OK:) OK final provides and requires look sane. N/A %check is present and all tests pass. OK shared libraries are added to the regular linker search paths with proper scriptlets OK owns the directories it creates. OK doesn't own any directories it shouldn't. OK no duplicates in %files. OK file permissions are appropriate. Except rpmlint warning. OK correct scriptlets present. OK code, not content. OK documentation is small, so no -docs subpackage is necessary. There's quite a lot of documentation, maybe it's worth of subpackage? What do you think? OK %docs are not necessary for the proper functioning of the package. OK headers in -devel OK pkgconfig files in -devel OK no libtool .la droppings. OK not a GUI app. OK obsoletes and provides of the obsoleted package are valid
ping?
pong I'll look at this soon, but I have some higher priority tasks right now. Also we are discussing some changes with upstream that will change packaging (probably significantly), so it's not that bad if we wait a little with spec review.
(In reply to comment #4) > The spec file look fine, just two comments: > > - Please use %global instead of %define, see: fixed > - Buildroot tag is no longer required, see: removed > BAD license field matches the actual license. > > Only GPLv2+ is in the spec file, but the nut-client subpackage holds GPLv3+ fixed > BAD license is open source-compatible. License text included in package. > > License texts are not included, but are not included in upstream tarball too > (although both are mentioned in COPYING - LICENSE-GPL2, LICENSE.GPL3). It would > be nice to point upstream to ship them. included rpmlint: I checked all messages it reports (for spec and all rpms). Some of the errors reported here were no longer valid. For others: - W: macro-in-comment - fixed - W: wrong-file-end-of-line-encoding - fixed - E: non-readable ... 0640L - these files contain secrets, permissions are correct - W: no-manual-page-for-binary - a few of them were fixed, there are binaries like blazer_ser and blazer_usb, which have common man page called just blazer. Also there are very small binaries, just drivers, where man page would be bigger than file itself - E: non-executable-script /usr/lib/python2.7/site-packages/PyNUT.py - I have 30 files in that directory on my system and only one fo them is executable, I presume these permissions are correct - W: dangerous-command-in-%post - it's because of non-trivial sysv->systemd migration (services are split differently, different configuration etc..) - W: no-dependency-on nut/nut-libs/libnut - library is in nut-client - W: spelling-error %description - false positive > OK documentation is small, so no -docs subpackage is necessary. > > There's quite a lot of documentation, maybe it's worth of subpackage? What do > you think? Not worth in my opinion. Also (related not only to this comment) there is "unified nut packaging effort" in upstream so nut is packaged the same way in fedora, opensuse, mandriva, debian,... So, for packaging changes in fedora, I'm going to fix only packaging guidelines violations
Is there anything left or can we close this review?
Sorry for (very) late response. rpmlint: ./nut.spec:125: W: macro-in-comment %patch4 ./nut.spec:131: W: macro-in-comment %patch10 ./nut.spec:132: W: macro-in-comment %patch11 ./nut.spec: W: patch-not-applied Patch4: nut-2.6.5-ipmifix.patch Any reason to not apply Patch4? Patches number 10 and 11 are not defined at all, removing comments with them will do no harm. ./nut.spec:387: E: hardcoded-library-path in /lib/systemd/system-shutdown/nutshutdown Isn't it possible to use some rpm macro here? nut-client.x86_64: W: one-line-command-in-%postun /sbin/ldconfig nut-devel.x86_64: E: incorrect-fsf-address /usr/include/nutscan-ip.h Just to let you know about it, really minor issues. Otherwise it looks fine, IMO the review can be closed.
(In reply to Vitezslav Crhonek from comment #9) > Sorry for (very) late response. > > rpmlint: > ./nut.spec:125: W: macro-in-comment %patch4 > ./nut.spec:131: W: macro-in-comment %patch10 > ./nut.spec:132: W: macro-in-comment %patch11 > ./nut.spec: W: patch-not-applied Patch4: nut-2.6.5-ipmifix.patch > > Any reason to not apply Patch4? it was required for new release of freeipmi that broke api (patch), got reverted (disabled patch) and released new version with compatible api (patch no longer needed) I've removed it > Patches number 10 and 11 are not defined at all, removing comments with them > will do no harm. done > ./nut.spec:387: E: hardcoded-library-path in > /lib/systemd/system-shutdown/nutshutdown > > Isn't it possible to use some rpm macro here? there is no usable macro for this, afaik there are only 2 packages using this path, so probably insufficient demand :) > nut-client.x86_64: W: one-line-command-in-%postun /sbin/ldconfig modified > nut-devel.x86_64: E: incorrect-fsf-address /usr/include/nutscan-ip.h I will notify upstream, but won't patch it myself. Not worth it
I agree with your changes and explanations. However, I see commit 4a8e9a8d14e23b1978e8cbca77da3f661924d3d3 (spec cleanup) in master branch, but it just removes nut-2.6.5-ipmifix.patch, no changes to the spec file. The package cannot be built now... You probably didn't commit spec file changes by mistake?
fixed
Thanks, closing.