Bug 458826

I was working up a review for this (which isn't really easy) but I noticed that the rpmlint output from the packages I pulled from koji doesn't seem to match the spec.  For example:
  s390utils.src: W: invalid-license GPL
  s390utils.src: W: no-url-tag
and yet there's a URL tag and "GPLv2 and CPL" in the spec you linked.

Looks like I forgot to rebuild the latest version in Koji, it is being built right now - http://s390.koji.fedoraproject.org/koji/taskinfo?taskID=2256.  The differences between the 2 version should be cosmetic only, not functionally and the content of SRPM/Spec URLs is authoritative.

few notes:
- it is a rebase of the most recent RHEL 5 package to the latest sources
- remaining (and relevant) patches will be sent to IBM

I haven't a hope of testing this, but of course I can review for form and rpmlint still provides some useful information.  I guess the first thing I notice, though, is that the packages in your koji instance have an .fc8 dist tag even though they were build on dist-f9.  Any idea what's happened there?

  s390utils.src:134: W: configure-without-libdir-spec
This is OK; it's not an autoconf-generated configure script.
  s390utils.src: E: description-line-too-long   - qetharp to query and purge 
   address data in the OSA and HiperSockets hardware
This goes to exactly 80 characters.  I'm not sure if rpmlint is off by one or if the limit is really 79 or less, but it might be better to wrap it.

  s390utils.s390x: E: executable-sourced-script /etc/profile.d/s390.csh 0755
  s390utils.s390x: E: executable-sourced-script /etc/profile.d/s390.sh 0755
Generally there's no reason for these to be executable, although this wouldn't be the only package to make this error.

  s390utils.s390x: E: init-script-without-chkconfig-postin  
   /etc/rc.d/init.d/dumpconf
  s390utils.s390x: E: init-script-without-chkconfig-postin 
   /etc/rc.d/init.d/mon_procd
  s390utils.s390x: E: init-script-without-chkconfig-postin 
   /etc/rc.d/init.d/mon_statd
  s390utils.s390x: E: init-script-without-chkconfig-preun 
   /etc/rc.d/init.d/dumpconf
  s390utils.s390x: E: init-script-without-chkconfig-preun 
   /etc/rc.d/init.d/mon_procd
  s390utils.s390x: E: init-script-without-chkconfig-preun 
   /etc/rc.d/init.d/mon_statd
Seems like the scriptlets to add these services are missing completely.

  s390utils.s390x: E: non-readable /etc/udev/rules.d/56-dasd.rules 0700
  s390utils.s390x: E: non-readable /etc/udev/rules.d/56-zfcp.rules 0700
  s390utils.s390x: E: script-without-shebang /etc/udev/rules.d/56-dasd.rules
  s390utils.s390x: E: script-without-shebang /etc/udev/rules.d/56-zfcp.rules
Any reason for the udev rules to not be 644 like the rest of them?

There are several "non-readable" and "non-standard-executable-perm" complaints form the main package.  I believe these are OK.

  s390utils.s390x: E: subsys-not-used /etc/rc.d/init.d/dumpconf
  s390utils.s390x: E: subsys-not-used /etc/rc.d/init.d/mon_procd
  s390utils.s390x: E: subsys-not-used /etc/rc.d/init.d/mon_statd
Not sure what's up here.  None of these drop locks in /var/lib/subsys, which is generally expected of daemons.  I'm not sure what to make of these, and the wiki is offline so I can't get to the initscript guidelines.

  s390utils.s390x: W: file-not-utf8 /usr/share/doc/s390utils-1.7.0/README
Should be converted.

  s390utils.s390x: W: no-soname /usr/lib64/src_vipa.so
I'm not sure what this is about; it could even be an artifact of running rpmlint on an i386 machine.  Also, I think some would complain about this being in _libdir when nothing will ever link against it.

  s390utils.s390x: W: service-default-enabled /etc/rc.d/init.d/dumpconf
  s390utils.s390x: W: service-default-enabled /etc/rc.d/init.d/mon_procd
  s390utils.s390x: W: service-default-enabled /etc/rc.d/init.d/mon_statd
Services shouldn't be enabled by default without a good reason.  Is there one?

We chatted about the LD_PRELOAD bit on IRC; I just don't understand why LD_LIBRARY_PATH needs to be modified.

*** Bug 226385 has been marked as a duplicate of this bug. ***

So it's been nearly five months since my comments; did you want to progress with this review?  I'll close it soon if there's no response.

There was done quite a lot of work during the time, but the koji hub was broken for few months so there was no way to publicly check whether the package builds.

You can find a preliminary package at https://s390.koji.fedoraproject.org/koji/taskinfo?taskID=42 and I will post next official review candidate during the next days.

Updated Spec URL: http://fedora.danny.cz/s390/s390utils.spec
Updated SRPM URL: http://fedora.danny.cz/s390/s390utils-1.8.0-4.el5.src.rpm

ChangeLog:
- update to 1.8.0
- cleanup for Fedora compliance
- removed setting of LD_LIBRARY_PATH in the src_vipa.sh script, even the author didn't remember why it was there, but it was probably related multilib

Koji scratch build:
https://s390.koji.fedoraproject.org/koji/taskinfo?taskID=433

commented rpmlint output:
s390utils-base.s390x: W: non-conffile-in-etc /etc/profile.d/s390.sh
s390utils-base.s390x: W: non-conffile-in-etc /etc/profile.d/s390.csh
 => no problem

s390utils-base.s390x: W: no-soname /usr/lib64/src_vipa.so
 => no soname is passed to the linker, it is a preload-only library

s390utils-base.s390x: W: incoherent-init-script-name dumpconf
s390utils-cpuplugd.s390x: W: incoherent-init-script-name cpuplugd
s390utils-mon_statd.s390x: W: incoherent-init-script-name mon_statd
 => result of using variable for service name in the scripts

s390utils-libzfcphbaapi.s390x: W: shared-lib-calls-exit /usr/lib64/libzfcphbaapi.so.0.0.4 exit
 => it is author's intention to call exit

s390utils.s390x: E: devel-dependency s390utils-libzfcphbaapi-devel
s390utils.s390x: E: no-binary
 => s390utils is a meta-package that installs all subpackages to simulate the old s390utils package

A few additional comments:

Is there really no download location for the three tarballs?  If there's any way to get them, you need to either use full URLs on the Source: lines or, if the URLs are weird and RPM can't handle them, include them as comments.  See http://fedoraproject.org/wiki/Packaging/SourceURL.

In a multiple license scenario, there needs to be some documentation of which parts of the package are under which license.  See http://fedoraproject.org/wiki/Packaging/LicensingGuidelines

I'm not really sure there's any point in having "Linux -" in those summary entries.  Isn't it a bit obvious?

The compiler ends up being called with -O3.  I'm not sure this is a big deal, really, but generally we like to see the default set of compiler flags applied.

I'm pretty sure that everything else about this package is OK, but there's a lot there.

(In reply to comment #8)
> A few additional comments:
> 
> Is there really no download location for the three tarballs?  If there's any
> way to get them, you need to either use full URLs on the Source: lines or, if
> the URLs are weird and RPM can't handle them, include them as comments.  See
> http://fedoraproject.org/wiki/Packaging/SourceURL.

Direct URLs for the sources are added with user-clickable URLs in comments.

> In a multiple license scenario, there needs to be some documentation of which
> parts of the package are under which license.  See
> http://fedoraproject.org/wiki/Packaging/LicensingGuidelines

Each subpackage should have proper license tag and I will work with the IBM developers on a licensing overview for the individual utilities.
 
> I'm not really sure there's any point in having "Linux -" in those summary
> entries.  Isn't it a bit obvious?
> 
> The compiler ends up being called with -O3.  I'm not sure this is a big deal,
> really, but generally we like to see the default set of compiler flags applied.

Thanks for catching this, parts other than the s390-tools were even built without our CFLAGS.

> I'm pretty sure that everything else about this package is OK, but there's a
> lot there.  

yes, it is ...


Updated Spec URL: http://fedora.danny.cz/s390/s390utils.spec
Updated SRPM URL: http://fedora.danny.cz/s390/s390utils-1.8.0-5.el5.src.rpm

Koji scratch build:
http://s390.koji.fedoraproject.org/koji/taskinfo?taskID=23348

I don't think there is anything else I can reasonably check.  I didn't paste in a run through my usual checklist but I did look over everything that I could actually check.

There's one issue I see, which I don't think is major: the license on the metapackage is a bit confusing, because it only contains a single text file which isn't itself "GPLv2 and GPLv2+ and CPL".  I don't really know what's appropriate, but I don't think that using the union of the licenses of the packages that will be pulled in is any worse than saying "Public Domain" or whatever.

There's also the interesting question of whether LD_PRELOAD is considered "linking" and how that intersects with the incompatibility of the CPL with other licenses.  I don't think it matters for this package because, among the binaries included in this package, there's no GPL/CPL linking going on as far as I can tell.

Anyway, this package looks vastly better than the original, and I don't see anything else to complain about.  Thanks for your patience.

APPROVED

(In reply to comment #10)
> I don't think there is anything else I can reasonably check.  I didn't paste in
> a run through my usual checklist but I did look over everything that I could
> actually check.

We hope to have Fedora/s390x installable during late April or early May. And with the Hercules emulator anybody can try it.
 
> There's one issue I see, which I don't think is major: the license on the
> metapackage is a bit confusing, because it only contains a single text file
> which isn't itself "GPLv2 and GPLv2+ and CPL".  I don't really know what's
> appropriate, but I don't think that using the union of the licenses of the
> packages that will be pulled in is any worse than saying "Public Domain" or
> whatever.
>
> There's also the interesting question of whether LD_PRELOAD is considered
> "linking" and how that intersects with the incompatibility of the CPL with
> other licenses.  I don't think it matters for this package because, among the
> binaries included in this package, there's no GPL/CPL linking going on as far
> as I can tell.

I see you have already opened that on fedora-legal, thanks.

> Anyway, this package looks vastly better than the original, and I don't see
> anything else to complain about.  Thanks for your patience.

Well, it's rather me who should thank for patience.

> APPROVED  

Thanks for the review.

New Package CVS Request
=======================
Package Name: s390utils
Short Description: Utilities and daemons for IBM System/z
Owners: sharkcz
Branches:

cvs done.

looks like the F-11 branch wasn't created during the mass branching

Package Change Request
======================
Package Name: s390utils
New Branches: F-11
Owners: sharkcz

cvs done.

imported and built