[Bug 2263790] New: Review Request: python-yq - Command-line YAML/XML processor - jq wrapper for YAML/XML documents
https://bugzilla.redhat.com/show_bug.cgi?id=2263790
Bug ID: 2263790 Summary: Review Request: python-yq - Command-line YAML/XML processor - jq wrapper for YAML/XML documents Product: Fedora Version: rawhide Status: NEW Component: Package Review Assignee: nobody@fedoraproject.org Reporter: pemensik@redhat.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
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
https://bugzilla.redhat.com/show_bug.cgi?id=2263790
--- Comment #1 from Petr Menšík pemensik@redhat.com --- This package built on koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=113370880
https://bugzilla.redhat.com/show_bug.cgi?id=2263790
Fedora Review Service fedora-review-bot@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- URL| |https://pypi.org/project/yq | |/
--- Comment #2 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Copr build: https://copr.fedorainfracloud.org/coprs/build/7008135 (succeeded)
Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-rev...
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.
https://bugzilla.redhat.com/show_bug.cgi?id=2263790
--- Comment #3 from Martin Hoyer mhoyer@redhat.com --- 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://bugzilla.redhat.com/show_bug.cgi?id=2263790
Cristian Le fedora@lecris.me changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |fedora@lecris.me Flags| |needinfo?(pemensik@redhat.c | |om)
--- Comment #5 from Cristian Le fedora@lecris.me --- @pemensik@redhat.com Could you re-run/edit the spec url? the review.txt and copr build have expired
https://bugzilla.redhat.com/show_bug.cgi?id=2263790
Xavier Bachelot xavier@bachelot.org changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |xavier@bachelot.org Status|NEW |ASSIGNED
https://bugzilla.redhat.com/show_bug.cgi?id=2263790
Petr Menšík pemensik@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(pemensik@redhat.c | |om) |
--- Comment #6 from Petr Menšík pemensik@redhat.com --- (In reply to Cristian Le from comment #5)
@pemensik@redhat.com 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.
https://bugzilla.redhat.com/show_bug.cgi?id=2263790
--- Comment #7 from Martin Hoyer mhoyer@redhat.com --- (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.
https://bugzilla.redhat.com/show_bug.cgi?id=2263790
--- Comment #8 from Martin Hoyer mhoyer@redhat.com --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=2263790
--- Comment #9 from Dusty Mabe dustymabe@redhat.com --- 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?
https://bugzilla.redhat.com/show_bug.cgi?id=2263790
--- Comment #10 from Cristian Le fedora@lecris.me --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=2263790
Cristian Le fedora@lecris.me changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |needinfo?(pemensik@redhat.c | |om)
--- Comment #11 from Cristian Le fedora@lecris.me ---
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.
https://bugzilla.redhat.com/show_bug.cgi?id=2263790
Petr Menšík pemensik@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |2074467
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=2074467 [Bug 2074467] Review Request: yq - Portable command-line YAML, JSON and XML processor
https://bugzilla.redhat.com/show_bug.cgi?id=2263790
--- Comment #12 from Petr Menšík pemensik@redhat.com --- (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-y...
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.
https://bugzilla.redhat.com/show_bug.cgi?id=2263790
--- Comment #13 from Martin Hoyer mhoyer@redhat.com --- (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/e507357e3aa72b1fdf0b27f1f9bbb87bf3bd137... (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
https://bugzilla.redhat.com/show_bug.cgi?id=2263790
--- Comment #15 from Cristian Le fedora@lecris.me --- 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?
https://bugzilla.redhat.com/show_bug.cgi?id=2263790
--- Comment #16 from Martin Hoyer mhoyer@redhat.com --- +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
https://bugzilla.redhat.com/show_bug.cgi?id=2263790
Petr Menšík pemensik@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Depends On| |2276522 Flags|needinfo?(pemensik@redhat.c | |om) |
--- Comment #17 from Petr Menšík pemensik@redhat.com --- 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
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=2276522 [Bug 2276522] Review Request: yq - Yq is a portable command-line YAML, JSON, XML, CSV, TOML and properties processor
https://bugzilla.redhat.com/show_bug.cgi?id=2263790
Petr Menšík pemensik@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks|2074467 |
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=2074467 [Bug 2074467] Review Request: yq - Portable command-line YAML, JSON and XML processor
https://bugzilla.redhat.com/show_bug.cgi?id=2263790
--- Comment #18 from Martin Hoyer mhoyer@redhat.com --- (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?
https://bugzilla.redhat.com/show_bug.cgi?id=2263790
--- Comment #19 from Petr Menšík pemensik@redhat.com --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=2263790
--- Comment #20 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Created attachment 2037072 --> https://bugzilla.redhat.com/attachment.cgi?id=2037072&action=edit The .spec file difference from Copr build 7008135 to 7608901
https://bugzilla.redhat.com/show_bug.cgi?id=2263790
Fedora Review Service fedora-review-bot@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Keywords| |AutomationTriaged
--- Comment #21 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Copr build: https://copr.fedorainfracloud.org/coprs/build/7608901 (succeeded)
Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-rev...
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.
https://bugzilla.redhat.com/show_bug.cgi?id=2263790
Martin Hoyer mhoyer@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |needinfo?(michel@michel-slm | |.name) CC| |michel@michel-slm.name
--- Comment #22 from Martin Hoyer mhoyer@redhat.com --- 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?
https://bugzilla.redhat.com/show_bug.cgi?id=2263790
Michel Lind michel@michel-slm.name changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(michel@michel-slm | |.name) |
--- Comment #23 from Michel Lind michel@michel-slm.name --- (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-dj... (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-dj...)
https://bugzilla.redhat.com/show_bug.cgi?id=2263790
Martin Hoyer mhoyer@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |needinfo?(michel@michel-slm | |.name)
--- Comment #24 from Martin Hoyer mhoyer@redhat.com --- 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/#_incomp... https://docs.fedoraproject.org/en-US/packaging-guidelines/Conflicts/#_binary...
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?
https://bugzilla.redhat.com/show_bug.cgi?id=2263790
--- Comment #25 from Petr Menšík pemensik@redhat.com --- I agree alternatives seems the correct way to choose, since those tools seems to have compatible common interface.
But because we arrive after Go project, even though it seems it is newer, I made xqp alternative name for xq tool. They are not candidates for alternatives, since they do not share common parsing options. I made yqp provided by python3-yq package and conflicting yq name provided by python3-yq-alt, which still uses alternatives. But until also golang project uses alternatives, it conflicts with yq.
https://bugzilla.redhat.com/show_bug.cgi?id=2263790
--- Comment #26 from Petr Menšík pemensik@redhat.com --- Spec URL: https://github.com/pemensik/yq/raw/fedora/python-yq.spec SRPM URL: https://pemensik.fedorapeople.org/srpm/python-yq-3.4.3-12.fc41.src.rpm
/usr/bin/yq is provided only by python3-yq-alt, which conflicts with yq package explicitly. I think alternatives should be used for them, but this makes both installable on the same system, until yq is modified.
https://bugzilla.redhat.com/show_bug.cgi?id=2263790
--- Comment #27 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Created attachment 2037572 --> https://bugzilla.redhat.com/attachment.cgi?id=2037572&action=edit The .spec file difference from Copr build 7608901 to 7620822
https://bugzilla.redhat.com/show_bug.cgi?id=2263790
--- Comment #28 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Copr build: https://copr.fedorainfracloud.org/coprs/build/7620822 (succeeded)
Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-rev...
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.
https://bugzilla.redhat.com/show_bug.cgi?id=2263790
Petr Menšík pemensik@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |2292652
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=2292652 [Bug 2292652] yq command should support alternatives
https://bugzilla.redhat.com/show_bug.cgi?id=2263790
--- Comment #29 from Martin Hoyer mhoyer@redhat.com --- Thanks Petr! Might I suggest starting with `pyp2spec yq --license Apache-2.0`, as there are some inconsistencies with the reference? For example:
URL: https://pypi.org/project/yq/ VCS: git:%{srcforge} Source0: %{pypi_source yq} --- URL: https://github.com/kislyuk/yq Source: %{pypi_source yq}
(also it's not srcforge)
Other than that:
BuildRequires: sed
I believe this is redundant
Requires(post): %{_bindir}/alternatives
Not saying it's wrong, but `update-alternatives` is being used in the guidelines - https://docs.fedoraproject.org/en-US/packaging-guidelines/Alternatives/
Patch1: yq-tomlkit-f39.patch
Again, nothing wrong with that, but since there already is sed workaround for the setuptools_scm, also for F39, why not have two seds instead?
%{?python_provide:%python_provide python3-%{srcname}}
I believe this is also redundant
sed -e 's/^version_file/#write_to/' -i pyproject.toml
Just curious why is write_to commented out
https://bugzilla.redhat.com/show_bug.cgi?id=2263790
--- Comment #30 from Petr Menšík pemensik@redhat.com --- VCS is used because I modify it manually and want to have VCS present, along with better presentation for end users on pypi. Alternatives path were wrong, fixed it.
Spec URL: https://github.com/pemensik/yq/raw/fedora/python-yq.spec SRPM URL: https://pemensik.fedorapeople.org/srpm/python-yq-3.4.3-12.fc41.src.rpm
(In reply to Martin Hoyer from comment #29)
Thanks Petr! Might I suggest starting with `pyp2spec yq --license Apache-2.0`, as there are some inconsistencies with the reference? For example:
URL: https://pypi.org/project/yq/ VCS: git:%{srcforge} Source0: %{pypi_source yq}
URL: https://github.com/kislyuk/yq Source: %{pypi_source yq}
(also it's not srcforge)
What is meant by srcforge if github.com is not one of them?
Other than that:
BuildRequires: sed
I believe this is redundant
Requires(post): %{_bindir}/alternatives
Not saying it's wrong, but `update-alternatives` is being used in the guidelines - https://docs.fedoraproject.org/en-US/packaging-guidelines/Alternatives/
I have modified it to use just alternatives, because they are real binary, update-alternatives just a symlink. Not sure what variant is actually more preferred, but manual page references just alternatives. That is why I chosen to use that for a new package. But used wrong _bindir path, updated it in place.
Patch1: yq-tomlkit-f39.patch
Again, nothing wrong with that, but since there already is sed workaround for the setuptools_scm, also for F39, why not have two seds instead?
Patch is actually more safe, when surrounding lines may change meaning of done change. Also sed itself does not guarantee the change were done successfully, patch does.
%{?python_provide:%python_provide python3-%{srcname}}
I believe this is also redundant
sed -e 's/^version_file/#write_to/' -i pyproject.toml
Just curious why is write_to commented out
It seemed to me this works well in normal mockbuild, but were needed just in my local build attempts. Using shipped part of release seems better than generating anything from fedora distgit. Review targets rawhide, where this is unnecessary.
https://bugzilla.redhat.com/show_bug.cgi?id=2263790
--- Comment #31 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Copr build: https://copr.fedorainfracloud.org/coprs/build/7627514 (succeeded)
Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-rev...
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.
https://bugzilla.redhat.com/show_bug.cgi?id=2263790
Martin Hoyer mhoyer@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |needinfo?(fedora@lecris.me) | |needinfo?(mhroncok@redhat.c | |om) CC| |mhroncok@redhat.com
--- Comment #32 from Martin Hoyer mhoyer@redhat.com ---
What is meant by srcforge if github.com is not one of them?
You're assigning `%global srcforge https://github.com/kislyuk/yq%60. I'd expect such variable name to be pointing to sourceforge vcs. Am I missing something?
I have modified it to use just alternatives
Ack.
Patch is actually more safe
Ack.
@fedora@lecris.me Do you want to take this review? I'm not in love with the idea of doing `-alt`, but feel like this whole thing is way over my head and don't want to block things unnecessarily.
@mhroncok@redhat.com Any chance you could take a quick look, pretty please?
https://bugzilla.redhat.com/show_bug.cgi?id=2263790
Miro Hrončok mhroncok@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(mhroncok@redhat.c | |om) |
--- Comment #33 from Miro Hrončok mhroncok@redhat.com ---
Any chance you could take a quick look, pretty please?
On what exactly?
https://bugzilla.redhat.com/show_bug.cgi?id=2263790
--- Comment #34 from Martin Hoyer mhoyer@redhat.com --- (In reply to Miro Hrončok from comment #33)
Any chance you could take a quick look, pretty please?
On what exactly?
The spec file. https://copr-dist-git.fedorainfracloud.org/cgit/@fedora-review/fedora-review...
https://bugzilla.redhat.com/show_bug.cgi?id=2263790
--- Comment #35 from Miro Hrončok mhroncok@redhat.com --- OK.
MY highly opinionated feedback:
%global srcname yq
This is unnecessary and makes the specfile harder to read. It would be easier to read if all occurrences of %{srcname} were replaced with yq.
%global srcforge https://github.com/kislyuk/yq
So is this. It would be easier to read if the single occurrence of %{srcforge} was replaced with https://github.com/kislyuk/yq
This is not the URL of the project, it is merely a URL of the package on PyPI. Upstream says their homepage is https://github.com/kislyuk/yq
Patch1: yq-tomlkit-f39.patch
There is no reason to number patches. The patch has no comment that explains why it is needed. The patch has no commit message either. The name is not obvious enough. We usually use sed for trivial changes like this.
%if 0%{?fedora} && 0%{?fedora} <= 39 BuildRequires: sed %endif
There is no reason to conditionalize this BuildRequires, it makes the specfile needlessly complicated. It is always present, so if you want to be explicit, just BuildRequire it unconditionally; if you want to be concise, don't BuildRequire it at all.
%global _description %{expand: ...}
%description %{_description}
This is inserting an undesired newline at the beginning of the description. Put %description %{_description} on a single line to avoid that.
%{?python_provide:%python_provide python3-%{srcname}}
%python_provide is unnecessary and deprecated. The provides are generated automatically without it.
# Until Go yq provides alternatives, conflict with that package Conflicts: yq
If there are no alternatives in yq, don't add them here. Or coordinate. But don't have it halfway. This way it adds all the disadvantages of alternatives without the benefit of alternatives.
%if 0%{?fedora} && 0%{?fedora} <= 39 ... %autopatch -M 1 -p1 %endif
There is no reason to apply this patch conditionally. Use %autosetup without -N.
%generate_buildrequires
Other sections are separated by 2 newlines. This section is separated by 1 newline. It seems inconsistent.
# xq command is already taken # ... mv %{buildroot}%{_bindir}/{xq,xqp} mv %{buildroot}%{_bindir}/{yq,yqp}
Why xqp, why yqp? The comment should try to explain that.
touch %{buildroot}%{_bindir}/yq
Why use alternatives to this command only?
# python3 setup.py test is failing. Run test directly.
Moreover, setup.py test is deprecated, so not using it is the right thing to do.
%license LICENSE
The %license is already included in %{pyproject_files}, see:
$ rpm -ql --licensefiles -p python3-yq-3.4.3-12.fc41.noarch.rpm /usr/lib/python3.13/site-packages/yq-3.4.3.dist-info/LICENSE /usr/share/licenses/python3-yq/LICENSE
It should not be manually repeated in the %files section. Add -l flag to %pyproject_save_files instead to assert the %license is there even in newer versions.
----
General note about using alternatives:
Are both tools' CLI APIs identical? If they are not, don't use alternatives.
IF they ar identical: As a user, what is the benefit for me to use either version? If there is no difference, do I really need to have the option to choose?
https://bugzilla.redhat.com/show_bug.cgi?id=2263790
Cristian Le fedora@lecris.me changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(fedora@lecris.me) |needinfo?(mhroncok@redhat.c | |om)
--- Comment #36 from Cristian Le fedora@lecris.me --- You guys pretty much covered all of the comments I would have and more :D, so unless @mhroncok@redhat.com takes it on, I can see it through as well.
I only have some minor random comments: - how will `%{_sbindir}/alternatives` work w.r.t. sbin/bin merger. - from what I can see xq-golang is similar with the `xq` provided here, so it should do the same Conflicts/Alternatives
I can go either way w.r.t. Conflicts vs Alternatives. And I guess the main thing we should do here is to discuss which one to go for:
Ultimately these tools have completely different approaches, where python-yq calls `jq` internally, while golang ones are completely self-contained, but I don't think that is evident enough for the user to make a decision on which version to go with.
Personally I would choose the python version since it seems to be just a thin wrapper against the python "standard" libraries (ignoring the question of which yaml library is used), so I can see this as more stable, but then the golang parsers can potentially be faster.
If it was possible to add some description about each alternatives, I would vote for Alternatives, but right now I am split between these two options.
Can someone also play the devil's advocate for the Conflicts case?
https://bugzilla.redhat.com/show_bug.cgi?id=2263790
Miro Hrončok mhroncok@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(mhroncok@redhat.c | |om) |
--- Comment #37 from Miro Hrončok mhroncok@redhat.com --- Folks, stop needinfoing me for no reason.
how will `%{_sbindir}/alternatives` work w.r.t. sbin/bin merger.
%_sbindir will be /usr/bin so this will eventually work.
As for what will provide /usr/sbin/alternatives, I asked at https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/...
Can someone also play the devil's advocate for the Conflicts case?
Alternatives are horrible. They use scriplets. They are almost impossible to get rid of. If the tools have different CLI options, it is forbidden to use them.
package-review@lists.fedoraproject.org
-
bugzilla＠redhat.com