Spec URL: https://pemensik.fedorapeople.org/python-yq.spec SRPM URL: https://pemensik.fedorapeople.org/python-yq-3.2.3-1.fc40.src.rpm Description: Command-line YAML/XML processor - jq wrapper for YAML/XML documents. Fedora Account System Username: pemensik
This package built on koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=113370880
Copr build: https://copr.fedorainfracloud.org/coprs/build/7008135 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2263790-python-yq/fedora-rawhide-x86_64/07008135-python-yq/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
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 :)
https://koji.fedoraproject.org/koji/taskinfo?taskID=113598134
@pemensik Could you re-run/edit the spec url? the review.txt and copr build have expired
(In reply to Cristian Le from comment #5) > @pemensik Could you re-run/edit the spec url? the review.txt and > copr build have expired Just use fedora-review -b 2263790 to create your local build with local review.txt, that will wait on your disk until you do not need it anymore. The only thing you need for that is to be member of mock group on your computer and installed fedora-review tool.
(In reply to Petr Menšík from comment #6) > Just use fedora-review -b 2263790 to create your local build with local > review.txt, Sure, I think we were just hoping that if you incorporate some of the suggestions from comment#3 it would trigger a new build/review.txt There was also a bug on fedora-review last time I've tried it. Works fine now.
ok if somebody else want to take this, feel free, as I don't know what's happening. There was no reply to notes to comment#3, so if it's expected to be closed as-is, I'd rather not be the reviewer.
I guess this one won't conflict with the other yq (https://bugzilla.redhat.com/show_bug.cgi?id=2074467) because this one will be named `python-yq` at least for the RPM name. Could it be confusing if we have two in Fedora?
Both `yq` should be compatible with each other and compatible with `jq`. No reason why we can't have both implementations as long as both are prefixed with `python/go`. Not sure if each one needs to mark the other as conflicting explicitly or if that is handled by `Provides` automatically.