Bump Go versions to 1.21.1

[fmeum]

What type of PR is this?

Uncomment one line below and remove others.

Bug fix
Feature
Documentation
Other

What does this PR do? Why is it needed?

Which issues(s) does this PR fix?

Fixes #

Other notes for review

[hawkingrei]

We are updating to go1.21 at pingcap/tidb#45923. we have met the same problem. but we have to add some environment to work around it.

[sluongng]

Seems like 2 main issues:

• strip_test.go: need a regex update to handle ? character inside the stack trace
• Gazelle: go_repository.bzl needs to sanitize GOPROXY before feeding it to fetch_repo (i.e. strip out empty spaces)

[sluongng]

There seems to be a 3rd problem: @test_chdir_remote//sub:go_default_test failed because the init() in bzltestutil package was not loaded.

Not quite sure why though.

[sluongng]

Ah it's stated here: https://tip.golang.org/doc/go1.21#language

Package initialization order is now specified more precisely. The new algorithm is:

- Sort all packages by import path.
- Repeat until the list of packages is empty:
  + Find the first package in the list for which all imports are already initialized.
  + Initialize that package and remove it from the list.

This may change the behavior of some programs that rely on a specific initialization ordering that was not expressed by explicit imports. The behavior of such programs was not well defined by the spec in past releases. The new rule provides an unambiguous definition.

[sluongng]

https://go.dev/doc/toolchain need to take care of this as well. @fmeum i think we could patch the env file when we download the sdk to set it at a fixed version?

[fmeum]

https://go.dev/doc/toolchain need to take care of this as well. @fmeum i think we could patch the env file when we download the sdk to set it at a fixed version?

Could we also just set GOTOOLCHAIN=local in all Go actions? That seems simpler and we would need to do it anyway as the user Go env file could take precedence over the one shipped with the Go SDK.

[sluongng]

I would reserve the env var to downstream usages. I.e. if there is a rule that depend on a specific Go toolchain version, they could override using that.

But tbh, i am not entirely sure 🤔

[fmeum]

I would reserve the env var to downstream usages. I.e. if there is a rule that depend on a specific Go toolchain version, they could override using that.

But tbh, i am not entirely sure 🤔

I think that we can never allow this behavior as it would result in a build action downloading a Go SDK. For the sake of hermeticity, we have to stick to toolchain resolution being the only way to bring in an SDK.

We could look into reading toolchain directives from go.mod in the go_sdk extension to automatically download the correct toolchain, but that sounds like quite a bit of effort for only a mild gain in convenience.

[sluongng]

Yeah make sense.

I just don't like the word local here as in our RBE environment, we do have a host's Go SDK, managed by the container image, and another Go SDK which is managed by input root digest of the action.

The sets the default Go toolchain: local indicates the bundled Go toolchain (the one that shipped with the go command being run), and otherwise must be a specific Go toolchain name, such as go1.21.0.

I guess either local or a specific version would work.

[sluongng]

@dragonsinth I think you were involved with setting up @test_chdir_remote//sub:go_default_test so I want to get your comment here.

In Go 1.21, it seems like the order of init() being loaded between different packages has been fixed to be sorted and deterministic. It's no longer guaranteed that bzltestutil.init() is loaded before test_chdir.init(). I think a sensible fix here would be to remove the test_chdir.init() entirely and deem that the result of bzltestutil.init() is available in other packages' init() to be determined based on package name and thus, unreliable, and therefore, not supported. wdyt?

I tried to look around and tested renaming packages to include an aaa prefix, hoping to change the sorting order but could not.

[sluongng]

☝️ I sent #3655 to remove the test_chdir.init() func. Let's discuss the details in the PR to reduce noise.

[dragonsinth]

@dragonsinth I think you were involved with setting up @test_chdir_remote//sub:go_default_test so I want to get your comment here.

In Go 1.21, it seems like the order of init() being loaded between different packages has been fixed to be sorted and deterministic. It's no longer guaranteed that bzltestutil.init() is loaded before test_chdir.init(). I think a sensible fix here would be to remove the test_chdir.init() entirely and deem that the result of bzltestutil.init() is available in other packages' init() to be determined based on package name and thus, unreliable, and therefore, not supported. wdyt?

I tried to look around and tested renaming packages to include an aaa prefix, hoping to change the sorting order but could not.

I think the only real answer is to have test.bzl set the correct working directory when invoking the Go test binary. (I'm not sure why it's not already this way.). All the complicated stuff using the Go init() was a bit of a hack to try to make Go testing work as expected within bazel-- just having the right CWD going in would be far preferrable.

[sluongng]

@dragonsinth Realistically I don't think modifying test.bzl gonna help. The reason is that: the test rules could only expose the test binary to be run and are shielded away from how Bazel runs it. Bazel could decide to run it within various types of local sandboxes, or send it to a remote server to execute the test.

The closest thing we could do is to wrap the go binary with a sh_binary that do the

(
  cd <target dir> && ./go-test-binary <args>
)

but I am sure this would create more problems as well.

Overall I think we should simply ask folks to stop relying on init() order and move their logic into their tests, or parse+handle the Bazel test env variables themselves.

---

@fmeum I think most of the identified issues with 1.21.0 have been fixed. The remaining tasks are:

• Gazelle release and upgrade Gazelle in rules_go to get the GOPROXY fix in.
• Merge bzltestutil: document init() behavior change in Go 1.21 #3655

I will leave these to maintainers folks to handle. 🙏

[dragonsinth]

@dragonsinth Realistically I don't think modifying test.bzl gonna help. The reason is that: the test rules could only expose the test binary to be run and are shielded away from how Bazel runs it. Bazel could decide to run it within various types of local sandboxes, or send it to a remote server to execute the test.

The closest thing we could do is to wrap the go binary with a sh_binary that do the

(
  cd <target dir> && ./go-test-binary <args>
)

I had this thought, but I don't know starlark well enough to try it out. I imagine it should just work tho, especially if the invocation were an exec.

Overall I think we should simply ask folks to stop relying on init() order and move their logic into their tests, or parse+handle the Bazel test env variables themselves.

I mean, the core issue is that go test and bazel test should be as close to the same as is feasible, and what working directory you start in is a pretty big difference; that was the original motivation for the init() approach, just having parity between the two. It's disappointing that this will break. :/

[sluongng]

FYI it might be worth moving to 1.21.1 instead of 1.21.0

[fmeum]

@sluongng @linzhp Do you happen to have an idea why the crypto package is breaking? It doesn't seem to be a Gazelle dependency.

[sluongng]

@fmeum it seems like the version of x/crypto was changed to an older(?) version that does not contain that test target generated in popular_repo test.

fmeum-patch-2 ~/work/bazelbuild/rules_go> bazel query //external:org_golang_x_crypto --output=build
# /Users/sluongng/work/bazelbuild/rules_go/WORKSPACE:83:21
go_repository(
  name = "org_golang_x_crypto",
  generator_name = "org_golang_x_crypto",
  generator_function = "gazelle_dependencies",
  importpath = "golang.org/x/crypto",
  version = "v0.0.0-20190308221718-c2843e01d9a2",
  sum = "h1:VklqNMn3ovrHsnt90PveolxSbWFaJdECFbxSq0Mqo2M=",
)
# Rule org_golang_x_crypto instantiated at (most recent call last):
#   /Users/sluongng/work/bazelbuild/rules_go/WORKSPACE:83:21                                                 in <toplevel>
#   /private/var/tmp/_bazel_sluongng/5c02d8a9b82454292bdfef7b6f9ac04e/external/bazel_gazelle/deps.bzl:272:11 in gazelle_dependencies
#   /private/var/tmp/_bazel_sluongng/5c02d8a9b82454292bdfef7b6f9ac04e/external/bazel_gazelle/deps.bzl:359:18 in _maybe
# Rule go_repository defined at (most recent call last):
#   /private/var/tmp/_bazel_sluongng/5c02d8a9b82454292bdfef7b6f9ac04e/external/bazel_gazelle/internal/go_repository.bzl:340:32 in <toplevel>

Loading: 0 packages loaded


fmeum-patch-2 ~/work/bazelbuild/rules_go> git revert HEAD
[fmeum-patch-2 cea7dd85] Revert "Bump Go versions to 1.21.1"
 2 files changed, 7 insertions(+), 7 deletions(-)


fmeum-patch-2 ~/work/bazelbuild/rules_go> bazel query //external:org_golang_x_crypto --output=build
# /Users/sluongng/work/bazelbuild/rules_go/WORKSPACE:155:14
go_repository(
  name = "org_golang_x_crypto",
  generator_name = "org_golang_x_crypto",
  generator_function = "popular_repos",
  importpath = "golang.org/x/crypto",
  urls = ["https://codeload.github.com/golang/crypto/zip/5ea612d1eb830b38bc4e914e37f55311eb58adce"],
  strip_prefix = "crypto-5ea612d1eb830b38bc4e914e37f55311eb58adce",
  type = "zip",
)

So there are 2 ways to fix this: upgrade x/crypto dependency in popular_repos test (easier?) OR ensure gazelle deps are not overriding rules_go deps.

[sluongng]

I made a quick patch here for your reference master...sluongng:sluongng/patch-pop-repos

[fmeum]

@sluongng That patch looks great, could you submit it as a separate PR? I also submitted bazelbuild/bazel-gazelle#1631.

[sluongng]

That patch partially depends on the Gazelle upgrade though.

Do you want me to include the Gazelle upgrade in it? Or I could wait until after the Gazelle minor release (with your PR above) and upgrade rules_go + patch popular_repos.

[fmeum]

That patch partially depends on the Gazelle upgrade though.

Do you want me to include the Gazelle upgrade in it? Or I could wait until after the Gazelle minor release (with your PR above) and upgrade rules_go + patch popular_repos.

Nevermind then, I will just include the commit here (if you don't mind).

[sluongng]

Please feel free to 👍

[fmeum]

@sluongng Any idea what's up with the secretbox test? We could exclude it, but maybe this hints at a bigger problem.

[sluongng]

hmm weird... it passes on my Mac M1, which suggests that it's something to do specifically with the amd64 implementation of salsa20 (reported in the stack trace) 🤔

Looking at git log from x/crypto, the affected code path seems to be nacl/secretbox_test -> nacl/secretbox -> salsa, which has not been updated for a year.

I tried to reproduce it using BuildBuddy repo setup (+ same x/crypto and gazelle version), using BuildBuddy RBE (running on AMD64), here https://buildbuddy.buildbuddy.io/invocation/ba4a5e0c-b679-4b35-98c4-c4e683a62291?actionDigest=9715dff71e2a95b40f8aeafabb2dbc26a25cadd7200b8a369ed139f2cafd6cd6#action but the test passed 🙃

So I don't have a good way of reproducing this issue on my end.

[fmeum]

Hmm, it looks like the GOPROXY issue is back as well. I thought we had fixed this.

[fmeum]

I found the remaining issue: #3696

[fmeum]

@sluongng There is a single remaining failure in @org_golang_x_tools. Did you encounter this before - maybe I force-pushed over a fix.

[sluongng]

compilepkg: missing strict dependencies:
--
  | C:\b\gc472yia\execroot\io_bazel_rules_go\external\org_golang_x_tools\internal\lockedfile\internal\filelock\filelock_windows.go: import of "golang.org/x/sys/windows"
  | No dependencies were provided.

I have not seen this before no. I would look into how Gazelle is treating that repo, or give #3678 a try.

I might have time to dive into this later next week.

[fmeum]

Recent versions of Gazelle do generate the correct BUILD file, so I just patched that in manually. It's most likely just a result of us not having updated golang.org/x/tools in a long time.