Bug 2263790 - Review Request: python-yq - Command-line YAML/XML processor - jq wrapper for YAML/XML documents
Summary: Review Request: python-yq - Command-line YAML/XML processor - jq wrapper for ...
Keywords:
Status: ASSIGNED
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Martin Hoyer
QA Contact: Fedora Extras Quality Assurance
URL: https://pypi.org/project/yq/
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2024-02-11 22:36 UTC by Petr Menšík
Modified: 2024-04-15 16:36 UTC (History)
6 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed:
Type: ---
Embargoed:
mhoyer: fedora-review?


Attachments (Terms of Use)

Description Petr Menšík 2024-02-11 22:36:05 UTC
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

Comment 1 Petr Menšík 2024-02-11 22:36:09 UTC
This package built on koji:  https://koji.fedoraproject.org/koji/taskinfo?taskID=113370880

Comment 2 Fedora Review Service 2024-02-11 22:39:09 UTC
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.

Comment 3 Martin Hoyer 2024-02-16 18:03:51 UTC
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 :)

Comment 5 Cristian Le 2024-03-05 16:46:40 UTC
@pemensik Could you re-run/edit the spec url? the review.txt and copr build have expired

Comment 6 Petr Menšík 2024-03-28 16:48:33 UTC
(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.

Comment 7 Martin Hoyer 2024-03-28 18:18:12 UTC
(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.

Comment 8 Martin Hoyer 2024-04-10 08:46:42 UTC
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.

Comment 9 Dusty Mabe 2024-04-15 16:10:26 UTC
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?

Comment 10 Cristian Le 2024-04-15 16:36:55 UTC
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.


Note You need to log in before you can comment on or make changes to this bug.