Bug 2293636

I chose to disable the tests by default because a few of them require the presence of the proprietary NVIDIA driver.

This is my first attempt at packaging a Go module.  Here are some doubts that I have about it.

The Git tags and versions used by upstream (eg., v0.12.4-0) are not valid RPM versions because they contain a dash.  That's why I chose to define '%tag' and left the %version as 0.

The generated Provides don't have 'golang(github.com/NVIDIA/go-nvml)'.  I hope that's alright because the top-level source directory doesn't have any Go sources.  So, users of this module need to import the sub-paths.

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

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2293636-golang-github-nvidia-nvml/fedora-rawhide-x86_64/07646718-golang-github-nvidia-nvml/fedora-review/review.txt

Found issues:

- No gcc, gcc-c++ or clang found in BuildRequires
  Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/C_and_C++/

Please know that there can be false-positives.

---
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.

> I chose to disable the tests by default because a few of them require the presence of the proprietary NVIDIA driver.

Can add this as a comment before the bcond_with?

> The Git tags and versions used by upstream (eg., v0.12.4-0) are not valid RPM versions because they contain a dash.  That's why I chose to define '%tag' and left the %version as 0.

What a strange way to version the package. One option would be to use:

Version:                0.12.4.0
%global tag             v0.12.4-0

WDYT? You'll need to generate the package with `go2rpm -q github.com/NVIDIA/go-nvml -t v0.12.4-0` and modify the Version entry.

> The generated Provides don't have 'golang(github.com/NVIDIA/go-nvml)'.  I hope that's alright because the top-level source directory doesn't have any Go sources.  So, users of this module need to import the sub-paths.

It's correct and not an issue. If you check the example in the README file you'll see they're not using top-level dir, but the whole path as expected: https://github.com/NVIDIA/go-nvml/blob/main/README.md?plain=1#L59

On top of these changes, would it be possible to update to latest go2rpm (1.12.0) package?

(In reply to Debarshi Ray from comment #1)
> The Git tags and versions used by upstream (eg., v0.12.4-0) are not valid
> RPM versions because they contain a dash.  That's why I chose to define
> '%tag' and left the %version as 0.

I filed an upstream request about not using a hyphen or dash in the Git tag:
https://github.com/NVIDIA/go-nvml/issues/126

Thanks for the review!

(In reply to Mikel Olasagasti Uranga from comment #3)
> > I chose to disable the tests by default because a few of them require the presence of the proprietary NVIDIA driver.
> 
> Can add this as a comment before the bcond_with?

Sure.  Added.
 
> > The Git tags and versions used by upstream (eg., v0.12.4-0) are not valid RPM versions because they contain a dash.  That's why I chose to define '%tag' and left the %version as 0.
> 
> What a strange way to version the package. One option would be to use:
> 
> Version:                0.12.4.0
> %global tag             v0.12.4-0
> 
> WDYT? You'll need to generate the package with `go2rpm -q
> github.com/NVIDIA/go-nvml -t v0.12.4-0` and modify the Version entry.

Okay!  That definitely makes the RPM's version a lot saner.  Thanks for the tip.  :)

> > The generated Provides don't have 'golang(github.com/NVIDIA/go-nvml)'.  I hope that's alright because the top-level source directory doesn't have any Go sources.  So, users of this module need to import the sub-paths.
> 
> It's correct and not an issue. If you check the example in the README file
> you'll see they're not using top-level dir, but the whole path as expected:
> https://github.com/NVIDIA/go-nvml/blob/main/README.md?plain=1#L59

Okay.
 
> On top of these changes, would it be possible to update to latest go2rpm
> (1.12.0) package?

For some reason, running go2rpm inside my Fedora 40 Toolbx container is erroring out with a Python traceback that seems to be due to 'git config user.name' finishing with exit code 1, even though I have:
$ env | grep GIT
GIT_COMMITTER_NAME=Debarshi Ray
GIT_AUTHOR_EMAIL=rishi
GIT_COMMITTER_EMAIL=rishi
GIT_AUTHOR_NAME=Debarshi Ray

I will dig into it.

Meanwhile, I have uploaded the other changes:

Spec URL: https://rishi.fedorapeople.org/golang-github-nvidia-nvml.spec
SRPM URL: https://rishi.fedorapeople.org/golang-github-nvidia-nvml-0.12.4.0-1.fc38.src.rpm

Created attachment 2038185 [details]
The .spec file difference from Copr build 7646718 to 7662267

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

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2293636-golang-github-nvidia-nvml/fedora-rawhide-x86_64/07662267-golang-github-nvidia-nvml/fedora-review/review.txt

Found issues:

- No gcc, gcc-c++ or clang found in BuildRequires
  Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/C_and_C++/

Please know that there can be false-positives.

---
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.

(In reply to Debarshi Ray from comment #1)
> I chose to disable the tests by default because a few of them require the
> presence of the proprietary NVIDIA driver.

I tried the tests with the proprietary NVIDIA driver version 550.78 installed.  They failed in the exact same way as they do without it.  So, I expanded the comment to clarify it with more details.

(In reply to Mikel Olasagasti Uranga from comment #3)
> On top of these changes, would it be possible to update to latest go2rpm
> (1.12.0) package?

Done.  I hope I didn't mess it up when hand-editing the *.spec file.

Is %{goname} deprecated?  I am asking because go2rpm doesn't use it.  I see that the packaging guidelines still use it: https://docs.fedoraproject.org/en-US/packaging-guidelines/Golang/

> Is %{goname} deprecated?  I am asking because go2rpm doesn't use it.  I see that the packaging guidelines still use it: https://docs.fedoraproject.org/en-US/packaging-guidelines/Golang/

Documentation needs some love.

The template was changed here:

https://gitlab.com/fedora/sigs/go/go2rpm/-/commit/8b6977dbeca1a7e54921bfe08136520a039b279f

This package was generated using go2rpm, which simplifies the review.

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated


- [x] The latest version is packaged or packaging an earlier version is justified.
- [x] The License tag reflects the package contents and uses the correct identifiers.
- [x] The package builds successfully in mock.
- [x] Package is installable (checked by fedora-review).
- [x] There are no relevant rpmlint errors.
- [x] The package runs tests in %check.
- [x] `%goipath` is set correctly.
- [-] The package's binaries don't conflict with binaries already in the distribution. (Some Go projects include utility binaries with very generic names)
- [-] There are no `%{_bindir}/*` wildcards in %files. (go2rpm includes these by default)
- [x] The package does not use `%gometa -f` if it has dependents that still build for %ix86.
- [x] The package complies with the Golang and general Packaging Guidelines.

Package approved! On import, don't forget to do the following:

- [ ] Add the package to release-monitoring.org
- [ ] Give go-sig privileges (at least commit) on the package
- [ ] Close the review bug by referencing its ID in the rpm changelog and the Bodhi ticket.
- [ ] Consider configuring Packit service to help with maintenance

Thanks!

(In reply to Mikel Olasagasti Uranga from comment #10)
> > Is %{goname} deprecated?  I am asking because go2rpm
> doesn't use it.  I see that the packaging guidelines
> still use it:
> https://docs.fedoraproject.org/en-US/packaging-guidelines/Golang/
> 
> Documentation needs some love.
> 
> The template was changed here:
> 
> https://gitlab.com/fedora/sigs/go/go2rpm/-/commit/
> 8b6977dbeca1a7e54921bfe08136520a039b279f

I see, thanks for the reference.

The commit message says:
"We now hardcode the Name field to the value of goname,"

... and the changes replace %{goname} with the name unconditionally:
Name:           %{goname}
Name:           {{ name }}

So, are we not supposed to use %{goname} in *.spec files anymore?  go-rpm-macros doesn't seem to have deprecated it, but I saw this commit:
https://pagure.io/go-rpm-macros/c/39aa1bbc6ad33a3b603d2f2a16ab186c84c2342a

... which looks related, but it's not clear to me how.  :)

The Pagure repository was created at https://src.fedoraproject.org/rpms/golang-github-nvidia-nvml

(In reply to Mikel Olasagasti Uranga from comment #11)
>
> [...]
>
> Package approved! On import, don't forget to do the following:

Thanks, Mikel.
 
> - [ ] Add the package to release-monitoring.org
> - [ ] Give go-sig privileges (at least commit) on the package
> - [ ] Close the review bug by referencing its ID in the rpm changelog and
> the Bodhi ticket.
> - [ ] Consider configuring Packit service to help with maintenance

Okay, I will do those.

Requested Git repository:
https://pagure.io/releng/fedora-scm-requests/issue/63279

Requested Git branches for f40 and f39:
https://pagure.io/releng/fedora-scm-requests/issue/63280
https://pagure.io/releng/fedora-scm-requests/issue/63281

(In reply to Debarshi Ray from comment #12)
> (In reply to Mikel Olasagasti Uranga from comment #10)
> > > Is %{goname} deprecated?  I am asking because go2rpm
> > doesn't use it.  I see that the packaging guidelines
> > still use it:
> > https://docs.fedoraproject.org/en-US/packaging-guidelines/Golang/
> > 
> > Documentation needs some love.
> > 
> > The template was changed here:
> > 
> > https://gitlab.com/fedora/sigs/go/go2rpm/-/commit/
> > 8b6977dbeca1a7e54921bfe08136520a039b279f
> 
> I see, thanks for the reference.
> 
> The commit message says:
> "We now hardcode the Name field to the value of goname,"
> 
> ... and the changes replace %{goname} with the name unconditionally:
> Name:           %{goname}
> Name:           {{ name }}
> 
> So, are we not supposed to use %{goname} in *.spec files anymore? 

Correct, no need to use goname, name works just fine.

> go-rpm-macros doesn't seem to have deprecated it, but I saw this commit:
> https://pagure.io/go-rpm-macros/c/39aa1bbc6ad33a3b603d2f2a16ab186c84c2342a
> 
> ... which looks related, but it's not clear to me how.  :)

Not a macro expert myself, so not sure if something else should be changed. You can try to ask in https://lists.fedoraproject.org/archives/list/golang@lists.fedoraproject.org/ or Fedora Golang room in Matrix.

FEDORA-2024-390b5c7d9a (golang-github-nvidia-nvml-0.12.4.0-1.fc40) has been submitted as an update to Fedora 40.
https://bodhi.fedoraproject.org/updates/FEDORA-2024-390b5c7d9a

FEDORA-2024-d33b6e25fc (golang-github-nvidia-nvml-0.12.4.0-1.fc39) has been submitted as an update to Fedora 39.
https://bodhi.fedoraproject.org/updates/FEDORA-2024-d33b6e25fc

Added https://github.com/NVIDIA/go-nvml to Anitya:
https://release-monitoring.org/project/373214/

... and enabled monitoring in Fedora:
https://src.fedoraproject.org/rpms/golang-github-nvidia-nvml

FEDORA-2024-390b5c7d9a has been pushed to the Fedora 40 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf install --enablerepo=updates-testing --refresh --advisory=FEDORA-2024-390b5c7d9a \*`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2024-390b5c7d9a

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

FEDORA-2024-d33b6e25fc has been pushed to the Fedora 39 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf install --enablerepo=updates-testing --refresh --advisory=FEDORA-2024-d33b6e25fc \*`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2024-d33b6e25fc

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

FEDORA-2024-390b5c7d9a (golang-github-nvidia-nvml-0.12.4.0-1.fc40) has been pushed to the Fedora 40 stable repository.
If problem still persists, please make note of it in this bug report.

FEDORA-2024-d33b6e25fc (golang-github-nvidia-nvml-0.12.4.0-1.fc39) has been pushed to the Fedora 39 stable repository.
If problem still persists, please make note of it in this bug report.