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).
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
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
Koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=377067
[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.
No reason not to update; the review request has just been languishing without attention. Updated package should be done by tomorrow.
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
! 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.
Michel: got a few moments to move this review along ?
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
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 ?
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.
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:
cvs done.
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
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
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
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
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
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
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.
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.