Hide Forgot
Spec URL: http://cern.ch/straylen/rpms/rootplot/rootplot.spec SRPM URL: http://cern.ch/straylen/rpms/rootplot/rootplot-1.1-1.fc13.src.rpm Description: ROOT is a powerful data analysis tool within the particle physics community, and the primary lens through which we see our data. The framework includes quite a bit of graphical capabilities, but producing high-quality graphics output was not the first priority in designing its capabilities or its interface. It becomes useful, then, to consider using an outside library focused on graphics for producing final plots. The pyROOT interface to ROOT makes it easy to have ROOT objects interact with other python modules. The goal of rootplot is to enable easy plotting of ROOT histograms using the full-featured and mature matplotlib library.
Informal review: OK source files match upstream: 5095f72025132711c472b5a0db6417be13492b6ce09a74dfea9afe27165a6000 rootplot-1.1.tar.gz OK package meets naming and versioning guidelines. NOT OK specfile is properly named, is cleanly written and uses macros consistently. OK dist tag is present. OK build root is correct. OK license field matches the actual license. OK license is open source-compatible. license text not included upstream. NOT OK latest version is being packaged. OK BuildRequires are proper. OK %clean is present. OK package builds in koji. http://koji.fedoraproject.org/koji/taskinfo?taskID=2608031 OKpackage installs properly. OK rpmlint is silent. (3 spellcheck warnings which can be ignored.) OK final provides and requires are sane: rpm -q --provides rootplot rootplot = 1.1-1.fc14 pm -q --requires rootplot /usr/bin/python numpy python(abi) = 2.7 python-matplotlib root-python rpmlib(CompressedFileNames) <= 3.0.4-1 rpmlib(FileDigests) <= 4.6.0-1 rpmlib(PartialHardlinkSets) <= 4.0.4-1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1 rpmlib(PayloadIsXz) <= 5.2-1 N/A %check is present and all tests pass: N/A no shared libraries are added to the regular linker search paths. OK owns the directories it creates. NOT OK doesn't own any directories it shouldn't. OK no duplicates in %files. OK file permissions are appropriate. OK no scriptlets present. OK documentation is small, so no -docs subpackage is necessary. OK %docs are not necessary for the proper functioning of the package. OK no headers. OK no pkgconfig files. OK no libtool .la droppings. N/A desktop files valid and installed properly. So the remaining issues are: - Use macros rather than vars consistently (e.g. buildroot). - You can use "python" directly. - The latest upstream version is 2.2. - Use %{python_sitelib}/root2matplotlib etc. so that you do not own all of sitelib. 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 Adjusting the import line results in a segmentation violation, which may very well be ROOT's fault.
As a more constructive side remark: Rebuilding the rpms with current source (rootplot 2.2) and adjusting as necessary makes the mentioned example run without a glitch. So this will be OK with an updated spec.
Actually rpmlint output is rootplot.noarch: E: explicit-lib-dependency python-matplotlib rootplot.noarch: W: spelling-error Summary(en_US) matplotlib -> matelote, matrilocal, marplot rootplot.noarch: W: spelling-error %description -l en_US pyROOT -> pyrostat, pyrope, pyrone rootplot.noarch: W: spelling-error %description -l en_US matplotlib -> matelote, matrilocal, marplot rootplot.noarch: W: no-manual-page-for-binary rootplot rootplot.noarch: W: no-manual-page-for-binary tree2hists rootplot.src: W: spelling-error Summary(en_US) matplotlib -> matelote, matrilocal, marplot rootplot.src: W: spelling-error %description -l en_US pyROOT -> pyrostat, pyrope, pyrone rootplot.src: W: spelling-error %description -l en_US matplotlib -> matelote, matrilocal, marplot 2 packages and 0 specfiles checked; 1 errors, 8 warnings. All of these are OK, though. The explicit lib dependency is only relevant in binary packages, where RPM automatically adds dependencies on the necessary shared libraries. (In reply to comment #1) > So the remaining issues are: > > - Use macros rather than vars consistently (e.g. buildroot). Actually, the spec file does not mix styles. The only thing this applies is using $RPM_OPT_FLAGS vs %{optflags} and $RPM_BUILD_ROOT vs %{buildroot}. The choice between these is a matter of personal preference. > - You can use "python" directly. Yes, there's not much sense in using macros for standard stuff, such as %{__rm}, %{__mv}, %{__mkdir}, %{__mkdir_p} and so on even if they are implemented in RPM. Although, using them is not forbidden. > - The latest upstream version is 2.2. > - Use %{python_sitelib}/root2matplotlib etc. so that you do not own all of > sitelib. This is not a problem per se, the rootplot package only ends up owning the directories that are in %{python_sitelib} *in the build root*. What ends up being owned by %{python_sitelib}/* is (on F14) /usr/lib/python2.7/site-packages/root2matplotlib /usr/lib/python2.7/site-packages/rootplot-1.1-py2.7.egg-info /usr/lib/python2.7/site-packages/rootplot_scripts 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. - Patch0 is missing a comment in the spec file. Please document its purpose, e.g. # Patch to remove shbangs from Python library Patch0: rootplot-rm-shbangs.patch - 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'd scrap the %if stuff altogether. *** Review: MUST: The package does not yet exist in Fedora. The Review Request is not a duplicate. OK MUST: The spec file for the package is legible and macros are used consistently. OK MUST: The package must be named according to the Package Naming Guidelines. OK MUST: The spec file name must match the base package %{name}. OK MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines. OK MUST: The License field in the package spec file must match the actual license. OK - License is MIT Modern style with sublicense http://fedoraproject.org/wiki/Licensing/MIT MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. OK 016f11836fb3764dd89dd5f0c290816b rootplot-1.1.tar.gz 016f11836fb3764dd89dd5f0c290816b ../SOURCES/rootplot-1.1.tar.gz MUST: The package MUST successfully compile and build into binary rpms. OK MUST: The spec file MUST handle locales properly. N/A MUST: Optflags are used and time stamps preserved. OK MUST: Packages containing shared library files must call ldconfig. N/A MUST: A package must own all directories that it creates or require the package that owns the directory. OK MUST: Files only listed once in %files listings. OK MUST: Debuginfo package is complete. OK MUST: Permissions on files must be set properly. OK MUST: Large documentation files must go in a -doc subpackage. N/A MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. ~OK - Maybe add PKG-INFO to %doc? MUST: Header files must be in a -devel package. N/A MUST: Static libraries must be in a -static package. N/A MUST: If a package contains library files with a suffix then library files ending in .so 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. N/A MUST: Packages does not contain any .la libtool archives. N/A MUST: Desktop files are installed properly. N/A MUST: No file conflicts with other packages and no general names. OK SHOULD: %{?dist} tag is used in release. OK SHOULD: If the package does not include license text(s) as separate files from upstream, the packager should query upstream to include it. NEEDSWORK - No license texts in the tarball. SHOULD: The package builds in mock. OK EPEL: Clean section exists. OK EPEL: Buildroot cleaned before install. OK EPEL: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'. N/A ** Please tend to the issues reported above before import to git. Especially update to newest upstream as instructed by Michael in comment #2, and check that running the examples work. This package has been APPROVED
(In reply to comment #3) > Actually rpmlint output is > rootplot.noarch: E: explicit-lib-dependency python-matplotlib > rootplot.noarch: W: spelling-error Summary(en_US) matplotlib -> matelote, > matrilocal, marplot > rootplot.noarch: W: spelling-error %description -l en_US pyROOT -> pyrostat, > pyrope, pyrone > rootplot.noarch: W: spelling-error %description -l en_US matplotlib -> > matelote, matrilocal, marplot > rootplot.noarch: W: no-manual-page-for-binary rootplot > rootplot.noarch: W: no-manual-page-for-binary tree2hists > rootplot.src: W: spelling-error Summary(en_US) matplotlib -> matelote, > matrilocal, marplot > rootplot.src: W: spelling-error %description -l en_US pyROOT -> pyrostat, > pyrope, pyrone > rootplot.src: W: spelling-error %description -l en_US matplotlib -> matelote, > matrilocal, marplot > 2 packages and 0 specfiles checked; 1 errors, 8 warnings. On my side that was rpmlint on spec and srpm. Sorry. > > - Use macros rather than vars consistently (e.g. buildroot). > > Actually, the spec file does not mix styles. The only thing this applies is > using $RPM_OPT_FLAGS vs %{optflags} and $RPM_BUILD_ROOT vs %{buildroot}. The > choice between these is a matter of personal preference. It uses different styles for different variables ($RPM_BUILD_ROOT vs. %{_bindir}). If that's not a problem I'm fine with it. > > - Use %{python_sitelib}/root2matplotlib etc. so that you do not own all of > > sitelib. > > This is not a problem per se, the rootplot package only ends up owning the > directories that are in %{python_sitelib} *in the build root*. I've heard someone say - The %files section needs improvement. The statement %{python_sitearch}/ ends up owning the system python directory, which is not wanted. Is this different?
(In reply to comment #4) > (In reply to comment #3) > > Actually, the spec file does not mix styles. The only thing this applies is > > using $RPM_OPT_FLAGS vs %{optflags} and $RPM_BUILD_ROOT vs %{buildroot}. The > > choice between these is a matter of personal preference. > > It uses different styles for different variables ($RPM_BUILD_ROOT vs. > %{_bindir}). If that's not a problem I'm fine with it. Yes, this is no problem. There's no shell variable declared by rpmbuild to point to %{_bindir}. However, the functions of $RPM_BUILD_ROOT and %{buildroot} are identical, and so are those of $RPM_OPT_FLAGS and %{optflags}. > I've heard someone say > > - The %files section needs improvement. The statement > %{python_sitearch}/ > ends up owning the system python directory, which is not wanted. > > Is this different? You had %{python_sitearch}/ which indeed ends up owning the system python directory. Here Steve has %{python_sitearch}/* which only ends up owning whatever is in the system python directory of the buildroot.
(In reply to comment #5) > (In reply to comment #4) > > I've heard someone say > > > > - The %files section needs improvement. The statement > > %{python_sitearch}/ > > ends up owning the system python directory, which is not wanted. > > > > Is this different? > > You had > %{python_sitearch}/ > which indeed ends up owning the system python directory. Here Steve has > %{python_sitearch}/* > which only ends up owning whatever is in the system python directory of the > buildroot. The star makes the difference. Thanks for the clarification! -- Fedora Bugzappers volunteer triage team https://fedoraproject.org/wiki/BugZappers
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.
(In reply to comment #7) > __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. .. does this even build? You're missing the directory %{python_sitelib}/rootplot_scripts/ along with its contents. As a stylistic issue, I don't see any point in using %dir %{python_sitelib}/rootplot %{python_sitelib}/rootplot/* since both %{python_sitelib}/rootplot or %{python_sitelib}/rootplot/ have the same function. I prefer the latter version, which demonstrates that this is a directory. > 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. OK. I'm just saying nothing breaks even if you don't use the conditionals at all and just declare the macro as usual. RHEL5 is going to be around for a long time, still. F12 is practically dead already, it's EOLin in a couple of weeks.
(In reply to comment #8) > .. does this even build? You're missing the directory > %{python_sitelib}/rootplot_scripts/ > along with its contents. > rootplot 2.2 has a different (simpler, better) directory structure, there's only rootplot/ and the egg.
Interesting I had always assumed that > %{python_sitelib}/rootplot >or > %{python_sitelib}/rootplot/ were different but you are correct the second one does include the directory as well. Have opted for %{python_sitelib}/rootplot/ for the same reasons as you suggest. http://cern.ch/straylen/rpms/rootplot/rootplot.spec http://cern.ch/straylen/rpms/rootplot/rootplot-2.2.1-2.fc13.src.rpm Will request in a couple of days. Thanks again. Steve.
New Package SCM Request ======================= Package Name: rootplot Short Description: Plots ROOT data with matplotlib Owners: stevetraylen Branches: f13 f14 el6 InitialCC: I'll make a el5 if when root gets to el5.
Git done (by process-git-requests).
rootplot-2.2.1-3.fc14 has been submitted as an update for Fedora 14. https://admin.fedoraproject.org/updates/rootplot-2.2.1-3.fc14
rootplot-2.2.1-3.fc13 has been submitted as an update for Fedora 13. https://admin.fedoraproject.org/updates/rootplot-2.2.1-3.fc13
rootplot-2.2.1-3.fc13 has been pushed to the Fedora 13 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 rootplot'. You can provide feedback for this update here: https://admin.fedoraproject.org/updates/rootplot-2.2.1-3.fc13
rootplot-2.2.1-3.fc13 has been pushed to the Fedora 13 stable repository. If problems still persist, please make note of it in this bug report.
rootplot-2.2.1-3.fc14 has been pushed to the Fedora 14 stable repository. If problems still persist, please make note of it in this bug report.
Package Change Request ====================== Package Name: rootplot New Branches: el5 Owners: stevetraylen p.s I noticed that el5 is branched from devel where as el6 is branched from f12 which does not obviously make sense. Maybe both should come from devel now. Steve.
rootplot-2.2.1-3.el5 has been submitted as an update for Fedora EPEL 5. https://admin.fedoraproject.org/updates/rootplot-2.2.1-3.el5
rootplot-2.2.1-3.el6 has been submitted as an update for Fedora EPEL 6. https://admin.fedoraproject.org/updates/rootplot-2.2.1-3.el6
rootplot-2.2.1-3.el5 has been pushed to the Fedora EPEL 5 stable repository. If problems still persist, please make note of it in this bug report.
rootplot-2.2.1-3.el6 has been pushed to the Fedora EPEL 6 stable repository. If problems still persist, please make note of it in this bug report.