https://bugzilla.redhat.com/show_bug.cgi?id=1389695
Bug ID: 1389695 Summary: Review Request: python-wcsaxes - A Python framework for plotting astronomical and geospatial data Product: Fedora Version: rawhide Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: sergio.pasra@gmail.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org
Spec URL: https://guaix.fis.ucm.es/~spr/fedora/python-wcsaxes.spec SRPM URL: https://guaix.fis.ucm.es/~spr/fedora/python-wcsaxes-0.9-1.fc24.src.rpm Description: WCSAxes is a framework for making plots of Astronomical data in Matplotlib. Fedora Account System Username: sergiopr
https://bugzilla.redhat.com/show_bug.cgi?id=1389695
Iryna Shcherbina ishcherb@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |ishcherb@redhat.com
--- Comment #1 from Iryna Shcherbina ishcherb@redhat.com --- Hi Sergio,
the spec file looks good, but I have a couple of suggestions you might consider:
* Please use the %{srcname} macro you have defined in the `SourceO` tag:
Source0: https://pypi.io/packages/source/w/wcsaxes/wcsaxes-%%7Bversion%7D.tar.gz
change to
`Source0: https://pypi.io/packages/source/w/%%7Bsrcname%7D/%%7Bsrcname%7D-%%7Bversion%...
* To improve readability please use multiple lines for `BuildRequires` tag:
BuildRequires: python2-devel python3-devel xorg-x11-server-Xvfb
* You can use macro %{summary}, which will contain content of the Summary tag (it's generated automatically, you do not have to define it). Just keep the first Summary tag as it is and in python2/3- subpackages you can use `Summary: %{summary}`.
* It is better to avoid using wildcards in the %files section to have an idea what is installed with the package. Doing the following change would make it more safe (same for python3- subpackage):
%{python2_sitelib}/*
change to
%{python2_sitelib}/%{srcname} %{python2_sitelib}/%{srcname}-%{version}-py?.?.egg-info
* Due to the above, the following file gets to the generated RPMs. It is generated during tests and does not belong in the package:
/usr/lib/python3.5/site-packages/test.png /usr/lib/python2.7/site-packages/test.png
---- Another issue is that I believe you are running tests in %{buildroot} to avoid the problem with pytest-mpl dependency (https://github.com/astrofrog/wcsaxes/commit/83fec6e82bbae751f0241da6933ae425...) and ignore wcsaxes/setup.cfg settings. However, when installing the package and running tests with wcsaxes.test() (as it is stated in the documentation), I am running into multiple errors:
import file mismatch: imported module 'wcsaxes.tests.test_coordinate_helpers' has this __file__ attribute:
/builddir/build/BUILDROOT/python-wcsaxes-0.9-1.fc24.x86_64/usr/lib/python3.5/site-packages/wcsaxes/tests/test_coordinate_helpers.py which is not the same as the test file we want to collect: /usr/lib/python3.5/site-packages/wcsaxes/tests/test_coordinate_helpers.py HINT: remove __pycache__ / .pyc files and/or use a unique basename for your test file modules
That happens because running tests in the %{buildroot} generates files, e.g. /usr/lib/python3.5/site-packages/test.png (mentioned above). Instead of changing the directory, try specifying another config when running tests (with a -c option) or ignoring it, to fix the above issues. Another option would be to clean up after tests.
Apart from a couple of small suggestions above, everything looks fine. Note, that it is an informal package review, as I am not sponsored yet, but I can do a formal one once I am.
https://bugzilla.redhat.com/show_bug.cgi?id=1389695
--- Comment #2 from Sergio Pascual sergio.pasra@gmail.com --- Thanks for the review Iryna, wcsaxes.test() definitively doesn't work with this spec file
https://bugzilla.redhat.com/show_bug.cgi?id=1389695
--- Comment #3 from Iryna Shcherbina ishcherb@redhat.com --- Hi Sergio,
Is there still an interest in adding this package to Fedora? Let me know if you need any help.
https://bugzilla.redhat.com/show_bug.cgi?id=1389695
--- Comment #4 from Sergio Pascual sergio.pasra@gmail.com --- I'm considering to package pytest-mpl before this one
https://bugzilla.redhat.com/show_bug.cgi?id=1389695
Iryna Shcherbina ishcherb@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |CLOSED Resolution|--- |DUPLICATE Last Closed| |2016-11-18 04:41:35
--- Comment #5 from Iryna Shcherbina ishcherb@redhat.com --- Hi Sergio, python-wcsaxes is already in Fedora.
*** This bug has been marked as a duplicate of bug 1395273 ***
https://bugzilla.redhat.com/show_bug.cgi?id=1389695
--- Comment #6 from Sergio Pascual sergio.pasra@gmail.com --- Great, thank you for the review anyway
package-review@lists.fedoraproject.org