Bug 2263790

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.

> So having jq in BuildRequires is good, but are you sure about `Requires: jq`?

Checked the source a bit and it uses `subprocess` to call `jq`.

> %global _description

I like the version you've posted better. Is it ok even if it's not in an upstream source? Didn't check the accuracy of the description.

Petr, would you want to continue with this review? I don't want to open another review request before checking in with you.

(In reply to Cristian Le from comment #10)
> 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.

Hmm, as matter of fact, current spec provides %{_bindir}/yq, just as bug #2074467 package. So they will conflict once passed both. In order to both be installable on single system, at least one of them need to rename the conflicting binary.

A way around that would be using alternatives subsystem, where both could provide the same name and user would choose a preferred one. But that would require interchangable parameters, at least to some extent.

I don't remember my exact workflow when creating the package. I think I haven't used any tool to create spec file, but used existing package as a template with manual changes.

I have found OpenSUSE rpm spec at https://build.opensuse.org/projects/devel:languages:python/packages/python-yq/files/python-yq.spec?expand=1

I am not confident lowering of explicit requirements would be safe without using it that first. I think such lowering should happen only for older releases, if any.

Yes, I would like to finish this review.

(In reply to Petr Menšík from comment #12)
> I am not confident lowering of explicit requirements

fyi, it should be pretty safe: https://github.com/kislyuk/yq/commit/e507357e3aa72b1fdf0b27f1f9bbb87bf3bd1378
(for context, toml 10.0 is from 2018, while tomlkit 11.4 has been released in August 2022)
Also, it could also be lowered on F39(now that F38 is eol) and EPEL9:

%if 0%{?fedora} == 39 || 0%{?rhel} == 9
sed -i 's/tomlkit >= 0.11.6/tomlkit >= 0.11.4/g' setup.py
%endif

s/also/only

Could we workshop the idea of `alternatives`?

I am not sure that we should be as thorough about it, and just have either the user or Require/BuildRequires handle which version they want. Afaik, they support `|` operators. We don't seem to have `python_alternative` macros and I think there is used for something else like having multiple `python-x.y` versions.

Are there other packages to reference?

+1 for alternatives https://docs.fedoraproject.org/en-US/packaging-guidelines/Alternatives/

How about using suffixes, yq-python and yq-go? 
Once installed, it could be something like this, hopefully with the go package doing the same. 
(not sure which one of the two should have higher priority)

%post
update-alternatives --install %{_bindir}/yq %{name} /usr/bin/yq-python 10

%preun
if [ $1 -eq 0 ]; then
  update-alternatives --remove %{name} %{_bindir}/yq-python
fi

Found out we already have xq tool in Fedora, so I cannot use xq tool name without alternatives as well. Made a post in Go based yq review [1]. In Debian they hit it too, xq-python is their name of binary.
It seems at least some compability is implemented, as mentioned in [2]. So yes, alternatives might work for yq to some extent.

I have wondered where did you find nice description, which I have failed to find on upstream. It turns out your description is from concurrent yq package [3]. I do not think reusing content from different project is okay, at least without mentioning it.

I think about using just shortened binary name, yqp for python. If not used with alternatives, it would be better to type less. Anyway, turns out yq Go based command is already present in Fedora 39, not this alternative. 

1. https://bugzilla.redhat.com/show_bug.cgi?id=2074467#c10
2. https://github.com/mikefarah/yq/issues/193
3. https://github.com/mikefarah/yq?tab=readme-ov-file#yq

(In reply to Petr Menšík from comment #17)
> I have wondered where did you find nice description, which I have failed to
> find on upstream. It turns out your description is from concurrent yq
> package [3]. I do not think reusing content from different project is okay,
> at least without mentioning it.

oh, nice catch. It wasn't intentional, sorry.  
In that case, I'm not entirely sure what to put in the description, but don't like using summary personally. Perhaps it coul mention that it is the python version and that go-based package also exists and a link to upstream documentation?

Spec URL: https://github.com/pemensik/yq/raw/fedora/python-yq.spec
SRPM URL: https://pemensik.fedorapeople.org/srpm/python-yq-3.4.3-9.fc41.src.rpm

Created attachment 2037072 [details]
The .spec file difference from Copr build 7008135 to 7608901

Copr build:
https://copr.fedorainfracloud.org/coprs/build/7608901
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2263790-python-yq/fedora-rawhide-x86_64/07608901-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.

Petr,
I've coincidentally had a chance to briefly discuss yq with Michel, who did the #2276522. 
With Neal, they've mentioned having the two packages conflicting each other, instead the use of alternatives.

Michel, would you help here in order to have the same logic in both packages please?

(In reply to Martin Hoyer from comment #22)
> Petr,
> I've coincidentally had a chance to briefly discuss yq with Michel, who did
> the #2276522. 
> With Neal, they've mentioned having the two packages conflicting each other,
> instead the use of alternatives.
> 
> Michel, would you help here in order to have the same logic in both packages
> please?

yup, of course. Have we aligned on what virtual provide to use?

What came up, IIRC is

- use a virtual provide, so we can do this (name is up for discussion)

Provides:  yq-bin
Conflicts: yq-bin

I also wonder if we can just say 'Conflicts: /usr/bin/yq' and if that could be enough.

- the Python module does not need to conflict, only the Python binary, so the binaries should be shipped in a separate subpackage (so you can have the Python module installed on the same system as the Go binary, if needed)

Happy to adjust the Go package once we get this sorted.

Example of this being used by the different Django packages: https://src.fedoraproject.org/rpms/python-django4.2/blob/rawhide/f/python-django4.2.spec (you need to suppress rpmlint because it will be unhappy about the unversioned provides: https://src.fedoraproject.org/rpms/python-django4.2/blob/rawhide/f/python-django4.2.rpmlintrc)

I've done some more reading of packaging guidelines and from what I can tell, using alternatives is the preferred way? See:
https://docs.fedoraproject.org/en-US/packaging-guidelines/Conflicts/#_incompatible_binary_files_with_conflicting_naming_and_stubborn_upstreams
https://docs.fedoraproject.org/en-US/packaging-guidelines/Conflicts/#_binary_name_conflicts

From user perspective, there probably will never be a use-case for having the both executables installed, but on the other hand, why not. It would save us splitting the package and any potential issues with the usage of conflicts. 
Maybe(likely) I'm missing something. @Michel, if it's not too much to ask, could you explain why the Conflicts is considered the better option here please?