[cmd/builder] Disable cgo by default #10040

evan-bradley · 2024-04-29T17:59:48Z

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.

evan-bradley · 2024-04-29T18:02:19Z

cmd/builder/internal/builder/main.go

+		initialEnv, err := getGoEnv(cfg)
+		if err != nil {
+			return err
+		}
+		env = append(env, initialEnv...)


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.

evan-bradley · 2024-04-29T18:04:17Z

cmd/builder/internal/builder/main.go

+	return strings.Split(str, "\n"), nil
+}
+
+func runGoCommand(cfg Config, env []string, args ...string) ([]byte, error) {


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 · 2024-04-29T18:09:38Z

Codecov Report

Attention: Patch coverage is 70.83333% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 91.55%. Comparing base (1a5da25) to head (a299604).
Report is 63 commits behind head on main.

Files	Patch %	Lines
cmd/builder/internal/builder/main.go	70.83%	2 Missing and 5 partials ⚠️

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.
📢 Have feedback on the report? Share it here.

evan-bradley · 2024-04-29T18:16:53Z

failed to compile the OpenTelemetry Collector distribution: go subcommand failed with args '[build -trimpath -o  ->ldflags=-s -w]': exit status 1, error message: warning: GOPATH set to GOROOT () has no effect
           	            	go: module cache not found: neither GOMODCACHE nor GOPATH is set
           	            	go: module cache not found: neither GOMODCACHE nor GOPATH is set

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.

odeke-em · 2024-04-29T23:08:09Z

cmd/builder/internal/builder/main.go

@@ -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 {


You could pass in nil instead of []string{}

Good catch! I'll update this.

odeke-em · 2024-04-29T23:08:21Z

cmd/builder/internal/builder/main.go

@@ -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 {


Ditto about using nil

odeke-em · 2024-04-29T23:08:29Z

cmd/builder/internal/builder/main.go

@@ -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")


Ditto about using nil

TylerHelmuth · 2024-04-29T23:18:31Z

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?

evan-bradley · 2024-04-30T15:38:46Z

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 cgo_enabled option that can be set to true to allow enabling cgo. If cgo is left disabled at build time and a component that requires cgo is used, our contributing guidelines suggest printing a warning and defaulting to a no-op.

github-actions · 2024-05-15T03:16:17Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[cmd/builder] Disable cgo by default

a299604

evan-bradley requested a review from a team as a code owner April 29, 2024 17:59

evan-bradley requested a review from atoulme April 29, 2024 17:59

evan-bradley commented Apr 29, 2024

View reviewed changes

odeke-em reviewed Apr 29, 2024

View reviewed changes

github-actions bot added the Stale label May 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[cmd/builder] Disable cgo by default #10040

[cmd/builder] Disable cgo by default #10040

evan-bradley commented Apr 29, 2024

evan-bradley Apr 29, 2024

evan-bradley Apr 29, 2024

codecov bot commented Apr 29, 2024 •

edited

evan-bradley commented Apr 29, 2024 •

edited

odeke-em Apr 29, 2024

evan-bradley Apr 30, 2024

odeke-em Apr 29, 2024

odeke-em Apr 29, 2024

TylerHelmuth commented Apr 29, 2024

evan-bradley commented Apr 30, 2024

github-actions bot commented May 15, 2024

[cmd/builder] Disable cgo by default #10040

Are you sure you want to change the base?

[cmd/builder] Disable cgo by default #10040

Conversation

evan-bradley commented Apr 29, 2024

Description

Link to tracking issue

Testing

Documentation

evan-bradley Apr 29, 2024

Choose a reason for hiding this comment

evan-bradley Apr 29, 2024

Choose a reason for hiding this comment

codecov bot commented Apr 29, 2024 • edited

Codecov Report

evan-bradley commented Apr 29, 2024 • edited

odeke-em Apr 29, 2024

Choose a reason for hiding this comment

evan-bradley Apr 30, 2024

Choose a reason for hiding this comment

odeke-em Apr 29, 2024

Choose a reason for hiding this comment

odeke-em Apr 29, 2024

Choose a reason for hiding this comment

TylerHelmuth commented Apr 29, 2024

evan-bradley commented Apr 30, 2024

github-actions bot commented May 15, 2024

codecov bot commented Apr 29, 2024 •

edited

evan-bradley commented Apr 29, 2024 •

edited