Bug 507697

Summary: Review Request: python-migrate0.5 - schema migration tools for SQLAlchemy
Product: [Fedora] Fedora Reporter: Luke Macken <lmacken>
Component: Package ReviewAssignee: Alan Pevec (Fedora) <apevec>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: a.badger, apevec, apevec, fedora-package-review, notting, pfrields, toshio
Target Milestone: ---Flags: apevec: fedora-review+
kevin: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: python-migrate0.5-0.5.3-7.el5 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2010-03-17 19:11:52 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On: 507695    
Bug Blocks: 468804    
Attachments:
Description Flags
Fix unittests none

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.