Bug 507697 - Review Request: python-migrate0.5 - schema migration tools for SQLAlchemy
Summary: Review Request: python-migrate0.5 - schema migration tools for SQLAlchemy
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Alan Pevec (Fedora)
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 507695
Blocks: 468804
TreeView+ depends on / blocked
 
Reported: 2009-06-23 19:47 UTC by Luke Macken
Modified: 2016-09-20 02:39 UTC (History)
7 users (show)

Fixed In Version: python-migrate0.5-0.5.3-7.el5
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-03-17 19:11:52 UTC
Type: ---
Embargoed:
apevec: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)
Fix unittests (1.60 KB, patch)
2009-10-02 01:54 UTC, Toshio Ernie Kuratomi
no flags Details | Diff

Description Luke Macken 2009-06-23 19:47:30 UTC
Spec URL: http://lmacken.fedorapeople.org/rpms/python-migrate0.5.spec
SRPM URL: http://lmacken.fedorapeople.org/rpms/python-migrate0.5-0.5.3-3.fc10.src.rpm
Description:
Schema migration tools for SQLAlchemy designed to support an agile approach
to database design and make it easier to keep development and production
databases in sync as schema changes are required.  It allows you to manage.
atabase change sets and database repository versioning.

Comment 1 Luke Macken 2009-06-23 19:48:09 UTC
This is the 0.5 package of python-migrate, designed to work alongside of 0.4 python-migrate for Fedora 11 and below.

Comment 2 Parag AN(पराग) 2009-06-24 07:28:26 UTC
What is need of this package in F11 and below? I see python-migrate-0.5.3-1.fc11 
as latest package in F-11

Comment 3 Toshio Ernie Kuratomi 2009-06-25 02:51:54 UTC
Correct.  This package is needed for EPEL4/5.  It could be used on F-10 as well although I don't know if lmacken plans on branching it there.

Comment 4 Alan Pevec (Fedora) 2009-09-29 11:32:18 UTC
Local build on F10 fails %check when both python-sqlalchemy0.5-0.5.5-1.fc10.noarch and python-sqlalchemy-0.4.8-1.fc10.noarch are installed:
pkg_resources.VersionConflict: (SQLAlchemy 0.4.8 (/usr/lib/python2.5/site-packages), Requirement.parse('sqlalchemy>=0.5'))

In mock it fails with:
File "/builddir/build/BUILD/sqlalchemy-migrate-0.5.3/migrate/versioning/base/const.py", line 7, in <module>
from sqlalchemy.util import OrderedDict
ImportError: No module named sqlalchemy.util

root.log shows that python-sqlalchemy0.5 does get installed in the mock chroot, but probably pkg_resources.require should be used instead of import?

Comment 5 Alan Pevec 2009-10-01 09:37:07 UTC
Same error with koji build --scratch dist-5E-epel-testing-candidate ~/Download/python-migrate0.5-0.5.3-3.fc10.src.rpm

Luke, do you want to fix this or just drop %check ?

Comment 6 Luke Macken 2009-10-01 18:55:23 UTC
Ugh, I loathe setuptools.

So, with this patch, the test suite runs fine:

    --- migrate/__init__.py.orig    2009-10-01 09:44:50.865973980 -0400
    +++ migrate/__init__.py 2009-10-01 09:44:51.972973558 -0400
    @@ -4,3 +4,6 @@
        :mod:`migrate.changeset` that allows to define database schema changes
        using Python.
     """
    +__requires__ = 'SQLAlchemy>=0.5'
    +import pkg_resources
    +pkg_resources.require('SQLAlchemy>=0.5')

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

However, when adding python-sqlalchemy (0.3) to the build requires, it explodes with a VersionConflict.

I've lost many a day in setuptools-hell.  Not today though, so I'm in favor of dropping the %check until we can figure this out.

Comment 7 Toshio Ernie Kuratomi 2009-10-01 21:35:05 UTC
You probably don't want to drop the %check as it's alerting you to a real problem.  Looking into this....

Comment 8 Toshio Ernie Kuratomi 2009-10-02 00:19:49 UTC
Okay adding:
    __requires__ = 'SQLAlchemy>=0.5'
    import pkg_resources
    pkg_resources.require('SQLAlchemy>=0.5')

to the top of setup.py fixes the tests *that use the inprocess python*.  The tests in test_shell.py are not fixed by this because they fork a subprocess to do their work.  In production, the script that invokes these is a light wrapper created by setuptools.  Those use __requires__='' in order to perform that function.  In the test, the migrate/versioning/shell.py file is being invoked directly.  So __requires__ is not being set.

There's several ways to fix these tests.  We could have a wrapper that mimics setuptool's wrapper.  We could add __requires__ ; import pkg_resources to versioning/shell.py.  We could set the PYTHON_PATH environment variable in test_shell.py when it invokes the shell.py script so that it finds the proper SQLAlchemy.

Of these, I think that the best one for upstream is the wrapper.  A wrapper is how the script will be invoked from the shell and the test purports to test invokation of the script from the shell so it matches up best.  Second best (and probably the easiest if we aren't going to get it upstream as well) is the environment variable solution.  I'll go about making this happen.

Comment 9 Toshio Ernie Kuratomi 2009-10-02 01:54:17 UTC
Created attachment 363412 [details]
Fix unittests

As promised, one part of this adds the __requires__ lines to setup.py which fixes most of the unittests.  The other adds the sqlalchemy path to PYTHONPATH before we call shell.py as a script.  koji scratch build that works even when we have both python-sqlalchemy-0.3 and python-sqlachemy0.5-0.5.5 installed.

Comment 10 Luke Macken 2009-10-11 04:53:27 UTC
Thank you for the patch, Toshio.  However, I am still having the VersionConflict, in various modules, when both SA 0.3 and 0.5 are installed at the same time and I try to pkg_resources.require it.

Comment 11 Luke Macken 2009-10-13 23:03:31 UTC
Ignore my previous comment, I was using an older broken version of the package at the time.

http://lmacken.fedorapeople.org/rpms/python-migrate0.5-0.5.3-6.fc11.src.rpm
http://lmacken.fedorapeople.org/rpms/python-migrate0.5.spec

* Tue Oct 13 2009 Luke Macken <lmacken> - 0.5.3-6
- Patch from Toshio to get the test suite running (#507697)

Comment 12 Alan Pevec 2009-10-18 12:09:50 UTC
Getting back to this review, scratch EPEL build worked:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1745762

* in PKG-INFO there's License: MIT which is enough, but packager SHOULD query upstream to include the license text file

* there's typo in %description:
 %description
 Schema migration tools for SQLAlchemy designed to support an agile approach
 to database design and make it easier to keep development and production
-databases in sync as schema changes are required.  It allows you to manage 
-atabase change sets and database repository versioning.
+databases in sync as schema changes are required.  It allows you to manage
+database change sets and database repository versioning.

* in python_sitelib definition, %global should be used instead of %define:
https://fedoraproject.org/wiki/Packaging/Python#System_Architecture

* rpmlint complaints:
[1] python-migrate0.5.noarch: E: script-without-shebang /usr/lib/python2.4/site-pack
ages/sqlalchemy_migrate-0.5.3-py2.4.egg/migrate/changeset/__init__.py
...(for all .py files in python_sitelib)
[2] python-migrate0.5.noarch: E: zero-length /usr/lib/python2.5/site-packages/easy-install.pth
[3] python-migrate0.5.noarch: E: non-executable-script /usr/lib/python2.5/site-packages/sqlalchemy_migrate-0.5.3-py2.5.egg/migrate/versioning/templates/manage.py_tmpl 0644 /usr/bin/env


ad [1] if easy_install cannot be made to install modules with 644, workaround could be to use defattr:

+%defattr(0644,root,root,0755)
 %{python_sitelib}/*

ad [2] there's touch easy-install.pth in spec, removing it avoids this and easy_install still works fine

ad [3] .py_templ should probably be moved to docs and marked %doc

Comment 13 Luke Macken 2009-10-19 18:03:11 UTC
http://lmacken.fedorapeople.org/rpms/python-migrate0.5-0.5.3-7.fc11.src.rpm
http://lmacken.fedorapeople.org/rpms/python-migrate0.5.spec

* Mon Oct 19 2009 Luke Macken <lmacken> - 0.5.3-7
- Use %%global instead of %%define
- Fix a typo in the description
- Fix module permissions
- Remove easy-install.pth

With regard to [3], those are Paster templates that migrate uses to generate various migration scripts, afaict.  I'm not sure if we want to mark them as docs.

Comment 14 Luke Macken 2009-10-31 23:11:35 UTC
ping?

Comment 15 Alan Pevec (Fedora) 2009-11-06 18:27:46 UTC
Sorry for the delay, let's wrap this up today!

MUST Items:
[+] MUST: rpmlint must be run on every package.

python-migrate0.5.noarch: E: non-executable-script /usr/lib/python2.4/site-packages/sqlalchemy_migrate-0.5.3-py2.4.egg/migrate/versioning/templates/manage.py_tmpl 0644 /usr/bin/env

=> ok, this is a template, not a script.

[+] MUST: The package must be named according to the Package Naming Guidelines.
[+] MUST: The spec file name must match the base package %{name}
[+] MUST: The package must meet the Packaging Guidelines.
[+] MUST: The package must be licensed with a Fedora approved license and meet
the Licensing Guidelines.
[+] MUST: The License field in the package spec file must match the actual
license.
[n/a] MUST: 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.
[+] MUST: The spec file must be written in American English.
[+] MUST: The spec file for the package MUST be legible.
[+] MUST: The sources used to build the package must match the upstream source,
as provided in the spec URL.

8ce48470efac8cbcdf4ee9af725efd70  sqlalchemy-migrate-0.5.3.tar.gz

[+] MUST: The package must successfully compile and build into binary rpms on
at least one supported architecture.

scratch build in Koji dist-5E-epel-testing-candidate:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1791956

also builds on RHEL5.4 with python-migrate-0.4.5-3.el5 installed

[n/a] MUST: If the package does not successfully compile, build or work on an
architecture, then those architectures should be listed in the spec in
ExcludeArch.
[+] MUST: All build dependencies must be listed in BuildRequires
[n/a] MUST: The spec file MUST handle locales properly. This is done by using
the %find_lang macro.
[n/a] MUST: Every binary RPM package which stores shared library files (not
just symlinks) in any of the dynamic linker's default paths, must call ldconfig
in %post and %postun.
[n/a] MUST: If the package is designed to be relocatable, the packager must
state this fact in the request for review
[+] MUST: 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.
[+] MUST: A package must not contain any duplicate files in the %files listing.
[+] MUST: Permissions on files must be set properly. Executables should be set
with executable permissions, for example. Every %files section must include a
%defattr(...) line.
[+] MUST: Each package must have a %clean section, which contains rm -rf
%{buildroot} (or $RPM_BUILD_ROOT).
[+] MUST: Each package must consistently use macros, as described in the macros
section of Packaging Guidelines.
[+] MUST: The package must contain code, or permissible content. This is
described in detail in the code vs. content section of Packaging Guidelines.
[+] MUST: Large documentation files should go in a doc subpackage.
[+] MUST: If a package includes something as %doc, it must not affect the
runtime of the application.
[n/a] MUST: Header files must be in a -devel package.
[n/a] MUST: Static libraries must be in a -static package.
[n/a] MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'
(for directory ownership and usability).
[n/a] MUST: If a package contains library files with a suffix (e.g.
libfoo.so.1.1), then library files that end in .so (without suffix) must go in
a -devel package.
[n/a] MUST: In the vast majority of cases, devel packages must require the base
package using a fully versioned dependency: Requires: %{name} =
%{version}-%{release}
[n/a] MUST: Packages must NOT contain any .la libtool archives, these should be
removed in the spec.
[n/a] MUST: 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.
[+] MUST: Packages must not own files or directories already owned by other
packages.
[+] MUST: At the beginning of %install, each package MUST run rm -rf
%{buildroot} (or $RPM_BUILD_ROOT).
[+] MUST: All filenames in rpm packages must be valid UTF-8.


APPROVED for EL-5 and F-10(if needed) since >= F-11 have python-migrate >= 0.5

Comment 16 Luke Macken 2009-11-10 19:45:03 UTC
New Package CVS Request
=======================
Package Name: python-migrate0.5
Short Description: schema migration tools for SQLAlchemy 
Owners: lmacken toshio
Branches: EL-5

Comment 17 Kevin Fenzi 2009-11-11 03:36:30 UTC
cvs done.

Comment 18 Fedora Update System 2009-11-11 04:54:53 UTC
python-migrate0.5-0.5.3-7.el5 has been submitted as an update for Fedora EPEL 5.
http://admin.fedoraproject.org/updates/python-migrate0.5-0.5.3-7.el5

Comment 19 Fedora Update System 2009-11-11 17:20:31 UTC
python-migrate0.5-0.5.3-7.el5 has been pushed to the Fedora EPEL 5 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-migrate0.5'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/EL-5/FEDORA-EPEL-2009-0839

Comment 20 Fedora Update System 2010-03-17 19:11:41 UTC
python-migrate0.5-0.5.3-7.el5 has been pushed to the Fedora EPEL 5 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.