Skip to content
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

[go.mod] Change go directive to 1.21 #1441

Merged
merged 1 commit into from
Apr 25, 2024

Conversation

mx-psi
Copy link
Contributor

@mx-psi mx-psi commented Apr 17, 2024

Relates to #1438

Signed-off-by: Pablo Baeyens pablo.baeyens@datadoghq.com

@mx-psi
Copy link
Contributor Author

mx-psi commented Apr 17, 2024

@lmb would you mind approving the workflows?

@lmb
Copy link
Collaborator

lmb commented Apr 24, 2024

@mx-psi sorry, I got sidetracked!

@lmb lmb marked this pull request as ready for review April 24, 2024 13:33
@lmb lmb requested a review from a team as a code owner April 24, 2024 13:33
Copy link
Collaborator

@lmb lmb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like things work! Not sure what broke originally. Mind adding some context to the commit message?

On b3432fb, the minimum supported
Go version was updated to `1.21.0`. This changes the version to `1.21`
so that we are using a language version. If not using toolchain, this
seems to be the preferred convention in the ecosystem and golang/go.

Signed-off-by: Pablo Baeyens <pablo.baeyens@datadoghq.com>
@mx-psi mx-psi force-pushed the mx-psi/ebpf-go-mod-no-bugfix branch from 52a3d47 to fd6c445 Compare April 24, 2024 18:27
@mx-psi
Copy link
Contributor Author

mx-psi commented Apr 24, 2024

I amended the commit to include a more descriptive commit message :)

@ti-mo ti-mo merged commit eaef6a7 into cilium:main Apr 25, 2024
15 checks passed
@lmb
Copy link
Collaborator

lmb commented May 10, 2024

So, I've figured out why I didn't do it this way from the start: it breaks toolchain forward compat. Trying to run a command inside the module with go < 1.21 doesn't work since 1.21 is not a valid toolchain.

I'm not exactly sure what the consequences of that are, but overall it makes me think that the original change was the right one.

@mx-psi
Copy link
Contributor Author

mx-psi commented May 10, 2024

Trying to run a command inside the module with go < 1.21 doesn't work since 1.21 is not a valid toolchain.

Can you give me an example of the command you are trying to run? If it causes trouble, I surely don't want to do this on my project either!

@lmb
Copy link
Collaborator

lmb commented May 10, 2024

This is for a different repo, but the same rules apply:

$ go1.21.0 build
$ go mod edit -go=1.22
$ grep -F 1.22 go.mod
go 1.22
$ go1.21.0 build
go: downloading go1.22 (linux/amd64)
go: download go1.22 for linux/amd64: toolchain not available
$ go mod edit -go=1.22.0
$ go1.21.0 build
go: downloading go1.22.0 (linux/amd64)
$ 

I guess toolchain go1.22.0 would also fix this, but that seems more cumbersome for little (no?) gain.

@mx-psi
Copy link
Contributor Author

mx-psi commented May 10, 2024

@lmb Looks like this is getting fixed upstream: golang/go@27ed85d. The fix is also being backported to Go 1.21 (golang/go/issues/67235) and Go 1.22 (golang/go/issues/67236).

@lmb
Copy link
Collaborator

lmb commented May 10, 2024

Thanks for digging this out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants