eclipseo opened a new pull-request against the project: `go-rpm-macros` that you are following: `` Fix goname generation to match versioning guildelines ``
To reply, visit the link below or just reply to this email https://pagure.io/go-rpm-macros/pull-request/56
gotmax23 commented on the pull-request: `Fix goname generation to match versioning guildelines` that you are following: `` I'd add a new flag to configure whether or not to use the new naming scheme and remove the hardcoded list. ``
To reply, visit the link below or just reply to this email https://pagure.io/go-rpm-macros/pull-request/56
eclipseo commented on the pull-request: `Fix goname generation to match versioning guildelines` that you are following: `` @gotmax23 Can I have you feedback as soon as possible on these changes? ``
To reply, visit the link below or just reply to this email https://pagure.io/go-rpm-macros/pull-request/56
gotmax23 commented on the pull-request: `Fix goname generation to match versioning guildelines` that you are following: `` To copy what I wrote on Matrix:
eclipseo: on mobile, but I think it should be the other way around. the old behavior should stay to preserve backwards compatibility.
go2rpm can default to adding the flag, but the macro implementation should make the new behavior opt in
Also, it'd be better to add an argument to the rpmname over using an RPM macro for this. ``
To reply, visit the link below or just reply to this email https://pagure.io/go-rpm-macros/pull-request/56
gotmax23 commented on the pull-request: `Fix goname generation to match versioning guildelines` that you are following: ``
Also, it'd be better to add an argument to the rpmname over using an RPM macro for this.
Hmm, that's not as easy as it looks. This would also require passing the flag to the other Go macros that call `indexedgoipaths()` or `rpmname()`. I suppose I'd be fine with using a macro here, but it should be namespaced (e.g. `go_use_new_versioning 1`). ``
To reply, visit the link below or just reply to this email https://pagure.io/go-rpm-macros/pull-request/56
gotmax23 commented on the pull-request: `Fix goname generation to match versioning guildelines` that you are following: `` See my (only lightly tested) suggestion in https://pagure.io/fork/gotmax23/go-rpm-macros/commits/alt_fix_goname. ``
To reply, visit the link below or just reply to this email https://pagure.io/go-rpm-macros/pull-request/56
eclipseo commented on the pull-request: `Fix goname generation to match versioning guildelines` that you are following: `` I disagree, the default behavior should be the ones which respect the guidelines, and the flag should be added to the incorrect packages. The exception should be the incorrect behavior, not the correct. I dont want to add yet another flag to gometa that could be easily forgotten while it should be the default behavior
Also, it'd be better to add an argument to the rpmname over using an RPM macro for this. Hmm, that's not as easy as it looks. This would also require passing the flag to the other Go macros that call indexedgoipaths() or rpmname(). I suppose I'd be fine with using a macro here, but it should be namespaced (e.g. go_use_new_versioning 1).
Yes I started to do it that way but it was impacting too many functions.
Ok for versioning. ``
To reply, visit the link below or just reply to this email https://pagure.io/go-rpm-macros/pull-request/56
gotmax23 commented on the pull-request: `Fix goname generation to match versioning guildelines` that you are following: ``
The exception should be the incorrect behavior, not the correct. I dont want to add yet another flag to gometa that could be easily forgotten while it should be the default behavior
I understand that, but at the same time, I'm unwilling to break backwards compatibility. This is likely to cause confusing behavior across branches and other issues. Unless their are serious bugs with the %goname code, we cannot make changes willy-nilly without affecting many packages inside and outside the Fedora repositories. Also, this is a breaking change that I would not feel comfortable backporting to stable branches, whether Fedora or (EP)EL. ``
To reply, visit the link below or just reply to this email https://pagure.io/go-rpm-macros/pull-request/56
eclipseo commented on the pull-request: `Fix goname generation to match versioning guildelines` that you are following: ``
This is likely to cause confusing behavior across branches and other issues
Unlikely, we know the list of packages that will be affected and we just need update theses ones in stable branches in a side tag with the go-rpm-macros package.
we cannot make changes willy-nilly without affecting many packages inside and outside the Fedora repositories
For inside, we know the impact. For outside Fedora, 1) this is not our issue 2) the impact would only be on versioned golang packages, which itself is a very limited scope, 3) The impact would only be the name of the package, it wouldn't cause problems like if it was in dist-git and fail the build.
Also, this is a breaking change that I would not feel comfortable backporting to stable branches, whether Fedora or (EP)EL.
I don't understand why for stable branches, we know the changes we have made and the impact it will have. There is not reason it will impact the thousands of non versioned packages, and will affect the list of packages we already know about.
For EPEL, there is basically no impact as we don't have libraries packages there. We don' t even need to backport there.
See my (only lightly tested) suggestion in https://pagure.io/fork/gotmax23/go-rpm-macros/commits/alt_fix_goname.
I don't get the change you made to gorpmname. ``
To reply, visit the link below or just reply to this email https://pagure.io/go-rpm-macros/pull-request/56
gotmax23 commented on the pull-request: `Fix goname generation to match versioning guildelines` that you are following: ``
Also, this is a breaking change that I would not feel comfortable backporting to stable branches, whether Fedora or (EP)EL.
I don't understand why for stable branches, we know the changes we have made and the impact it will have. There is not reason it will impact the thousands of non versioned packages, and will affect the list of packages we already know about.
For EPEL, there is basically no impact as we don't have libraries packages there. We don' t even need to backport there.
There are at least four packages in EPEL 9 that would be impacted by this change.
``` $ fedrq pkgs -b epel9 -s -Fname * | rg 'golang-.*-\d+$' golang-gopkg-check-1 golang-gopkg-russross-blackfriday-2 golang-gopkg-yaml-2 golang-gopkg-yaml-3 ```
I do not feel comfortable backporting this to older Fedoras and definitely not in EPEL. It's a breaking change that will cause packages to stop building in distgit.
Also, the lists we have been working off of aren't even all inclusive. compat-golang-*-devel packages also use this logic:
``` $ fedrq pkgs -S -b rawhide -Fname compat-*golang*-devel | rg 'compat-golang-.*-\d+-devel$' | wc -l 82 $ fedrq pkgs -S -b epel9 -Fname compat-*golang*-devel | rg 'compat-golang-.*-\d+-devel$' | wc -l 4 ```
We'd have to identify all of these impacted packages and then make a bunch of mass changes accross distgit branches, while being quite careful not to break anything. We have enough brokenness in the Go ecosystem, and I don't think we need to add more potential pitfalls. I'd rather make a oneline change in go2rpm to add this flag everywhere than have to rebuild and mass change 100-200 packages
See my (only lightly tested) suggestion in https://pagure.io/fork/gotmax23/go-rpm-macros/commits/alt_fix_goname.
I don't get the change you made to gorpmname.
I made it so it always uses a `_` to separate a version from the import path when `go_use_new_versioning` is set and otherwise use a `-`. Did I not understand what the FPC asked for? ``
To reply, visit the link below or just reply to this email https://pagure.io/go-rpm-macros/pull-request/56
eclipseo commented on the pull-request: `Fix goname generation to match versioning guildelines` that you are following: ``
I made it so it always uses a _ to separate a version from the import path when go_use_new_versioning is set and otherwise use a -. Did I not understand what the FPC asked for?
Where in the FPC does it say this ?
They have either suggested :
Our suggestions are:
If the package names would continue to use a hyphen as separator for the version specifier ("-vN"), we would like the package suffix to be "-vN" as well. If you would like the current Go naming practices to remain in place, we would consider documenting the exception for using a hyphen as a separator for this purpose.
The other option is to actually respect the guidelines, which is what I wanted to do, which is no hyphen, just the version number at the end of the package. Of the package ends with a number, then we add an underscore, then the version number.
Your code is not doing that. You code add a underscore in all cases and does not check that the package names end with a number or not as far as i understand, which was covered by the condition in my code:
```if string.find(prior, "%d$") then return prior .. "_" .. version else return prior .. version ```
Also, the lists we have been working off of aren't even all inclusive. compat-golang-*-devel packages also use this logic:
There is no standalone compat package afaik, all compat packages are subpackages of standards packages.
There are at least four packages in EPEL 9 that would be impacted by this change.
I never intended to propagate this change to EPEL. EPEL is stable like this, we almost never build libraries there. ``
To reply, visit the link below or just reply to this email https://pagure.io/go-rpm-macros/pull-request/56
eclipseo commented on the pull-request: `Fix goname generation to match versioning guildelines` that you are following: `` Also this was tested in COPR with no changes in the packages names. We can ask for a mass rebuilt in a side tag if necessary. ``
To reply, visit the link below or just reply to this email https://pagure.io/go-rpm-macros/pull-request/56
eclipseo commented on the pull-request: `Fix goname generation to match versioning guildelines` that you are following: `` @gotmax23 @alexsaezm
Could you check again the current situation:
- we got the -L flag that enable go_use_new_versioning - go_use_new_versioning allow to add more processing to rpmname to remove the hyphen or add a underscore if the package name ends with a number
In [go2rpm](https://pagure.io/GoSIG/go2rpm/pull-request/31#request_diff) we have - the -L/ --use-new-versioning flag, which is the default now that adds the -L flag to gometa in the template.
Example of a package with the new flag:
https://copr.fedorainfracloud.org/coprs/eclipseo/macros-fix3/build/6558806/
Get well soon Max! ``
To reply, visit the link below or just reply to this email https://pagure.io/go-rpm-macros/pull-request/56
gotmax23 commented on the pull-request: `Fix goname generation to match versioning guildelines` that you are following: `` Thanks @eclipseo for all your work on this! I'm good with this approach. I have some comments about the go2rpm side, but I think we can merge and release the go-rpm-macros part now to unblock those packages. Please fix the commit message before merging, though, as it still mentions the `-l` flag. ``
To reply, visit the link below or just reply to this email https://pagure.io/go-rpm-macros/pull-request/56
eclipseo commented on the pull-request: `Fix goname generation to match versioning guildelines` that you are following: `` Could you add the comment on teh go2rpm MR? I'd like to do both at once ``
To reply, visit the link below or just reply to this email https://pagure.io/go-rpm-macros/pull-request/56
gotmax23 commented on the pull-request: `Fix goname generation to match versioning guildelines` that you are following: `` Ack. Let's ship it! ``
To reply, visit the link below or just reply to this email https://pagure.io/go-rpm-macros/pull-request/56
eclipseo merged a pull-request against the project: `go-rpm-macros` that you are following.
Merged pull-request:
`` Fix goname generation to match versioning guildelines ``
golang@lists.fedoraproject.org
-
Maxwell G -
Robert-André Mauchin