Bug 226209

(!!) 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.