Bug 736062 - Review Request: ppc64-diag - Linux for Power Platform Diagnostics
Summary: Review Request: ppc64-diag - Linux for Power Platform Diagnostics
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Nils Philippsen
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 731419
TreeView+ depends on / blocked
 
Reported: 2011-09-06 14:50 UTC by Karsten Hopp
Modified: 2011-09-12 15:17 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-09-12 15:17:37 UTC
nphilipp: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Karsten Hopp 2011-09-06 14:50:50 UTC
Spec URL: http://people.redhat.com/karsten/ppc64-diag.spec
SRPM URL: http://people.redhat.com/karsten/SRPMS/ppc64-diag-2.4.2-1.src.rpm
Description: 


Hi there !

IBM asks us to include ppc64diag in F16 (on secondary arch PowerPC only).
A function test is most likely impossible as there aren't that many people out there with F16 on a PowerPC machine, but I'd appreciate it if someone could do at least the other checks that don't require access to PPC hardware and review the package. IBM will then do the functional tests afterwards.

Comment 1 Nils Philippsen 2011-09-07 10:31:50 UTC
Items marked "GOOD" or "PASS" fulfil the guidelines or they don't apply to this package.
Items marked "CHECK" aren't covered by the guidelines but you should check and fix them anyway in my opinion.
Items marked "BAD" violate the guidelines in some point and need to be fixed.

- GOOD/CHECK(?): rpmlint gives these warnings/errors on the source RPM (binary arches not checked because this package is exclusively for one secondary arch to which I don't have direct access):

    nils@gibraltar:~/devel/reviews/fedora/ppc64-diag> rpmlint ppc64-diag-2.4.2-1.src.rpm
    ppc64-diag.src: W: spelling-error %description -l en_US servicelog -> service log, service-log, serviceable
    ppc64-diag.src: W: spelling-error %description -l en_US config -> con fig, con-fig, configure
    ppc64-diag.src:43: E: hardcoded-library-path in $RPM_BUILD_ROOT/lib/systemd/system/
    ppc64-diag.src:44: E: hardcoded-library-path in $RPM_BUILD_ROOT/lib/systemd/system/
    ppc64-diag.src:69: E: hardcoded-library-path in /lib/systemd/system/ppc64-diag.service
    1 packages and 0 specfiles checked; 3 errors, 2 warnings.

  -> please check if "servicelog" should rather be spelled "service log" (if it's a special PPC phrase it might be written together, but I don't know that)

  -> "config" is part of a file name mentioned in the description -- does that file need to be mentioned in the package description at all?

  -> /lib/systemd/system is the same on all architectures, but the %_unitdir macro should be used: http://fedoraproject.org/wiki/Packaging:Systemd#Filesystem_locations

- GOOD/CHECK: named according to naming guidelines, but doesn't have a dist tag (cf. http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Using_the_.25.7B.3Fdist.7D_Tag).
- GOOD: spec file name matches package name
- GOOD/CHECK: licensed well (EPL), maybe ping upstream to clarify what is released under which license -- http://sourceforge.net/projects/linux-diag/ mentions that the software is licensed as "Eclipse Public License, GNU General Public License (GPL), GNU Library or Lesser General Public License (LGPL)" but this isn't mentioned anywhere in the tarball (it only has the EPL license)
- GOOD: license field match actual license
- GOOD: package ships license text as %doc
- GOOD: package is written in American English
- GOOD: spec file is legible
- BAD: sources used to build the package must match the upstream source: upstream only has tarballs up to version 2.4.0:

    nils@gibraltar:~/devel/reviews/fedora/ppc64-diag> spectool -g ppc64-diag.spec
    Getting http://downloads.sourceforge.net/project/linux-diag/ppc64-diag/2.4.2/ppc64-diag-2.4.2.tar.gz to ./ppc64-diag-2.4.2.tar.gz
      % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                     Dload  Upload   Total   Spent    Left  Speed
      0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
    curl: (22) The requested URL returned error: 404

- PASS/CHECK: can't check if package successfully builds into binary package in mock (secondary arch only)
- GOOD: package uses ExclusiveArch: to restrict it to the architectures for which it is intended
- PASS/CHECK: can't check if it lists all build requirements
- BAD/CHECK: the systemd service file should probably list the service type as "forking"
- BAD/CHECK: use "make %{?_smp_mflags}" to build in parallel unless this package doesn't support parallel builds
- PASS: no locale specific files
- PASS: no shared library files included
- GOOD: doesn't bundle system libraries
- PASS: package not marked as relocatable
- BAD: package must own all directories that it creates:

  %{_datadir}/ppc64-diag

  --> it'd probably be best/easiest to just list this directory in the file list so all stuff beneath it gets included recursively

- GOOD: doesn't list files more than once
- GOOD/CHECK: permissions on files set properly, but I find it strange that each file is listed with its own %attr() directive, I'd rather ensure that files get installed with correct permissions and get rid of the %attr()s
- BAD: The package doesn't use macros consistently:

    /usr/libexec -> %_libexecdir
    /usr/sbin -> %_sbindir

- GOOD: package contains code
- PASS: no large documentation
- GOOD: %doc doesn't affect runtime
- PASS: no header files
- PASS: no static libraries
- PASS: doesn't contains libraries with suffix
- PASS: no -devel subpackage
- PASS: no libtool archive files on account of no libraries
- PASS: no GUI app
- GOOD: doesn't own files or directories owned by other packages
- GOOD: all file names are valid UTF-8

- CHECK: rather than /usr/bin/yacc, /usr/sbin/lsvpd, require the byacc, lsvpd packages which contain them
- CHECK: the dependency in /sbin/chkconfig seems bogus to me, the chkconfig patch unnecessary as the script isn't used as an init script anymore
- CHECK: some scripts use perl, I'm not sure if this dependency is generated by rpmbuild or should be listed explicitly

Comment 2 Karsten Hopp 2011-09-07 12:15:50 UTC
Updated spec file and srpm/rpm are now available at
http://people.redhat.com/karsten/SPECS/ppc64-diag.spec
http://people.redhat.com/karsten/SRPMS/ppc64-diag-2.4.2-2.fc16.src.rpm
http://people.redhat.com/karsten/RPMS/ppc64-diag-2.4.2-2.fc16.ppc64.rpm


- servicelog is the name of a package, we can't change that easily to service-log. But I've changed the description to 'service log'

- I'd like to keep the pointer to the config file in the description as long as the man pages are missing that describe where the daemon can be configured.

- replaced systemd path with %{_unitdir}

- added dist tag

- license is EPL, http://sourceforge.net/projects/linux-diag/ is a page for several other packages, too. Those have these other licenses.

- the tarball somehow got deleted from sourceforge, I've requested that it gets uploaded again in https://bugzilla.redhat.com/show_bug.cgi?id=731419#c6


- added Type=forking to systemd service file

- added smp flags

- package now owns %{_datadir}/ppc64-diag

- all references for /ppc64-diag/ subdirectories got replaced by %{name}
- fixed usage of %_libexecdir and %_sbindir

- changed file requirements to package requirements

- removed chkconfig dependency, that was for the sysv-initscript

- perl dependency gets added automatically:
# rpm -qp --requires ppc64-diag-2.4.2-2.fc16.ppc64.rpm| grep perl
/usr/bin/perl  
perl(Getopt::Long)


The permissions after a make install are messed up, that's why each file has a %attr:
ppc64-diag.ppc64: E: non-standard-executable-perm /usr/share/ppc64-diag/message_catalog/e1000e 0744L
ppc64-diag.ppc64: E: script-without-shebang /usr/share/ppc64-diag/message_catalog/e1000e
ppc64-diag.ppc64: E: non-standard-executable-perm /usr/share/ppc64-diag/message_catalog/with_regex/cxgb3 0744L
ppc64-diag.ppc64: E: script-without-shebang /usr/share/ppc64-diag/message_catalog/with_regex/cxgb3
ppc64-diag.ppc64: E: non-standard-executable-perm /usr/share/ppc64-diag/message_catalog/with_regex/gpfs 0744L
ppc64-diag.ppc64: E: script-without-shebang /usr/share/ppc64-diag/message_catalog/with_regex/gpfs
ppc64-diag.ppc64: E: non-standard-executable-perm /usr/share/ppc64-diag/ppc64_diag_mkrsrc 0744L
ppc64-diag.ppc64: E: non-standard-executable-perm /usr/share/ppc64-diag/message_catalog/with_regex/e1000e 0744L
ppc64-diag.ppc64: E: script-without-shebang /usr/share/ppc64-diag/message_catalog/with_regex/e1000e
ppc64-diag.ppc64: E: non-standard-executable-perm /usr/share/ppc64-diag/message_catalog/cxgb3 0744L
ppc64-diag.ppc64: E: script-without-shebang /usr/share/ppc64-diag/message_catalog/cxgb3
ppc64-diag.ppc64: E: non-standard-executable-perm /usr/share/ppc64-diag/ppc64_diag_servagent 0744L
ppc64-diag.ppc64: E: non-standard-executable-perm /usr/share/ppc64-diag/message_catalog/exceptions 0744L
ppc64-diag.ppc64: E: script-without-shebang /usr/share/ppc64-diag/message_catalog/exceptions
ppc64-diag.ppc64: E: non-standard-executable-perm /usr/share/ppc64-diag/ppc64_diag_notify 0744L
ppc64-diag.ppc64: E: non-standard-executable-perm /usr/share/ppc64-diag/message_catalog/gpfs 0744L
ppc64-diag.ppc64: E: script-without-shebang /usr/share/ppc64-diag/message_catalog/gpfs
ppc64-diag.ppc64: E: non-standard-executable-perm /usr/share/ppc64-diag/ppc64_diag_setup 0744L
ppc64-diag.ppc64: E: non-standard-executable-perm /usr/share/ppc64-diag/message_catalog/reporters 0744L
ppc64-diag.ppc64: E: script-without-shebang /usr/share/ppc64-diag/message_catalog/reporters
ppc64-diag.ppc64: E: non-standard-executable-perm /usr/share/ppc64-diag/servevent_parse.pl 0744L
ppc64-diag.ppc64: E: script-without-shebang /usr/share/ppc64-diag/servevent_parse.pl
ppc64-diag.ppc64: E: non-standard-executable-perm /usr/share/ppc64-diag/ppc64_diag_migrate 0744L

Comment 3 Nils Philippsen 2011-09-07 13:11:15 UTC
(In reply to comment #2)
> - servicelog is the name of a package, we can't change that easily to
> service-log. But I've changed the description to 'service log'

OK. Rpmlint now only complains about the missing upstream tarball, "config" and that a number of binaries don't have man pages (maybe pass that on to upstream):

    nils@gibraltar:~/devel/reviews/fedora/ppc64-diag> rpmlint ppc64-diag-2.4.2-2.fc16.ppc64.rpm
    ppc64-diag.ppc64: W: spelling-error %description -l en_US config -> con fig, con-fig, configure
    ppc64-diag.ppc64: W: no-manual-page-for-binary convert_dt_node_props
    ppc64-diag.ppc64: W: no-manual-page-for-binary rtas_errd
    ppc64-diag.ppc64: W: no-manual-page-for-binary add_regex
    ppc64-diag.ppc64: W: no-manual-page-for-binary extract_platdump
    ppc64-diag.ppc64: W: no-manual-page-for-binary diag_encl
    1 packages and 0 specfiles checked; 0 errors, 6 warnings.

> - I'd like to keep the pointer to the config file in the description as long as
> the man pages are missing that describe where the daemon can be configured.

OK

> - replaced systemd path with %{_unitdir}

I guess this got pulled in via another dependency, but to be safe systemd-units should be required directly for building (forgot about that above, sorry):

    BuildRequires: systemd-units

> - added dist tag
>
> - license is EPL, http://sourceforge.net/projects/linux-diag/ is a page for
> several other packages, too. Those have these other licenses.

OK

> - the tarball somehow got deleted from sourceforge, I've requested that it gets
> uploaded again in https://bugzilla.redhat.com/show_bug.cgi?id=731419#c6

Do you have an idea how long it will take until the file is available again?

> - added Type=forking to systemd service file
>
> - added smp flags
>
> - package now owns %{_datadir}/ppc64-diag
>
> - all references for /ppc64-diag/ subdirectories got replaced by %{name}
> - fixed usage of %_libexecdir and %_sbindir
>
> - changed file requirements to package requirements
>
> - removed chkconfig dependency, that was for the sysv-initscript
>
> - perl dependency gets added automatically:
> # rpm -qp --requires ppc64-diag-2.4.2-2.fc16.ppc64.rpm| grep perl
> /usr/bin/perl
> perl(Getopt::Long)

OK

> The permissions after a make install are messed up, that's why each file has a
> %attr:
> ppc64-diag.ppc64: E: non-standard-executable-perm
> /usr/share/ppc64-diag/message_catalog/e1000e 0744L
[...]
> ppc64-diag.ppc64: E: non-standard-executable-perm
> /usr/share/ppc64-diag/ppc64_diag_migrate 0744L

OK, please ask upstream to fix that.

I approve this package, just add that systemd-units build dep and ensure that upstream makes the tarball available again soon.

You can now continue with step 8 for contributors in the package review process: http://fedoraproject.org/wiki/Package_Review_Process#Contributor

Comment 4 Karsten Hopp 2011-09-08 10:30:19 UTC
There will be no manpages for the binaries. I've got confirmation in https://bugzilla.redhat.com/show_bug.cgi?id=731419#c8 that those are helper programs that aren't meant for direct usage.


The tarball is back again, the directory with it got accidentally deleted.

I've added the buildrequirement for systemd-units  in ppc64-diag-2.4.2-3.fc16.src.rpm

Comment 5 Karsten Hopp 2011-09-08 11:07:28 UTC
New Package SCM Request
=======================
Package Name: ppc64-diag
Short Description: Linux for Power Platform Diagnostics
Owners: karsten
Branches: f16
InitialCC:

Comment 6 Nils Philippsen 2011-09-08 12:50:11 UTC
(In reply to comment #4)
> There will be no manpages for the binaries. I've got confirmation in
> https://bugzilla.redhat.com/show_bug.cgi?id=731419#c8 that those are helper
> programs that aren't meant for direct usage.

In that case they should probably be moved to %_libexecdir, e.g. to /usr/libexec/ppc64-diag, to satisfy FHS.

Comment 7 Karsten Hopp 2011-09-08 13:31:41 UTC
ok, I'll fix that before I import the package

Comment 8 Gwyn Ciesla 2011-09-09 12:35:40 UTC
Git done (by process-git-requests).


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