Bug 808258

Hi,

first, BuildRoot is no longer needed, unless you target EPEL 5

so does :
%defattr(-,root,root,-) 
%clean with rm -Rf
rm -Rf in %install 

( cf https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag )

* using %{python_sitelib} mean that the directory will be owned by this rpm, and that should be avoid.

* BuildRequires:  python-devel python-setuptools

it is easier to review patch with 2 lines for each buildRequires ( IMHO )

and you should explicitely say if this is for python 2 or 3 ( https://fedoraproject.org/wiki/Packaging:Python#BuildRequires ), due to the current transition.

* there seems to be some test to run, but they are not run by %check, any reason ? 

* As a side note, I think you could try to convince upstream of pushing tarball to pypi as well, since having a tarball called "0.95" is rather ugly ( and I think this may be against the guideline : https://fedoraproject.org/wiki/Packaging:SourceURL ).

Hi Michael, issues addressed, updated files here:

spec: http://fedorapeople.org/~grover/new/python-sh.spec
srpm: http://fedorapeople.org/~grover/new/python-sh-0.95-2.el6.src.rpm

> Description: A Python module to simplify calling shell commands.
> Also known as PBS, or Python Bash Scripting.
> 
> Upstream project is currently called "pbs". Due to "python-pbs" being taken for
> Fedora, upstream has agreed to python-sh

Why not call the rpm package python-bash-scripting then?

(In reply to comment #3)
> > Description: A Python module to simplify calling shell commands.
> > Also known as PBS, or Python Bash Scripting.
> > 
> > Upstream project is currently called "pbs". Due to "python-pbs" being taken for
> > Fedora, upstream has agreed to python-sh
> 
> Why not call the rpm package python-bash-scripting then?

Because that's not it's real name either. See

https://github.com/amoffat/pbs/issues/33#issuecomment-4563784

Upstream agreed to python-sh pkgname too, and may change upstream name to sh in the future:

https://github.com/amoffat/pbs/issues/28

Hi Michael, any more comments on the latest revision in comment 2?

*** Bug 832588 has been marked as a duplicate of this bug. ***

I noticed that another review request was opened by someone else for this package.  I've encouraged him to review this one instead.

I took a quick look since packages with conflciting names can often cause problems.  It looks like there's no file-level conflicts between this package and python-pbs as python-pbs uses a directory-based module and this package uses a file-based module but I'd very strongly recommend against shipping the pbs.py{,c,o} files.  If I was reviewing, I'd block the package on it.  The problem is that a system having both python-pbs and python-sh installed will not be able to import one or the other of the modules.

Simply moving the %{python_sitelib}/pbs.py{,c,o} files to %{python_sitelib}/sh.py{,c,o} instead of creating a symlink should resolve that.  Note that the egg-info for python-pbs is pbs_python and the egg-info for this package is python_pbs.  This is not a direct conflict but it is confusing... I'd recommend (but perhaps not block) this package on removing the python_pbs egg-info for this package.  Also regarding egg-info, you should probably ship a python_sh egg-info.  You can use a symlink or a mv depending on whether you're going to remove the python_pbs egginfo.

Hi Andy.  I will take the review over if Michael doesn't mind but I'll wait a few days for him to respond before nabbing ownership.

I think the sooner we can get 'pbs' ported to 'sh' upstream, the better.

You can take it, I forgot I started this review :/

Thanks, Michael.  :)

Andy, let's wait just a little while for a response from amoffat on https://github.com/amoffat/pbs/pull/64  ; I'd really like his feedback.

If he's unresponsive, you can probably run that script yourself to setup a fork-in-name-only.  We should probably edit the script to denote in the description and README that python-sh is just a rename of the pbs module with links to the relevant upstream issues.  It'd be a shame to unnecessarily confuse developers.

Hi Ralph.

Even if upstream isn't receptive (it's been 6 days, how long were you thinking we should wait?), I don't know if we'd need to "fork", we could just run the translation (and add the note to the README saying what we did) both in the build process. It wouldn't be ideal, but it would let us move forward on getting this included, and could easily be removed if the issue is addressed upstream.

Also, I'm assuming you'd be ok with co-maintaining this with me? :)

Hi Andy - that sounds good!  I'm all-in with co-maintaining this one.  :)

For formality's sake, can you run the script and patch the README like you said but also add a link back to this bug?  Upload a new release of the srpm and spec and I'll do the reviewer's duty.

Hi Ralph, thanks to you and amoffat, upstream released an updated tarball that should be 100% "sh" with no "pbs". Updated .spec and srpms:

spec: http://fedorapeople.org/~grover/new/python-sh.spec
srpm: http://fedorapeople.org/~grover/new/python-sh-0.107-1.fc17.src.rpm

Tested, seems to work fine, please review.

Looks great.  APPROVED!


Package Review
==============

Key:
- = N/A
x = Pass
! = Fail
? = Not evaluated



==== Generic ====
[x]: EXTRA Rpmlint is run on all installed packages.
     Note: No rpmlint messages.
[x]: EXTRA Spec file according to URL is the same as in SRPM.
[x]: MUST Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: MUST Package successfully compiles and builds into binary rpms on at
     least one supported primary architecture.
[x]: MUST %build honors applicable compiler flags or justifies otherwise.
[x]: MUST All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[x]: MUST Buildroot is not present
     Note: Unless packager wants to package for EPEL5 this is fine
[x]: MUST Package contains no bundled libraries.
[x]: MUST Changelog in prescribed format.
[x]: MUST Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     Note: Clean would be needed if support for EPEL is required
[x]: MUST Sources contain only permissible code or content.
[x]: MUST Each %files section contains %defattr if rpm < 4.4
     Note: Note: defattr macros not found. They would be needed for EPEL5
[x]: MUST Macros in Summary, %description expandable at SRPM build time.
[-]: MUST Package contains desktop file if it is a GUI application.
[x]: MUST Package requires other packages for directories it uses.
[x]: MUST Package uses nothing in %doc for runtime.
[x]: MUST Package is not known to require ExcludeArch.
[x]: MUST Permissions on files are set properly.
[x]: MUST Package does not contain duplicates in %files.
[x]: MUST Spec file lacks Packager, Vendor, PreReq tags.
[x]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf would be needed if support for EPEL5 is required
[x]: MUST Large documentation files are in a -doc subpackage, if required.
[x]: MUST If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %doc.
[x]: MUST License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "*No copyright* UNKNOWN", "MIT/X11 (BSD like)" For detailed output of
     licensecheck see file: /home/threebean/devel/FedoraReview/808258-python-
     sh/licensecheck.txt
[x]: MUST Package consistently uses macros (instead of hard-coded directory
     names).
[x]: MUST Package is named using only allowed ascii characters.
[x]: MUST Package is named according to the Package Naming Guidelines.
[x]: MUST Package does not generate any conflict.
[x]: MUST Package obeys FHS, except libexecdir and /usr/target.
[x]: MUST Package must own all directories that it creates.
[x]: MUST Package does not own files or directories owned by other packages.
[x]: MUST Package installs properly.
[x]: MUST Requires correct, justified where necessary.
[x]: MUST Rpmlint is run on all rpms the build produces.
     Note: No rpmlint messages.
[x]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: MUST Spec file is legible and written in American English.
[x]: MUST Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[-]: MUST Package contains a SysV-style init script if in need of one.
[x]: MUST File names are valid UTF-8.
[-]: MUST Useful -debuginfo package or justification otherwise.
[x]: SHOULD Reviewer should test that the package builds in mock.
[x]: SHOULD If the source package does not include license text(s) as a
     separate file from upstream, the packager SHOULD query upstream to
     include it.
[x]: SHOULD Dist tag is present.
[x]: SHOULD No file requires outside of /etc, /bin, /sbin, /usr/bin,
     /usr/sbin.
[x]: SHOULD Final provides and requires are sane (rpm -q --provides and rpm -q
     --requires).
[x]: SHOULD Package functions as described.
[x]: SHOULD Latest version is packaged.
[x]: SHOULD Package does not include license text files separate from
     upstream.
[x]: SHOULD SourceX is a working URL.
[x]: SHOULD Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: SHOULD Package should compile and build into binary rpms on all supported
     architectures.
[x]: SHOULD %check is present and all tests pass.
[x]: SHOULD Packages should try to preserve timestamps of original installed
     files.
[x]: SHOULD Spec use %global instead of %define.

Rpmlint
-------
Checking: python-sh-0.107-1.fc17.src.rpm
          python-sh-0.107-1.fc17.noarch.rpm
2 packages and 0 specfiles checked; 0 errors, 0 warnings.


Rpmlint (installed packages)
----------------------------
# rpmlint python-sh
python-sh.noarch: I: enchant-dictionary-not-found en_US
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
# echo 'rpmlint-done:'

Requires
--------
python-sh-0.107-1.fc17.noarch.rpm (rpmlib, GLIBC filtered):
    
    python(abi) = 2.7

Provides
--------
python-sh-0.107-1.fc17.noarch.rpm:
    
    python-sh = 0.107-1.fc17

MD5-sum check
-------------
/home/threebean/devel/FedoraReview/808258-python-sh/upstream/sh-0.107.tar.gz :
  MD5SUM this package     : d92e8a7b5c729e3d4620a5964563ed54
  MD5SUM upstream package : d92e8a7b5c729e3d4620a5964563ed54


Generated by fedora-review 0.2.0git
External plugins:

New Package SCM Request
=======================
Package Name: python-sh
Short Description: Python module to simplify calling shell commands
Owners: grover ralph
Branches: f16 f17 el6
InitialCC:

Git done (by process-git-requests).

python-sh-0.107-1.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/python-sh-0.107-1.fc16

python-sh-0.107-1.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/python-sh-0.107-1.fc17

python-sh-0.107-1.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/python-sh-0.107-1.el6

python-sh-0.107-1.fc17 has been pushed to the Fedora 17 testing repository.

python-sh-0.107-1.fc16 has been pushed to the Fedora 16 stable repository.

python-sh-0.107-1.fc17 has been pushed to the Fedora 17 stable repository.

python-sh-0.107-1.el6 has been pushed to the Fedora EPEL 6 stable repository.