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=608332 --- Comment #7 from Steve Traylen <steve.traylen@xxxxxxx> 2010-11-19 16:34:57 EST --- Michael and Jussi, all very valuable, thankyou. I've tried to comment on everything: __ONE__ >NOT OK specfile is properly named, is cleanly written and uses macros >consistently. This was use of $RPM_BUILD_ROOT consistently but also use of %{_bindir} that was questioned. As was commented this is fine unlike using $RPM_BUILD_ROOT and %{buildroot}. I have change %{} everywhere just as it is tidier I agree. __TWO__ >NOT OK latest version is being packaged. I have updated to 2.2.1 __THREE__ > You can use "python" directly. I could for sure,... but the reason for choosing %{__python} is that I hope to offer the package on EPEL5 with python26 pending root appearing for python26 within EPEL5 and then this will make it easier to build a secondary module for python26. __FOUR__ Also, running the first example http://packages.python.org/rootplot/plot_directive/pyplots/first.py through python gives Traceback (most recent call last): File "first.py", line 1, in <module> import rootplot.root2matplotlib as r2m ImportError: No module named rootplot.root2matplotlib As commented this has been corrected with latest upstream. __FIVE__ >Still, IMHO using wildcards when they are not absolutely necessary is a bit of >bad style; for instance you won't notice if e.g. egg-info isn't built for some >reason. I highly recommend being more verbose, i.e. using >%{python_sitelib}/root2matplotlib/ >%{python_sitelib}/rootplot_scripts/ >%{python_sitelib}/rootplot-%{version}-py*.egg-info >instead of the needlessly general wildcard glob. Okay I agree using %{python_sitelib}/* is lazy. I have gone for a middle ground of %{python_sitelib}/rootplot-%{version}-* %dir %{python_sitelib}/rootplot %{python_sitelib}/rootplot/* and all these name spaces are definitely mine as it were. __SIX__ >Patch0 is missing a comment in the spec file. Please document its purpose. Done. # Removes some #!/usr/bin/env python from some python libs. Patch0: rootplot-rm-shbangs.patch __SEVEN__ >>I'm not sure why you want to use the conditionals in >>%if ! (0%{?fedora} > 12 || 0%{?rhel} > 5) >>%{!?python_sitelib: %global python_sitelib %(%{__python} -c "from >> distutils.sysconfig import get_python_lib; print(get_python_lib())")} >>%endif I add conditionals since it's then obvious that in the future when RHEL5 and FEDORA12 are dead and berried this can be removed completely. ... I'm keeping it. http://cern.ch/straylen/rpms/rootplot/rootplot.spec http://cern.ch/straylen/rpms/rootplot/rootplot-2.2.1-1.fc13.src.rpm are new packages, I'll leave them here for a week or so before requesting GIT in case you have any comments. Thanks again, Steve. -- 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