https://bugzilla.redhat.com/show_bug.cgi?id=1795077
--- Comment #2 from Fabian Affolter <mail(a)fabian-affolter.ch> ---
(In reply to Fabio Valentini from comment #1)
Hi, I'll review your package.
Thanks
Some recommendations:
1) I assume you're hiding tests behind the api_key bcond because they
require an actual API key and internet access?
So do you run tests locally with "rpmbuild -bb --with api_key"? Maybe add
a comment for the bcond or in %check.
Comment added. Also, create the file for the API key during the build process.
2) I recommend you use HTTPS for the URL as well. Then you can also
replace
the URL prefix of the Source0 with %{url}.
GitHub is making sure with a HTTP 301 message that one uses HTTPS. Changed
anyway. Seems to be coming from pyp2rpm as this is the URL that is used in the
setup.py file (
https://github.com/achillean/shodan-python/pull/119).
%{url} could be an option. If the reviewer wants that then I will add it.
Otherwise I prefer to keep things "copy-and-pastable" for humans in the spec
file.
Also, why are you not using %{pypi_source} directly? Do the pypi
sources
miss some files? If so, adding a comment why you're using the GitHub tarball
instead would be helpful for anybody who's looking at the package.
GitHub is the upstream location for the source and not PyPI. As a long as the
project is providing proper tarballs I think that we should stick to the
original upstream location and not a third-party delivery mechanism.
3) You could deduplicate the %description, with something like:
%global _description %{expand:
The official Python library and CLI for Shodan Shodan is a search engine for
Internet-connected devices. Google lets you search for websites, Shodan lets
you search for devices. This library provides developers easy access to all
of the data stored in Shodan in order to automate tasks and integrate into
existing tools.}
Then you can use:
%description %{_description}
and
%description python3-%{pypi_name} %{_description}
Again, this removes the possibility to re-use the description outside of the
spec file with copy-and-paste. It makes sense if you have a dozen of
subpackages and want to use the same text but for regular packages it doesn't
add much value from my point of view.
4) Since you're shipping both python3-shodan and shodan binary
packages, you
could just name the source package shodan directly.
But that's a matter of style, I guess.
If you'll keep the source package named python-shodan, then "%files -n
python-%{pypi_name}-doc" is superfluous and could be just "%files doc".
From my point of view, it's at the packager's discretion. This comes down to
the decision if it is more a tool or a lib. In most cases the CLI part is an
add-on to the lib.
5) I guess you're removing a stray shebang line from worldmap.py
in %prep?
Adding a descriptive comment would be great.
I really think that we should not add comments to every standard/common
building block like remove shebang, fix permissions, remove the egg, remove the
left-overs from the docs generation, etc.
https://github.com/achillean/shodan-python/pull/118
6) For listing python3 modules, please use trailing slashes:
%{python3_sitelib}/%{pypi_name}/
%{python3_sitelib}/%{pypi_name}-*.egg-info/
This will prevent upgrade issues if these ever change from directories to
files.
You could also use something like this instead of the second line:
%{python3_sitelib}/%{pypi_name}-%{version}-py%{python3_version}.egg-info/
Then you'd not even need the glob.
Fixed.
Let me know if these points are helpful, and when the package is
ready for
final review.
Thank you for your feedback.
* Tue Jan 28 2020 Fabian Affolter <mail(a)fabian-affolter.ch> - 1.21.3-2
- Fix ownership
- Improve the check workflow (rhbz#1795077)
Updated files:
Spec URL:
https://fab.fedorapeople.org/packages/SRPMS/python-shodan.spec
SRPM URL:
https://fab.fedorapeople.org/packages/SRPMS/python-shodan-1.21.3-2.fc31.s...
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component