Bug 193712

Review for this package:-
      Mock Build Results for i386 
      - Successfully built for i386  
MUST Items:
     - MUST: rpmlint shows no error 
     - MUST: The package is named according to the Package Naming Guidelines.
     - MUST: The spec file name matching the base package sos, in the format
sos.spec
      - MUST: This package meets the Packaging Guidelines.
      - MUST: The package is licensed with an open-source compatible license GPL.
      - MUST: The License field in the package sos.spec file NOT included in
tarball.
      - MUST: The sources used to build the package matches the upstream source,
as provided in the spec URL. md5sum is correct.
      - MUST: This package owns all directories that it creates. 
      - MUST: This package did not contain any duplicate files in the %files
listing.
      - MUST: This package  have a %clean section, which contains %{__rm} rm -rf
${RPM_BUILD_ROOT}.
      - MUST: This package used macros.
      - MUST: Document files are included like README.
    
 Also,     
       * This package also followed optimized .pyo files installation successfully.
       * You have followed Python Packaging Guidelines for installing module in
pythin_sitelib also.

What you need to do is INCLUDE 
       -LICENSE file
       -Source tarball URL
       * UPDATE tarball and SPEC file and reupload your package.        

Howdy, Steve.  Since this is your first Extras package, I did a bit of searching
but didn't see any other reviews or package submissions by you.  Might I suggest
that you take a look at http://fedoraproject.org/wiki/Extras/HowToGetSponsored
for some tips.  Basically, a sponsor has to decide whether or not it looks as if
you understand the packaging guidelines; a single package submission is often
not enough for us to determine that.  Have you commented on other reviews? 
Since you're a Red Hat employee, do you maintain any packages in Core that we
might look at?  Feel free to let us know.

Now, some comments on the package at hand.  From the above comments one might
thing that everything is smooth sailing, but I had some trouble.  In fact, the
package doesn't build for me in mock.  I get down to the end and then:

Processing files: sos-0.1-5.fc6
error: File not found:
/var/tmp/sos-0.1-5.fc6-root-mockbuild/usr/lib64/python2.4/site-packages/sos
plus a bunch of cascading errors.  The cause seems to be simple; this is a
noarch package, but you specify %{python_sitearch} for your files.  On i386 the
locations happen to be identical, but on x86_64 arch-specific libraries go under
/usr/lib64.  Use %{python_sitelib} instead; things will build then.

Some other issues:

You shouldn't use Vendor:.

There's no reason for Provides: sos; a package automatically provides itself. 
This causes the following rpmlint error:
E: sos useless-explicit-provides sos

Don't indent your %description.

I'm assuming you're the upstream for this and that there's no location where the
source tarball can be downloaded directory.  This is acceptable.

BR: python is redundant; it's already in the buildroot.  FC3 and later all have
the required python version so there won't be any problems with the version.

Requires: python is also redundant; RPM will find the python(abi) dependency itself.

You obsolete sysreport, but don't provide it.  Generally obsoletes are provided
so that people can still install under the old names.  However, if the obsoleted
package version was never in Core or Extras then this is acceptable (and you
should consider just removing the obsolete).  This causes the following rpmlint
error:
E: sos obsolete-not-provided sysreport

rpmlint also complains about many of the python source files:
E: sos non-executable-script
/usr/lib/python2.4/site-packages/sos/plugins/general.py 0644
(and so on).  The problem is that these files start with
#!/usr/bin/env python
but they're not actually scripts.  Are those files supposed to do anything if
you run them?  If so, they should be executable.  If not, they shouldn't have
the shebang line.  Python programs seem to do this often but I haven't ever
understood why.

Note that rpm will byte-compile and optomize every single .py file it finds. 
This results in your packaging the .pyo files, which shouldn't be packaged. 
Generally you should %ghost these in the file list.

On behalf of Steve C:

I've just rolled new packages with corrections that Steve made in the source;
the only thing I can see from the above list is that the .pyo files are still
packaged.  I didn't ghost these out for two reasons: 1) I can't find the
guideline in the FE packaging requirements that says these should be ghosted,
and 2) the main python package in FC doesn't ghost out .pyo files, so it seems
more consistent to go ahead and package the .pyo's.

New packages located at:
SPEC: http://www.berningeronline.net/sos.spec
SRPM: http://www.berningeronline.net/sos-0.1-6.src.rpm

I'm providing these packages to try and speed the review along since Steve has
been swamped with moving to a new house and there are lots of people inside RH
who'd like to see this accepted.  Steve will remain the main contact for this
package.

Is Steve back in action?  The python guidelines were changed to remove the
recommendation that the .pyo files be ghosted, so that's good.  The package
still builds fine and the only rpmlint issue is:

W: sos setup-not-quiet

which can be eliminated by adding "-q" to the %setup line.  Not a big deal at all.

Is there a proper upstream source or URL for this package now?

Regarding sponsorship, lately I've been sponsoring Red Hat folks with just a
single package submittion if they've responded quickly to comments.  I know
everyone's busy with FC6 and RHEL5 at the moment, but let's see how it goes.

I'm here. I'll make the setup quiet, and I'm also ready to deliver a new release
that fixes a couple of minor problems. I'll roll that today or tonight and make
it available.

The project is hosted at sos.108.redhat.com, and that is the upstream. I'll also
make sure that's reflected in the package.

The source RPM and spec file are available at http://people.redhat.com/sconklin,
and the release is tagged r0-1-10 in the source tree.

I've discovered a regression during a code cleanup. I'll have new packages out
as soon as I can.

The following are available at http://people.redhat.com/sconklin

sos-0.1-11.noarch.rpm
sos-0.1-11.src.rpm
sos.spec

The regression is fixed, and a lot of cleanup has been done to remove inaccurate
comments, unused variables, and improve readability

The versioning has been updated in order to separate the upstreamn from the RPM
release. Since this represents a major change to the versioning, I went ahead
and pushed to upstream release number 1.0

The following are available at http://people.redhat.com/sconklin

sos-1.0-1.noarch.rpm
sos-1.0-1.src.rpm
sos.spec

Hey Steve. Sorry for the delay here... 

Do you still wish to submit this package?
Is the 1.3 version on your people page the current one that should be reviewed? 

Wow, I guess I really dropped the ball here, because Steve did everything that
was asked of him and now nine months later he still hasn't gotten his review.

All I can do is promise to take care of this quickly if Steve is still around
and still has interest in getting this into the distro.  Just let me know.

Yes, it would still be great to get this into fedora. development is taking
place at sos.108.redhat.com, and the package is already shipping with RHEL5.
The r1-5 tag on sos is what's about to be pushed out as a fastrack rhel5 errata,
so would be a reasonable version to review. I'll be shifting the primary
maintenance duties for this package to someone else in red hat support very
soon, as I'm transferring to the engineering group and want this to continue to
be encouraged from the support group.

Unfortunately I'm something of a bonehead and I have a bunch of review tickets
in flight in any case, so I need to ask if there's any possibility that whoever
is going to work on moving this ticket forward wrap up a quick spec and srpm and
drop  links in this ticket.

OK, this is the version that is currently pending release (awaiting QA ack) for
RHEL5.1, it is the same as the upstream at sos.108.redhat.com.

http://people.redhat.com/sconklin/sos-1.4-1.src.rpm
http://people.redhat.com/sconklin/sos.spec

Glad to help!

Steve

Unfortunately this doesn't build for me (in mock building from a rawhide repo):

Executing(%install): /bin/sh -e /var/tmp/rpm-tmp.11747
+ umask 022
+ cd /builddir/build/BUILD
+ cd sos-1.4
+ LANG=C
+ export LANG
+ unset DISPLAY
+ rm -rf /var/tmp/sos-1.4-1.fc8-root-mockbuild
+ /usr/bin/python setup.py -q install --root=/var/tmp/sos-1.4-1.fc8-root-mockbuild
error: invalid Python installation: unable to open
/usr/lib64/python2.5/config/Makefile (No such file or directory)
error: Bad exit status from /var/tmp/rpm-tmp.11747 (%install)

I'm not anywhere near being a python expert; perhaps a build dependency on
python-devel is needed?

I'm traveling, but I'll check this out as soon as possible next week.

Adding the build dependency got things building.

OK, first rpmlint.  Just one complaint, which is a blocker:

Looks like there's a missing changelog entry for 1.4:
   W: sos incoherent-version-in-changelog 1.3-3 1.4-1.fc8

Some other issues:

I'm prepared to accept that a real tarball will make it somewhere public at some
point.  Until then, could you include a couple of lines of comment indicating
how you check out the source that's in the tarball?

You need to make sure a copy of the license text is included in the package.

Of course there's the aforementioned python-devel dependency issue.

I'm not sure why you have the manual dependency on python.  rpm will sort that
out for you and you'll have an automatic dependency like "python(abi) = 2.5".

Really, these are pretty minor issues and should be easily fixed up.

Review:
? Can't check that source files match upstream.
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* summary is OK.
* description is OK.
* dist tag is present.
* build root is OK.
* license field matches the actual license.
* license is open source-compatible.
X license text not included in package.
* latest version is being packaged (I suppose; it's an SVN checkout)
X BuildRequires are proper (python-devel is needed).
* %clean is present.
* package builds in mock (development, x86_64).
* package installs properly
X rpmlint has a valid complaint.
X final provides and requires are sane:
   sos = 1.4-1.fc8
  =
   /usr/bin/env
 X python >= 0:2.3
   python(abi) = 2.5
* %check is not present; no test suite upstream.  I installed it on a rawhide 
   machine and it seemed to run OK.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* no scriptlets present.
* code, not content.
* documentation is small, so no -docs subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.

Clearing FE-NEEDSPONSOR since I clicked the necessary checkbox.

Anything happening here?

Yes, expect something soon.

I have built a new package trying to address all the problems mentioned in this
BZ and tagged it with r1-6 on svn.

The source code from upstream can be checked out with the following command:

  svn checkout https://sos.108.redhat.com/svn/sos/tags/r1-6 sos-r1-6

rpmlint is not returning any error. Please let me know if there is anything else
that needs to be fixed.

  http://people.redhat.com/neslami/sos/sos-1.6-3.src.rpm
  http://people.redhat.com/neslami/sos/sos.spec

Thank you.

-- Navid

Odd; this package has grown rpmlint issues since I looked at it last.  I'm not
sure how you would not be seeing any errors.  Maybe you neglected to run it
against both the SRPM and the built RPM.

W: sos redundant-prefix-tag
  Please remove Prefix:.
  While you're at it, remove Vendor: as well.

E: sos no-cleaning-of-buildroot %install
  Somehow "rm -rf ${RPM_BUILD_ROOT}" was removed from the %install section of 
  the previous submission.  It needs to come back.

In fact, it looks like someone junked the almost-complete existing spec and
started over with some new spec having , so I need to do a complete re-review. 
That's really disappointing; I've spent a lot if time on this already.

The instructions for fetching the source must be included in the spec.  The
instructions that you've given in comment #23 don't work for me; I'm prompted
for a password and I don't know what to enter.

The summary seems to be mostly content-free; it needs to include at least a
little information about what the package does.  Maybe "System information
gethering tools" or somesuch.

The dist tag seems to have disappeared.  It's not mandatory, but if not present
I have to ask: are you sure you don't want it?  Do you understand the
requirements of multi-branch maintenance without the dist tag?

Because you've switched to using the ill-advices --record option to setup.py,
you're now not including any of the directories that you should be including.
If you put the %python_sitelib definition back in the beginning of the spec
(which you simply must do in any case, unless you want some complicated hack
that parses the INSTALLED_FILES file to figure out where the directories are),
then a %files section consisting simply of:
   %{_sbindir}/sosreport
   %{python_sitelib}/sos/
   %{_mandir}/man1/sosreport.1*
   %doc LICENSE README TODO
should work fine.

Review:
? can't compare source files against upstream.
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
X summary is nondescriptive.
* description is OK.
? dist tag is not present.
* build root is OK.
* license field matches the actual license.
* license is open source-compatible.
* license text included in package.
* latest version is being packaged.
* BuildRequires are proper.
* %clean is present.
* package builds in mock (development, x86_64).
* package installs properly
X rpmlint has valid complaints.
* final provides and requires are sane:
   sos = 1.6-3
  =
   /usr/bin/env
   python(abi) = 2.5
* %check is not present; no test suite upstream.  I tested it before and 
   I'll make the assumption that it still works OK.
X does not own most of the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* no scriptlets present.
* code, not content.
* documentation is small, so no -docs subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.

Jason,

apparently the .spec file generated by bdist_rpm is not what was used in the
package you originally reviewed, hence the differences you observed. I will
rebuild a package using a custom .spec according to your review and resubmit in
the next few days.

Thank you.

-- Navid

The bdist_rpm bits are notoriously...  unreliable - at producing a sane spec
file.  That's why I created an independent spec file and it was updated in
parallel with the Makefile way back when.  You should be able to recover that
spec file and adjust it accordingly - bdist_rpm is bad mojo.

When you say "the instructions for fetching the source must be included in the
spec", does that mean that Source0 has to be valid URL or instead just to
include a few commented lines explaining how to use svn to fetch the source ?
Thanks.

If there's no URL to fetch directly, you need to include instructions for
duplicating the source that you include in the SRPM.

See http://fedoraproject.org/wiki/Packaging/SourceURL for more information.

I reverted back to the custom sos.spec and applied all the suggestions from your
review. The new package and .spec file can be found here:

  http://people.redhat.com/neslami/sos/sos-1.6-4.src.rpm
  http://people.redhat.com/neslami/sos/sos.spec

rpmlint now (really) shows no error with both the SRPM and the built RPM.

Please let me know if I accidentally left anything out.

-- Navid

I think that in the new spec there's no reason to use --record or mess with
INSTALLED_FILES because it's no longer being used.

Generally the name of the package isn't used in the summary.

The svn export doesn't match what's in the tarball.  The tarball is missing
Changelog, example_plugins, install-rpm.sh, Makefile, prep-rpm.sh, pylintrc,
sosreport.1 and sos.spec.  The checkout is missing PKG-INFO and sosreport.1.gz.

Perhaps someone is compressing the manpage manually?  There's no point; rpm will
do it for you, and the %files section you have will handle it.  I don't know
about the rest of the mismatched files.  The Changelog file seems to be
something that should certainly be packaged.

I made the following changes:

* fixed manpage (let rpmbuild compress it)
* removed --record and mangling of INSTALL_FILES
* fixed summary
* svn export and tarball are identical
* added ChangeLog to file-set

The new files can be found here:

  http://people.redhat.com/neslami/sos/sos-1.6-5.src.rpm
  http://people.redhat.com/neslami/sos/sos.spec

Thank you.

OK, cool.  The source matches upstream, summary looks good, the spec is cneal
with no extraneous bits, rpmlint is quiet.  I'd say it's ready to go.

APPROVED

Umm, ping?

New Package CVS Request
=======================
Package Name: sos
Short Description: A set of tools to gather troubleshooting information from a
system
Owners: neslami
Branches: FC-6 F-7 EL-4 EL-5 OLPC-2
InitialCC: sos-project

Package Change Request
======================
Package Name: sos
[Updated InitialCC: ]

Looks like InitialCC requires the email to have an associated BZ account.
sos-project is a mailing-list, let's just leave it empty for now. Thanks.

CVS done.

Jason,

has the sos-project email address been removed as InitialCC ? The reason I'm
asking is subscribers to that mailing-list are receiving automated requests
asking to join BZ.

Thanks.

-- Navid

Yes, I removed the CC as requested.  Check owners.list for yourself if you like.  

Is there some reason this package has not been imported yet?

OK, folks, it's been another month.  If nothing happens soon, I will revoke my
approvals and sponsorships and have the empty package directories removed from
the repository.

I'm sorry Jason, this is my fault. I will get back on it ASAP.

-- Navid

Jason,

I think I've managed to import and build SoS correctly as per instructions in
http://fedoraproject.org/wiki/PackageMaintainers/Join - should I go ahead and
close this as NEXTRELEASE ?

Thank you.

-- Navid

If the package is built and pushed out to the branches you want it pushed to,
then you can go ahead and close the ticket.  I don't, however, see that it was
built for F6, F7 or the OLPC branch and it doesn't look like it was pushed to
EPEL either.  Since those branches were in the CVS request, I'd guess that you
have some additional builds and update requests to make.

After three months, I'm just going to close this ticket to get it out of my
list.  It looks like it's in rawhide and F8 at least.