-
Notifications
You must be signed in to change notification settings - Fork 576
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
update go directives in go.mod files #6379
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mattsains, first of all thank you for PR. I appreciate that you want to contribute and improve OpenTelemetry Go.
Could you please provide repro steps of the issue and provide a more clear description why you expect the go
directive in go.mod
to be updated?
This prevents this repository from building on newer versions of go.
It is not true. I am using Go 1.23.3 and I am able to build this repository.
Or maybe you meant "older versions of Go"? E.g. do you want to contribute to this repository while you have Go 1.21 installed on your machine?
From https://go.dev/doc/toolchain:
The choice of Go toolchain being used depends on the GOTOOLCHAIN environment setting and the go and toolchain lines in the main module’s go.mod file
From https://go.dev/ref/mod#modules-overview
The main module is the module containing the directory where the go command is invoked.
From what I understand, such change could make sense for modules that are also providing binaries (contain main
packages) so that someone can easily build a binary using older Go language version. It does not feel applicable for modules providing libraries. E.g. such change may make sense for OpenTelemetry Collector.
If you are building a binary consuming OpenTelemetry, the modified go.mod
files in this PR are not taken into consideration when selecting the Go toolchain.
Long story short, I think that such changes is important for modules providing binaries (main packages) and for modules that are only providing libraries (non-main packages).
On the other hand, changing go 1.22
to go 1.22.0
for modules providing libraries should not make any difference for so it is also a kind of an acceptable change.
At the same the alternative for modules providing binaries (e.g. for OpenTelemetry Collector), possibly cleaner, would be to use the toolchain
directive and have e.g.
go 1.22
toolchain 1.23.3
This should make packages useable from Go 1.22 and at the same time one building a binaries in cmd
would use latest Go 1.23.3 toolchain (with security fixes, runtime improvements etc).
Besides the following would also work:
go 1.22.0
toolchain 1.23.3
On the other side, I see that a similar change was done in golang.org/x/mod
here: https://go-review.googlesource.com/c/mod/+/605935. However, I do not really understand if it was really necessary.
I want to point out that currently the Go Modules Reference has go 1.12
in their example. There is not a single example with go 1.x.y
(where x >= 21
) syntax. Similarly, in https://go.dev/doc/modules/managing-dependencies we have go.mod with go 1.16.
PS. I hope I have not missed anything important. I did my best to explain it clear as I think that understanding how Go chooses the toolchain is not simple and is not commonly understood by Go (even very experienced) developers.
@bcmills, I would be thankful if you review my comment. I have a feeling that the Go docs/blog should add some recommendations on how to use the go
and toolchain
directives on top of describing how they work. I also put a comment here: golang/go#62278 (comment)
Additional reference:
I am marking as "request changes" just to understand the issue more and also maybe get some insights/recommendations from someone from the Go team.
@pellared , thanks for your detailed feedback. I didn't realise that I was actually running an old golang install - I was running go1.21 for some reason when I encountered this issue running the precommit target:
That caused me to look around for solutions and I found the github issue linked above. Since I solved this for myself by simply upgrading my go install, I'm happy to just close this PR, unless you feel like it's important to support go 1.21 |
@mattsains, no need to feel sorry. I think it is a good learning material even for myself.
No. I do not think it is important. I would prefer if the |
Go updated their toolchain to not accept directives of the form
go a.b
. You must provide eithergo a.b.c
, or include atoolchain
directive as well. This prevents this repository from building on newer versions of go. More details: golang/go#62278