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

Put absolute paths into -extldflags #4217

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 38 additions & 1 deletion go/tools/builders/env.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the comment

// cgoAbsEnvFlags are all the flags that need absolute path in cgoEnvVars
cgoAbsEnvFlags = []string{"-I", "-L", "-isysroot", "-isystem", "-iquote", "-include", "-gcc-toolchain", "--sysroot", "-resource-dir", "-fsanitize-blacklist", "-fsanitize-ignorelist"}
// cgoAbsPlaceholder is placed in front of flag values that must be absolutized
Expand Down Expand Up @@ -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).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// 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.

// It is better to use flags with absolute paths in the -extldflags option (in this case, they will not be saved inside the manifest).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we just put all flags into -extldflags instead? What's the advantage of splitting them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 reasons:

  1. I usually prefer to fix only broken things)
  2. 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

func splitAbsEnvVar(originVar string, absArs []string) (string, string, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func splitAbsEnvVar(originVar string, absArs []string) (string, string, error) {
func splitAbsEnvVar(originVar string, absArgs []string) (string, string, error) {

and below.

var absolute []string
var nonAbsolute []string
absNext := false
splited, err := splitQuoted(originVar)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
splited, err := splitQuoted(originVar)
split, err := splitQuoted(originVar)

and below

if err != nil {
return "", "", err
}
for i := range splited {
if absNext {
absolute = append(absolute, splited[i])
absNext = false
continue
}
isAbsolute := false
for _, f := range absArs {
if !strings.HasPrefix(splited[i], f) {
continue
}
isAbsolute = true
absolute = append(absolute, splited[i])
possibleValue := splited[i][len(f):]
if len(possibleValue) == 0 {
absNext = true
}
break
}
if !isAbsolute {
nonAbsolute = append(nonAbsolute, splited[i])
}
}
return strings.Join(absolute, " "), strings.Join(nonAbsolute, " "), nil
}
14 changes: 13 additions & 1 deletion go/tools/builders/stdlib.go
Original file line number Diff line number Diff line change
Expand Up @@ -154,11 +154,23 @@ You may need to use the flags --cpu=x64_windows --compiler=mingw-gcc.`)
break
}
}
absoluteLdFlags, nonAbsoluteLdFlags, err := splitAbsEnvVar(os.Getenv("CGO_LDFLAGS"), cgoAbsEnvFlags)
if err != nil {
return fmt.Errorf("error splitting cgo ldflags: %v", err)
}
os.Setenv("CGO_LDFLAGS", nonAbsoluteLdFlags)
if len(absoluteLdFlags) > 0 {
ldflags = append(ldflags, "-extldflags")
splitFlags := strings.Split(absoluteLdFlags, " ")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are splitting the flags we just joined. Instead, could we have the helper function return string slices?

absArgs(splitFlags, cgoAbsEnvFlags)
ldflags = append(ldflags, splitFlags...)
}

installArgs = append(installArgs, "-gcflags="+allSlug+strings.Join(gcflags, " "))
installArgs = append(installArgs, "-ldflags="+allSlug+strings.Join(ldflags, " "))
installArgs = append(installArgs, "-asmflags="+allSlug+strings.Join(asmflags, " "))

if err := absCCCompiler(cgoEnvVars, cgoAbsEnvFlags); err != nil {
if err := absCCCompiler(cgoAbsEnvVars, cgoAbsEnvFlags); err != nil {
return fmt.Errorf("error modifying cgo environment to absolute path: %v", err)
}

Expand Down
2 changes: 1 addition & 1 deletion go/tools/builders/stdliblist.go
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ func stdliblist(args []string) error {
}
os.Setenv("CC", quotePathIfNeeded(abs(ccEnv)))

if err := absCCCompiler(cgoEnvVars, cgoAbsEnvFlags); err != nil {
if err := absCCCompiler(cgoAbsEnvVars, cgoAbsEnvFlags); err != nil {
return fmt.Errorf("error modifying cgo environment to absolute path: %v", err)
}

Expand Down