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.
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
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
(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
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
New Package SCM Request ======================= Package Name: ppc64-diag Short Description: Linux for Power Platform Diagnostics Owners: karsten Branches: f16 InitialCC:
(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.
ok, I'll fix that before I import the package
Git done (by process-git-requests).