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=741945 --- Comment #3 from James Laska <jlaska@xxxxxxxxxx> 2011-09-30 08:19:54 EDT --- (In reply to comment #2) > Overall looks good, koji build is green Thanks for the review! > http://koji.fedoraproject.org/koji/taskinfo?taskID=3388155 > > A few nits > > * BuildRoot is no longer needed and should be removed Thanks, removed. I also replaced the use of $RPM_OPT_FLAGS with %{optflags} for consistent macro use. > * same w/ the rm buildroot in the install and clean sections Looks like that isn't required for F13 and newer. I'll probably leave this for EL6 packages, unless I can confirm that it's no longer needed there either. > * Do we still need the exceptions for the older non-supported Fedora versions? > I see these in the python guidelines though so if they are required you can > just ignore this note I left them mainly for EL5 or EL6 compat. I could be pedantic, and they're just not needed anymore. I'll try to confirm. > * is python-devel required as a build time dependency (again see this in the > python guideliness, but this is my first python package review so I could be > mistaken) Excellent catch. Normally, yes. The package python-devel provides the distutils library which is needed by many python packages that 'import distutils' from inside their setup.py. In the case of isodate, it's doing 'from setuptools import setup'. For that, I've added a BuildRequires: python-setuptools. *BUT*, I missed that I'm relying on distutils for the %{python_sitelib} macro. So it is needed. Thanks! > From what I can tell, everything else looks good, so APPROVED Thanks again. I updated the .spec and src.rpm posted from comment#0, and will request SCM shortly. -- 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. _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review