-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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/builder] Disable cgo by default #10040
Conversation
initialEnv, err := getGoEnv(cfg) | ||
if err != nil { | ||
return err | ||
} | ||
env = append(env, initialEnv...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a huge fan of having this here, but when setting CGO_ENABLED=0
without it, I get this locally:
exit status 1, error message: warning: GOPATH set to GOROOT () has no effect
go: module cache not found: neither GOMODCACHE nor GOPATH is set
Looking at the output of go env
when the env var is set vs. when it isn't, the output is different. So I just got the output when the env var is unset and copy it into the variables for when we set it. This feels off, but at the same time if the go
cli is changing other variables in unexpected ways, perhaps it's safer for us to explicitly set these.
return strings.Split(str, "\n"), nil | ||
} | ||
|
||
func runGoCommand(cfg Config, env []string, args ...string) ([]byte, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a huge fan of the new function signature considering env
won't often be used. I think if we add an option to pass env vars to the go build command, we may be able to just read from cfg
and eliminate this.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10040 +/- ##
==========================================
- Coverage 91.57% 91.55% -0.02%
==========================================
Files 360 360
Lines 16719 16738 +19
==========================================
+ Hits 15310 15325 +15
- Misses 1073 1075 +2
- Partials 336 338 +2 ☔ View full report in Codecov by Sentry. |
It looks like my workaround didn't work on Windows. If anyone has any ideas, they would be welcome, otherwise I'll spend some time later to dig into this. |
@@ -139,7 +169,7 @@ func GetModules(cfg Config) error { | |||
return nil | |||
} | |||
|
|||
if _, err := runGoCommand(cfg, "mod", "tidy", "-compat=1.21"); err != nil { | |||
if _, err := runGoCommand(cfg, []string{}, "mod", "tidy", "-compat=1.21"); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could pass in nil
instead of []string{}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! I'll update this.
@@ -194,7 +224,7 @@ func downloadModules(cfg Config) error { | |||
retries := 3 | |||
failReason := "unknown" | |||
for i := 1; i <= retries; i++ { | |||
if _, err := runGoCommand(cfg, "mod", "download"); err != nil { | |||
if _, err := runGoCommand(cfg, []string{}, "mod", "download"); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto about using nil
@@ -235,7 +265,7 @@ func (c *Config) allComponents() []Module { | |||
|
|||
func (c *Config) readGoModFile() (string, map[string]string, error) { | |||
var modPath string | |||
stdout, err := runGoCommand(*c, "mod", "edit", "-print") | |||
stdout, err := runGoCommand(*c, []string{}, "mod", "edit", "-print") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto about using nil
What happens to ocb users who depended on CGO being enabled? Will their builds work but have issues at runtime or will they just fail to build? |
There's a |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Description
Disables cgo by default in the builder. I'm proposing this as a breaking change because I doubt it will affect many users, most of whom I expect to be using
CGO_ENABLED=0
already, or at a minimum prefer cgo to be disabled. If we would like to have a transition period for this, I can adjust the PR and plan for that.Link to tracking issue
Fixes #10028
Testing
I can't find a good way to directly test this that doesn't involve either setting up a lot of scaffolding to either create a mock
go
cli or injects the necessary code into a top-level function. To keep things simple I have just relied on the existing test suite. I had thought of trying to test the output binary, but Go disables cgo if no compiler is detected on the system, so the test wouldn't be reliable across developer machines.In the future, if we emit a Dockerfile from the builder, we can test that leaving the option as a default runs inside a
scratch
image.Documentation
Updated the builder readme.