turn relative path to absolute path in environment's cgo's flags #1536
Conversation
|
something went wrong with travis... Could you restart it ? |
There was a problem hiding this comment.
Thanks for working on this! Sorry I wasn't able to review earlier.
I agree with the overall goal of this PR, but I'd like to suggest a couple changes to the implementation.
-
The Go tools don't interpret quotes or escapes in cgo flags passed as environment variables. See
envListin cmd/go/internal/work/exec.go; it just usesstrings.Fieldsto split up the flags. We can do the same; we shouldn't useshlex.gofor this. -
Since this process is pretty invasive, there should be an explicit function that does this for a list of environment variables. Let's call it
absEnv. -
We don't pass cgo flags to most builders. This function only needs to be called in cgo.go and stdlib.go.
|
okay will work on that |
|
Hey @jayconrod |
go/tools/builders/cgo.go
Outdated
| @@ -180,6 +180,12 @@ func run(args []string) error { | |||
| } | |||
| } | |||
|
|
|||
| // Modifying CGO flags to use only absolute path | |||
| // because go is having its own sandbox, all CGO flags must use absolute path | |||
| if err := absEnv([]string{"CGO_CFLAGS", "CGO_CXXFLAGS", "CGO_CPPFLAGS", "CGO_LDFLAGS"}, []string{"-I", "-L", "-isysroot", "-isystem", "-iquote", "-include", "-gcc-toolchain", "--sysroot"}); err != nil { | |||
There was a problem hiding this comment.
perhaps they should be defined in a global variable up in env.go
|
Change looks good, but some tests are failing on BuildKite. Specifically |
|
on it |
|
i ve removed absEnv from cgo.go. It s only useful in stdlib.go which is build with go build. |
|
Merged. Thanks for submitting this. I guess it makes sense this wouldn't be needed in cgo -- only the stdlib builder changes the working directory internally. Other builders should be able to tolerate relative paths. |
Make the gopackagesdriver work when using a C/C++ toolchain with sysroots. We need to turn the relative in flag like --sysroot into an absolute. Address the todo by using the code already available in the stdlib.go as part of bazelbuild#1536.
Make the gopackagesdriver work when using a C/C++ toolchain with sysroots. We need to turn the relative in flag like --sysroot into an absolute. Address the todo by using the code already available in the stdlib.go as part of #1536.
This pr can be merge as it, but it s main focus is to discuss a "rules_go" way to fix the problem.
Problem:
cgo flags will sometime be setup with relative path in the environment but Go will make its own sandbox and wont be able to use those path.
Solution:
make all the path absolute in cgo environment variable.
I have no better solution than to embbed shlex in rules_go and use it on the environmnet variables.