https://bugzilla.redhat.com/show_bug.cgi?id=2263790 --- Comment #3 from Martin Hoyer <mhoyer@xxxxxxxxxx> --- Hi Petr, Please note I'm not in the packagers group yet and this is my first review. Unfortunately the template in comment#2 seem to be missing, broken or expired, so I thought I would write a few nitpicks and later we could re-run it. To follow the Fedora Python Packaging Guidelines as close as possible, there are a few things that I'd change in the specfile. I believe you are corerct not to package the 'test' optional extras, and you are correctly using the included tests in the %check part. Same goes to not using %pyproject_buildrequires -t switch, as test only require `jq`, while the test optional extras requirements include "linters" and other packages that aren't needed. So having jq in BuildRequires is good, but are you sure about `Requires: jq`? The global variables with name and source are not being used in the guidelines examples, as well as other syntax and macros. While I'm not saying it's inherently wrong, I would change the: ``` %{?!python3_pkgversion:%global python3_pkgversion 3} # OpenSUSE uses alternative Go based tool: # https://github.com/mikefarah/yq %global srcname yq %global srcforge https://github.com/kislyuk/yq Name: python-yq Version: 3.2.3 Release: %autorelease Summary: Command-line YAML/XML processor - jq wrapper for YAML/XML documents License: Apache-2.0 URL: https://pypi.org/project/yq/ VCS: git:%{srcforge} Source0: %pypi_source BuildArch: noarch BuildRequires: python%{python3_pkgversion}-devel BuildRequires: jq ``` to ``` Name: python-yq Version: 3.2.3 Release: %autorelease Summary: Command-line YAML/XML processor - jq wrapper for YAML/XML documents License: Apache-2.0 URL: https://github.com/kislyuk/yq Source: %{pypi_source yq} BuildArch: noarch BuildRequires: python3-devel BuildRequires: jq ``` For sake of readability and full compliance with the guidelines. Next, there is the description, I don't think using summary as description is a good practice. ``` %description %{summary}. ``` Instead, I'd do: ``` %global _description %{expand: Lightweight and portable command-line YAML, JSON and XML processor. yq uses jq like syntax but works with yaml files as well as json, xml, properties, csv and tsv. It doesn't yet support everything jq does - but it does support the most common operations and functions, and more is being added continuously.} %description %_description %package -n python3-yq Summary: %{summary} %description -n python3-yq %_description ``` (Note the 80 character line limit) Lastly, while the package is building as is on F40, the F39 has a minor dependency issue with tomlkit version. We could lower the required version in the respective branches from 0.11.6 to 0.11.4 in the %prep part: ``` # Lowering tomlkit version for <=F39 and EPEL9 sed -i 's/tomlkit >= 0.11.6/tomlkit >= 0.11.4/g' setup.py ``` I've check the changelog for 0.11.5 and 0.11.6 and there does not appear anything "breaking" that could cause issues, correct me if I'm wrong. This would allow building on all supported Fedora version as well as on EPEL9. I think EPEL9 might not work with %autorelease, but other than that it builds fine. Here is a F38 build with spec file I've put together based on pyp2spec. Hope it helps as a reference :) -- You are receiving this mail because: You are always notified about changes to this product and component You are on the CC list for the bug. https://bugzilla.redhat.com/show_bug.cgi?id=2263790 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202263790%23c3 -- _______________________________________________ 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, report it: https://pagure.io/fedora-infrastructure/new_issue
