Bug 1389695
| Summary: | Review Request: python-wcsaxes - A Python framework for plotting astronomical and geospatial data | ||
|---|---|---|---|
| Product: | [Fedora] Fedora | Reporter: | Sergio Pascual <sergio.pasra> |
| Component: | Package Review | Assignee: | Nobody's working on this, feel free to take it <nobody> |
| Status: | CLOSED DUPLICATE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
| Severity: | medium | Docs Contact: | |
| Priority: | medium | ||
| Version: | rawhide | CC: | ishcherb, mhroncok, package-review |
| Target Milestone: | --- | ||
| Target Release: | --- | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Whiteboard: | |||
| Fixed In Version: | Doc Type: | If docs needed, set a value | |
| Doc Text: | Story Points: | --- | |
| Clone Of: | Environment: | ||
| Last Closed: | 2016-11-18 09:41:35 UTC | Type: | --- |
| Regression: | --- | Mount Type: | --- |
| Documentation: | --- | CRM: | |
| Verified Versions: | Category: | --- | |
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
| Cloudforms Team: | --- | Target Upstream Version: | |
| Embargoed: | |||
|
Description
Sergio Pascual
2016-10-28 09:19:55 UTC
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-%{version}.tar.gz
change to
`Source0: https://pypi.io/packages/source/w/%{srcname}/%{srcname}-%{version}.tar.gz`
* 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/83fec6e82bbae751f0241da6933ae425c0e9664b#diff-8b1c3fd0d4a6765c16dfd18509182f9dR8) 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.
Thanks for the review Iryna, wcsaxes.test() definitively doesn't work with this spec file Hi Sergio, Is there still an interest in adding this package to Fedora? Let me know if you need any help. I'm considering to package pytest-mpl before this one Hi Sergio, python-wcsaxes is already in Fedora. *** This bug has been marked as a duplicate of bug 1395273 *** Great, thank you for the review anyway |