Skip to content

Commit

Permalink
bzltestutil: move os.Chdir call into new package (#3681)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
jayconrod authored Sep 7, 2023
1 parent 4526df1 commit f03a723
Show file tree
Hide file tree
Showing 6 changed files with 171 additions and 96 deletions.
2 changes: 1 addition & 1 deletion go/private/rules/test.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ def _go_test_impl(ctx):
# We add "+initfirst/" to the package path so the package is initialized
# before user code. See comment above the init function
# in bzltestutil/init.go.
test_gc_linkopts.extend(["-X", "+initfirst/github.com/bazelbuild/rules_go/go/tools/bzltestutil.RunDir=" + run_dir])
test_gc_linkopts.extend(["-X", "+initfirst/github.com/bazelbuild/rules_go/go/tools/bzltestutil/chdir.RunDir=" + run_dir])

# Now compile the test binary itself
test_library = GoLibrary(
Expand Down
7 changes: 2 additions & 5 deletions go/tools/bzltestutil/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,14 @@ load("//go:def.bzl", "go_test", "go_tool_library")
go_tool_library(
name = "bzltestutil",
srcs = [
"init.go",
"lcov.go",
"test2json.go",
"wrap.go",
"xml.go",
],
# We add "+initfirst/" to the package path so this package is initialized
# before user code. See comment above the init function in init.go.
importmap = "+initfirst/github.com/bazelbuild/rules_go/go/tools/bzltestutil",
importpath = "github.com/bazelbuild/rules_go/go/tools/bzltestutil",
visibility = ["//visibility:public"],
deps = ["//go/tools/bzltestutil/chdir"],
)

go_test(
Expand All @@ -34,7 +31,7 @@ go_test(
filegroup(
name = "all_files",
testonly = True,
srcs = glob(
srcs = ["//go/tools/bzltestutil/chdir:all_files"] + glob(
["**"],
exclude = ["testdata/*"],
),
Expand Down
21 changes: 21 additions & 0 deletions go/tools/bzltestutil/chdir/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
load("//go:def.bzl", "go_tool_library")

go_tool_library(
name = "chdir",
srcs = ["init.go"],
# We add "+initfirst/" to the package path so this package is initialized
# before user code. See comment above the init function in init.go.
importmap = "+initfirst/github.com/bazelbuild/rules_go/go/tools/bzltestutil/chdir",
importpath = "github.com/bazelbuild/rules_go/go/tools/bzltestutil/chdir",
visibility = ["//go/tools/bzltestutil:__pkg__"],
)

filegroup(
name = "all_files",
testonly = True,
srcs = glob(
["**"],
exclude = ["testdata/*"],
),
visibility = ["//visibility:public"],
)
143 changes: 143 additions & 0 deletions go/tools/bzltestutil/chdir/init.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
// Copyright 2020 The Bazel Authors. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

// Package chdir provides an init function that changes the current working
// directory to RunDir when the test executable is started by Bazel
// (when TEST_SRCDIR and TEST_WORKSPACE are set).
//
// This hides a difference between Bazel and 'go test': 'go test' starts test
// executables in the package source directory, while Bazel starts test
// executables in a directory made to look like the repository root directory.
// Tests frequently refer to testdata files using paths relative to their
// package directory, so open source tests frequently break unless they're
// written with Bazel specifically in mind (using go/runfiles).
//
// For this init function to work, it must be called before init functions
// in all user packages.
//
// In Go 1.20 and earlier, the package initialization order was underspecified,
// other than a requirement that each package is initialized after all its
// transitively imported packages. We relied on the linker initializing
// packages in the order their imports appeared in source, so we import
// bzltestutil (and transitively, this package) from the generated test main
// before other packages.
//
// In Go 1.21, the package initialization order was clarified, and the
// linker implementation was changed. See
// https://go.dev/ref/spec#Program_initialization or
// https://go.dev/doc/go1.21#language.
//
// > Given the list of all packages, sorted by import path, in each step the
// > first uninitialized package in the list for which all imported packages
// > (if any) are already initialized is initialized. This step is repeated
// > until all packages are initialized.
//
// To ensure this package is initialized before user code without injecting
// edges into the dependency graph, we implement the following hack:
//
// 1. Add the prefix '+initfirst/' to this package's path with the 'importmap'
// attribute. '+' is the first allowed character that sorts higher than
// letters. Because we're using 'importmap' and not 'importpath', this
// package may be imported in .go files without the prefix.
// 2. Put this init function in a separate package that only imports "os".
// Previously, this function was in bzltestutil, but bzltest util imports
// several other std packages may be get initialized later. For example,
// the "sync" package is initialized after a user package named
// "github.com/a/b" that only imports "os", and because bzltestutil imports
// "sync", it would get initialized even later. A user package that imports
// nothing may still be initialized before "os", but we assume "os"
// is needed to observe the current directory.
package chdir

// This package should import nothing other than "os"
// and packages imported by "os" (run 'go list -deps os').
import "os"

var (
// Initialized by linker.
RunDir string

// Initial working directory.
TestExecDir string
)

const isWindows = os.PathSeparator == '\\'

func init() {
var err error
TestExecDir, err = os.Getwd()
if err != nil {
panic(err)
}

// Check if we're being run by Bazel and change directories if so.
// TEST_SRCDIR and TEST_WORKSPACE are set by the Bazel test runner, so that makes a decent proxy.
testSrcDir, hasSrcDir := os.LookupEnv("TEST_SRCDIR")
testWorkspace, hasWorkspace := os.LookupEnv("TEST_WORKSPACE")
if hasSrcDir && hasWorkspace && RunDir != "" {
abs := RunDir
if !filepathIsAbs(RunDir) {
abs = filepathJoin(testSrcDir, testWorkspace, RunDir)
}
err := os.Chdir(abs)
// Ignore the Chdir err when on Windows, since it might have have runfiles symlinks.
// https://github.com/bazelbuild/rules_go/pull/1721#issuecomment-422145904
if err != nil && !isWindows {
panic("could not change to test directory: " + err.Error())
}
if err == nil {
os.Setenv("PWD", abs)
}
}
}

// filepathIsAbs is a primitive version of filepath.IsAbs. It handles the
// cases we are likely to encounter but is not specialized at compile time
// and does not support DOS device paths (\\.\UNC\host\share\...) nor
// Plan 9 absolute paths (starting with #).
func filepathIsAbs(path string) bool {
if isWindows {
// Drive-letter path
if len(path) >= 3 &&
('A' <= path[0] && path[0] <= 'Z' || 'a' <= path[0] && path[0] <= 'z') &&
path[1] == ':' &&
(path[2] == '\\' || path[2] == '/') {
return true
}

// UNC path
if len(path) >= 2 && path[0] == '\\' && path[1] == '\\' {
return true
}
return false
}

return len(path) > 0 && path[0] == '/'
}

// filepathJoin is a primitive version of filepath.Join. It only joins
// its arguments with os.PathSeparator. It does not clean arguments first.
func filepathJoin(base string, parts ...string) string {
n := len(base)
for _, part := range parts {
n += 1 + len(part)
}
buf := make([]byte, 0, n)
buf = append(buf, []byte(base)...)
for _, part := range parts {
buf = append(buf, os.PathSeparator)
buf = append(buf, []byte(part)...)
}
return string(buf)
}
88 changes: 0 additions & 88 deletions go/tools/bzltestutil/init.go

This file was deleted.

6 changes: 4 additions & 2 deletions go/tools/bzltestutil/wrap.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ import (
"strconv"
"strings"
"sync"

"github.com/bazelbuild/rules_go/go/tools/bzltestutil/chdir"
)

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

0 comments on commit f03a723

Please sign in to comment.