Red Hat Bugzilla – Bug 485607
Review Request: scitools - A Python library for scientific computing
Last modified: 2009-03-17 19:45:11 EDT
Spec URL: http://jsmidt.fedorapeople.org/SciTools.spec SRPM URL: http://jsmidt.fedorapeople.org/SciTools-0.4-0.fc10.src.rpm Description: SciTools is a Python package containing lots of useful tools for scientific computing in Python. The package is built on top of other widely used packages such as NumPy, SciPy, ScientificPython, Gnuplot, etc. Webpage: http://code.google.com/p/scitools/
- Name should be scitools, not SciTools. * You need to change also "setup -q" to "setup -q -n SciTools-%{version}" - Double Requires: gnuplot, remove the latter one. - Missing BuildRequires: python-devel - Missing python_sitelib macro, see http://fedoraproject.org/wiki/Packaging/Python - Install section should be %install rm -rf $RPM_BUILD_ROOT %{__python} setup.py install --root $RPM_BUILD_ROOT - Files should be %{python_sitelib}/*egg-info %{python_sitelib}/%{name} instead of %{_libdir}/* as the latter version would own system directories, and doesn't work on 64-bit architectures.
- Missing Requires: ScientificPython. - Actually you don't need Requires: gnuplot at all, since gnuplot-py already pulls in gnuplot. - I'd only Requires: numpy, scipy, ScientificPython and gnuplot-py for the base package, and branch the other suggested requirements into an -extras package. At the moment it would pull in python-numeric, python-numarray, vtk-python, python-matplotlib, PyX, veusz (and the rest of the suggested requirements once they get into Fedora).
I don't think I can accept the package even after the aforementioned modifications, since there are files in %{_bindir} that probably shouldn't be there. Also most of them have really general names, which may clash with other packages. /usr/bin/_gnuplot.py /usr/bin/_gnuplot.pyc /usr/bin/_gnuplot.pyo /usr/bin/file2interactive.py /usr/bin/file2interactive.pyc /usr/bin/file2interactive.pyo /usr/bin/floatdiff.py /usr/bin/floatdiff.pyc /usr/bin/floatdiff.pyo /usr/bin/floatdiff.verify /usr/bin/gnuplot.bat /usr/bin/hgtools.py /usr/bin/hgtools.pyc /usr/bin/hgtools.pyo /usr/bin/profiler.py /usr/bin/profiler.pyc /usr/bin/profiler.pyo /usr/bin/ps2mpeg.py /usr/bin/ps2mpeg.pyc /usr/bin/ps2mpeg.pyo /usr/bin/pyreport /usr/bin/regression.py /usr/bin/regression.pyc /usr/bin/regression.pyo /usr/bin/subst.py /usr/bin/subst.pyc /usr/bin/subst.pyo /usr/bin/timer.py /usr/bin/timer.pyc /usr/bin/timer.pyo
Thank you for taking the time to review this package. I have made the changes you have requested. The new package is found here: Spec URL: http://jsmidt.fedorapeople.org/scitools.spec SRPM URL: http://jsmidt.fedorapeople.org/scitools-0.4-0.1.fc10.src.rpm By your last comment, are you saying scitools is a lost cause? Did you want me to remove all files from %{_bindir} which are not executables? Should I move all the files in %{_bindir} to %{_bindir}/scitools? Should I try to rename the files? Is there anything you would suggest? Also, there was a directory full of examples I put in /usr/share/scitools.
(In reply to comment #4) > By your last comment, are you saying scitools is a lost cause? > Did you want me to remove all files from %{_bindir} which are not executables? > Should I move all the files in %{_bindir} to %{_bindir}/scitools? > Should I try to rename the files? > Is there anything you would suggest? > > Also, there was a directory full of examples I put in /usr/share/scitools. No, I suggest you contact upstream and ask them to correct the problem. Moving and/or renaming files should be done only after consultation, since this breaks compatibility with upstream. I'm not very familiar with Python, and so I don't know if the files need to be in %{_bindir}. My guess is, though, that they should be in the package's sitelib directory instead. I'd put the examples in the documentation, since people normally look there for documentation :)
Okay, I have contacted upstream. In the meantime I have added the -extras package and moved examples to the doc section. I took the dependencies for the extras package from your suggestions plus the packages mentioned on the website: http://code.google.com/p/scitools/wiki/Installation Spec URL: http://jsmidt.fedorapeople.org/scitools.spec SRPM URL: http://jsmidt.fedorapeople.org/scitools-0.4-0.2.fc10.src.rpm
Okay, so I didn't get everything on the list :P You are missing python-pmw (goes together with blt). Remove %doc from extras. Requires: grace should be requires: pygrace (which pulls in grace). pygrace does not exist in Fedora yet, but it seemed so simple that I made a package (see review request below). https://bugzilla.redhat.com/show_bug.cgi?id=485617
Okay, here are the latest files with the +python-pmw grace->pygrace and -%doc in extras changes: Spec URL: http://jsmidt.fedorapeople.org/scitools.spec SRPM URL: http://jsmidt.fedorapeople.org/scitools-0.4-0.3.fc10.src.rpm
Feedback from the upstream author: " I've examined the files in the bin directory and removed some of the too specialized files and also modified/updated some of them. Now the directory contains _gnuplot.py gnuplot.bat pyreport file2interactive.py profiler.py regression.py floatdiff.py ps2mpeg.py subst.py All of these, except pyreport, are scripts developed or used in my "Python for Computational Science" book (a new README file in the bin directory briefly explains what the remaining files here are used for). The files may well go to another bin directory (/usr/local/bin, for instance). The only problem I can foresee is that they go to a directory that users do not have in their PATH. Another possibility is to say that these files (except pyreport) are for the mentioned book only, and since this book has a special bin directory I can easily move them there. This is okay with me if SciTools users do not think they lose much. What they lose is basically the following: gnuplot.bat, _gnuplot.py: ability to run Gnuplot on Windows as it is run on Unix file2interactive.py: quick creation of interactive sessions (maybe more a utility for a book writer...) profiler.py: trivial front-end, but makes profiling a one-line command regression.py, floatdiff.py: original tools supporting regression tests with floating-point numbers (i.e., reference results change with hardware) subst.py: cross-platform sed-like substitution command pyreport in this dir allows the original pyreport program to work with scitools.easyviz. Tell me what you think. An update to v0.6 was just committed to the svn repository (incl. the updates mentioned above). "
Should the solution be move all the /bin files to the doc directory except pyreport and maybe a couple others and have a README in the doc directory saying "These /bin files are needed to run the examples from the book, if you want to run these example move them to somewhere in your $PATH such as your current directory or add this directory to your $PATH"?
(In reply to comment #10) > Should the solution be move all the /bin files to the doc directory except > pyreport and maybe a couple others and have a README in the doc directory > saying "These /bin files are needed to run the examples from the book, if you > want to run these example move them to somewhere in your $PATH such as your > current directory or add this directory to your $PATH"? Well, that is one option; another would be to put the files in a package-provided dir that is added in the system $PATH. Looking at the reply for upstream, I'd put the files under %doc, since the package works without them; but this is your decision to make. Just as long as the files don't interfere with other packages. pyreport is a probable troublemaker, since (as the author stated) there is a program called pyreport, even though it is not in Fedora at the moment.
I will take of the review of pygrace and so I am interested in the progress of this package. :-) Joseph is this your first package? Are you sponsored?
This is my first package. With any luck I will try to have the whole thing completed today. :) I don't know about being sponsored, but Jussi above is doing a great job helping me get through the packaging process.
(In reply to comment #13) > This is my first package. With any luck I will try to have the whole thing > completed today. :) I don't know about being sponsored, but Jussi above is > doing a great job helping me get through the packaging process. Well, in that case I can't formally review the package, you need to get a sponsor to do it; see http://fedoraproject.org/wiki/PackageMaintainers/Join#Get_Sponsored
I am sponsor that was why I was asking. :-)
(In reply to comment #15) > I am sponsor that was why I was asking. :-) Well then, that's nice. I didn't check if Joseph was already in the packager database, but the mistakes in the packaging did raise some suspicion in my mind :P
Jussi, please continue the review. You have been doing a fantastic job. :-) I coordinate all the issues regarding Joseph sponsorship. :-) Please give your nod when you think that all issues have been dealt.
(In reply to comment #17) > Jussi, please continue the review. You have been doing a fantastic job. :-) > I coordinate all the issues regarding Joseph sponsorship. :-) > > Please give your nod when you think that all issues have been dealt. Thanks. Okay, if you say so. Reassigning to myself.
Joseph as part of the sponsorship process to show that you understand the packaging review scheme there some steps to be fulfilled. I will ask you to review (without approving) pygrace (mentioned by Jussi above) and any other package that you choose that is not yet reviewed. PS: Talking with someone two timezones away while typing in this thread can result in missing words or worse. ;-) "I am _a_ sponsor..." "I _will_ coordinate..."
Okay, what I decided to do was put bin files in the documentation section with a README.bin also in the %doc to explain what is going on. Spec URL: http://jsmidt.fedorapeople.org/scitools.spec SRPM URL: http://jsmidt.fedorapeople.org/scitools-0.4-0.4.fc10.src.rpm Patch: http://jsmidt.fedorapeople.org/scitools_bin_README.patch I will also start reviewing pygrace.
- Hmm, so README.bin is a new file? Don't supply it as a patch, then, just make it another source. Also, as the file is related to the Fedora package, it should be named README.Fedora as a distinction to other files that have been provided by upstream. - "- Moved _bindir to documentation" is sloppy: the system binary directory has not been moved! The entry should be something like: "- Moved files from %%{_bindir} to documentation to prevent clashes." Note the double %% to prevent rpmbuild from expanding the macro in the changelog. - Your release numbering is a bit unconventional, normally releases are numbered starting from 1 and with integers, not decimals. - Also, please run rpmlint on the releases you create (both SRPM/spec file and compiled binary RPMS including the debuginfo packages), and paste the output in the bugzilla entry (here). This helps both you and the reviewer. I will try to do the review tomorrow.
Okay, I have ran rpmlint and here are the results. (I see there are quite a few warnings and errors. I would fix them, but I am confused how. Could I get a hint?) Spec URL: http://jsmidt.fedorapeople.org/scitools.spec SRPM URL: http://jsmidt.fedorapeople.org/scitools-0.4-1.fc10.src.rpm --------------------------------------------------------------------- rpmlint scitools-0.4-1.fc10.src.rpm scitools.src: W: no-%build-section 1 packages and 0 specfiles checked; 0 errors, 1 warnings. --------------------------------------------------------------------- rpmlint scitools-extras-0.4-1.fc10.i386.rpm: scitools-extras.i386: W: no-documentation scitools-extras.i386: E: explicit-lib-dependency python-matplotlib scitools-extras.i386: E: devel-dependency dx-devel 1 packages and 0 specfiles checked; 2 errors, 1 warnings. --------------------------------------------------------------------- rpmlint scitools-0.4-1.fc10.i386.rpm "scitools.i386: W: spurious-executable-perm /usr/share/doc/scitools-0.4/examples/slice_demo1.py scitools.i386: W: spurious-executable-perm /usr/share/doc/scitools-0.4/examples/slice_demo2.py scitools.i386: E: non-executable-script /usr/lib/python2.5/site-packages/scitools/Gui.py 0644 scitools.i386: E: non-executable-script /usr/lib/python2.5/site-packages/scitools/convergencerate.py 0644 scitools.i386: W: spurious-executable-perm /usr/share/doc/scitools-0.4/examples/isosurface_demo3.py scitools.i386: W: spurious-executable-perm /usr/share/doc/scitools-0.4/examples/streamline_demo1.py scitools.i386: W: spurious-executable-perm /usr/share/doc/scitools-0.4/examples/alternatives/plotdemo2.py scitools.i386: W: spurious-executable-perm /usr/share/doc/scitools-0.4/examples/alternatives/plotdemo4.py scitools.i386: E: non-executable-script /usr/lib/python2.5/site-packages/scitools/StringFunction.py 0644 scitools.i386: W: spurious-executable-perm /usr/share/doc/scitools-0.4/examples/plotdemo0.py scitools.i386: W: spurious-executable-perm /usr/share/doc/scitools-0.4/examples/plotdemo4.py scitools.i386: E: non-executable-script /usr/lib/python2.5/site-packages/scitools/odesolve.py 0644 scitools.i386: W: spurious-executable-perm /usr/share/doc/scitools-0.4/examples/contourslice_demo1.py scitools.i386: E: non-executable-script /usr/lib/python2.5/site-packages/scitools/ParameterInterface.py 0644 scitools.i386: W: spurious-executable-perm /usr/share/doc/scitools-0.4/examples/plotdemo3.py scitools.i386: E: non-executable-script /usr/lib/python2.5/site-packages/scitools/PrmDictBase.py 0644 scitools.i386: E: non-executable-script /usr/lib/python2.5/site-packages/scitools/BoxField.py 0644 scitools.i386: E: non-executable-script /usr/lib/python2.5/site-packages/scitools/redirect_io.py 0644 scitools.i386: W: spurious-executable-perm /usr/share/doc/scitools-0.4/examples/streamtube_demo1.py scitools.i386: E: non-executable-script /usr/lib/python2.5/site-packages/scitools/filetable.py 0644 scitools.i386: E: non-executable-script /usr/lib/python2.5/site-packages/scitools/Lumpy.py 0644 scitools.i386: W: spurious-executable-perm /usr/share/doc/scitools-0.4/examples/streamribbon_demo2.py scitools.i386: W: spurious-executable-perm /usr/share/doc/scitools-0.4/examples/streamline_demo3.py scitools.i386: W: spurious-executable-perm /usr/share/doc/scitools-0.4/examples/mesh_demo.py scitools.i386: W: spurious-executable-perm /usr/share/doc/scitools-0.4/examples/alternatives/plotdemo1.py scitools.i386: E: non-executable-script /usr/lib/python2.5/site-packages/scitools/_update.py 0644 scitools.i386: W: spurious-executable-perm /usr/share/doc/scitools-0.4/examples/contourslice_demo2.py scitools.i386: W: wrong-file-end-of-line-encoding /usr/share/doc/scitools-0.4/bin/gnuplot.bat scitools.i386: W: spurious-executable-perm /usr/share/doc/scitools-0.4/examples/isosurface_demo2.py scitools.i386: E: non-executable-script /usr/lib/python2.5/site-packages/scitools/easyviz/movie.py 0644 scitools.i386: E: non-executable-script /usr/lib/python2.5/site-packages/scitools/DrawFunction.py 0644 scitools.i386: W: spurious-executable-perm /usr/share/doc/scitools-0.4/examples/plotdemo1.py scitools.i386: E: non-executable-script /usr/lib/python2.5/site-packages/scitools/multipleloop.py 0644 scitools.i386: E: non-executable-script /usr/lib/python2.5/site-packages/scitools/FunctionSelector.py 0644 scitools.i386: W: spurious-executable-perm /usr/share/doc/scitools-0.4/examples/alternatives/plotdemo3.py scitools.i386: E: non-executable-script /usr/lib/python2.5/site-packages/scitools/Regression.py 0644 scitools.i386: E: non-executable-script /usr/lib/python2.5/site-packages/scitools/BoxGrid.py 0644 scitools.i386: E: non-executable-script /usr/lib/python2.5/site-packages/scitools/NumPyDB.py 0644 scitools.i386: W: spurious-executable-perm /usr/share/doc/scitools-0.4/examples/streamline_demo2.py scitools.i386: E: non-executable-script /usr/lib/python2.5/site-packages/scitools/ppimport.py 0644 scitools.i386: E: non-executable-script /usr/lib/python2.5/site-packages/scitools/CanvasCoords.py 0644 scitools.i386: W: spurious-executable-perm /usr/share/doc/scitools-0.4/examples/streamtube_demo2.py scitools.i386: W: spurious-executable-perm /usr/share/doc/scitools-0.4/examples/streamribbon_demo1.py scitools.i386: W: spurious-executable-perm /usr/share/doc/scitools-0.4/examples/plottest.py scitools.i386: W: spurious-executable-perm /usr/share/doc/scitools-0.4/examples/plotdemo2.py scitools.i386: W: spurious-executable-perm /usr/share/doc/scitools-0.4/examples/isosurface_demo1.py scitools.i386: E: no-binary scitools.i386: W: doc-file-dependency /usr/share/doc/scitools-0.4/examples/contourslice_demo1.py /usr/bin/env scitools.i386: W: doc-file-dependency /usr/share/doc/scitools-0.4/examples/contourslice_demo2.py /usr/bin/env scitools.i386: W: doc-file-dependency /usr/share/doc/scitools-0.4/examples/isosurface_demo1.py /usr/bin/env scitools.i386: W: doc-file-dependency /usr/share/doc/scitools-0.4/examples/isosurface_demo2.py /usr/bin/env scitools.i386: W: doc-file-dependency /usr/share/doc/scitools-0.4/examples/isosurface_demo3.py /usr/bin/env scitools.i386: W: doc-file-dependency /usr/share/doc/scitools-0.4/examples/mesh_demo.py /usr/bin/env scitools.i386: W: doc-file-dependency /usr/share/doc/scitools-0.4/examples/slice_demo1.py /usr/bin/env scitools.i386: W: doc-file-dependency /usr/share/doc/scitools-0.4/examples/slice_demo2.py /usr/bin/env scitools.i386: W: doc-file-dependency /usr/share/doc/scitools-0.4/examples/streamline_demo1.py /usr/bin/env scitools.i386: W: doc-file-dependency /usr/share/doc/scitools-0.4/examples/streamline_demo2.py /usr/bin/env scitools.i386: W: doc-file-dependency /usr/share/doc/scitools-0.4/examples/streamline_demo3.py /usr/bin/env scitools.i386: W: doc-file-dependency /usr/share/doc/scitools-0.4/examples/streamribbon_demo1.py /usr/bin/env scitools.i386: W: doc-file-dependency /usr/share/doc/scitools-0.4/examples/streamribbon_demo2.py /usr/bin/env scitools.i386: W: doc-file-dependency /usr/share/doc/scitools-0.4/examples/streamtube_demo1.py /usr/bin/env scitools.i386: W: doc-file-dependency /usr/share/doc/scitools-0.4/examples/streamtube_demo2.py /usr/bin/env 1 packages and 0 specfiles checked; 21 errors, 41 warnings."
- You can correct the warning about the spec file by adding a %build section. - extras: matplotlib is ok, but dx-devel should probably be dx. - You must add BuildArch: noarch to the spec file to correct the no binary error. (There's nothing architecture specific in the package, so it's enough to build it once, you don't need separate i386, x86_64 etc packages). José: please correct me if I'm wrong, but AFAIK the non-executable script errors and spurious file perms warnings in files under /usr/lib/python2.5/site-packages/ don't cause any action. The doc file dependency warnings show, that maybe it's wiser after all to put the examples in /usr/share/%{name}-%{version}/examples. Remember to add a note about the examples to the package readme.
I have made the above changes so far except moving the examples since I wanted to ask: Is this really necessary. I know rpmlint is complaining, but end users will want to look to the documentation to see examples. I know whenever I want examples I look to the documentation. But if it is needed, I will change it.
(In reply to comment #24) > I have made the above changes so far except moving the examples since I wanted > to ask: Is this really necessary. I know rpmlint is complaining, but end users > will want to look to the documentation to see examples. I know whenever I want > examples I look to the documentation. > > But if it is needed, I will change it. For me it's okay if the examples stay in %doc.
Created attachment 332336 [details] rpmlint for scitools Here are the new files with the updates we discussed: Spec URL: http://jsmidt.fedorapeople.org/scitools.spec SRPM URL: http://jsmidt.fedorapeople.org/scitools-0.4-2.fc10.src.rpm
OK, removing FE-NEEDSPONSOR (I am sponsoring).
I have adjusted the Summary for the new name. One small suggestion, replace Source0: http://scitools.googlecode.com/files/SciTools-0.4.tar.gz with Source0: http://scitools.googlecode.com/files/SciTools-%{version}.tar.gz That avoid to edit that line every time there is a new release. This is a personal preference so you are free to ignore it. Some small nitpicks: * The spec file uses both %{buildroot} and $RPM_BUILD_ROOT, the guidelines warn against using both. Since there is only one instance of $RPM_BUILD_ROOT I suggest to move that to %{buildroot}. * In the website and in %description the capitalization of gnuplot is wrong. Clearly this is really (really) a minor point. I am not sure about the need of an extras sub-package. I would expect that most of the users of this package will have the full set installed. In any case I suggest to move all the dependencies to the main package and only do something about it if there are complains later that this is a problem.
I agree that the examples should stay in %doc (I asked about the same thing in pygrace so... ;-) ).
(In reply to comment #28) > I am not sure about the need of an extras sub-package. I would expect that most > of the users of this package will have the full set installed. In any case I > suggest to move all the dependencies to the main package and only do something > about it if there are complains later that this is a problem. I don't agree; the package should not be bloated. You don't need all of the packages to use SciTools. Also one must remember that there are still people using dialup connections, for whom downloading lots and lots of extra stuff is a problem. However, you might want to add a note about the extras metapackage to the %description, e.g. "For full functionality you may want to install the scitools-extras package."
A few notes more: When you copy the package README, use 'cp -a' instead of 'cp' to preserve the time stamp. (Also, you might want to add a comment about removing the bindir after installation.)
The package adheres to the Fedora Packaging and Review Guidelines, and is thus ACCEPTED. (You can do the tidying bits mentioned above once you import to CVS.)
(In reply to comment #30) > > I don't agree; the package should not be bloated. You don't need all of the > packages to use SciTools. Also one must remember that there are still people > using dialup connections, for whom downloading lots and lots of extra stuff is > a problem. It is not so much the fact that I may agree or not with that stance but instead the question to know if the use of a sub-package is the right technical solution to this problem. > However, you might want to add a note about the extras metapackage to the > %description, e.g. "For full functionality you may want to install the > scitools-extras package." I agree.
Okay, here is the final version with all the changes: Spec URL: http://jsmidt.fedorapeople.org/scitools.spec SRPM URL: http://jsmidt.fedorapeople.org/scitools-0.4-3.fc10.src.rpm Will request CVS.
New Package CVS Request ======================= Package Name: scitools Short Description: A Python library for scientific computing Owners: jsmidt Branches: F-9 F-10 EL-4 EL-5 InitialCC:
Joseph: You must create a bugzilla account with your email address that is in the Fedora account system and use that. You cannot use a @fedoraproject.org address as that is only a forward.
Put the comment about the extras (also) in the main %description, so that users find it. Also: grace (at the moment) does not exist in EL-4, which means that pygrace is not available in EL-4, which in turn means that scitools can't be installed on EL-4. You can circumvent this by checking the EPEL repositories to see which of the packages -extras depends on aren't available, and omit those requirements. Example: %if 0%{?rhel} != 4 Requires: pygrace %endif
Dear CVS admins: This is the account I signed up with as josephsmidt@gmail.com, my Fedora Account Email. (You will find that josephsmidt@gmail.com is taken since it is this account) I have changed the changelog in the .spec to reflect the gmail address. I verified with this account that jsmidt@fedoraproject.org also works, and now that is the only email it shows. I have emailed the bugzilla people about it. Here are the new sources: Spec URL: http://jsmidt.fedorapeople.org/scitools.spec SRPM URL: http://jsmidt.fedorapeople.org/scitools-0.4-4.fc10.src.rpm
Okay, email is fixed now.
Thanks! cvs done.
Joseph: remember to close the Review Request once your package hits the distro. (You can make the update manager do this, when you submit the package.) Closing.