This service will be undergoing maintenance at 00:00 UTC, 2017-10-23 It is expected to last about 30 minutes
Bug 193712 - Review Request: sos
Review Request: sos
Status: CLOSED WORKSFORME
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2006-05-31 15:22 EDT by Steve Conklin
Modified: 2007-12-19 13:34 EST (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-12-19 13:34:30 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tibbs: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Steve Conklin 2006-05-31 15:22:25 EDT
Spec URL: http://people.redhat.com/sconklin/sos.spec
SRPM URL: http://people.redhat.com/sconklin/sos-0.1-5.src.rpm
Description: Sos is a set of tools that gathers information about system hardware and configuration. The information can then be used for diagnostic purposes and debugging. Sos is commonly used to help support technicians and developers.

Sos implements a pluggable architecture that allows package maintainers to deliver plugins that will collect information that's useful to them in debugging and maintaining their packages.

This is my first package and I need a sponsor
Comment 1 Parag AN(पराग) 2006-06-19 05:54:09 EDT
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.        
Comment 2 Jason Tibbitts 2006-06-23 13:36:23 EDT
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.
Comment 3 John Berninger 2006-07-10 11:34:41 EDT
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.
Comment 4 Jason Tibbitts 2006-08-12 23:25:36 EDT
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.
Comment 5 Steve Conklin 2006-08-14 09:25:35 EDT
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.
Comment 6 Steve Conklin 2006-08-14 14:18:46 EDT
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.
Comment 7 Steve Conklin 2006-08-16 11:04:55 EDT
I've discovered a regression during a code cleanup. I'll have new packages out
as soon as I can.
Comment 8 Steve Conklin 2006-08-21 10:18:34 EDT
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
Comment 9 Steve Conklin 2006-08-30 14:51:20 EDT
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
Comment 11 Kevin Fenzi 2007-06-01 01:07:53 EDT
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? 
Comment 12 Jason Tibbitts 2007-06-01 01:09:32 EDT
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.
Comment 13 Steve Conklin 2007-06-01 11:58:56 EDT
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.
Comment 14 Jason Tibbitts 2007-06-06 11:31:06 EDT
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.
Comment 15 Steve Conklin 2007-06-06 12:18:52 EDT
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
Comment 16 Jason Tibbitts 2007-06-07 23:52:15 EDT
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?
Comment 17 Steve Conklin 2007-06-08 08:49:31 EDT
I'm traveling, but I'll check this out as soon as possible next week.
Comment 18 Jason Tibbitts 2007-06-08 13:38:02 EDT
Adding the build dependency got things building.
Comment 19 Jason Tibbitts 2007-06-08 16:07:08 EDT
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.
Comment 20 Jason Tibbitts 2007-06-10 00:51:56 EDT
Clearing FE-NEEDSPONSOR since I clicked the necessary checkbox.
Comment 21 Jason Tibbitts 2007-06-26 18:34:46 EDT
Anything happening here?
Comment 22 Steve Conklin 2007-06-27 12:03:31 EDT
Yes, expect something soon.
Comment 23 Navid Sheikhol-Eslami 2007-07-03 04:14:33 EDT
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
Comment 24 Jason Tibbitts 2007-07-04 16:44:27 EDT
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.
Comment 25 Navid Sheikhol-Eslami 2007-07-04 17:54:58 EDT
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
Comment 26 John Berninger 2007-07-04 17:58:36 EDT
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.
Comment 27 Navid Sheikhol-Eslami 2007-07-04 18:19:09 EDT
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.
Comment 28 Jason Tibbitts 2007-07-04 18:54:13 EDT
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.
Comment 29 Navid Sheikhol-Eslami 2007-07-05 05:23:26 EDT
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
Comment 30 Jason Tibbitts 2007-07-06 00:14:16 EDT
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.
Comment 31 Navid Sheikhol-Eslami 2007-07-06 04:52:38 EDT
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.
Comment 32 Jason Tibbitts 2007-07-08 22:38:18 EDT
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
Comment 33 Jason Tibbitts 2007-07-19 20:34:41 EDT
Umm, ping?
Comment 34 Navid Sheikhol-Eslami 2007-07-26 08:08:30 EDT
New Package CVS Request
=======================
Package Name: sos
Short Description: A set of tools to gather troubleshooting information from a
system
Owners: neslami@redhat.com
Branches: FC-6 F-7 EL-4 EL-5 OLPC-2
InitialCC: sos-project@redhat.com
Comment 35 Navid Sheikhol-Eslami 2007-07-28 11:34:24 EDT
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.
Comment 36 Jason Tibbitts 2007-07-28 11:42:47 EDT
CVS done.
Comment 37 Navid Sheikhol-Eslami 2007-07-28 13:10:10 EDT
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
Comment 38 Jason Tibbitts 2007-07-28 13:32:16 EDT
Yes, I removed the CC as requested.  Check owners.list for yourself if you like.  
Comment 39 Jason Tibbitts 2007-08-15 11:07:52 EDT
Is there some reason this package has not been imported yet?
Comment 40 Jason Tibbitts 2007-09-12 14:04:05 EDT
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.
Comment 41 Navid Sheikhol-Eslami 2007-09-12 14:13:35 EDT
I'm sorry Jason, this is my fault. I will get back on it ASAP.

-- Navid
Comment 42 Navid Sheikhol-Eslami 2007-09-19 04:38:13 EDT
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
Comment 43 Jason Tibbitts 2007-09-20 19:12:53 EDT
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.
Comment 44 Jason Tibbitts 2007-12-19 13:34:30 EST
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.

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