Bug 497338 - Review Request: python-cclib - A library for processing results of computational chemistry packages
Summary: Review Request: python-cclib - A library for processing results of computatio...
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Peter Lemenkov
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 497339 498214
TreeView+ depends on / blocked
 
Reported: 2009-04-23 12:52 UTC by Susi Lehtola
Modified: 2009-05-09 04:12 UTC (History)
3 users (show)

Fixed In Version: 0.91-4.fc11
Clone Of:
Environment:
Last Closed: 2009-05-06 23:29:46 UTC
Type: ---
Embargoed:
lemenkov: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Susi Lehtola 2009-04-23 12:52:29 UTC
Spec URL:
http://theory.physics.helsinki.fi/~jzlehtol/rpms/python-cclib.spec

SRPM URL:
http://theory.physics.helsinki.fi/~jzlehtol/rpms/python-cclib-0.91-1.fc10.src.rpm

Upstream URL:
http://cclib.sourceforge.net/

Description:
cclib is an open source library, written in Python, for parsing and
interpreting the results of computational chemistry packages. The current
version, cclib 0.9, parses output files from ADF, GAMESS (US), GAMESS-UK,
Gaussian, Jaguar, Molpro, ORCA and PC GAMESS.


rpmlint output is clean.

Comment 1 Peter Lemenkov 2009-04-23 14:41:27 UTC
Missing BR: numpy.
OK, let's assume that you added this (koji scratchbuild with numpy - http://koji.fedoraproject.org/koji/taskinfo?taskID=1316865 )

REVIEW:

+ rpmlint is silent.
+ The package is named according to the Package Naming Guidelines.
+ The spec file name must match the base package %{name}, in the format %{name}.spec unless your package has an exemption. [2] .
+ The package meets the Packaging Guidelines .
+ The package is licensed with a Fedora approved license and meets the Licensing Guidelines .
+ The License field in the package spec file matches the actual license.
+ The file, containing the text of the license(s) for the package, is included in %doc.
+ The spec file is written in American English.
+ The spec file for the package is legible.
+ The sources used to build the package matches the upstream source:

[petro@Sulaco SOURCES]$ md5sum cclib-0.91.tar.gz*
64dddde2cf1ab50cf8cb7f2882c88025  cclib-0.91.tar.gz
64dddde2cf1ab50cf8cb7f2882c88025  cclib-0.91.tar.gz.1
[petro@Sulaco SOURCES]$

+/- The package successfully compiles and builds into binary rpms on at least one primary architecture. But only after adding numpy as BR:

http://koji.fedoraproject.org/koji/taskinfo?taskID=1316865

- All build dependencies must be listed in BuildRequires, so, please add numpy. Definitely there are more missing runtime python dependencies - here is an output of "find -name "*py" -exec grep import {} \; | sort|uniq":

http://peter.fedorapeople.org/cclib_imports.txt

Please, add all necessary runtime python dependencies.

+ The package owns all directories that it creates.
+ The package does not list a file more than once in the spec file's %files listings. 
+ Permissions on files are set properly.
+ The package has a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
+ The package consistently uses macros.
+ The package must contains code, or permissible content.
+ No large documentation files
+ Everytnong, the package includes as %doc, does not affect the runtime of the application.
+ Not a GUI application.
+ The package does not own files or directories already owned by other packages.
+ At the beginning of %install, the package runs rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
+ All filenames in rpm packages must be valid UTF-8.

So, please add the only missign BuildRequire and all missing Requires. 

Also, I'm not sure, that we should ship *.pyc-files in %doc, however this is not a blocker.

Comment 2 Susi Lehtola 2009-04-23 15:19:09 UTC
(In reply to comment #1)
> +/- The package successfully compiles and builds into binary rpms on at least
> one primary architecture. But only after adding numpy as BR:
> 
> http://koji.fedoraproject.org/koji/taskinfo?taskID=1316865
> 
> - All build dependencies must be listed in BuildRequires, so, please add numpy.

OK, sorry, didn't remember to test with mock :)
Added numpy depedencies.

> Definitely there are more missing runtime python dependencies - here is an
> output of "find -name "*py" -exec grep import {} \; | sort|uniq":
> 
> http://peter.fedorapeople.org/cclib_imports.txt
> 
> Please, add all necessary runtime python dependencies.

I don't think that there are any more, at least that's what the website says:
http://cclib.sourceforge.net/wiki/index.php/Install
At least they're not necessary for operation.

> So, please add the only missign BuildRequire and all missing Requires. 
> 
> Also, I'm not sure, that we should ship *.pyc-files in %doc, however this is
> not a blocker.  

Oh, thanks for picking this up. I forgot about it since at first I didn't get the tests working, but then figured out how to pass the additional library dir argument.

http://theory.physics.helsinki.fi/~jzlehtol/rpms/python-cclib.spec
http://theory.physics.helsinki.fi/~jzlehtol/rpms/python-cclib-0.91-2.fc10.src.rpm

Comment 3 Peter Lemenkov 2009-04-24 05:22:43 UTC
I just checked, which modules import whose additional python libraries, and found the following:

* src/cclib/bridge/cclib2biopython.py  explicitly imports python-biopython
* src/cclib/bridge/cclib2openbabel.py explicitly imports python-openbabel
* src/cclib/bridge/cclib2pyquante.py explicitly imports PyQuante

* src/cclib/method/volume.py tries to import PyQuante and vtk-python and fallbacks to work w/o using them instead. And I'm afraid, that further usage of PyQuante in volme.py is broken (there is no check for module_pyq != False later).
* many modules in src/cclib/method also imports numpy

* some modules in src/cclib/parser imports only numpy

* cclib/progress library tries to import PyQt3 first, then fallbacks to PyQt4 and after all imports text-based progress module.

Comment 4 Susi Lehtola 2009-04-24 06:06:50 UTC
Yeah. Well, I made a metapackage that pulls in the additional requirements. I don't want to put them in the main package, since the parser works fine without them.

http://theory.physics.helsinki.fi/~jzlehtol/rpms/python-cclib.spec
http://theory.physics.helsinki.fi/~jzlehtol/rpms/python-cclib-0.91-3.fc10.src.rpm

Comment 5 Susi Lehtola 2009-05-01 11:33:28 UTC
Ping?

Comment 6 Peter Lemenkov 2009-05-02 11:24:35 UTC
Still here. Sorry for the delay.

I'm afraid we cannot simply ignore explicit dependencies, needed for part of the package's functionality and safely move them all into subpackage. I think, that we may move to subpackage only vtk-python and PyQt4. And I advice you to rename supplementary subpackage to gui subpackage.

These dependencies should be moved to main package:

Requires:	PyQuante
Requires:	python-biopython
Requires:	python-openbabel

Comment 7 Susi Lehtola 2009-05-02 18:28:40 UTC
Well, in that case I don't think there is much sense in creating a supplementary package [this is a library, not a frontend]. I put everything in the main package.

Due to the additional requirements this package is quite big: PyQuante is ~4MB, biopython ~4MB, openbabel is ~3MB and the two latter pull in other stuff also.

http://theory.physics.helsinki.fi/~jzlehtol/rpms/python-cclib.spec
http://theory.physics.helsinki.fi/~jzlehtol/rpms/python-cclib-0.91-4.fc10.src.rpm

Comment 8 Peter Lemenkov 2009-05-03 04:33:30 UTC
Ok, I don't see other issues, so this package is

APPROVED.

Comment 9 Susi Lehtola 2009-05-03 05:52:41 UTC
Thanks for the review.

New Package CVS Request
=======================
Package Name: python-cclib
Short Description: A library for processing results of computational chemistry packages
Owners: jussilehtola
Branches: F-10 F-11
InitialCC:

Comment 10 Kevin Fenzi 2009-05-04 03:42:44 UTC
cvs done.

Comment 11 Fedora Update System 2009-05-04 06:43:25 UTC
python-cclib-0.91-4.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/python-cclib-0.91-4.fc10

Comment 12 Fedora Update System 2009-05-04 06:44:28 UTC
python-cclib-0.91-4.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/python-cclib-0.91-4.fc11

Comment 13 Fedora Update System 2009-05-06 23:29:40 UTC
python-cclib-0.91-4.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 14 Fedora Update System 2009-05-09 04:12:47 UTC
python-cclib-0.91-4.fc11 has been pushed to the Fedora 11 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.