https://bugzilla.redhat.com/show_bug.cgi?id=1795077 --- Comment #2 from Fabian Affolter <mail@xxxxxxxxxxxxxxxxxx> --- (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@xxxxxxxxxxxxxxxxxx> - 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.src.rpm -- 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 _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx
