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

cmd/go: default to CGO_ENABLED=0 when CC not set and default compiler not found #47251

Closed
jayconrod opened this issue Jul 16, 2021 · 9 comments
Closed
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. release-blocker
Milestone

Comments

@jayconrod
Copy link
Contributor

When we fixed #40042, we started including version information about the C compiler in cache action keys that use it. It looks like this:

HASH[build runtime/cgo]: "CC ID=\"gcc version 8.3.0 (Debian 8.3.0-6) \"\n"

This means that if the C compiler changes, Go packages in the cache and in GOROOT/pkg built with the old version will not be used.

Consequently, runtime/cgo, net, os/user, and other packages in std that use cgo by default are stale if the user's C compiler is different than the one used to build the Go binary distribution. It almost always is.

If the user does not have a C compiler installed and has not explicitly set CGO_ENABLED=0, go build will fail to build any package that depends on these packages, even if nothing else uses cgo. This is common when building inside a Docker container and in CI. I'm worried Go 1.17 will break a lot of builds without some mitigation. For example #47215 is caused by this.

One possibility is to default CGO_ENABLED to 0 if CC is not explicitly set, and the default C compiler (usually gcc) is not in PATH. std packages would be stale, but everything could be rebuilt without a C compiler.

This may cause subtle changes in behavior. The cgo versions of net and os/user use libc code to resolve DNS queries and find users. The pure Go versions don't use libc. Also, any package, even a pure Go package may use the cgo build tag for conditional compilation, and that would change.

This is a big behavioral change late in the release cycle, though I expect the diff will be small.

cc @rsc @ianlancetaylor @bcmills @matloob

@jayconrod jayconrod added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. release-blocker labels Jul 16, 2021
@jayconrod jayconrod added this to the Go1.17 milestone Jul 16, 2021
@nishanths
Copy link

nishanths commented Jul 16, 2021

One possibility is to [...] std packages would be stale, but everything could be rebuilt without a C compiler.

With this approach, does this mean that if one makes a local changes to package net (say, to debug an issue), and then builds a binary that imports net, the binary would not include the local changes to package net?

If so, this sounds bad, since it leads to programs being built that don't match the user's expectation, and the user has no way of knowing this is happening.

@ianlancetaylor
Copy link
Contributor

Interesting. I think that at this point in the release cycle we have to roll back the fix for #40042.

@jayconrod
Copy link
Contributor Author

With this approach, does this mean that if one makes a local changes to package net (say, to debug an issue), and then builds a binary that imports net, the binary would not include the local changes to package net?

@nishanths If there are local changes to net or any other package, go build would definitely still pick those up, regardless of this issue. The SHA-256 sum of each source file is part of a package's cache key, so if anything changes, the cache key will change, and the .a file compiled earlier (whether in $GOROOT/pkg or in $GOCACHE) won't be used.

@bcmills
Copy link
Contributor

bcmills commented Jul 16, 2021

Rolling back the fix for #40042 would be unfortunate, since it can lead to subtle problems in other ways (for example, linking against cached packages that were affected by subsequently-fixed bugs, or benchmarking cgo calls that do not accurately reflect the current C compiler).

As a middle ground, would it make sense to omit the CC ID part of the build ID only for packages marked as Standard for Go 1.17? That would allow the build to continue to link against the bundled standard-library packages, but still correctly rebuild stale user packages when the C compiler changes.

(Then we could consider a more holistic fix, such as the one proposed in this issue, for Go 1.18.)

@jayconrod
Copy link
Contributor Author

Interesting. I think that at this point in the release cycle we have to roll back the fix for #40042.

@ianlancetaylor That may be the right thing to do, though @bcmills' suggestion sounds like a good alternative, keeping most of the benefit of that fix.

We've been considering removing pre-compiled .a files from the distribution altogether. I'm planning to file a proposal for that in the next day or two. If we decide to do that, perhaps we should change CGO_ENABLED at the same time, since that change would cause the same problem.

@ianlancetaylor
Copy link
Contributor

Skipping the compiler ID for standard library packages occurred to me also. Let's do that for 1.17.

@gopherbot
Copy link

Change https://golang.org/cl/335409 mentions this issue: cmd/go: don't add C compiler ID to hash for standard library

@jayconrod
Copy link
Contributor Author

After some internal discussion, I'd like to withdraw this issue.

Changing the default of CGO_ENABLED based on whether a C compiler is in PATH is too magical. This can result in a subtle but substantial change to how our users' code is built, so we need to be extremely cautious with a change like this.

Two issues motivated this:

gopherbot pushed a commit that referenced this issue Jul 20, 2021
No test because a real test requires installing two different compilers.

For #40042
For #47251

Change-Id: Iefddd67830d242a119378b7ce20be481904806e4
Reviewed-on: https://go-review.googlesource.com/c/go/+/335409
Trust: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Jay Conrod <jayconrod@google.com>
@golang golang locked and limited conversation to collaborators Jul 20, 2022
@bcmills
Copy link
Contributor

bcmills commented Nov 22, 2022

This was implemented after all in CL 450739.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. release-blocker
Projects
None yet
Development

No branches or pull requests

5 participants