Put absolute paths into -extldflags #4217
Conversation
| @@ -33,7 +33,7 @@ import ( | |||
|
|
|||
| var ( | |||
| // cgoEnvVars is the list of all cgo environment variable | |||
| cgoEnvVars = []string{"CGO_CFLAGS", "CGO_CXXFLAGS", "CGO_CPPFLAGS", "CGO_LDFLAGS"} | |||
| cgoAbsEnvVars = []string{"CGO_CFLAGS", "CGO_CXXFLAGS", "CGO_CPPFLAGS"} | |||
| @@ -594,3 +594,40 @@ func useResponseFile(path string, argLen int) bool { | |||
| } | |||
| return false | |||
| } | |||
|
|
|||
| // The go build system collects the CGO_LDFLAGS from manifests (_go_.o) and uses them in the user module linking. | |||
| // In some cases, such as when using the distributed Bazel executor, these paths may not exist (different build agents uses different sandboxing paths). | |||
There was a problem hiding this comment.
| // In some cases, such as when using the distributed Bazel executor, these paths may not exist (different build agents uses different sandboxing paths). | |
| // In some cases, such as when using remote execution, these paths may not exist (different build agents uses different sandboxing paths). |
Does this also come up with sandboxed builds? The absolute path prefixes should already be different in that case.
| var absolute []string | ||
| var nonAbsolute []string | ||
| absNext := false | ||
| splited, err := splitQuoted(originVar) |
There was a problem hiding this comment.
| splited, err := splitQuoted(originVar) | |
| split, err := splitQuoted(originVar) |
and below
| // The go build system collects the CGO_LDFLAGS from manifests (_go_.o) and uses them in the user module linking. | ||
| // In some cases, such as when using the distributed Bazel executor, these paths may not exist (different build agents uses different sandboxing paths). | ||
| // It is better to use flags with absolute paths in the -extldflags option (in this case, they will not be saved inside the manifest). | ||
| func splitAbsEnvVar(originVar string, absArs []string) (string, string, error) { |
There was a problem hiding this comment.
| func splitAbsEnvVar(originVar string, absArs []string) (string, string, error) { | |
| func splitAbsEnvVar(originVar string, absArgs []string) (string, string, error) { |
and below.
|
|
||
| // The go build system collects the CGO_LDFLAGS from manifests (_go_.o) and uses them in the user module linking. | ||
| // In some cases, such as when using the distributed Bazel executor, these paths may not exist (different build agents uses different sandboxing paths). | ||
| // It is better to use flags with absolute paths in the -extldflags option (in this case, they will not be saved inside the manifest). |
There was a problem hiding this comment.
Could we just put all flags into -extldflags instead? What's the advantage of splitting them?
There was a problem hiding this comment.
2 reasons:
- I usually prefer to fix only broken things)
- I think it's a "go-way" for providing ldflags to dependencies (some kind of "transitive" flags). I'm afraid that changing this behavior car broke builds in some strange places. But it's a theory.
If you think that it's better to use -extldflags for all, I can implement it.
There was a problem hiding this comment.
It seems as if we somewhat lose the benefits of 2. as soon as we move any flag over, so I would be in favor of moving all of them. This aligns better with how Bazel works: We can track link flags of all transitive deps via providers, so there is no need to track them in executables.
The only potential breakage I can think of is for Go generated static archives that are considered shippable artifacts. That seems so rare that I don't think we should worry about it.
@linzhp @jayconrod Do you have opinions on this?
| os.Setenv("CGO_LDFLAGS", nonAbsoluteLdFlags) | ||
| if len(absoluteLdFlags) > 0 { | ||
| ldflags = append(ldflags, "-extldflags") | ||
| splitFlags := strings.Split(absoluteLdFlags, " ") |
There was a problem hiding this comment.
We are splitting the flags we just joined. Instead, could we have the helper function return string slices?
|
Could you post examples of the paths that cause cache misses? I would like to understand why this hasn't come up before. |
|
Before reviewing this, I think we need a clearer description of the problem being solved. Please open an issue explaining that and link here. What are the absolute paths coming in through cgo? Why is that happening? I suspect that the Go builder and the linker may be the wrong place to fix this. Maybe we can avoid absolute paths in the first place. |
|
I was unable to quickly reproduce the issue using the actual opensource tools (in our repository we use self-built compiler and sysroot). The original issue in our build occurred on an older version of basel and a different configuration of the buildbarn server (few months ago). I was too lazy to creating pool request immediately, which I now regret) |
Bug fix
The go build system stores information about build flags (and environment variables) in the
_go_.ofile (go package manifest). When you use cgo, data from "CGO_LDFLAGS" is added to this file as is and used during final binary linking. If you add absolute path to "CGO_LDFLAGS" environment, it will be save inside manifest too. However, when using a distributed build system such as BuildBarn, different build agents not share all paths (such as "bazel sandboxing" paths). If the_go_.ofile inside go library contains non-existent paths, linking with this module fails.This patch puts all flags with absolute paths into
-extldflags. In this case its not stored in the go package manifest.Problem can be reproduced only on specific cases (we use custom sysroot and distributed build system) so I don't know how to write good "common" test.