Closed linzhp closed 4 years ago
I think the best way to resolve this is to set the GOROOT
environment variable explicitly in your TestMain
.
runtime.GOROOT()
will return the value of the GOROOT
environment variable if it's not empty. Otherwise, it will return a value set by the linker, determined by the value of the GOROOT_FINAL
environment variable at link time.
rules_go explicitly sets GOROOT_FINAL=GOROOT
. This is the string you're seeing. That string replaces paths in std packages, so we have to set it to something constant for the build to be reproducible. We can't set it to the "real" GOROOT, because that may be different for every sandboxed action.
The generated test main can't really set GOROOT
automatically either. Most tests run without the Go toolchain. Even if a toolchain is available in runfiles, it won't be at a predictable location, and there may be multiple toolchains. This test hardcodes external/go_sdk/bin
, but the toolchain could be in any directory, maybe on the host system.
Tried the following and got the same error:
package xtools
import (
"fmt"
"os"
"path/filepath"
"testing"
"golang.org/x/tools/go/analysis"
"golang.org/x/tools/go/analysis/analysistest"
)
func Test(t *testing.T) {
a := &analysis.Analyzer{
Run: func(p *analysis.Pass) (interface{}, error) {
return nil, nil
},
}
testdata := analysistest.TestData()
analysistest.Run(t, testdata, a, "a")
}
func TestMain(t *testing.M) {
goRoot := "$TEST_SRCDIR/__main__/external/go_sdk"
_ = os.Setenv("GOROOT", os.ExpandEnv(goRoot))
_ = os.Setenv("PATH", os.ExpandEnv(fmt.Sprintf("%s:$PATH", filepath.Join(goRoot, "bin"))))
os.Exit(t.Run())
}
It turn out that runtime.GOROOT()
only honors the GOROOT
during init (https://github.com/golang/go/issues/22302). So I made it work with this:
package xtools
import (
"fmt"
"os"
"path/filepath"
"testing"
"golang.org/x/tools/go/analysis"
"golang.org/x/tools/go/analysis/analysistest"
)
func Test(t *testing.T) {
a := &analysis.Analyzer{
Run: func(p *analysis.Pass) (interface{}, error) {
return nil, nil
},
}
testdata := analysistest.TestData()
analysistest.Run(t, testdata, a, "a")
}
func init() {
goRoot := "$TEST_SRCDIR/__main__/external/go_sdk"
_ = os.Setenv("GOROOT", os.ExpandEnv(goRoot))
_ = os.Setenv("PATH", os.ExpandEnv(fmt.Sprintf("%s:$PATH", filepath.Join(goRoot, "bin"))))
}
Using os.Setenv("GOROOT", ...)
in an init()
isn't an airtight solution for this, it doesn't change the value for the current running program. However because of the testwrapper added in #2348, the os.Setenv
that changes the GOROOT from an init()
runs in the testwrapper, and alters the goroot before the test binary is executed. If the tests are run with --test_env=GO_TEST_WRAP=0
they still fail.
@jayconrod that is an unexpected side effect from the testwrapper I hadn't expected. Regardless of that I don't think exploiting this init/testwrapper interaction is a stable solution. runtime.GOROOT()
can't be changes when a test binary is running, which will make running golang.org/x/tools/go/analysis/analysistest
tests in bazel impossible.
As it needs to be set outside of the test binary, maybe we could add something to rules_go for this? Something like a go_test(..., with_toolchain=True)
or a go_test(..., tags=["needs-go-toolchain"])
, for which rules_go would add a data dependency on @go_sdk//:files
, sets up the required environment variables (PATH/GOROOT) using https://docs.bazel.build/versions/2.0.0/skylark/lib/testing.html#TestEnvironment?
However because of the testwrapper added in #2348, the os.Setenv that changes the GOROOT from an init() runs in the testwrapper, and alters the goroot before the test binary is executed. If the tests are run with --test_env=GO_TEST_WRAP=0 they still fail.
I'm not sure I follow. #2348 didn't add anything that sets GOROOT
in the environment of the child process, did it?
Could the test itself set GOROOT
, then fork/exec itself? That might be a more sound approach, at least on LInux.
As it needs to be set outside of the test binary, maybe we could add something to rules_go for this? Something like a go_test(..., with_toolchain=True) or a go_test(..., tags=["needs-go-toolchain"]), for which rules_go would add a data dependency on @go_sdk//:files, sets up the required environment variables (PATH/GOROOT) using https://docs.bazel.build/versions/2.0.0/skylark/lib/testing.html#TestEnvironment?
I think that would work, but I'm not sure I want to support it. It's difficult (as we are seeing) to run the Go command inside a Bazel sandbox, and it may not work in cross-compiling / remote environments.
Maybe a custom rule using the toolchain API is the way to go?
I'm not sure I follow. #2348 didn't add anything that sets GOROOT in the environment of the child process, did it?
No it did not, but consider
--- some_test.go ---
func init() {
log.Printf("before: %d, %s\n", os.Getpid(), runtime.GOROOT())
os.Setenv("GOROOT", os.ExpandEnv("$TEST_SRCDIR/__main__/external/go_sdk"))
log.Printf("after: %d, %s\n", os.Getpid(), runtime.GOROOT())
}
func TestMain(t *testing.M) {
log.Fatalf("main: %d, %s\n", os.Getpid(), runtime.GOROOT())
}
Running with/without the testwrapper:
$ bazel test //:go_default_test --test_env=GO_TEST_WRAP=0
2020/02/14 20:46:33 before: 28750, GOROOT
2020/02/14 20:46:33 after: 28750, GOROOT
2020/02/14 20:46:33 main: 28750, GOROOT
$ bazel test //:go_default_test --test_env=GO_TEST_WRAP=1
==================== Test output for //:go_default_test:
2020/02/14 20:46:49 before: 28843, GOROOT
2020/02/14 20:46:49 after: 28843, GOROOT
2020/02/14 20:46:49 before: 28844, /private/var/tmp/_bazel_robbert/b21ffbdb27bc569a3409d8e5058938fa/sandbox/darwin-sandbox/272/execroot/__main__/bazel-out/darwin-fastbuild/bin/darwin_amd64_stripped/go_default_test.runfiles/__main__/external/go_sdk
2020/02/14 20:46:49 after: 28844, /private/var/tmp/_bazel_robbert/b21ffbdb27bc569a3409d8e5058938fa/sandbox/darwin-sandbox/272/execroot/__main__/bazel-out/darwin-fastbuild/bin/darwin_amd64_stripped/go_default_test.runfiles/__main__/external/go_sdk
2020/02/14 20:46:49 main: 28844, /private/var/tmp/_bazel_robbert/b21ffbdb27bc569a3409d8e5058938fa/sandbox/darwin-sandbox/272/execroot/__main__/bazel-out/darwin-fastbuild/bin/darwin_amd64_stripped/go_default_test.runfiles/__main__/external/go_sdk
GOROOT
, but with the testwrapper always fork/execing itself /after/ all init()
's have run, changing GOROOT
in an init()
is now "works" for this case when using testwrapper (even though init() runs after the code that determines the value for runtime.GOROOT()
) . Pretty much your fork/exec suggestion but then implicitly done by the wrapper. Just wanted to flag that the testwrapper itself running inits that are part of the test code has some surprising effects.Could the test itself set GOROOT, then fork/exec itself? That might be a more sound approach, at least on LInux.
Yes this should probably work.
Understandable that this would complicate rules_go and you'd prefer not to. Could you elaborate on the cross-compiling/remote-exec issues you forsee? I guess @go_sdk
doesn't differentiate between exec/host/target platform?
Just wanted to flag that the testwrapper itself running inits that are part of the test code has some surprising effects.
That can't really be avoided if the testwrapper is linked into the same binary. inits
in all other packages will run first.
Relying on init
order is always a little dicy though. If the test itself explicitly forks / execs itself, that will guarantee the process that actually runs the tests has GOROOT
set when it starts.
Could you elaborate on the cross-compiling/remote-exec issues you forsee?
I imagine at some point it will be possible to build tests on one platform and execute them on another. It might already be possible. The Go toolchain would only work on the platform the test was built on.
I think we're in agreement then. I didn't intend to suggest relying on the init behavior is a good idea, but mostly wanted to highlight that setting GOROOT in init()
as suggested in the comment before first
closing the ticket worked not because general go supports it, but because of how the embedded wrapper behaves.
Separate of this thread, I would like a testwrapper that doesn't execute any test specific init functions that might have global side effects. I don't think thats possible with the current approach of linking the tests in, but if you have any suggestions I think that would be great. Tracking down why this init()
thing seemed to work was a little confusing.
We'll look at having the tests exec themselves after setting the variable or at a custom rule.
Separate of this thread, I would like a testwrapper that doesn't execute any test specific init functions that might have global side effects. I don't think thats possible with the current approach of linking the tests in, but if you have any suggestions I think that would be great. Tracking down why this init() thing seemed to work was a little confusing.
Not possible without a separate testwrapper
binary I'm afraid. I'm open to that, but, as I think you mentioned earlier on #236, it would interfere with debugging.
Given this complexity, maybe x/tools
should call os.Getenv("GOROOT")
instead of runtime.GOROOT()
?
In case people running into the same issue in the future, this is the code we ended up with:
func TestMain(m *testing.M) {
tempDir := os.Getenv("TEST_TMPDIR")
srcDir := os.Getenv("TEST_SRCDIR")
if len(tempDir) == 0 || len(srcDir) == 0 || runtime.GOROOT() != "GOROOT" {
os.Exit(m.Run())
}
cmd := exec.Command(os.Args[0], os.Args[1:]...)
// assuming the Bazel rule has `data = ["@go_sdk//:files"]`
goRoot := filepath.Join(srcDir, "__main__/external/go_sdk")
// Go executable requires a GOCACHE to be set after go1.12.
cmd.Env = append(os.Environ(),
"GOCACHE="+filepath.Join(tempDir, "cache"),
"GOROOT="+goRoot,
fmt.Sprintf("PATH=%s:%s", filepath.Join(goRoot, "bin"), os.Getenv("PATH")),
)
cmd.Stdout = os.Stdout
cmd.Stderr = os.Stderr
err := cmd.Run()
if err == nil {
os.Exit(0)
}
if xerr, ok := err.(*exec.ExitError); ok {
os.Exit(xerr.ExitCode())
}
fmt.Fprintln(os.Stderr, err)
// Exist with 6, in line with Bazel's RUN_FAILURE.
os.Exit(6)
}
Not sure if there is a better way to share this code
Is the problem resolved? The related PR is closed (not merged) https://github.com/golang/tools/pull/210
See my last comment above
I can't figure out how to get a go_tool_library test to run. I've tried @linzhp's solution of forking the binary and overriding the environment variables. If I do that I get the error:
go tool not available: exit status 2
Reproducible repo: https://github.com/jschaf/bazel-bug-go-tool/blob/master/lint_env/import_unsafe_test.go
load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test", "go_tool_library")
go_tool_library(
name = "lint",
srcs = ["import_unsafe.go"],
importpath = "github.com/jschaf/bazel-bug-go-tool/lint",
visibility = ["//visibility:public"],
deps = ["@org_golang_x_tools//go/analysis:go_tool_library"],
)
go_test(
name = "lint_test",
srcs = ["import_unsafe_test.go"],
data = ["@go_sdk//:files"],
embed = [":lint"],
deps = [
"@org_golang_x_tools//go/analysis:go_default_library",
"@org_golang_x_tools//go/analysis/analysistest:go_default_library",
],
)
As a sidenote, it looks like we can use go_library
instead of go_tool_library
. https://github.com/bazelbuild/rules_go/pull/2474
Edit: maybe not, I got an import cycle
.-> @io_bazel_rules_nogo//:nogo
| //build/go/lint:nogo
| //build/go/lint/channel_suffix:channel_suffix
| @io_bazel_rules_go//:go_context_data
`-- @io_bazel_rules_nogo//:nogo
Facing the same issue, any better solution then setting in TestMain?
We tried essentially the same approach as above -- setting up the right PATH/ENV vars during init
time: https://github.com/cockroachdb/cockroach/pull/61667/files. It's a bit unfortunate. I'm not sure what would be better, but only posting here as possible documentation for future readers.
I found that the GOROOT location made available was different from the one selected by @linzhp's TestMain, so here is my tweak to scan for GOROOT, since bazel doesn't really make any promises about the directory structure of the files it hands you.
func TestMain(m *testing.M) {
tempDir := os.Getenv("TEST_TMPDIR")
srcDir := os.Getenv("TEST_SRCDIR")
if len(tempDir) == 0 || len(srcDir) == 0 || runtime.GOROOT() != "GOROOT" {
os.Exit(m.Run())
}
cmd := exec.Command(os.Args[0], os.Args[1:]...)
goRoot, foundGOROOT := resolveGOROOT()
if !foundGOROOT {
fmt.Fprintln(
os.Stderr,
"Could not find GOROOT. Make sure you have `data = [\"@go_sdk//:files\"]` in your go_test bazel config block.",
)
os.Exit(6) // Exit with 6, in line with Bazel's RUN_FAILURE.
}
// Go executable requires a GOCACHE to be set after go1.12.
cmd.Env = append(os.Environ(),
"GOCACHE="+filepath.Join(tempDir, "cache"),
"GOROOT="+goRoot,
fmt.Sprintf("PATH=%s:%s", filepath.Join(goRoot, "bin"), os.Getenv("PATH")),
)
cmd.Stdout = os.Stdout
cmd.Stderr = os.Stderr
err := cmd.Run()
if err == nil {
os.Exit(0)
}
if xerr, ok := err.(*exec.ExitError); ok {
os.Exit(xerr.ExitCode())
}
fmt.Fprintln(os.Stderr, err)
// Exit with 6, in line with Bazel's RUN_FAILURE.
os.Exit(6)
}
func resolveGOROOT() (string, bool) {
runfiles, err := bazel.ListRunfiles()
if err == nil {
for _, r := range runfiles {
if strings.HasSuffix(r.Path, "go_sdk/bin/go") {
os.Setenv("PATH", path.Dir(r.Path) + ":" + os.Getenv("PATH"))
return path.Dir(path.Dir(r.Path)), true
}
}
}
return "", false
}
The bazel
package here is "github.com/bazelbuild/rules_go/go/tools/bazel"
https://github.com/golang/tools/commit/7dd52f09642e72aa8eda78359919c8129a51819b rejects Go command if
go env GOROOT
!=runtime.GOROOT()
. However, rules_go doesn't set the environment variableGOROOT
properly, andruntime.GOROOT()
reads GOROOT, resulting in the mismatch.cc @blico @abhinav @vitarb
What version of rules_go are you using?
v0.21.2
What version of gazelle are you using?
v0.20.0
What version of Bazel are you using?
2.1.0
Does this issue reproduce with the latest releases of all the above?
Yes
What operating system and processor architecture are you using?
macOS, amd64
What did you do?
Run this test:
What did you expect to see?
Test fail with error similar to
xtools/testdata: no such file or directory *os.PathError
What did you see instead?