This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 458826 - Review Request: s390utils - Linux/390 specific utilities
Review Request: s390utils - Linux/390 specific utilities
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Extras Quality Assurance
:
: 226385 (view as bug list)
Depends On:
Blocks: 459357 462976
  Show dependency treegraph
 
Reported: 2008-08-12 11:07 EDT by Dan Horák
Modified: 2009-04-20 10:12 EDT (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-04-20 10:12:23 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
tibbs: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Dan Horák 2008-08-12 11:07:43 EDT
Spec URL: http://fedora.danny.cz/s390/s390utils.spec
SRPM URL: http://fedora.danny.cz/s390/s390utils-1.7.0-1.el5.src.rpm

Description:
This package contains utilities related to Linux for S/390.
The most important programs contained in this package are:

  - The cmstools suite to list, check, copy and cat files from a CMS volume.
  - chccwdev, a script to generically change attributes of a ccw device.
  - dasdfmt, which is used to low-level format eckd-dasds with
    either the classic linux disk layout or the new z/OS
    compatible disk layout.
  - dasdview, which displays DASD and VTOC information and dumps the content
    of a DASD to the console.
  - fdasd, which is used to create or modify partitions on
    eckd-dasds formatted with the z/OS compatible disk layout.
  - osasnmpd, a subagent for net-snmp to access the OSA hardware.
  - qetharp to query and purge address data in the OSA and HiperSockets hardware
  - qethconf to configure IBM QETH function IPA, VIPA and Proxy ARP.
  - src_vipa.sh to start applications using VIPA capabilities
  - tunedasd, a tool to adjust tunable parameters on DASD devices
  - vmconvert, a tool to convert vm dumps to lkcd compatible dumps.
  - vmcp, a tool to send CP commands from a Linux guest to the VM.
  - zipl, which is used to make either dasds or tapes bootable
    for system IPL or system dump.
  - zdump, which is used to retrieve system dumps from either
    tapes or dasds.

scratch build at http://s390.koji.fedoraproject.org/koji/taskinfo?taskID=2254
Comment 1 Jason Tibbitts 2008-08-12 13:54:30 EDT
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.
Comment 2 Dan Horák 2008-08-12 14:21:06 EDT
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
Comment 3 Jason Tibbitts 2008-08-16 12:29:34 EDT
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.
Comment 4 Jason Tibbitts 2008-11-06 21:51:34 EST
*** Bug 226385 has been marked as a duplicate of this bug. ***
Comment 5 Jason Tibbitts 2009-03-07 17:39:37 EST
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.
Comment 6 Dan Horák 2009-03-08 11:47:20 EDT
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.
Comment 7 Dan Horák 2009-03-13 06:23:00 EDT
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@GLIBC_2.2
 => 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
Comment 8 Jason Tibbitts 2009-03-24 21:51:56 EDT
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.
Comment 9 Dan Horák 2009-03-25 08:08:51 EDT
(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
Comment 10 Jason Tibbitts 2009-03-25 19:44:19 EDT
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
Comment 11 Dan Horák 2009-03-26 07:20:27 EDT
(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.
Comment 12 Dan Horák 2009-03-26 07:24:14 EDT
New Package CVS Request
=======================
Package Name: s390utils
Short Description: Utilities and daemons for IBM System/z
Owners: sharkcz
Branches:
Comment 13 Kevin Fenzi 2009-03-27 16:27:53 EDT
cvs done.
Comment 14 Dan Horák 2009-04-17 16:14:41 EDT
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
Comment 15 Kevin Fenzi 2009-04-17 17:25:41 EDT
cvs done.
Comment 16 Dan Horák 2009-04-20 10:12:23 EDT
imported and built

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