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@xxxxxxxxxxxxx> 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. _______________________________________________ Fedora-package-review mailing list Fedora-package-review@xxxxxxxxxx http://www.redhat.com/mailman/listinfo/fedora-package-review