Bug 2009498 - Review Request: gojq - Pure Go implementation of jq
Summary: Review Request: gojq - Pure Go implementation of jq
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Maxwell G
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 2028445 (view as bug list)
Depends On: 2009494 2009496
Blocks: 1803302 2010367
TreeView+ depends on / blocked
 
Reported: 2021-09-30 19:26 UTC by Mikel Olasagasti Uranga
Modified: 2021-12-07 22:52 UTC (History)
3 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2021-12-07 22:52:56 UTC
Type: ---
Embargoed:
maxwell: fedora-review+


Attachments (Terms of Use)

Description Mikel Olasagasti Uranga 2021-09-30 19:26:06 UTC
Spec URL: https://mikel.olasagasti.info/tmp/fedora/golang-github-itchyny-gojq.spec
SRPM URL: https://mikel.olasagasti.info/tmp/fedora/golang-github-itchyny-gojq-0.12.5-1.fc34.src.rpm
Description: Pure Go implementation of jq
Fedora Account System Username: mikelo2

Comment 1 Maxwell G 2021-12-05 04:29:52 UTC
`0.12.6` was released a couple days ago. You should update to that. I don't think this is required, but I would recommend getting rid of `cmd/*` and other globs and explicitly refer to gojq. Also, what do you think about renaming the package that provides the `gojq` binary to `gojq`?

Comment 2 Mikel Olasagasti Uranga 2021-12-05 07:57:41 UTC
0.12.6 removes deps on golang-github-itchyny-flags, so yes, I'll update it asap.

About naming gojq, it makes sense. I'll adapt the spec file.

Comment 3 Sanne Raymaekers 2021-12-05 11:57:53 UTC
*** Bug 2028445 has been marked as a duplicate of this bug. ***

Comment 5 Maxwell G 2021-12-07 20:13:03 UTC
I am not going to go through the whole fedora-review template, as this package uses go2rpm. I reviewed based on this copr build: https://download.copr.fedorainfracloud.org/results/mikelo2/github-cli/fedora-rawhide-x86_64/03005882-gojq/

- The specfile is sane.
- License is correct
- Builds successfully in mock
- No rpmlint errors
- %check section passes

I have one other comment:

> Name:           gojq

I would change this to `Name: %{goname}` (but with the proper indentation) and then add `%global goname gojq` right below `%gometa` (with an empty line in between). You should do this for the rest of the packages where you overrode the `Name`, instead of completely removing `%goname`.


Also, there are still two globs left in the specfile:

> install -m 0755 -vp %{gobuilddir}/cmd/* %{buildroot}%{_bindir}/

> %{_bindir}/*

I like to avoid globs in my specfiles, when possible, but I guess it's up to you whether to keep them or not. This is how go2rpm generates specfiles, and it's not against the Go Packaging Guidelines.

When I aproove this package and you import it, don't forget to add `Fixes rhbz#2009498` to the rpm changelog and/or manually add this bug to the Bodhi update so it gets marked as CLOSED. I also added your package to Antiya/release-monitoring.org. I think you should give go-sig admin privileges when the package is imported, as well.

Thanks,
Maxwell

Comment 6 Mikel Olasagasti Uranga 2021-12-07 20:52:13 UTC
Spec URL: https://mikel.olasagasti.info/tmp/fedora/gojq.spec
SRPM URL: https://mikel.olasagasti.info/tmp/fedora/gojq-0.12.6-1.fc35.src.rpm

- Added `%global goname gojq` to use it Name
- Remove rest of globs from %install and %files

Comment 7 Maxwell G 2021-12-07 21:56:14 UTC
Package approved! Thanks for putting up with all my nitpicks 😄.

Comment 8 Mikel Olasagasti Uranga 2021-12-07 22:15:54 UTC
Thank you for your patience!

Comment 9 Gwyn Ciesla 2021-12-07 22:20:12 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/gojq

Comment 10 Fedora Update System 2021-12-07 22:51:59 UTC
FEDORA-2021-dd6a32be55 has been submitted as an update to Fedora 36. https://bodhi.fedoraproject.org/updates/FEDORA-2021-dd6a32be55

Comment 11 Fedora Update System 2021-12-07 22:52:56 UTC
FEDORA-2021-dd6a32be55 has been pushed to the Fedora 36 stable repository.
If problem still persists, please make note of it in this bug report.


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