Bug 878612 - Review Request: python-pexpect - Unicode-aware Pure Python Expect-like module
Review Request: python-pexpect - Unicode-aware Pure Python Expect-like module
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Toshio Ernie Kuratomi
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 622648 784947 785227 974244
  Show dependency treegraph
 
Reported: 2012-11-20 13:58 EST by Andrew McNabb
Modified: 2014-01-13 14:00 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-11-21 09:30:17 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
a.badger: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Andrew McNabb 2012-11-20 13:58:53 EST
Spec URL: http://aml.cs.byu.edu/~amcnabb/python-pexpect.spec
SRPM URL: http://aml.cs.byu.edu/~amcnabb/python-pexpect-2.5.1-1.fc17.src.rpm
Description:

This is a new package of the pexpect Python module. It is based on the pexpect-u branch, which supports Python 3 and Unicode. This new python-pexpect package will obsolete the current pexpect package (as suggested by the current maintainer in bug #785227 comment #4). This new package also follows the naming guidelines (see bug #622648).

Fedora Account System Username: amcnabb
Comment 1 Toshio Ernie Kuratomi 2012-11-20 14:42:09 EST
The provides and obsoletes need to be versioned: http://fedoraproject.org/wiki/Packaging:Guidelines#Renaming.2FReplacing_Existing_Packages
Comment 2 Andrew McNabb 2012-11-20 14:55:24 EST
(In reply to comment #1)
> The provides and obsoletes need to be versioned:
> http://fedoraproject.org/wiki/Packaging:Guidelines#Renaming.
> 2FReplacing_Existing_Packages

Thanks. I've now fixed this.

By the way, should I bump the release number when I make a change to the spec like this one, or should I just upload new files with the same release number?
Comment 3 Toshio Ernie Kuratomi 2012-11-20 15:09:24 EST
You should bump the release number.
Comment 4 Toshio Ernie Kuratomi 2012-11-20 15:12:45 EST
Review based on the current version:
ab24557ef52c1bc6ff79f81408d9357888dfa55a4d90657576822c24c36f077e  pexpect-u-2.5.1.tar.gz

Good:
* Source matches upstream and source url is canonical
* Package named according to the naming guidelines
* spec file matches package name
* License is MIT in the spec file and files distributed in the tarball
* LICENSE file included
* Spec is legible
* No locale files shipped
* Not an ELF shared library
* Macros used consistently
* Contains code, not conent
* No large doc
* Not a GUI app, no .desktop file

Cosmetic:
* Can change python-devel to python2-devel
* No need for -O1 in the build and install lines (brp-python-bytecompile will (re)do that)

Needswork:
* Change source http://pypi.python.org/packages/source/p/pexpect-u/pexpect-u-[..]
  to Source0: http://pypi.python.org/packages/source/p/pexpect-u/pexpect-u-[..]
  for consistency with other packages and so that spectool -g works  (notice the
  correction of missing ":" after source)
* %if (0%{?fedora} > 15 || 0%{?rhel} > 5) => should remove the || 0%{?rhel} > 5 portion because
  rhel6 doesn't have python3 and we've been told that we're not sure what the
  story for rhel7 is going to be.
* Tests are shipped with the package.  They should be run.  This can be done by
  BuildRequiring python-nose (python3-nose for the pythn3 subbpackage) and
  running nosetests (nosetests-3.2 for the python3 subpackage)
* Description section in the python3 subpackage should say this is for python3.
  Easiest way is just to tack on a one sentence paragraph that says:
  "This package contains the python3 version of this module"
* The comment in the %install section about doing the python3 install first is
  misleading -- there aren't any scripts in %{_bindir} to worry about.  Unless
  there is a reason that order matters here my suggestion is simply to remove
  the comment.

To Check still:
* rpmlint
* builds in koji
* Permissions set properly
* Package owns all files and directories that it creates and nothing more
* All filenames valid utf-8
Comment 5 Andrew McNabb 2012-11-20 15:13:45 EST
(In reply to comment #3)
> You should bump the release number.

This is now done, and the new source RPM is at: http://aml.cs.byu.edu/~amcnabb/python-pexpect-2.5.1-2.fc17.src.rpm
Comment 6 Toshio Ernie Kuratomi 2012-11-20 15:25:23 EST
Provides is correct in the new package.  Obsoletes: needs to be a little higher.

The latest version that I see in koji is: pexpect-2.3-8.fc18

So the Obsoletes should be:

Obsoletes: pexpect <= 2.3-9
Comment 7 Andrew McNabb 2012-11-20 15:54:30 EST
(In reply to comment #4)
> Review based on the current version:
> ab24557ef52c1bc6ff79f81408d9357888dfa55a4d90657576822c24c36f077e 
> pexpect-u-2.5.1.tar.gz
> 
> Cosmetic:
> * Can change python-devel to python2-devel

Done.

> * No need for -O1 in the build and install lines (brp-python-bytecompile
> will (re)do that)

Done.

> Needswork:
> * Change source
> http://pypi.python.org/packages/source/p/pexpect-u/pexpect-u-[..]
>   to Source0:
> http://pypi.python.org/packages/source/p/pexpect-u/pexpect-u-[..]
>   for consistency with other packages and so that spectool -g works  (notice
> the
>   correction of missing ":" after source)

Done.

> * %if (0%{?fedora} > 15 || 0%{?rhel} > 5) => should remove the || 0%{?rhel}
> > 5 portion because
>   rhel6 doesn't have python3 and we've been told that we're not sure what the
>   story for rhel7 is going to be.

Done.

> * Tests are shipped with the package.  They should be run.  This can be done
> by
>   BuildRequiring python-nose (python3-nose for the pythn3 subbpackage) and
>   running nosetests (nosetests-3.2 for the python3 subpackage)

Done.

> * Description section in the python3 subpackage should say this is for
> python3.
>   Easiest way is just to tack on a one sentence paragraph that says:
>   "This package contains the python3 version of this module"

Done.

> * The comment in the %install section about doing the python3 install first
> is
>   misleading -- there aren't any scripts in %{_bindir} to worry about. 
> Unless
>   there is a reason that order matters here my suggestion is simply to remove
>   the comment.

Done.

(In reply to comment #6)
> So the Obsoletes should be:
>
> Obsoletes: pexpect <= 2.3-9

Done.

The new source RPM is at:
http://aml.cs.byu.edu/~amcnabb/python-pexpect-2.5.1-3.fc17.src.rpm
Comment 8 Toshio Ernie Kuratomi 2012-11-20 17:02:22 EST
Good:
* previous comments are fixed

Needswork:
* running unittests shold be moved to a %check section:
%check

PYTHONSTARTUP="" nosetests

%if 0%{?with_python3}
pushd %{py3dir}
pushd build/lib
PYTHONSTARTUP="" nosetests-%{python3_version}

popd
popd
%endif # with_python3

* Note that I also found that the nosetests for python3 has to be invoked using that macro to determine which version of python3 is on the system.  F17 has python3-3.2; F19 and F18 have python3-3.3.

* I tried building in koji with the above %check section but there's one failing unittest (on python2).  You can see it here:
  http://kojipkgs.fedoraproject.org//work/tasks/761/4710761/build.log

  This also fails locally when I try to build it on F17 so it looks like a valid  bug.

* I reversed the order of the nosetest runs and found a different test failing on python3:
http://kojipkgs.fedoraproject.org//work/tasks/877/4710877/build.log

Looks like a BuildRequires: ed is needed.

Still to check:
* rpmlint
* Permissions set properly
* Package owns all files and directories that it creates and nothing more
* All filenames valid utf-8
Comment 9 Andrew McNabb 2012-11-20 17:16:59 EST
(In reply to comment #8)
> 
> Needswork:
> * running unittests shold be moved to a %check section:
> %check

Done.

> * Note that I also found that the nosetests for python3 has to be invoked
> using that macro to determine which version of python3 is on the system. 
> F17 has python3-3.2; F19 and F18 have python3-3.3.

Done.


> * I tried building in koji with the above %check section but there's one
> failing unittest (on python2).  You can see it here:
>   http://kojipkgs.fedoraproject.org//work/tasks/761/4710761/build.log
> 
>   This also fails locally when I try to build it on F17 so it looks like a
> valid  bug.
>
> * I reversed the order of the nosetest runs and found a different test
> failing on python3:
> http://kojipkgs.fedoraproject.org//work/tasks/877/4710877/build.log
> 
> Looks like a BuildRequires: ed is needed.

It looks like the problem is ed in both cases.

Added ed to BuildRequires.

The new source RPM is at:
http://aml.cs.byu.edu/~amcnabb/python-pexpect-2.5.1-4.fc17.src.rpm
Comment 10 Andrew McNabb 2012-11-20 17:59:40 EST
The RPM is now successfully building in koji without any errors:

http://koji.fedoraproject.org/koji/taskinfo?taskID=4711309
http://koji.fedoraproject.org/koji/taskinfo?taskID=4711308
Comment 11 Toshio Ernie Kuratomi 2012-11-20 18:40:13 EST
Good:
* Former problems taken care of
* Builds in koji
* Permissions set properly
* Package owns all files and directories that it creates and nothing more
* All filenames valid utf-8

rpmlint

* python3-pexpect.noarch: W: spelling-error %description -l en_US containts -> contains, con taints, con-taints

  Valid -- change to contains

* python3-pexpect.noarch: W: spelling-error %description -l en_US passwd -> passed, password

  False positive.  We're talking about /usr/bin/passwd here

python3-pexpect.noarch: W: spelling-error %description -l en_US pty -> pry, pt, pity

  False positive.

python3-pexpect.noarch: E: non-executable-script /usr/lib/python3.3/site-packages/pexpect/FSM.py 0644L /usr/bin/env

  This one is a python module that happens to have a shebang line and some code that can be executed if the module were run like a script.  However, that output is just an example of how to write code against the module's API -- it's not meant to actually be used on the system.  So this can be ignored.

* python-pexpect.noarch: E: zero-length /usr/lib/python2.7/site-packages/pexpect/tests/forwhich.sh
  python-pexpect.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/pexpect/tests/test_timeout_pattern.py 0644L /usr/bin/env
  [other non-executable scripts in the tests subdir]

  The tests aren't actually imported and used by the module.  Makes sense to just remove these from the built rpm like this:

  %files
  [..]
  %exclude %{python_sitelib}/pexpect/tests/

  %files -n python3-pexpect
  [..]
  %exclude %{python3_sitelib}/pexpect/tests/

4 packages and 1 specfiles checked; 29 errors, 9 warnings.

You can fix the one spelling error and add the %exclude's for the tests before you import this into git.

Package is APPROVED.
Comment 12 Andrew McNabb 2012-11-20 18:47:15 EST
(In reply to comment #11)
> * python3-pexpect.noarch: W: spelling-error %description -l en_US containts
> -> contains, con taints, con-taints
> 
>   Valid -- change to contains

Done.

> python3-pexpect.noarch: E: non-executable-script
> /usr/lib/python3.3/site-packages/pexpect/FSM.py 0644L /usr/bin/env
> 
>   This one is a python module that happens to have a shebang line and some
> code that can be executed if the module were run like a script.  However,
> that output is just an example of how to write code against the module's API
> -- it's not meant to actually be used on the system.  So this can be ignored.

Agreed.

> * python-pexpect.noarch: E: zero-length
> /usr/lib/python2.7/site-packages/pexpect/tests/forwhich.sh
>   python-pexpect.noarch: E: non-executable-script
> /usr/lib/python2.7/site-packages/pexpect/tests/test_timeout_pattern.py 0644L
> /usr/bin/env
>   [other non-executable scripts in the tests subdir]
> 
>   The tests aren't actually imported and used by the module.  Makes sense to
> just remove these from the built rpm like this:
> 
>   %files
>   [..]
>   %exclude %{python_sitelib}/pexpect/tests/
> 
>   %files -n python3-pexpect
>   [..]
>   %exclude %{python3_sitelib}/pexpect/tests/

Done.

> You can fix the one spelling error and add the %exclude's for the tests
> before you import this into git.
> 
> Package is APPROVED.

Thanks for the helpful (and extremely fast) review.
Comment 13 Andrew McNabb 2012-11-20 18:49:03 EST
New Package SCM Request
=======================
Package Name: python-pexpect
Short Description: Unicode-aware Pure Python Expect-like module
Owners: amcnabb
Branches: f18
InitialCC:
Comment 14 Jon Ciesla 2012-11-21 07:12:25 EST
Git done (by process-git-requests).
Comment 15 Fedora Update System 2012-11-21 10:04:57 EST
python-pexpect-2.5.1-5.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/python-pexpect-2.5.1-5.fc18
Comment 16 Fedora Update System 2012-11-27 00:06:25 EST
python-pexpect-2.5.1-5.fc18 has been pushed to the Fedora 18 stable repository.
Comment 17 leigh scott 2014-01-02 11:56:25 EST
Needed for my epel7 cinnamon port.

Package Change Request
======================
Package Name: python-pexpect
New Branches: epel7
Owners: leigh123linux
Comment 18 Jon Ciesla 2014-01-02 12:01:56 EST
No epel7 branches yet.
Comment 19 leigh scott 2014-01-12 08:20:44 EST
Needed for my epel7 cinnamon port.

Package Change Request
======================
Package Name: python-pexpect
New Branches: epel7
Owners: leigh123linux
Comment 20 Jon Ciesla 2014-01-13 07:48:48 EST
Any comment from the Fedora maintainers?
Comment 21 Andrew McNabb 2014-01-13 08:40:54 EST
(In reply to Jon Ciesla from comment #20)
> Any comment from the Fedora maintainers?

Nope--everything looks good.
Comment 22 Andrew McNabb 2014-01-13 08:42:27 EST
By the way, if the EPEL maintainers want to be added as Fedora maintainers, too, that would be great.
Comment 23 Jon Ciesla 2014-01-13 14:00:32 EST
Git done (by process-git-requests).

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