https://bugzilla.redhat.com/show_bug.cgi?id=2083142 --- Comment #4 from Iztok Fister Jr. <iztok@xxxxxxxxxxxxxxxxxx> --- I thank both reviewers for their helpful comments. All issues are now resolved in the revision. For a more accessible following of the changes, I am attaching point-to-point answers to the comments you raised. ------------------------------------ Review 1: =========== 1. I see some tests fail. Should we at least add comments to note what the failures are to begin with. Ideally, we should also report these upstream and where possible submit patches. ANSWER: Some files are missing also in the upstream repository. 2. - Optional: we could generate a man page (using `help2man` , for example) and include it in the package + submit it upstream. ANSWER: Man page generated. 3. - I don't think the make BR is being used for anything, so we should remove it. ANSWER: make BR was removed. ----------------- Review 2: =============== 1. It looks like this is purely a command-line tool and is not intended to be used as a Python library. It should therefore be named “cffconvert” rather than “python-cffconvert”. The appropriate 'python3dist(…)' Provides will still be generated. https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_application_naming To change the name, you should be able to change the title of this bug and re-upload spec/SRPM under the new name, rather than having to create a new bug. ANSWER: Done! 2. - Consider adding some or all of these as %doc, too: CHANGELOG.md CITATION.cff CONTRIBUTING.md ANSWER: Done! 3. There is a “gcloud” extra that should be packaged in accordance with: https://fedoraproject.org/wiki/Changes/PythonExtras#Extras_metapackages Assuming you have renamed the package to “cffconvert”, this can look like: %pyproject_extras_subpkg -n %{pypi_name} gcloud You can/should also install the corresponding BuildRequires: %pyproject_buildrequires -x gcloud Even if not needed in the tests, this helps confirm that the extras metapackage will be installable. ANSWER: Done! 4. - You should patch out the dependency on python3dist(pytest-cov): https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_linters Remove: BuildRequires: python3dist(pytest-cov) and in %prep, add something like: # https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_linters sed -r -i 's/--[^[:blank:]]*\bcov\b[^[:blank:]]*//' setup.cfg - It looks like you can fix all of the tests by changing %pytest -k '…' to %pytest -v which is a little bit crazy. I think the -v option is affecting how pytest handles the standard I/O descriptors, and the tests are sensitive to the difference. ANSWER: -v command is not able to discover test suite. 5. - This doesn’t seem to be needed: BuildRequires: make ANSWER: Done! 6. - A man page is always desired for a command-line tool: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_manpages You can generate an adequate one by adding: BuildRequires: help2man then, in %install: install -d '%{buildroot}%{_mandir}/man1' env PATH="${PATH}:%{buildroot}%{_bindir}" \ PYTHONPATH='%{buildroot}%{python3_sitelib}' \ help2man --no-info '%{pypi_name}' \ --output='%{buildroot}%{_mandir}/man1/%{pypi_name}.1' (doing this in %build could be possible but would be much fussier), then, in %files: %{_mandir}/man1/%{pypi_name}.1* ANSWER: Done! -- 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 https://bugzilla.redhat.com/show_bug.cgi?id=2083142 _______________________________________________ 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 Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
