Skip to content

Commit f03a723

Browse files
authored
bzltestutil: move os.Chdir call into new package (#3681)
* bzltestutil: move os.Chdir call into new package Sorry for missing this the first time in #3679. The package init algorithm was a little different than I thought, and I imagined that Go 1.21 was already in use, so I didn't look into it deeply. The problem was that even though "+initfirst/.../bzlutil" sorts lower than other packages, it imports "sync" and a number of other std packages that sort higher than "github.com/..." user packages. If a user package has no dependencies, it's eligible to be initialized before bzltestutil, even with the lower name. I moved the init function with the os.Chdir call into a separate tiny package that only imports "os" and "runtime". It should get initialized much earlier. Fixes #3675 * address review feedback * not
1 parent 4526df1 commit f03a723

File tree

6 files changed

+171
-96
lines changed

6 files changed

+171
-96
lines changed

go/private/rules/test.bzl

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,7 @@ def _go_test_impl(ctx):
132132
# We add "+initfirst/" to the package path so the package is initialized
133133
# before user code. See comment above the init function
134134
# in bzltestutil/init.go.
135-
test_gc_linkopts.extend(["-X", "+initfirst/github.com/bazelbuild/rules_go/go/tools/bzltestutil.RunDir=" + run_dir])
135+
test_gc_linkopts.extend(["-X", "+initfirst/github.com/bazelbuild/rules_go/go/tools/bzltestutil/chdir.RunDir=" + run_dir])
136136

137137
# Now compile the test binary itself
138138
test_library = GoLibrary(

go/tools/bzltestutil/BUILD.bazel

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,17 +3,14 @@ load("//go:def.bzl", "go_test", "go_tool_library")
33
go_tool_library(
44
name = "bzltestutil",
55
srcs = [
6-
"init.go",
76
"lcov.go",
87
"test2json.go",
98
"wrap.go",
109
"xml.go",
1110
],
12-
# We add "+initfirst/" to the package path so this package is initialized
13-
# before user code. See comment above the init function in init.go.
14-
importmap = "+initfirst/github.com/bazelbuild/rules_go/go/tools/bzltestutil",
1511
importpath = "github.com/bazelbuild/rules_go/go/tools/bzltestutil",
1612
visibility = ["//visibility:public"],
13+
deps = ["//go/tools/bzltestutil/chdir"],
1714
)
1815

1916
go_test(
@@ -34,7 +31,7 @@ go_test(
3431
filegroup(
3532
name = "all_files",
3633
testonly = True,
37-
srcs = glob(
34+
srcs = ["//go/tools/bzltestutil/chdir:all_files"] + glob(
3835
["**"],
3936
exclude = ["testdata/*"],
4037
),
Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
load("//go:def.bzl", "go_tool_library")
2+
3+
go_tool_library(
4+
name = "chdir",
5+
srcs = ["init.go"],
6+
# We add "+initfirst/" to the package path so this package is initialized
7+
# before user code. See comment above the init function in init.go.
8+
importmap = "+initfirst/github.com/bazelbuild/rules_go/go/tools/bzltestutil/chdir",
9+
importpath = "github.com/bazelbuild/rules_go/go/tools/bzltestutil/chdir",
10+
visibility = ["//go/tools/bzltestutil:__pkg__"],
11+
)
12+
13+
filegroup(
14+
name = "all_files",
15+
testonly = True,
16+
srcs = glob(
17+
["**"],
18+
exclude = ["testdata/*"],
19+
),
20+
visibility = ["//visibility:public"],
21+
)

go/tools/bzltestutil/chdir/init.go

Lines changed: 143 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,143 @@
1+
// Copyright 2020 The Bazel Authors. All rights reserved.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
// Package chdir provides an init function that changes the current working
16+
// directory to RunDir when the test executable is started by Bazel
17+
// (when TEST_SRCDIR and TEST_WORKSPACE are set).
18+
//
19+
// This hides a difference between Bazel and 'go test': 'go test' starts test
20+
// executables in the package source directory, while Bazel starts test
21+
// executables in a directory made to look like the repository root directory.
22+
// Tests frequently refer to testdata files using paths relative to their
23+
// package directory, so open source tests frequently break unless they're
24+
// written with Bazel specifically in mind (using go/runfiles).
25+
//
26+
// For this init function to work, it must be called before init functions
27+
// in all user packages.
28+
//
29+
// In Go 1.20 and earlier, the package initialization order was underspecified,
30+
// other than a requirement that each package is initialized after all its
31+
// transitively imported packages. We relied on the linker initializing
32+
// packages in the order their imports appeared in source, so we import
33+
// bzltestutil (and transitively, this package) from the generated test main
34+
// before other packages.
35+
//
36+
// In Go 1.21, the package initialization order was clarified, and the
37+
// linker implementation was changed. See
38+
// https://go.dev/ref/spec#Program_initialization or
39+
// https://go.dev/doc/go1.21#language.
40+
//
41+
// > Given the list of all packages, sorted by import path, in each step the
42+
// > first uninitialized package in the list for which all imported packages
43+
// > (if any) are already initialized is initialized. This step is repeated
44+
// > until all packages are initialized.
45+
//
46+
// To ensure this package is initialized before user code without injecting
47+
// edges into the dependency graph, we implement the following hack:
48+
//
49+
// 1. Add the prefix '+initfirst/' to this package's path with the 'importmap'
50+
// attribute. '+' is the first allowed character that sorts higher than
51+
// letters. Because we're using 'importmap' and not 'importpath', this
52+
// package may be imported in .go files without the prefix.
53+
// 2. Put this init function in a separate package that only imports "os".
54+
// Previously, this function was in bzltestutil, but bzltest util imports
55+
// several other std packages may be get initialized later. For example,
56+
// the "sync" package is initialized after a user package named
57+
// "github.com/a/b" that only imports "os", and because bzltestutil imports
58+
// "sync", it would get initialized even later. A user package that imports
59+
// nothing may still be initialized before "os", but we assume "os"
60+
// is needed to observe the current directory.
61+
package chdir
62+
63+
// This package should import nothing other than "os"
64+
// and packages imported by "os" (run 'go list -deps os').
65+
import "os"
66+
67+
var (
68+
// Initialized by linker.
69+
RunDir string
70+
71+
// Initial working directory.
72+
TestExecDir string
73+
)
74+
75+
const isWindows = os.PathSeparator == '\\'
76+
77+
func init() {
78+
var err error
79+
TestExecDir, err = os.Getwd()
80+
if err != nil {
81+
panic(err)
82+
}
83+
84+
// Check if we're being run by Bazel and change directories if so.
85+
// TEST_SRCDIR and TEST_WORKSPACE are set by the Bazel test runner, so that makes a decent proxy.
86+
testSrcDir, hasSrcDir := os.LookupEnv("TEST_SRCDIR")
87+
testWorkspace, hasWorkspace := os.LookupEnv("TEST_WORKSPACE")
88+
if hasSrcDir && hasWorkspace && RunDir != "" {
89+
abs := RunDir
90+
if !filepathIsAbs(RunDir) {
91+
abs = filepathJoin(testSrcDir, testWorkspace, RunDir)
92+
}
93+
err := os.Chdir(abs)
94+
// Ignore the Chdir err when on Windows, since it might have have runfiles symlinks.
95+
// https://github.com/bazelbuild/rules_go/pull/1721#issuecomment-422145904
96+
if err != nil && !isWindows {
97+
panic("could not change to test directory: " + err.Error())
98+
}
99+
if err == nil {
100+
os.Setenv("PWD", abs)
101+
}
102+
}
103+
}
104+
105+
// filepathIsAbs is a primitive version of filepath.IsAbs. It handles the
106+
// cases we are likely to encounter but is not specialized at compile time
107+
// and does not support DOS device paths (\\.\UNC\host\share\...) nor
108+
// Plan 9 absolute paths (starting with #).
109+
func filepathIsAbs(path string) bool {
110+
if isWindows {
111+
// Drive-letter path
112+
if len(path) >= 3 &&
113+
('A' <= path[0] && path[0] <= 'Z' || 'a' <= path[0] && path[0] <= 'z') &&
114+
path[1] == ':' &&
115+
(path[2] == '\\' || path[2] == '/') {
116+
return true
117+
}
118+
119+
// UNC path
120+
if len(path) >= 2 && path[0] == '\\' && path[1] == '\\' {
121+
return true
122+
}
123+
return false
124+
}
125+
126+
return len(path) > 0 && path[0] == '/'
127+
}
128+
129+
// filepathJoin is a primitive version of filepath.Join. It only joins
130+
// its arguments with os.PathSeparator. It does not clean arguments first.
131+
func filepathJoin(base string, parts ...string) string {
132+
n := len(base)
133+
for _, part := range parts {
134+
n += 1 + len(part)
135+
}
136+
buf := make([]byte, 0, n)
137+
buf = append(buf, []byte(base)...)
138+
for _, part := range parts {
139+
buf = append(buf, os.PathSeparator)
140+
buf = append(buf, []byte(part)...)
141+
}
142+
return string(buf)
143+
}

go/tools/bzltestutil/init.go

Lines changed: 0 additions & 88 deletions
This file was deleted.

go/tools/bzltestutil/wrap.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@ import (
2727
"strconv"
2828
"strings"
2929
"sync"
30+
31+
"github.com/bazelbuild/rules_go/go/tools/bzltestutil/chdir"
3032
)
3133

3234
// TestWrapperAbnormalExit is used by Wrap to indicate the child
@@ -117,8 +119,8 @@ func Wrap(pkg string) error {
117119
args = append([]string{"-test.v"}, args...)
118120
}
119121
exePath := os.Args[0]
120-
if !filepath.IsAbs(exePath) && strings.ContainsRune(exePath, filepath.Separator) && testExecDir != "" {
121-
exePath = filepath.Join(testExecDir, exePath)
122+
if !filepath.IsAbs(exePath) && strings.ContainsRune(exePath, filepath.Separator) && chdir.TestExecDir != "" {
123+
exePath = filepath.Join(chdir.TestExecDir, exePath)
122124
}
123125
cmd := exec.Command(exePath, args...)
124126
cmd.Env = append(os.Environ(), "GO_TEST_WRAP=0")

0 commit comments

Comments
 (0)