Bug 482902 - Review Request: gsh - aggregate several remote shells into one
Review Request: gsh - aggregate several remote shells into one
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jochen Schmitt
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-01-28 13:53 EST by Adam Miller
Modified: 2009-02-04 21:24 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-02-04 21:14:40 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
jochen: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
Propose cleanup SPEC file for gsh (1.86 KB, text/plain)
2009-01-29 10:44 EST, Jochen Schmitt
no flags Details

  None (edit)
Description Adam Miller 2009-01-28 13:53:03 EST
Spec URL: http://maxamillion.fedorapeople.org/gsh.spec
SRPM URL: http://maxamillion.fedorapeople.org/gsh-0.3-1.src.rpm

Description: gsh is used to launch several remote shells on many machines at the same time and control them from a single command prompt.
Comment 1 Jochen Schmitt 2009-01-28 15:26:10 EST
Good:
+ Basename of the SPEC files matches with package name
+ BuildRoot tag is ok
+ Could download upstream tar ball with spectool
+ Package contains a License tag
+ License tag contains GPLv2+ as a valid OSS license
+ Package contains a verbatin copy of the license text
+ License in the copyright notes in the source files matches
with specification on the license tag
+ Local build works fine
+ Koji build works fine
+ Package contains no subpackages
+ %doc stanza is small, so we need no separate doc subpackage
+ Build root will be cleaned at the beginning of %clean and %install
+ Package don't contains any scriptlets
+ %changelog is ok. 

Bad:
- Package doesn't fits with the python SPECE templates
- Build complaints duplicate in the %files stanza
- Package tar ball doesn't match with upstream one
Upstream tar ball:
6b925fe21bb84606b47a9a29d1eb88fb  gsh-0.3.tar.gz	
Packaged tar ball:
9b23c16f1265925989e2c1b4f17b04f9  ../SOURCES/gsh-0.3.tar.gz
- Rpmlint compaints binary package:
$ rpmlint gsh-0.3-1.fc10.noarch.rpm
gsh.noarch: E: non-executable-script /usr/lib/python2.5/site-packages/gsh/pity.py 0644
1 packages and 0 specfiles checked; 1 errors, 0 warnings.
- Please remove the gsh.egg-info directory from the sources in the %setup stanza
to make sure, theat the egg-infos will been built from source

Hints:
* It may be helpful, if you can read and unterstand the python packaging guideline as
https://fedoraproject.org/wiki/Packaging/Python

This is not a complete review, because the package has some severe errors and need some love from you.
Comment 2 Adam Miller 2009-01-28 17:10:40 EST
Spec URL: http://maxamillion.fedorapeople.org/gsh.spec
SRPM URL: http://maxamillion.fedorapeople.org/gsh-0.3-2.src.rpm


%changelog
* Wed Jan 28 2009 Adam Miller <maxamillion [AT] gmail.com> - 0.3-2
- Fixed rpmlint complaint, removed egg in setup, redownloaded source


I am not entirely sure what was meant by python SPECE templates as I didn't see any mention of those on the wiki page for python packages, but I did change up the spec in ways that I saw I was lacking in compliance with the docs. Looking forward to further review and suggestions. Thank you for your time.
Comment 3 Jochen Schmitt 2009-01-29 10:44:59 EST
Created attachment 330368 [details]
Propose cleanup SPEC file for gsh

Good:
+ Defining python_sitelib macro
+ Packaged tar ball matches with upstream
(md5sum: 6b925fe21bb84606b47a9a29d1eb88fb)
+ Local build works fine
+ Local install works fine
+ Simple call without argument rans without crash
+ Local uninstall works fine
+ Koji build works fine

Bad:
- '-n <name>' flag occurs twice in the %setup macro
- You should remove the gsh.egg-info directory
- %files stanza has a long list of duplicate files entries

I have attached a clenup SPEC file as a proposal for this package
Comment 4 Adam Miller 2009-01-29 12:42:47 EST
Spec URL: http://maxamillion.fedorapeople.org/gsh.spec
SRPM URL: http://maxamillion.fedorapeople.org/gsh-0.3-4.src.rpm

Only thing I edited from your proposed SPEC was remove the extra -n <name> entry since that was on your "Bad:" list. Thank you very much for your SPEC proposal.
Comment 5 Jochen Schmitt 2009-01-29 13:21:30 EST
God:
+ Duüöocated -n option was removed from %setup
+ Files stanza has no duplicated entries
+ egg-info directory was removed from source in the %setup stanza

Bad:
- the pity.py file is marked as execuatable, but all other files are not. I think we should mark all files in %{python_sitelib}/gsh as nonexecutables. You may remove the shebang line from the pity.py file.
Comment 6 Adam Miller 2009-01-29 14:12:53 EST
Spec URL: http://maxamillion.fedorapeople.org/gsh.spec
SRPM URL: http://maxamillion.fedorapeople.org/gsh-0.3-5.src.rpm

Added a patch to get rid of the shebang to satisfy rpmlint instead of forcing the file to be executable.
Comment 7 Jochen Schmitt 2009-01-29 15:25:55 EST
Good:
+ Package contains a patch
+ Patch seems to be reliable
+ Local build works fine
+ Rpmlint is silent on binary rpm.

You are APPROVED
Comment 8 Adam Miller 2009-01-29 16:12:49 EST
New Package CVS Request
=======================
Package Name: gsh
Short Description: aggregate several remote shells into one
Owners: maxamillion
Branches: EL-5 F-9 F-10
InitialCC:
Comment 9 Kevin Fenzi 2009-01-30 01:26:33 EST
cvs done.
Comment 10 Fedora Update System 2009-02-04 21:14:36 EST
gsh-0.3-5.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 11 Fedora Update System 2009-02-04 21:24:50 EST
gsh-0.3-5.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.

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