Bug 819134 - Review Request: python-okaara - python command line user interface development library
Review Request: python-okaara - python command line user interface developmen...
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Extras Quality Assurance
Depends On:
  Show dependency treegraph
Reported: 2012-05-04 22:05 EDT by Jay Dobies
Modified: 2014-02-10 16:13 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Last Closed: 2012-07-17 12:35:51 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tibbs: fedora‑review+
limburgher: fedora‑cvs+

Attachments (Terms of Use)

  None (edit)
Description Jay Dobies 2012-05-04 22:05:10 EDT
Spec URL: http://jdob.fedorapeople.org/okaara-src/python-okaara.spec
SRPM URL: http://jdob.fedorapeople.org/okaara-src/python-okaara-1.0.15-1.fc16.src.rpm
Description: Library of Python functions that facilitate the creation of command-line interfaces.
Comment 1 Jay Dobies 2012-05-11 15:48:28 EDT
Changes made based on reviewer feedback and uploaded to the following locations:

Spec URL: http://jdob.fedorapeople.org/okaara-src/python-okaara.spec
Comment 2 Jeff Ortel 2012-05-16 17:29:47 EDT
Packaging looks good Jay.  Few minor items:

1) Should have a copy of the license should be included in the source and referenced in the %doc.  See: http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text

2) Python packaging guidelines specifies python packages containing python2 source must have: BuildRequires: python2-devel.  See: http://fedoraproject.org/wiki/Packaging:Python 

3) Suggest:
- %{python_sitelib}/okaara/*
+ %{python_sitelib}/okaara/

Which will catch subdirectories under okaara if you refactor.

== MUST ==

[x] rpmlint must be run on the source rpm and all binary rpms the build produces.

$ rpmlint python-okaara-1.0.17-1.fc16.src.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

$ rpmlint python-okaara-1.0.17-1.fc16.noarch.rpm
python-okaara.noarch: W: no-documentation
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

[x] The package must be named according to the Package Naming Guidelines.
[x] The spec file name must match the base package %{name}, in the format %{name}.spec unless your package has an exemption.
[x] The package must meet the Packaging Guidelines.

[x] The package must be licensed with a Fedora approved license and meet the Licensing Guidelines.
[x] The License field in the package spec file must match the actual license.
[x] 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 must be included in %doc.

A copy of the license should be included in the source and referenced in the %doc.

Suggest as LICENSE file at source root and add:

See: http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text

[x] The spec file must be written in American English.
[x] The spec file for the package MUST be legible.
[x] The sources used to build the package must match the upstream source, as provided in the spec URL. Reviewers should use md5sum for this task. If no upstream URL can be specified for this package, please see the Source URL Guidelines for how to deal with this.
[x] The package MUST successfully compile and build into binary rpms on at least one primary architecture.
[x] If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch. Each architecture listed in ExcludeArch MUST have a bug filed in bugzilla, describing the reason that the package does not compile/build/work on that architecture. The bug number MUST be placed in a comment, next to the corresponding ExcludeArch line.

[x] All build dependencies must be listed in BuildRequires, except for any that are listed in the exceptions section of the Packaging Guidelines ; inclusion of those as BuildRequires is optional. Apply common sense.

Need: BuildRequires: python2-devel

See: http://fedoraproject.org/wiki/Packaging:Python 

[x] The spec file MUST handle locales properly. This is done by using the %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden.
[x] Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun.
[x] Packages must NOT bundle copies of system libraries.
[x] If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package. Without this, use of Prefix: /usr is considered a blocker.
[x] A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory. 
[x] A Fedora package must not list a file more than once in the spec file's %files listings. (Notable exception: license texts in specific situations)
[x] Permissions on files must be set properly. Executables should be set with executable permissions, for example. 
[x] Each package must consistently use macros. 
[x] The package must contain code, or permissable content. 
[x] Large documentation files must go in a -doc subpackage. (The definition of large is left up to the packager's best judgement, but is not restricted to size. Large can refer to either size or quantity). 
[x] If a package includes something as %doc, it must not affect the runtime of the application. To summarize: If it is in %doc, the program must run properly if it is not present.
[x] Static libraries must be in a -static package.
[x] Development files must be in a -devel package.
[x] In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name}%{?_isa} = %{version}-%{release} 
[x] Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built.
[x] Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section. If you feel that your packaged GUI application does not need a .desktop file, you must put a comment in the spec file with your explanation.
[x] Packages must not own files or directories already owned by other packages. The rule of thumb here is that the first package to be installed should own the files or directories that other packages may rely upon. This means, for example, that no package in Fedora should ever share ownership with any of the files or directories owned by the filesystem or man package. If you feel that you have a good reason to own a file or directory that another package owns, then please present that at package review time.
[x] All filenames in rpm packages must be valid UTF-8.

== SHOULD ==

[x] If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it.

Already noted.

[x] The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available.
[x] The reviewer should test that the package builds in mock.

Built in mock: fedora-16-x86_64

[x] The package should compile and build into binary rpms on all supported architectures.
[x] The reviewer should test that the package functions as described. A package should not segfault instead of running, for example.
[x] If scriptlets are used, those scriptlets must be sane. This is vague, and left up to the reviewers judgement to determine sanity.
[x] Usually, subpackages other than devel should require the base package using a fully versioned dependency. 
[x] The placement of pkgconfig(.pc) files depends on their usecase, and this is usually for development purposes, so should be placed in a -devel pkg. A reasonable exception is that the main pkg itself is a devel tool not installed in a user runtime, e.g. gcc or gdb.
[x] If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself.
[x] your package should contain man pages for binaries/scripts. If it doesn't, work with upstream to add them where they make sense.
Comment 3 Jay Dobies 2012-05-18 11:30:30 EDT
Changes made based on reviewer feedback and uploaded to the following

Spec URL: http://jdob.fedorapeople.org/okaara-src/python-okaara.spec
Comment 4 Jeff Ortel 2012-05-18 17:50:46 EDT
Thanks Jay.  One last thing. At the beginning of the .spec, you used %define to define the python_sitelib if not already defined.  The Packaging Guidelines request that packagers use %global instead of %define for global macros.

see: "%global preferred over %define"
Comment 6 Jeff Ortel 2012-06-04 16:28:23 EDT
Changes verified.


[jortel@localhost pulp]$ rpmlint python-okaara-1.0.19-1.fc16.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

[jortel@localhost noarch]$ rpmlint python-okaara-1.0.19-1.fc16.noarch.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

Package Approved.
Comment 7 Jay Dobies 2012-06-06 10:15:32 EDT
New Package SCM Request
Package Name: python-okaara
Short Description: Library of Python functions that facilitate the creation of command-line interfaces.
Owners: jdob
Branches: f16 f17
Comment 8 Gwyn Ciesla 2012-06-06 11:17:47 EDT
jdob is not a FAS account in the Packager group.
Comment 9 Jason Tibbitts 2012-07-02 13:50:15 EDT
Fixing up this ticket because the submitter never indicated that sponsorship was needed.
Comment 10 Jason Tibbitts 2012-07-03 16:05:26 EDT
OK, I'll do a full review and assign it back to jortel when I'm done so he gets the credit.  Given that I know you're only packaging this for f16 and up, I'll review based on that.  Unfortunately the previous review missed several things, so I have some more comments.

The python_sitelib macro definition is unnecessary in Fedora (only EL5 needs it)

BuildRoot is unnecessary in Fedora, as is the first line of %install, the entire %clean section and the %defattr line in %files.  (Only EL5 needs the first three; the latter is completely unnecessary.)

The Requires: python >= 2.4 line is unnecessary and somewhat misleading to boot.  RPM will add a proper python(abi) dependency, and a package built for f17 or rawhide won't install on something with only python 2.4 anyway.

Is your personal fedoraprople.org site the proper source for the tarball?  It appears that you are the upstream but it seems odd.

There's some kind of test available.  Is it possible to run it in a %check section?  

* source files match upstream.  sha256sum:
* 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.
* license field matches the actual license.
* license is open source-compatible.
* license text included in package.
* latest version is being packaged.
* BuildRequires are proper.
* package builds in mock (rawhide, x86_64).
* package installs properly.
* rpmlint is silent.
X final provides and requires are sane:
   python-okaara = 1.0.19-1.fc18
X  python >= 2.4
   python(abi) = 2.7

? %check is not present but there appears to be some sort of test suite.
* no bundled libraries.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* no generically named files.
* code, not content.
* documentation is small, so no -doc subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
Comment 11 Jay Dobies 2012-07-03 17:02:43 EDT
Question about how to handle the RHEL5 pieces. This project is in use by Red Hat and needs to be built for RHEL5 (this also explains the >= 2.4 line). Is the proper way to handle this to leave these items in the spec since (I believe) they aren't harming anything in Fedora or do I have to create a separate spec for RHEL5 only builds?
Comment 12 Jason Tibbitts 2012-07-03 17:13:47 EDT
No, they don't hurt anything; they're just useless in Fedora.  You only requested fedora branches so I made the easy assumption that the stuff wasn't necessary.  Personally I can't stand the stone-age complexity that supporting ancient stuff like EL5 requires, don't want it in any of my packages and avoid reviewing things that include it, but it's your package and I'm already committed.

You still need to conditionalize at least the python_sitelib macro to kick in only on EL5 as documented in the python packaging guidelines.  I thought that the explicit python dependency was unnecessary even on EL5 (it's not mentioned anywhere in the python guidelines) but I have no real way of knowing since I only use Fedora.
Comment 13 Jay Dobies 2012-07-03 17:26:45 EDT
I realized that too, I only requested Fedora branches because I didn't think our need for RHEL5 mattered for this process. Now I know :)

I will check on the sitelib macro and, at worst case, add a RHEL5 condition for it. I'll also add in the unit tests.

The only other one I'm unsure of is:
X final provides and requires are sane:
 python-okaara = 1.0.19-1.fc18

I checked the guidelines and I see a section about filtering out the auto-generated provides, but I'm not sure what's wrong with how it's set up today. Can you give me a little more detail on that issue?
Comment 14 Jason Tibbitts 2012-07-03 17:33:59 EDT
The whole bit is this:

X final provides and requires are sane:
   python-okaara = 1.0.19-1.fc18
X  python >= 2.4
   python(abi) = 2.7

indicating that the needless "python >= 2.4" dependency is the issue here.
Comment 15 Jason Tibbitts 2012-07-04 00:34:52 EDT
Reading things more closely, I was wrong about the need to conditionalize that puthon_sitelib macro on EL5.  The guidelines show a (complex) conditional for that, but the way the macro is defined, it only kicks in if it's not already defined so everything is fine.  Those guidelines really need a cleanup.

Still don't know if the python(abi) thing gets automatically done on EL5.
Comment 16 Jay Dobies 2012-07-08 15:30:02 EDT
- Removed the %defattr line under %files
- Added a %check section to run unit tests
- Removed the python requires line; I checked and RHEL5 automatically adds an abi requirement

I saw the guidelines section about conditionalizing the site macro but based on your last comment I didn't change what I had originally.

Spec URL: http://jdob.fedorapeople.org/okaara-src/python-okaara.spec
SRPM URL: http://jdob.fedorapeople.org/okaara-src/python-okaara-1.0.20-1.fc17.src.rpm
Comment 17 Jason Tibbitts 2012-07-08 16:59:01 EDT
Hmm, this doesn't build for me:

+ nosetests
ERROR: Failure: ImportError (No module named okaara.prompt)
Traceback (most recent call last):
  File "/usr/lib/python2.7/site-packages/nose/loader.py", line 390, in loadTestsFromName
    addr.filename, addr.module)
  File "/usr/lib/python2.7/site-packages/nose/importer.py", line 39, in importFromPath
    return self.importFromDir(dir_path, fqname)
  File "/usr/lib/python2.7/site-packages/nose/importer.py", line 86, in importFromDir
    mod = load_module(part_fqname, fh, filename, desc)
  File "/builddir/build/BUILD/python-okaara-1.0.20/test/test_prompt.py", line 13, in <module>
    import okaara.prompt
ImportError: No module named okaara.prompt
Ran 1 test in 0.003s
FAILED (errors=1)
error: Bad exit status from /var/tmp/rpm-tmp.lNKZ7r (%check)

Everything else looks good, though the comments are kind of funny.
Comment 18 Jay Dobies 2012-07-09 10:46:11 EDT
I had python path set up on my machine and forgot to add it to %check. This time I built to a new guest and the tests ran ok.

Spec URL: http://jdob.fedorapeople.org/okaara-src/python-okaara.spec
SRPM URL: http://jdob.fedorapeople.org/okaara-src/python-okaara-1.0.21-1.fc17.src.rpm
Comment 19 Jason Tibbitts 2012-07-09 12:10:33 EDT
Why did you reset the owner of this ticket to nobody?
Comment 20 Jay Dobies 2012-07-09 12:17:27 EDT
I have no idea how I did that. I changed it back to you.
Comment 21 Jason Tibbitts 2012-07-10 23:00:40 EDT
OK, cool.  Builds fine, and the tests appear to pass:
  Ran 12 tests in 0.011s

All complaints I had are fixed, so we're done.  I've sponsored you, and once the permissions propagate you should be able to make your SCM request and get this imported and build.

If you need assistance during the process, feel free to contact me on IRC in #fedora-devel, or via email.
Comment 22 Jay Dobies 2012-07-11 08:37:43 EDT
New Package SCM Request
Package Name: python-okaara
Short Description: Library of Python functions that facilitate the creation of command-line interfaces.
Owners: jdob
Branches: f16 f17
Comment 23 Jason Tibbitts 2012-07-13 19:00:15 EDT
Git done (by process-git-requests).
Comment 24 Jay Dobies 2012-07-17 12:35:51 EDT
Builds completed in koji, closing as per the new package process guidelines.
Comment 25 Alex Wood 2014-02-10 15:36:48 EST
Package Change Request
Package Name: python-okaara
New Branches: el5 el6 epel7
Owners: awood jdob
Comment 26 Gwyn Ciesla 2014-02-10 16:13:53 EST
Git done (by process-git-requests).

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