Probably we should either explicitly propagate or explicitly clear out every environment variable in the union of envcmd.MkEnv(), envcmd.ExtraEnvVars(), and envcmd.ExtraEnvVarsCostly(), except for the ones that aren't writable (as controlled by envcmd.checkEnvWrite).

That way we can at least make a conscious decision about which environment variables should affect the test.

CC @jayconrod @matloob

The current plan in #46292 is to rely on this testing issue being resolved to unblock the release, so transitively applying release-blocker label.

Hmm, I'm thinking about this some more.

If make.bash sets the environment variable CGO_CFLAGS='-mmacosx-version-min=10.13', won't that imply that users on MacOS will also see the net package as stale (and rebuild it) the first time they build anything that uses it?
Or does that setting somehow get baked into the resulting binaries?

The change to set CGO_CFLAGS='-mmacosx-version-min=10.13' was done as part the work to fix issues #36025 and #35459. Looking at it now, it does seem it might cause net package (or other cgo packages) to become stale if that environment variable is used as part of staleness calculation now and users don't set it. I remember that issue had a lot of complexity and nuance though, so there may be additional factors to consider here. (CC @cagedmantis, @jayconrod who worked on it last time.)

When I looked into it more recently for darwin/arm64 in CL 278435 I hadn't found a way to confirm that setting CGO_CFLAGS before make.bash was still needed to fix a problem, so that that's why the same change hasn't been applied for darwin/arm64 at this time.

I think the default -mmacosx-version-min flag probably ought to be baked into cmd/go, and then not set explicitly on the builder (CC @cherrymui for macOS and linker expertise).

I also wonder why we don't have the same problem with ARM builders from setting the GOARM flags: https://github.com/golang/build/blob/d0819edf598a7f08d0fa642ca67f9d2c094e775b/cmd/release/release.go#L421-L425

I also wonder why we don't have the same problem with ARM builders from setting the GOARM flags

It looks like cmd/go's TestScript is completely skipped on GOARCH=arm via testenv.SkipIfShortAndSlow.

I think the default -mmacosx-version-min flag probably ought to be baked into cmd/go, and then not set explicitly on the builder (CC @cherrymui for macOS and linker expertise).

If that's possible, I think it would likely be an improvement. The fewer special cases we need to maintain in x/build/cmd/release (which apply on top of make.bash and cmd/go but only during the release process and release testing), the better.

I wanted to verify that the original report in this issue applies more generally, so I tested on linux/amd64, and can confirm it also reproduces with something like this:

$ export CGO_CFLAGS='-O3'
$ ./make.bash # etc.
$ go test -run=TestScript/list_std_stale cmd/go

(The same test passes if CGO_CFLAGS in unset before make.bash.)

$ CGO_CFLAGS='-mmacosx-version-min=10.13' ./make.bash 
...
$ CGO_CFLAGS='' go list -f '{{.Stale}}' net
true
$ CGO_CFLAGS='-mmacosx-version-min=10.13' go list -f '{{.Stale}}' net
false

Apparently it is not reported stale if the same flag is passed at staleness check and at make.bash. Does TestScript/list_std_stale do anything special with the flags, like unsetting CGO_CFLAGS?

Does TestScript/list_std_stale do anything special with the flags, like unsetting CGO_CFLAGS?

TestScript (intentionally) only propagates a specific subset of environment variables. I think the idea is that the behavior of the toolchain ought to be independent of the user's environment — especially given that the Go toolchain may be installed by a system administrator but run by an individual user — and most users should not need to set CGO_CFLAGS explicitly.

We have another existing staleness check that is currently skipped on darwin configurations here:

If cmd/cgo is actually stale on the macOS builds that we're distributing to users, I'm not sure exactly what that implies — we don't generally cache binaries, so is it possible that cmd/cgo is being reinstalled on users' machines, or relinked from the build cache every time a macOS user builds any package involving cgo?

I'm not sure exactly what that implies — we don't generally cache binaries, so is it possible that cmd/cgo is being reinstalled on users' machines, or relinked from the build cache every time a macOS user builds any package involving cgo?

I don't think we check binary staleness when invoking a tool binary. The go command just invokes the installed binary, whether it is stale or not.

I think the idea is that the behavior of the toolchain ought to be independent of the user's environment — especially given that the Go toolchain may be installed by a system administrator but run by an individual user — and most users should not need to set CGO_CFLAGS explicitly.

This sounds like we may want to somehow bake the make.bash-time CGO_CFLAGS into the toolchain.

On the other hand, what if a user just wants to set CGO_CFLAGS when building the standard library and toolchain, but not when building user code? Or maybe nobody wants to do that...

On the other hand, what if a user just wants to set CGO_CFLAGS when building the standard library and toolchain, but not when building user code?

I could perhaps see someone setting CGO_FLAGS for building the toolchain.

I don't think that would make sense for the standard library, though — it would just cause each cgo-enabled standard-library package to be rebuilt when the user first builds a program that imports it.

Hmm, ok. So the impact of a CGO_CFLAGS mismatch is:

• Certain packages in std are reported as stale (because they might be changed by the flags).

  • These packages will be rebuilt, at a small one-time cost, when first needed as a go build or go test dependency.

• Certain binaries in cmd are reported as stale (by go list -json and similar commands). It isn't obvious to me whether that is inherently a problem for external tools.

• Certain tests in cmd may fail:

  • cmd/link.TestDWARF and cmd/link.TestDWARFiOS require that cmd/link itself is not stale.

    • At least parts of those tests are bypassed on darwin platforms due to cmd/link: TestDWARF*/testprog is flaky on darwin/amd64 #32218.
    • Parts of those tests explicitly clear out CGO_CFLAGS due to what seems to be the same problem described here — the comment links to cmd/go: explicitly specify macOS base SDK version when compiling go binary #35459, which is the issue for which the CGO_CFLAGS on the release builder was added in the first place.
      • The commit message for CL 216177 in which that was added notes “the --macosx-version-min flag is incompatible with some dwarf tests”, but does not describe the nature of the incompatibility. (Perhaps @cagedmantis has more context?)
      • cmd/link.TestBuildForTvOS also explicitly clears CGO_CFLAGS, even though it does not check for staleness.
    • The staleness check itself was added in CL 38855. The reason it is needed is not obvious to me, and is not documented in the test as far as I can tell. It is currently bypassed on the darwin builders due to x/build: "unexpected stale targets" on darwin builders #33598, and the non-bypassed parts of the test pass there anyway.

  • Multiple tests in cmd/go assume that packages in the Go distribution are not stale:

    • TestScript/build_package_not_stale_trailing_slash assumes that runtime, os, and io are not stale. It is not obvious to me whether os can depend on CGO_CFLAGS, particularly on the platforms that use C libraries for system calls.
    • TestScript/list_std_stale assumes that net is not stale. That test uses staleness as a proxy for whether the dependencies of the package are resolved identically inside and outside of std, but it could perhaps be refactored not to do that.
    • TestScript/test_race_install_cgo checks that cmd/cgo is not stale at the start of the test, but that property might no longer be important to the test as of CL 68338 (Oct. 2017): the test will no longer fail spuriously even if cmd/cgo is stale, as demonstrated by the fact that the test passes on the darwin builders today.
    • Tests that involve the -i flag check that the runtime package is not stale, because otherwise they would attempt to install it to GOROOT. (Tests must in general not assume that they can write to GOROOT.)
      • TestScript/test_relative_import_dash_i tests go test -i.
      • TestGoTestDashIDashOWritesBinary tests go test -i.
      • TestInstallDeps tests go install -i.
      • Note that as of CL 266368 (Go 1.16) the -i flag is deprecated (cmd/go: deprecate the -i flag #41696).

Looking at the above, several things stand out:

1. We have a lot of unintended technical debt surrounding CGO_CFLAGS and staleness checks. 😅
2. A mismatched CGO_CFLAGS setting is mostly harmless as long as the user doesn't run go test cmd.
3. Of the handful of cmd tests that do fail due to staleness, most (all?) arguably shouldn't, especially given that we already bypass some of those staleness checks on the darwin builders.

cmd/link.TestDWARF and cmd/link.TestDWARFiOS require that cmd/link itself is not stale.

I still think that test should not check linker staleness at all. (This was added just to help local development.)

It appears that TestScript/build_package_not_stale_trailing_slash is out-of-date anyway: the test is checking for a missed filepath.Clean, but even if I change GOROOT to a lexically-different symlink the targets are still not reported as stale.

The problem seems to be a build ID mismatch. The build ID stored in net.o does not match with the build ID used in Builder.useCache.

The problem occurs when setting any of the CGO flags. From cgo documentation:

When building, the CGO_CFLAGS, CGO_CPPFLAGS, CGO_CXXFLAGS, CGO_FFLAGS and
CGO_LDFLAGS environment variables are added to the flags derived from
these directives. Package-specific flags should be set using the
directives, not the environment variables, so that builds work in
unmodified environments. Flags obtained from environment variables
are not subject to the security limitations described above.

Adding an fmt.Println in the Builder.buildActionID function to print each processed package cflags, I got:

$ CGO_CFLAGS='-O3' ./make.bash
runtime/cgo [-O3 -Wall -Werror]
os/user [-O3]
net [-O3]
os/signal/internal/pty [-O3]
plugin [-O3]

and

go test -run=TestScript/list_std_stale -short -count=1 cmd/go
runtime/cgo [-g -O2 -Wall -Werror]
net [-g -O2]

On the other hand:

$ ./make.bash
runtime/cgo [-g -O2 -Wall -Werror]
os/user [-g -O2]
net [-g -O2]
os/signal/internal/pty [-g -O2]
plugin [-g -O2]

$ go env CGO_CFLAGS
-g -O2

Change https://golang.org/cl/322629 mentions this issue: cmd/go,cmd/link: do not check for staleness in most tests

I have a doubt: if for packages using cgo the build ID is meaningless, what's the purpose of creating a build ID for them?

The build ID is not meaningless — that determines whether the package needs to be rebuilt.

It just happens that “rebuilding a package” is not nearly as big a deal now that we always have a build cache.

Change https://golang.org/cl/381854 mentions this issue: cmd/go: rewrite TestScript/cgo_stale_precompiled to be agnostic to staleness