Skip to content

Update go 1.23 and golang.org/x/net v0.38.0 #19118

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

Merged
merged 7 commits into from
May 2, 2025
Merged

Conversation

julienp
Copy link
Contributor

@julienp julienp commented Apr 4, 2025

@julienp
Copy link
Contributor Author

julienp commented Apr 4, 2025

This change is part of the following stack:

Change managed by git-spice.

@julienp julienp marked this pull request as ready for review April 4, 2025 10:35
@julienp julienp requested a review from a team as a code owner April 4, 2025 10:35
This was referenced Apr 4, 2025
Base automatically changed from julienp/go-deps to master April 4, 2025 11:49
@julienp julienp added the do-not-merge Don't merge this PR label Apr 4, 2025
sdk/go.mod Outdated
go 1.22
go 1.23.0

toolchain go1.24.1
Copy link
Member

@justinvp justinvp Apr 30, 2025

Choose a reason for hiding this comment

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

While the toolchain directive may help to mitigate downstream effects of bumping to go 1.23.0, I'm wondering if we should actually avoid adding the toolchain directive as it forces users to use at least this version (1.24.1) which isn't really what we want.

I believe go mod tidy is adding this automatically, but that appears to be unintentional Go tooling behavior that has been addressed for Go 1.25, see: golang/go#65847.

As an example, the toolchain line isn't being specified in Go's /x repos (e.g. https://cs.opensource.google/go/x/crypto/+/master:go.mod).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah nice find! I was also wondering about this, but since go mod tidy always added it, I figured that was the way forward. If we leave the toolchain directive in, I don't think users need to /manually/ install 1.24.1. Go will do that automatically.

I do agree we're better off not adding it though. We'll have to watch out for this until the 1.25 release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually the toolchain directive does not impact user code, which imports our code. I created a Pulumi go program using the SDK from this PR with

fmt.Printf("runtime.Version = %s\n", runtime.Version())

and having go 1.23.0 in its go.mod file.

The version printed is the one from the ambient Go version.

With Go 1.22.12

% which go
/Users/julien/.local/share/mise/installs/go/1.22.12/bin/go
% go version
go version go1.23.0 darwin/arm64      # <- I believe this comes from `go.mod`. If I rename the local go.mod file, it prints 1.22.12
% pulumi preview
...
     runtime.Version = go1.23.0

With Go 1.23.7

% which go
/Users/julien/.local/share/mise/installs/go/1.23.7/bin/go
% go version
go version go1.23.7 darwin/arm64
% pulumi preview
...
    runtime.Version = go1.23.7

With Go 1.24.1

% which go
/Users/julien/.local/share/mise/installs/go/1.24.1/bin/go
% go version
go version go1.24.1 darwin/arm64
pulumi-go-toolchain % pulumi preview
...
    runtime.Version = go1.24.1

  • The go directive set the minimum version required for users.
  • The toolchain directive sets the version used when working inside the pu/pu repo, regardless of the go directive, but does not cascade down to importers of our code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One place where toolchain is harmful for us is in the tests. If tests/go.mod sets toolchain, then we'll always run with the toolchain, regardless of the CI matrix.

The same goes for all the Pulumi programs used in tests.

I removed the toolchain from all these, as well as from sdk and pkg.

Copy link
Contributor Author

@julienp julienp May 1, 2025

Choose a reason for hiding this comment

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

Also these toolchain directives come from dependabot, so we'll have to watch out for this.

Copy link
Member

Choose a reason for hiding this comment

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

Also these toolchain directives come from dependant, so we'll have to watch out for this.

I'm wondering if we should update the tidy script to strip toolchain from go.mod files and also add a test (maybe just a GHA job) that checks that we don't have toolchain in any go.mod in the repo. Apparently, setting GOTOOLCHAIN=local will prevent it from being added, but we probably need to forcibly strip it for cases where it was added.

The dependabot PRs often require us to manually run make tidy to fix up everything anyway. (I wish there was a way to make depandabot do that automatically). Maybe renovate can help with this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found this Dependabot issue dependabot/dependabot-core#11933 which seems like toolchain being added is a bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So my understanding is that dependabot should not be adding toolchain directives, but has in the past due to a bug, it was fixed, and started happening again, sometimes?

With this PR there should be no toolchain directives in our go mod files anymore.

I'll create an issue & PR to add a lint step to ensure there are no toolchain directives, that will help us catch this from our side or from dependabot. Does that sound ok to you @justinvp ?

@julienp julienp force-pushed the julienp/go-deps-2 branch from 25ba6fd to ab25eda Compare May 1, 2025 08:33
@julienp julienp removed the do-not-merge Don't merge this PR label May 2, 2025
@julienp julienp added this pull request to the merge queue May 2, 2025
Merged via the queue into master with commit edd5f81 May 2, 2025
97 of 98 checks passed
@julienp julienp deleted the julienp/go-deps-2 branch May 2, 2025 16:47
julienp added a commit that referenced this pull request May 5, 2025
CI runs a matrix of Go versions, and we want to ensure we’re always building and testing with that version. If we have `toolchain` directives in the `go.mod` files, we’d use those versions instead, defeating the purpose of the matrix.

See #19118 (comment)
pull bot pushed a commit to nebula-aac/pulumi that referenced this pull request May 5, 2025
…ulumi#19421)

CI runs a matrix of Go versions, and we want to ensure we’re always
building and testing with that version. If we have `toolchain`
directives in the `go.mod` files, we’d use those versions instead,
defeating the purpose of the matrix.

See pulumi#19118 (comment)

Fixes pulumi#19407
@pulumi-bot
Copy link
Contributor

This PR has been shipped in release v3.168.0.

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.

4 participants