[Bug 476600] Review Request: python-ZODB3 - Zope Object Database: Object Database and Persistence

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=476600





--- Comment #3 from Mads Kiilerich <mads at kiilerich.com>  2009-10-28 12:01:00 EDT ---
>> Mock build fails.
> 
> Against what? Rawhide? Fedora 11? Fedora 10? As you noted lower down in your
> initial review, there's a Python 2.6 incompatibility in this (Fedora 9!)
> package. I will update to the newer version later today, which should fix this
> issue.

Sorry, forget to say that I am using rawhide/f12. I assumed that was the
default for new packages and didn't consider the age of the review request.

>> > Source should not be modified in %prep; it is a kind of build process and
>> > belongs in %build. Has this problem with invalid hash-bangs been filed and
>> > discussed upstream?
> This is complete fiction. %prep is the place for modifying source -- patches,
> sed scriptlets, etc go here. Nothing has been filed upstream, because it's
> fairly minor change IMO. I would be glad to contact upstream about it if that's
> a blocker.

It wasn't intended to be fiction, but you are right that I was wrong. Sorry.

>> I am not familiar with CFLAGS combined with setup.py. I assume that the right
>> flags from python-devel is used automatically. I think the python-numeric
>> package is similar to this one in many ways, and even though it does many
>> things wrong it can perhaps be used for reference: It do not list CFLAGS. Can
>> you reference any documentation or prior art from other packages?
> 
> file:///etc/rpmdevtools/spectemplate-python.spec (if you have the rpmdevtools
> package installed).

Right. Diving into it I can see that many packages does it that way - but also
many noarch packages. And many python arch packages doesn't do it. It seems to
me like the Python packaging guidelines could use some clarification here?

>> A %check section should either be there or not - not just contain a disabled
>> checkout without further comments.
> 
> I forget why I have it disabled, but probably the tests either fail, or
> python-zope-testing wasn't yet in Fedora when this spec file was originally
> written. I think leaving a way to remember how to run the tests is valuable
> even if they are disabled currently.

Doesn't that mean that we need a comment about why it is disabled?

>> It seems like this package contains 4 quite independent python modules.
>> Consider putting them in independent packages: python-BTrees python-persistent
>> python-ZEO python-ZODB
> 
> I don't see any reason to; AFAIK they come from one upstream and are used only
> by themselves.

That might be true, even though I assume they are separate modules because
upstream considers them independent?

I don't feel comfortable with a package reserving such a generic term as
"persistent" from the global Python module namespace just for its internal use.

>> Hmm ... this brief review ends up being longer than the spec. IMHO there are
>> too many issues here. The packager should have worked more with the package
>> before filing a review request.
> 
> Off-topic and rude -- not to mention, plenty of the assertions in this "brief
> review" were incorrect.

Sorry, that wasn't the intention. The failing mock build and the pylint output
made me think that is wasn't ready for review yet and I was wasting my time.
One year of bitrot can however explain it. I guess I should have asked if it
still was ready before looking at it?

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.