Bug 430429 - Review Request: python-storm - An object-relational mapper (ORM) for Python
Review Request: python-storm - An object-relational mapper (ORM) for Python
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: David Timms
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-01-27 19:49 EST by Michel Alexandre Salim
Modified: 2008-09-24 20:23 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-09-24 20:10:12 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
dtimms: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Michel Alexandre Salim 2008-01-27 19:49:18 EST
Spec URL: http://salimma.fedorapeople.org/for_review/python/python-storm.spec
SRPM URL: http://salimma.fedorapeople.org/for_review/python/python-storm-0.11-1.fc9.src.rpm
Description:
Storm is an object-relation mapper (ORM) for the Python language. In
simple terms, that kind of system allows rows from a relational
database to be seen as objects in an object-oriented language like
Python.

Features:

 * Clean and lightweight API offers a short learning curve and long-
   erm maintainability.
 * Storm is developed in a test-driven manner. An untested line of
   code is considered a bug.
 * Storm needs no special class constructors, nor imperative base
   classes.
 * Storm is well designed (different classes have very clear
   boundaries, with small and clean public APIs).
 * Designed from day one to work both with thin relational databases,
   such as SQLite, and big iron systems like PostgreSQL and MySQL.
 * Storm is easy to debug, since its code is written with a KISS
   principle, and thus is easy to understand.
 * Designed from day one to work both at the low end, with trivial
   small databases, and the high end, with applications accessing
   billion row tables and committing to multiple database backends.
 * It's very easy to write and support backends for Storm (current
   backends have around 100 lines of code).
Comment 1 Michel Alexandre Salim 2008-01-27 19:52:27 EST
Note: I've not split out the DB backends, seeking reviewer's opinion whether
that is necessary or not (all current backends have tests for whether the needed
library is installed or not).

Benefit of splitting:
- an application using Storm with backend X could just Requires: python-storm-x
and that will pull both python-storm and the Python binding to X
- (trivial) saving of disk space for unneeded backends

Disadvantage:
More packages in the repository

Comment 2 Michel Alexandre Salim 2008-01-27 20:16:44 EST
Changed my mind, we don't want downstream packagers to have to know which
binding Storm uses; that breaks the abstraction. SRPM (same URL) now split; each
backend provides %{name}-backend = %{version}-%{release}, which is required by
the base package, so at least one backend must be installed at any given time
Comment 3 Michel Alexandre Salim 2008-01-27 20:34:39 EST
Koji scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=377067
Comment 4 David Timms 2008-06-06 21:10:59 EDT
[OK] source in .src.rpm matches upstream md5sum:
md5sum storm-0.11.tar.bz2.from_source_url 
../../src/redhat/SOURCES/storm-0.11.tar.bz2 
348cdc3b0d857bd7b5f8524e8337c44c  storm-0.11.tar.bz2.from_source_url
348cdc3b0d857bd7b5f8524e8337c44c  ../../src/redhat/SOURCES/storm-0.11.tar.bz2

[ !] Latest upstream release:
    It seems that you created the review just a day before upstream released a
new version 0.12. From memory there is a requirement to package the most recent
upstream release. Would you like to update to the current version ? {if not why
not ?}

Also from memory, there is a suggestion to join upstream mailing lists / forums,
so that issues / release notifications will be seen.
Comment 5 Michel Alexandre Salim 2008-06-10 01:12:11 EDT
No reason not to update; the review request has just been languishing without
attention. Updated package should be done by tomorrow.

Comment 6 Michel Alexandre Salim 2008-06-18 12:46:31 EDT
OK, sorry for the delay; here it is. Bumped up version, touched up the
description a bit to match that on the home page. I just put it through the
upstream tutorial, using the SQLite backend.

Spec URL: http://salimma.fedorapeople.org/for_review/python/python-storm.spec
SRPM URL:
http://salimma.fedorapeople.org/for_review/python/python-storm-0.12-1.fc9.src.rpm
Comment 7 David Timms 2008-06-19 19:36:20 EDT
! needs work.
OK no problemo
?? can't say/don't understand.

[OK] rpmlint .src.rpm = quiet

$ rpmbuild -ba:
+ /usr/lib/rpm/find-debuginfo.sh /home/davidt/src/redhat/BUILD/storm-0.12
find: debug: No such file or directory
    from what others have indicated, this is normal for no-arch python.

[OK] rpmlint /home/davidt/src/redhat/RPMS/noarch/python-storm*.rpm
python-storm-mysql.noarch: W: no-documentation
python-storm-postgresql.noarch: W: no-documentation
python-storm-sqlite.noarch: W: no-documentation

[OK] included source matches upstream: md5sum storm*bz*
python-storm-0.12-1.fc9.src/storm-0.12.tar.bz2 
976a332dadd214612c359df63360fc51  storm-0.12.tar.bz2.from_www
976a332dadd214612c359df63360fc51  python-storm-0.12-1.fc9.src/storm-0.12.tar.bz2
[OK] package name proceeded with python since it;s a python library.
[OK] spec named %name.spec
[OK] source tarball contains no prebuilt binaries/libraries.
[OK] files placed into FHS locations {x4 rpms}
[OK] changelog in standard format
[OK] correctly omits Packager, vendor, copyright, prereq 
    , includes license tag.
[OK] summary <80 chars, no ending period
[OK] source0 is correct:
    http://launchpad.net/storm/trunk/0.12/+download/storm-0.12.tar.bz2
[OK] buildroot set at second most proferred location.
[OK] %install correctly erases buildroot before build
[OK] mock build succeeds.
[OK] rpmdiff between default build and mock build shows only time
    {T} differences for folders and the pre-compiled .py[co] files
[OK] description is column limited to <80 chars, no manual/doc info.
[OK] charset is ascii
[OK] docs {license, readme, todo} included. readme points to web for docs.
[OK] debuginfo not expected in noarch application.
[OK] no static libraries, rpath, self copies of already packaged libaries
    etc, since pure python project.
[OK] no config, initscripts, desktop files since it's a db devel library.
[OK] variable style is used consistently
[OK] not multilingual
[OK] timestamps are kept
[OK] make is python based, so smp_flags not required.
[OK] no scriptlets, no conditional dependencies
[OK] is library code not content
[OK] provides backend for each of the backend subpackages seems to make sense.
[OK] dirs/files owned by main package, except the individual backend 
    files {whose directory is created by the main package}.
[OK] not a web app, shoudln't conflict with other packages.
[OK] python sitelib is correctly included at top of spec.
[OK] eggs are build
[OK] no files in %{_bindir} and %{_sbindir}.
[OK] %install setup.py install -O1 --skip-build. as requested for python packages.
=====
[? ] license indication matches upstream web site
"Storm is licensed under the [WWW] LGPL 2.1. Contributions must 
    have copyright assigned to Canonical."
- copyright assigned seems to infer additional requirements ?
- "... Lesser General Public License as
# published by the Free Software Foundation; either version 2.1 of
# the License, or (at your option) any later version"
    so LGPLv2+ seems correct.
- eg: cache.py has no licence header - should it be requested ?
- tz.py: "PSF License" not sure what that is ?
    not in http://fedoraproject.org/wiki/Licensing
    
[??] %files: perhaps should own only it's python-storm dir and below,
    according to:
    http://fedoraproject.org/wiki/Packaging/Python
    %{python_sitelib}/modulename/*.py
 
[??] python eggs: do we need to do anything with them ?

[??] are/should the unit tests be run as part of the package build ?
    upstream suggests these as important, but is that something fedora packages
usually do ?
    
[??] %files: %exclude %{python_sitelib}/storm/cextensions.c  
    is this the accepted way to do this ?
    
[ !] the main package includes the same files that are in the 3 backend sub packages

[ !] description missed a 't' in 'erm maintainability'.

[??] no time yet to test built package installation functionality aka:
"- SHOULD: The reviewer should test that the package functions as described.
A package should not segfault instead of running, for example."

Michel: pretty close, just a fwe minor tweaks, I think.
Comment 8 David Timms 2008-07-01 08:56:46 EDT
Michel: got a few moments to move this review along ?
Comment 9 Michel Alexandre Salim 2008-07-14 15:25:10 EDT
PSF = Python Software Foundation license, the same license as Python itself.
It's GPL-compatible so it's also LGPL-compatible (see
/usr/share/doc/python-2.5.1/LICENSE)

As for the copyright-assignment, that's the same rule used by the Free Software
Foundation; I believe it only applies to contributions that is to be
incorporated into upstream.

I'll ask upstream about the missing copyright; the commit log for that file
shows that Gustavo Niemeyer, who writes the rest of Storm, committed it, so
unless we hear otherwise it should probably be OK.

Unit tests are now run (need to probably ask the maintainer of rpmdevtools to
add it to the skeleton specs)

I've now included only %{python_sitelib}/storm* -- this includes the egginfo
automatically, and lets us avoid having to use an %if test -- the test will get
annoying once RHEL's python supports eggs as well

The last thing is the exclusion of files -- it achieves the same result as using
rm, and is safer as you can't accidentally clobber files outside the buildroot.

http://salimma.fedorapeople.org/for_review/python/python-storm-0.12-2.fc9.src.rpm
Comment 10 David Timms 2008-07-14 21:48:24 EDT
OK, thanks for the update, the changes take into account my previous concerns.

(In reply to comment #9)
> Unit tests are now run
...
Ran 0 tests in 0.000s

OK
(tests=0, failures=0, errors=0)

Running doctests...
[tests/tutorial.txt]
(tests=124, failures=0, errors=0)

Total test cases: 1809
Total doctests: 124
Total failures: 0
Total errors: 0
...
I can see that the unit tests are now run, and succeed. The following questions
are more for my own learning, rather than highlighting any particular issue:
- Is it normal for the unit test outputs to be echoed ?
- In the fedora build system, does that mean the unit test results will appear
in the build.log ?
- If a test fails, does that stop the build process automatically {ie at the end
of the unit tests}, so that a test failing means the package build wont complete ?
- Or does a packager have to manually check the build.log to confirm %check
succeeded ?

> The last thing is the exclusion of files -- it achieves the same result as
> using rm, and is safer as you can't accidentally clobber files outside the
> buildroot.
That's a good tip {that should be in PackagingGuidelines ?}.

* new: When it's in the repo proper, it seems that yum install python-storm
would also install one of the backends, to satisfy Requires: python-storm-backend

I guess yum will choose either the alphabetically lowest or highest of the 
3 backends, apparently randomly ? I think this would be normal for rpm packages
though, wouldn't it ?

* new: tutorial.txt - do you think it should be placed with docs {although it is
easy to find if user goes to the package's url} ?

* I ran through the tutorial itself, and found only one issue: the last part of
the tutorial - Debugging
   1 >>> import sys
   2 >>> from storm.tracer import debug

This gives me:
>>> import sys
>>> from storm.tracer import debug
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: No module named tracer

I searched for debug and tracer in the storm source, and it doesn't get
mentioned. Perhaps this is a separate source, incomplete upstream, or did the
packaging miss something ?
Comment 11 David Timms 2008-07-26 08:19:03 EDT
Michel: I asked a while ago about the tutorial not working with the release:
https://answers.launchpad.net/storm/+question/39285
but haven't had any answer yet, but that isn't really relevant to the review.

In fact, looking back over my last queries, I don't see anything that should
stop the approval of the package.

This package (python-storm) is APPROVED.
Comment 12 Michel Alexandre Salim 2008-08-30 00:55:27 EDT
New Package CVS Request
=======================
Package Name: python-storm
Short Description: An object-relational mapper (ORM) for Python
Owners: salimma
Branches: EL-5 F-8 F-9
InitialCC:
Comment 13 Kevin Fenzi 2008-08-30 16:51:38 EDT
cvs done.
Comment 14 Fedora Update System 2008-08-31 00:39:54 EDT
python-storm-0.13-1.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/python-storm-0.13-1.fc9
Comment 15 Fedora Update System 2008-08-31 00:40:42 EDT
python-storm-0.13-1.fc8 has been submitted as an update for Fedora 8.
http://admin.fedoraproject.org/updates/python-storm-0.13-1.fc8
Comment 16 Fedora Update System 2008-09-01 18:54:58 EDT
python-storm-0.13-2.fc8 has been submitted as an update for Fedora 8.
http://admin.fedoraproject.org/updates/python-storm-0.13-2.fc8
Comment 17 Fedora Update System 2008-09-01 18:55:27 EDT
python-storm-0.13-2.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/python-storm-0.13-2.fc9
Comment 18 Fedora Update System 2008-09-10 02:59:36 EDT
python-storm-0.13-2.fc8 has been pushed to the Fedora 8 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update python-storm'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F8/FEDORA-2008-7365
Comment 19 Fedora Update System 2008-09-10 03:16:35 EDT
python-storm-0.13-2.fc9 has been pushed to the Fedora 9 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update python-storm'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F9/FEDORA-2008-7390
Comment 20 Fedora Update System 2008-09-24 20:10:06 EDT
python-storm-0.13-2.fc8 has been pushed to the Fedora 8 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 21 Fedora Update System 2008-09-24 20:23:05 EDT
python-storm-0.13-2.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.