From a88632793cbd9cce2c1384fcfd7a92c954f0bb05 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Tue, 8 Mar 2022 00:27:30 +0000 Subject: [PATCH 01/18] stdliblist: Fix for Go 1.18 by replicating stdlib The stdliblist operation that the gopackagesdriver relies on fails on Go 1.18rc1 with the following error: ``` external/go_sdk/src/crypto/elliptic/p256_asm.go:24:12: pattern p256_asm_table.bin: cannot embed irregular file p256_asm_table.bin ``` We see this failure because Bazel builds a symlink tree of the GOROOT run `go list` with. However, since [CL 380475][1], the Go standard library uses `go:embed` to embed a file in `crypto/elliptic`, but `go:embed` does not support symlinks. [1]: https://go.dev/cl/380475 Fix this by having stdliblist copy the relevant portions of the GOROOT to run `go list` with. This matches [what the stdlib action does][2]. [2]: https://github.com/bazelbuild/rules_go/blob/e9a7054ff11a520e3b8aceb76a3ba44bb8da4c94/go/tools/builders/stdlib.go#L54-L57 Resolves #3080 --- go/tools/builders/stdliblist.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/go/tools/builders/stdliblist.go b/go/tools/builders/stdliblist.go index ec7e3161ae..8d26fa6146 100644 --- a/go/tools/builders/stdliblist.go +++ b/go/tools/builders/stdliblist.go @@ -158,19 +158,22 @@ func stdliblist(args []string) error { return err } + execRoot := abs(".") + if err := replicate(os.Getenv("GOROOT"), execRoot, replicatePaths("src", "pkg/tool", "pkg/include")); err != nil { + return err + } + // Ensure paths are absolute. absPaths := []string{} for _, path := range filepath.SplitList(os.Getenv("PATH")) { absPaths = append(absPaths, abs(path)) } os.Setenv("PATH", strings.Join(absPaths, string(os.PathListSeparator))) - os.Setenv("GOROOT", abs(os.Getenv("GOROOT"))) + os.Setenv("GOROOT", execRoot) // Make sure we have an absolute path to the C compiler. // TODO(#1357): also take absolute paths of includes and other paths in flags. os.Setenv("CC", abs(os.Getenv("CC"))) - execRoot := abs(".") - cachePath := abs(*out + ".gocache") defer os.RemoveAll(cachePath) os.Setenv("GOCACHE", cachePath) From 6c94802405ff0d248c13125b029651d180605377 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Tue, 8 Mar 2022 17:18:16 +0000 Subject: [PATCH 02/18] test/stdlib: Depend on _list_json Add a dependency on `GoStdLib._list_json` to the stdlib test. This ensures that we can successfully build the JSON data needed by the gopackagesdriver. --- tests/core/stdlib/stdlib_files.bzl | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/core/stdlib/stdlib_files.bzl b/tests/core/stdlib/stdlib_files.bzl index b2b1fb8dae..841c9079e8 100644 --- a/tests/core/stdlib/stdlib_files.bzl +++ b/tests/core/stdlib/stdlib_files.bzl @@ -26,10 +26,11 @@ pure_transition = transition( def _stdlib_files_impl(ctx): # When a transition is used, ctx.attr._stdlib is a list of Target instead # of a Target. Possibly a bug? - libs = ctx.attr._stdlib[0][GoStdLib].libs + stdlib = ctx.attr._stdlib[0][GoStdLib] + libs = stdlib.libs runfiles = ctx.runfiles(files = libs) return [DefaultInfo( - files = depset(libs), + files = depset(libs + [stdlib._list_json]), runfiles = runfiles, )] From fa5c6595dc33216fc38263f1bf77a1b1f229890b Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Tue, 8 Mar 2022 21:45:20 +0000 Subject: [PATCH 03/18] Fix file paths relative to OUTPUT_BASE The prior version of this fix was incomplete because it generated incorrect relative paths. For example, before a path would be: __BAZEL_OUTPUT_BASE__/external/go_sdk/src/archive/tar/common.go But with the prior version of this change. __BAZEL_OUTPUT_BASE__/src/archive/tar/common.go It would drop the external/go_sdk from the path because the flattening logic in stdliblist makes these relative to the execRoot. We cannot overwrite external/go_sdk in execRoot because that's a path controlled by Bazel. Instead, create a parallel external/go_sdk under a new directory "root", and flatten paths relative to that. --- go/tools/builders/stdliblist.go | 43 ++++++++++++++++++++++++++++++--- 1 file changed, 40 insertions(+), 3 deletions(-) diff --git a/go/tools/builders/stdliblist.go b/go/tools/builders/stdliblist.go index 8d26fa6146..7298eea969 100644 --- a/go/tools/builders/stdliblist.go +++ b/go/tools/builders/stdliblist.go @@ -18,6 +18,7 @@ import ( "bytes" "encoding/json" "flag" + "fmt" "go/build" "os" "path/filepath" @@ -145,6 +146,42 @@ func flatPackageForStd(execRoot string, pkg *goListPackage) *flatPackage { return newPkg } +// In Go 1.18, the standard library started using go:embed directives. +// When Bazel runs this action, it does so inside a sandbox where GOROOT points +// to an external/go_sdk directory that contains a symlink farm of all files in +// the Go SDK. +// If we run "go list" with that GOROOT, this action will fail because those +// go:embed directives will refuse to include the symlinks in the sandbox. +// +// To work around this, cloneRoot creates a copy of external/go_sdk into a new +// directory "root" while retaining its path relative to the root directory. +// So "$OUTPUT_BASE/external/go_sdk" becomes +// "$OUTPUT_BASE/root/external/go_sdk". +// This ensures that file paths in the generated JSON are still valid. +// +// cloneRoot returns the new root directory and the new GOROOT we should run +// under. +func cloneRoot(execRoot, goroot string) (newRoot string, newGoroot string, err error) { + relativeGoroot, err := filepath.Rel(abs(execRoot), abs(goroot)) + if err != nil { + // GOROOT has to be a subdirectory of execRoot. + // Otherwise we're breaking the sandbox. + return "", "", fmt.Errorf("GOROOT %q is not relative to execution root %q: %v", goroot, execRoot) + } + + newRoot = filepath.Join(execRoot, "root") + newGoroot = filepath.Join(newRoot, relativeGoroot) + if err := os.MkdirAll(newGoroot, 01755); err != nil { + return "", "", err + } + + if err := replicate(goroot, newGoroot, replicatePaths("src", "pkg/tool", "pkg/include")); err != nil { + return "", "", err + } + + return newRoot, newGoroot, nil +} + // stdliblist runs `go list -json` on the standard library and saves it to a file. func stdliblist(args []string) error { // process the args @@ -158,8 +195,8 @@ func stdliblist(args []string) error { return err } - execRoot := abs(".") - if err := replicate(os.Getenv("GOROOT"), execRoot, replicatePaths("src", "pkg/tool", "pkg/include")); err != nil { + execRoot, goroot, err := cloneRoot(".", os.Getenv("GOROOT")) + if err != nil { return err } @@ -169,7 +206,7 @@ func stdliblist(args []string) error { absPaths = append(absPaths, abs(path)) } os.Setenv("PATH", strings.Join(absPaths, string(os.PathListSeparator))) - os.Setenv("GOROOT", execRoot) + os.Setenv("GOROOT", goroot) // Make sure we have an absolute path to the C compiler. // TODO(#1357): also take absolute paths of includes and other paths in flags. os.Setenv("CC", abs(os.Getenv("CC"))) From 41c0139713a9938aa0ecc33a0e6e29fb9bf3c0d0 Mon Sep 17 00:00:00 2001 From: Xiaoyang Tan Date: Thu, 12 May 2022 00:03:37 -0700 Subject: [PATCH 04/18] address comments --- go/tools/builders/stdliblist.go | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/go/tools/builders/stdliblist.go b/go/tools/builders/stdliblist.go index 7298eea969..50bed68889 100644 --- a/go/tools/builders/stdliblist.go +++ b/go/tools/builders/stdliblist.go @@ -18,8 +18,8 @@ import ( "bytes" "encoding/json" "flag" - "fmt" "go/build" + "io/ioutil" "os" "path/filepath" "strings" @@ -110,29 +110,29 @@ func stdlibPackageID(importPath string) string { return "@io_bazel_rules_go//stdlib:" + importPath } -func execRootPath(execRoot, p string) string { - dir, _ := filepath.Rel(execRoot, p) +func cloneBasePath(cloneBase, p string) string { + dir, _ := filepath.Rel(cloneBase, p) return filepath.Join("__BAZEL_OUTPUT_BASE__", dir) } -func absoluteSourcesPaths(execRoot, pkgDir string, srcs []string) []string { +func absoluteSourcesPaths(cloneBase, pkgDir string, srcs []string) []string { ret := make([]string, 0, len(srcs)) - pkgDir = execRootPath(execRoot, pkgDir) + pkgDir = cloneBasePath(cloneBase, pkgDir) for _, src := range srcs { ret = append(ret, filepath.Join(pkgDir, src)) } return ret } -func flatPackageForStd(execRoot string, pkg *goListPackage) *flatPackage { +func flatPackageForStd(cloneBase string, pkg *goListPackage) *flatPackage { // Don't use generated files from the stdlib - goFiles := absoluteSourcesPaths(execRoot, pkg.Dir, pkg.GoFiles) + goFiles := absoluteSourcesPaths(cloneBase, pkg.Dir, pkg.GoFiles) newPkg := &flatPackage{ ID: stdlibPackageID(pkg.ImportPath), Name: pkg.Name, PkgPath: pkg.ImportPath, - ExportFile: execRootPath(execRoot, pkg.Target), + ExportFile: cloneBasePath(cloneBase, pkg.Target), Imports: map[string]string{}, Standard: pkg.Standard, GoFiles: goFiles, @@ -161,15 +161,13 @@ func flatPackageForStd(execRoot string, pkg *goListPackage) *flatPackage { // // cloneRoot returns the new root directory and the new GOROOT we should run // under. -func cloneRoot(execRoot, goroot string) (newRoot string, newGoroot string, err error) { - relativeGoroot, err := filepath.Rel(abs(execRoot), abs(goroot)) +func cloneRoot(cloneBase, relativeGoroot string) (newRoot string, newGoroot string, err error) { + goroot := filepath.Join(cloneBase, relativeGoroot) + + newRoot, err = ioutil.TempDir(cloneBase, "root-*") if err != nil { - // GOROOT has to be a subdirectory of execRoot. - // Otherwise we're breaking the sandbox. - return "", "", fmt.Errorf("GOROOT %q is not relative to execution root %q: %v", goroot, execRoot) + return "", "", err } - - newRoot = filepath.Join(execRoot, "root") newGoroot = filepath.Join(newRoot, relativeGoroot) if err := os.MkdirAll(newGoroot, 01755); err != nil { return "", "", err @@ -195,7 +193,7 @@ func stdliblist(args []string) error { return err } - execRoot, goroot, err := cloneRoot(".", os.Getenv("GOROOT")) + cloneBase, goroot, err := cloneRoot(".", goenv.sdk) if err != nil { return err } @@ -241,7 +239,7 @@ func stdliblist(args []string) error { if err := decoder.Decode(&pkg); err != nil { return err } - if err := encoder.Encode(flatPackageForStd(execRoot, pkg)); err != nil { + if err := encoder.Encode(flatPackageForStd(cloneBase, pkg)); err != nil { return err } } From 1de0b739a5c7c860ad889ac166f016de0f645ba5 Mon Sep 17 00:00:00 2001 From: Xiaoyang Tan Date: Thu, 12 May 2022 16:51:46 -0700 Subject: [PATCH 05/18] wip --- go/private/actions/stdlib.bzl | 2 ++ go/tools/builders/BUILD.bazel | 13 +++++++ go/tools/builders/env.go | 8 +++++ go/tools/builders/stdliblist.go | 51 ++++++++++++++++------------ go/tools/builders/stdliblist_test.go | 51 ++++++++++++++++++++++++++++ 5 files changed, 103 insertions(+), 22 deletions(-) create mode 100644 go/tools/builders/stdliblist_test.go diff --git a/go/private/actions/stdlib.bzl b/go/private/actions/stdlib.bzl index 80f4011fa5..187249cd0f 100644 --- a/go/private/actions/stdlib.bzl +++ b/go/private/actions/stdlib.bzl @@ -52,8 +52,10 @@ def _should_use_sdk_stdlib(go): go.mode.link == LINKMODE_NORMAL) def _build_stdlib_list_json(go): + print("this is the go_sdk_path: %s\n" % go.sdk.root_file.dirname) out = go.declare_file(go, "stdlib.pkg.json") args = go.builder_args(go, "stdliblist") + args.add("-sdk", go.sdk.root_file.dirname) args.add("-out", out) go.actions.run( inputs = go.sdk_files, diff --git a/go/tools/builders/BUILD.bazel b/go/tools/builders/BUILD.bazel index d4924ff84a..f500b55888 100644 --- a/go/tools/builders/BUILD.bazel +++ b/go/tools/builders/BUILD.bazel @@ -23,6 +23,19 @@ go_test( ], ) +go_test( + name = "stdliblist_test", + size = "small", + data = ["@go_sdk//:files"], + srcs = [ + "stdliblist.go", + "stdliblist_test.go", + "env.go", + "flags.go", + "replicate.go", + ], +) + filegroup( name = "builder_srcs", srcs = [ diff --git a/go/tools/builders/env.go b/go/tools/builders/env.go index d05992579e..9c78d50b0e 100644 --- a/go/tools/builders/env.go +++ b/go/tools/builders/env.go @@ -44,6 +44,9 @@ var ( // See ./README.rst for more information about handling arguments and // environment variables. type env struct { + // cwd is the path to the working directory + wd string + // sdk is the path to the Go SDK, which contains tools for the host // platform. This may be different than GOROOT. sdk string @@ -71,6 +74,7 @@ func envFlags(flags *flag.FlagSet) *env { flags.StringVar(&env.installSuffix, "installsuffix", "", "Standard library under GOROOT/pkg") flags.BoolVar(&env.verbose, "v", false, "Whether subprocess command lines should be printed") flags.BoolVar(&env.shouldPreserveWorkDir, "work", false, "if true, the temporary work directory will be preserved") + flags.StringVar(&env.wd, "wd", ".", "working director default to dot") return env } @@ -121,6 +125,10 @@ func (e *env) goTool(tool string, args ...string) []string { // and additional arguments. func (e *env) goCmd(cmd string, args ...string) []string { exe := filepath.Join(e.sdk, "bin", "go") + if len(e.wd) > 0 { + exe = filepath.Join(e.wd, exe) + } + if runtime.GOOS == "windows" { exe += ".exe" } diff --git a/go/tools/builders/stdliblist.go b/go/tools/builders/stdliblist.go index 50bed68889..3df60d480e 100644 --- a/go/tools/builders/stdliblist.go +++ b/go/tools/builders/stdliblist.go @@ -19,7 +19,6 @@ import ( "encoding/json" "flag" "go/build" - "io/ioutil" "os" "path/filepath" "strings" @@ -110,14 +109,17 @@ func stdlibPackageID(importPath string) string { return "@io_bazel_rules_go//stdlib:" + importPath } -func cloneBasePath(cloneBase, p string) string { +// labelledPath replace the cloneBase with output base label +func labelledPath(cloneBase, p string) string { dir, _ := filepath.Rel(cloneBase, p) return filepath.Join("__BAZEL_OUTPUT_BASE__", dir) } +// absoluteSourcesPaths replace cloneBase of the absolution +// paths with the label for all source files in a package func absoluteSourcesPaths(cloneBase, pkgDir string, srcs []string) []string { ret := make([]string, 0, len(srcs)) - pkgDir = cloneBasePath(cloneBase, pkgDir) + pkgDir = labelledPath(cloneBase, pkgDir) for _, src := range srcs { ret = append(ret, filepath.Join(pkgDir, src)) } @@ -132,7 +134,7 @@ func flatPackageForStd(cloneBase string, pkg *goListPackage) *flatPackage { ID: stdlibPackageID(pkg.ImportPath), Name: pkg.Name, PkgPath: pkg.ImportPath, - ExportFile: cloneBasePath(cloneBase, pkg.Target), + ExportFile: labelledPath(cloneBase, pkg.Target), Imports: map[string]string{}, Standard: pkg.Standard, GoFiles: goFiles, @@ -153,31 +155,29 @@ func flatPackageForStd(cloneBase string, pkg *goListPackage) *flatPackage { // If we run "go list" with that GOROOT, this action will fail because those // go:embed directives will refuse to include the symlinks in the sandbox. // -// To work around this, cloneRoot creates a copy of external/go_sdk into a new -// directory "root" while retaining its path relative to the root directory. +// To work around this, cloneGoRoot creates a copy of external/go_sdk into a new +// cloneBase directory while retaining its path relative to the root directory. // So "$OUTPUT_BASE/external/go_sdk" becomes -// "$OUTPUT_BASE/root/external/go_sdk". +// tmp_directory/external/go_sdk", which will be set at GOROOT later. // This ensures that file paths in the generated JSON are still valid. // -// cloneRoot returns the new root directory and the new GOROOT we should run +// cloneGoRoot returns the new GOROOT we should run // under. -func cloneRoot(cloneBase, relativeGoroot string) (newRoot string, newGoroot string, err error) { - goroot := filepath.Join(cloneBase, relativeGoroot) - - newRoot, err = ioutil.TempDir(cloneBase, "root-*") +func cloneGoRoot(execRoot, relativeGoroot string, cloneBase string) (newGoRoot string, err error) { + goroot := filepath.Join(execRoot, relativeGoroot) if err != nil { - return "", "", err + return "", err } - newGoroot = filepath.Join(newRoot, relativeGoroot) - if err := os.MkdirAll(newGoroot, 01755); err != nil { - return "", "", err + newGoRoot = filepath.Join(cloneBase, relativeGoroot) + if err := os.MkdirAll(newGoRoot, 01755); err != nil { + return "", err } - if err := replicate(goroot, newGoroot, replicatePaths("src", "pkg/tool", "pkg/include")); err != nil { - return "", "", err + if err := replicate(goroot, newGoRoot, replicatePaths("src", "pkg/tool", "pkg/include")); err != nil { + return "", err } - return newRoot, newGoroot, nil + return newGoRoot, nil } // stdliblist runs `go list -json` on the standard library and saves it to a file. @@ -193,7 +193,15 @@ func stdliblist(args []string) error { return err } - cloneBase, goroot, err := cloneRoot(".", goenv.sdk) + cloneBase, cleanup, err := goenv.workDir() + if err != nil { + return err + } + defer func() { + cleanup() + }() + + cloneGoRoot, err := cloneGoRoot(goenv.wd, goenv.sdk, cloneBase) if err != nil { return err } @@ -204,7 +212,7 @@ func stdliblist(args []string) error { absPaths = append(absPaths, abs(path)) } os.Setenv("PATH", strings.Join(absPaths, string(os.PathListSeparator))) - os.Setenv("GOROOT", goroot) + os.Setenv("GOROOT", cloneGoRoot) // Make sure we have an absolute path to the C compiler. // TODO(#1357): also take absolute paths of includes and other paths in flags. os.Setenv("CC", abs(os.Getenv("CC"))) @@ -231,7 +239,6 @@ func stdliblist(args []string) error { if err := goenv.runCommandToFile(jsonData, listArgs); err != nil { return err } - encoder := json.NewEncoder(jsonFile) decoder := json.NewDecoder(jsonData) for decoder.More() { diff --git a/go/tools/builders/stdliblist_test.go b/go/tools/builders/stdliblist_test.go new file mode 100644 index 0000000000..5d92b1cb50 --- /dev/null +++ b/go/tools/builders/stdliblist_test.go @@ -0,0 +1,51 @@ +package main + +import ( + "bufio" + "encoding/json" + "fmt" + "io/ioutil" + "os" + "strings" + "testing" +) + +func Test_stdliblist(t *testing.T) { + fmt.Println("cur:" + abs(".")) + testDir := t.TempDir() + f, _ := ioutil.TempFile(testDir, "*") + + // test files are at TEST_SRCDIR, but this test is run at + // TEST_SRCDIR/TEST_WORKSPACE/... + // since -sdk is assumed to be a relative path to execRoot + // (go.sdk.root_file.dirname), thus setting wd to + // TEST_SRCDIR so that go_sdk is discoverable + test_args := []string{ + fmt.Sprintf("-out=%s", f.Name()), + fmt.Sprintf("-sdk=%s", "go_sdk"), + fmt.Sprintf("-wd=%s", os.Getenv("TEST_SRCDIR")), + } + err := stdliblist(test_args) + if err != nil { + t.Errorf("calling stdliblist got err: %v", err) + } + scanner := bufio.NewScanner(f) + for scanner.Scan() { + var result flatPackage + jsonLineStr := scanner.Text() + if err := json.Unmarshal([]byte(jsonLineStr), &result); err != nil { + t.Errorf("cannot parse result line %s \n to goListPackage{}: %v\n", err) + } + if !strings.HasPrefix(result.ID, "@io_bazel_rules_go//stdlib") { + t.Errorf("ID should be prefixed with @io_bazel_rules_go//stdlib :%s", jsonLineStr) + } + if !strings.HasPrefix(result.ExportFile, "__BAZEL_OUTPUT_BASE__") { + t.Errorf("export file should be prefixed with __BAZEL_OUTPUT_BASE__ :%s", jsonLineStr) + } + for _, gofile := range result.GoFiles { + if !strings.HasPrefix(gofile, "__BAZEL_OUTPUT_BASE__/go_sdk") { + t.Errorf("All go files should be prefixed with __BAZEL_OUTPUT_BASE__/go_sdk :%s", jsonLineStr) + } + } + } +} From bcb289fffa56b6acdb45d69d149c5572da2f1c88 Mon Sep 17 00:00:00 2001 From: Xiaoyang Tan Date: Thu, 12 May 2022 19:05:39 -0700 Subject: [PATCH 06/18] rebase recover tests/core/stdlib/stdlib_files.bzl --- go/private/actions/stdlib.bzl | 1 - go/tools/builders/stdliblist.go | 4 +- go/tools/builders/stdliblist_test.go | 85 ++++++++++++++-------------- 3 files changed, 44 insertions(+), 46 deletions(-) diff --git a/go/private/actions/stdlib.bzl b/go/private/actions/stdlib.bzl index 187249cd0f..6725c4134d 100644 --- a/go/private/actions/stdlib.bzl +++ b/go/private/actions/stdlib.bzl @@ -52,7 +52,6 @@ def _should_use_sdk_stdlib(go): go.mode.link == LINKMODE_NORMAL) def _build_stdlib_list_json(go): - print("this is the go_sdk_path: %s\n" % go.sdk.root_file.dirname) out = go.declare_file(go, "stdlib.pkg.json") args = go.builder_args(go, "stdliblist") args.add("-sdk", go.sdk.root_file.dirname) diff --git a/go/tools/builders/stdliblist.go b/go/tools/builders/stdliblist.go index 3df60d480e..e7cfa3dca8 100644 --- a/go/tools/builders/stdliblist.go +++ b/go/tools/builders/stdliblist.go @@ -158,12 +158,12 @@ func flatPackageForStd(cloneBase string, pkg *goListPackage) *flatPackage { // To work around this, cloneGoRoot creates a copy of external/go_sdk into a new // cloneBase directory while retaining its path relative to the root directory. // So "$OUTPUT_BASE/external/go_sdk" becomes -// tmp_directory/external/go_sdk", which will be set at GOROOT later. +// {cloneBase}/external/go_sdk", which will be set at GOROOT later. // This ensures that file paths in the generated JSON are still valid. // // cloneGoRoot returns the new GOROOT we should run // under. -func cloneGoRoot(execRoot, relativeGoroot string, cloneBase string) (newGoRoot string, err error) { +func cloneGoRoot(execRoot, relativeGoroot, cloneBase string) (newGoRoot string, err error) { goroot := filepath.Join(execRoot, relativeGoroot) if err != nil { return "", err diff --git a/go/tools/builders/stdliblist_test.go b/go/tools/builders/stdliblist_test.go index 5d92b1cb50..0a2955bd04 100644 --- a/go/tools/builders/stdliblist_test.go +++ b/go/tools/builders/stdliblist_test.go @@ -1,51 +1,50 @@ package main import ( - "bufio" - "encoding/json" - "fmt" - "io/ioutil" - "os" - "strings" - "testing" + "bufio" + "encoding/json" + "fmt" + "io/ioutil" + "os" + "strings" + "testing" ) func Test_stdliblist(t *testing.T) { - fmt.Println("cur:" + abs(".")) - testDir := t.TempDir() - f, _ := ioutil.TempFile(testDir, "*") + testDir := t.TempDir() + f, _ := ioutil.TempFile(testDir, "*") - // test files are at TEST_SRCDIR, but this test is run at - // TEST_SRCDIR/TEST_WORKSPACE/... - // since -sdk is assumed to be a relative path to execRoot - // (go.sdk.root_file.dirname), thus setting wd to - // TEST_SRCDIR so that go_sdk is discoverable - test_args := []string{ - fmt.Sprintf("-out=%s", f.Name()), - fmt.Sprintf("-sdk=%s", "go_sdk"), - fmt.Sprintf("-wd=%s", os.Getenv("TEST_SRCDIR")), - } - err := stdliblist(test_args) - if err != nil { - t.Errorf("calling stdliblist got err: %v", err) - } - scanner := bufio.NewScanner(f) - for scanner.Scan() { - var result flatPackage - jsonLineStr := scanner.Text() - if err := json.Unmarshal([]byte(jsonLineStr), &result); err != nil { - t.Errorf("cannot parse result line %s \n to goListPackage{}: %v\n", err) - } - if !strings.HasPrefix(result.ID, "@io_bazel_rules_go//stdlib") { - t.Errorf("ID should be prefixed with @io_bazel_rules_go//stdlib :%s", jsonLineStr) - } - if !strings.HasPrefix(result.ExportFile, "__BAZEL_OUTPUT_BASE__") { - t.Errorf("export file should be prefixed with __BAZEL_OUTPUT_BASE__ :%s", jsonLineStr) - } - for _, gofile := range result.GoFiles { - if !strings.HasPrefix(gofile, "__BAZEL_OUTPUT_BASE__/go_sdk") { - t.Errorf("All go files should be prefixed with __BAZEL_OUTPUT_BASE__/go_sdk :%s", jsonLineStr) - } - } - } + // test files are at TEST_SRCDIR, but this test is run at + // TEST_SRCDIR/TEST_WORKSPACE/... + // since -sdk is assumed to be a relative path to execRoot + // (go.sdk.root_file.dirname), thus setting wd to + // TEST_SRCDIR so that go_sdk is discoverable + test_args := []string{ + fmt.Sprintf("-out=%s", f.Name()), + fmt.Sprintf("-sdk=%s", "go_sdk"), + fmt.Sprintf("-wd=%s", os.Getenv("TEST_SRCDIR")), + } + err := stdliblist(test_args) + if err != nil { + t.Errorf("calling stdliblist got err: %v", err) + } + scanner := bufio.NewScanner(f) + for scanner.Scan() { + var result flatPackage + jsonLineStr := scanner.Text() + if err := json.Unmarshal([]byte(jsonLineStr), &result); err != nil { + t.Errorf("cannot parse result line %s \n to goListPackage{}: %v\n", err) + } + if !strings.HasPrefix(result.ID, "@io_bazel_rules_go//stdlib") { + t.Errorf("ID should be prefixed with @io_bazel_rules_go//stdlib :%s", jsonLineStr) + } + if !strings.HasPrefix(result.ExportFile, "__BAZEL_OUTPUT_BASE__") { + t.Errorf("export file should be prefixed with __BAZEL_OUTPUT_BASE__ :%s", jsonLineStr) + } + for _, gofile := range result.GoFiles { + if !strings.HasPrefix(gofile, "__BAZEL_OUTPUT_BASE__/go_sdk") { + t.Errorf("All go files should be prefixed with __BAZEL_OUTPUT_BASE__/go_sdk :%s", jsonLineStr) + } + } + } } From 89e0cf6d0684207bc814d5608cf922ece7314147 Mon Sep 17 00:00:00 2001 From: Xiaoyang Tan Date: Thu, 12 May 2022 21:41:58 -0700 Subject: [PATCH 07/18] buildifier --- go/tools/builders/BUILD.bazel | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/go/tools/builders/BUILD.bazel b/go/tools/builders/BUILD.bazel index f500b55888..17f3fe8222 100644 --- a/go/tools/builders/BUILD.bazel +++ b/go/tools/builders/BUILD.bazel @@ -26,14 +26,14 @@ go_test( go_test( name = "stdliblist_test", size = "small", - data = ["@go_sdk//:files"], srcs = [ - "stdliblist.go", - "stdliblist_test.go", "env.go", "flags.go", "replicate.go", + "stdliblist.go", + "stdliblist_test.go", ], + data = ["@go_sdk//:files"], ) filegroup( From 68b94d11427f559570c02b22fb99d3dda2c4aed9 Mon Sep 17 00:00:00 2001 From: Xiaoyang Tan Date: Thu, 12 May 2022 22:12:16 -0700 Subject: [PATCH 08/18] clean up --- .bazelci/presubmit.yml | 1 + go/tools/builders/BUILD.bazel | 2 + go/tools/builders/env.go | 2 +- go/tools/builders/stdliblist.go | 10 +-- go/tools/builders/stdliblist_test.go | 92 +++++++++++++++------------- 5 files changed, 57 insertions(+), 50 deletions(-) diff --git a/.bazelci/presubmit.yml b/.bazelci/presubmit.yml index 2ab856edeb..2c62debc75 100644 --- a/.bazelci/presubmit.yml +++ b/.bazelci/presubmit.yml @@ -264,6 +264,7 @@ tasks: - "-@org_golang_x_text//language:language_test" - "-@org_golang_x_tools//cmd/splitdwarf/internal/macho:macho_test" - "-@test_chdir_remote//sub:go_default_test" + - "-//go/tools/builders:stdliblist_test" # fails on Windows due to #2491 - "-//tests:buildifier_test" # requires bash - "-//tests/core/cgo:dylib_client" - "-//tests/core/cgo:dylib_test" diff --git a/go/tools/builders/BUILD.bazel b/go/tools/builders/BUILD.bazel index 17f3fe8222..192c67993c 100644 --- a/go/tools/builders/BUILD.bazel +++ b/go/tools/builders/BUILD.bazel @@ -34,6 +34,8 @@ go_test( "stdliblist_test.go", ], data = ["@go_sdk//:files"], + rundir = ".", + deps = ["//go/tools/bazel"], ) filegroup( diff --git a/go/tools/builders/env.go b/go/tools/builders/env.go index 9c78d50b0e..694754c90b 100644 --- a/go/tools/builders/env.go +++ b/go/tools/builders/env.go @@ -44,7 +44,7 @@ var ( // See ./README.rst for more information about handling arguments and // environment variables. type env struct { - // cwd is the path to the working directory + // wd is the path to the working directory wd string // sdk is the path to the Go SDK, which contains tools for the host diff --git a/go/tools/builders/stdliblist.go b/go/tools/builders/stdliblist.go index e7cfa3dca8..dbc52ac1be 100644 --- a/go/tools/builders/stdliblist.go +++ b/go/tools/builders/stdliblist.go @@ -18,6 +18,7 @@ import ( "bytes" "encoding/json" "flag" + "fmt" "go/build" "os" "path/filepath" @@ -165,9 +166,6 @@ func flatPackageForStd(cloneBase string, pkg *goListPackage) *flatPackage { // under. func cloneGoRoot(execRoot, relativeGoroot, cloneBase string) (newGoRoot string, err error) { goroot := filepath.Join(execRoot, relativeGoroot) - if err != nil { - return "", err - } newGoRoot = filepath.Join(cloneBase, relativeGoroot) if err := os.MkdirAll(newGoRoot, 01755); err != nil { return "", err @@ -197,13 +195,11 @@ func stdliblist(args []string) error { if err != nil { return err } - defer func() { - cleanup() - }() + defer func() { cleanup() }() cloneGoRoot, err := cloneGoRoot(goenv.wd, goenv.sdk, cloneBase) if err != nil { - return err + return fmt.Errorf("failed to clone new go root %v", err) } // Ensure paths are absolute. diff --git a/go/tools/builders/stdliblist_test.go b/go/tools/builders/stdliblist_test.go index 0a2955bd04..189a0762b3 100644 --- a/go/tools/builders/stdliblist_test.go +++ b/go/tools/builders/stdliblist_test.go @@ -1,50 +1,58 @@ package main import ( - "bufio" - "encoding/json" - "fmt" - "io/ioutil" - "os" - "strings" - "testing" + "bufio" + "encoding/json" + "fmt" + "io/ioutil" + "path/filepath" + "strings" + "testing" + + "github.com/bazelbuild/rules_go/go/tools/bazel" ) func Test_stdliblist(t *testing.T) { - testDir := t.TempDir() - f, _ := ioutil.TempFile(testDir, "*") + testDir := t.TempDir() + f, _ := ioutil.TempFile(testDir, "*") + + // test files are at run file directory, but this test is run at + // {runfile directory}/bazel.TestWorkspace() + // since -sdk is assumed to be a relative path to execRoot + // (go.sdk.root_file.dirname), thus setting wd to + // {runfile directory} so that go_sdk is discoverable + // {runfile directory} is the parent directory of bazel.RunfilesPath() + runFilesPath, err := bazel.RunfilesPath() + if err != nil { + t.Error("failed to find runfiles path") + } + test_args := []string{ + fmt.Sprintf("-out=%s", f.Name()), + fmt.Sprintf("-sdk=%s", "go_sdk"), + fmt.Sprintf("-wd=%s", filepath.Dir(filepath.Clean(runFilesPath))), + } - // test files are at TEST_SRCDIR, but this test is run at - // TEST_SRCDIR/TEST_WORKSPACE/... - // since -sdk is assumed to be a relative path to execRoot - // (go.sdk.root_file.dirname), thus setting wd to - // TEST_SRCDIR so that go_sdk is discoverable - test_args := []string{ - fmt.Sprintf("-out=%s", f.Name()), - fmt.Sprintf("-sdk=%s", "go_sdk"), - fmt.Sprintf("-wd=%s", os.Getenv("TEST_SRCDIR")), - } - err := stdliblist(test_args) - if err != nil { - t.Errorf("calling stdliblist got err: %v", err) - } - scanner := bufio.NewScanner(f) - for scanner.Scan() { - var result flatPackage - jsonLineStr := scanner.Text() - if err := json.Unmarshal([]byte(jsonLineStr), &result); err != nil { - t.Errorf("cannot parse result line %s \n to goListPackage{}: %v\n", err) - } - if !strings.HasPrefix(result.ID, "@io_bazel_rules_go//stdlib") { - t.Errorf("ID should be prefixed with @io_bazel_rules_go//stdlib :%s", jsonLineStr) - } - if !strings.HasPrefix(result.ExportFile, "__BAZEL_OUTPUT_BASE__") { - t.Errorf("export file should be prefixed with __BAZEL_OUTPUT_BASE__ :%s", jsonLineStr) - } - for _, gofile := range result.GoFiles { - if !strings.HasPrefix(gofile, "__BAZEL_OUTPUT_BASE__/go_sdk") { - t.Errorf("All go files should be prefixed with __BAZEL_OUTPUT_BASE__/go_sdk :%s", jsonLineStr) - } - } - } + err = stdliblist(test_args) + if err != nil { + t.Errorf("calling stdliblist got err: %v", err) + } + scanner := bufio.NewScanner(f) + for scanner.Scan() { + var result flatPackage + jsonLineStr := scanner.Text() + if err := json.Unmarshal([]byte(jsonLineStr), &result); err != nil { + t.Errorf("cannot parse result line %s \n to goListPackage{}: %v\n", err) + } + if !strings.HasPrefix(result.ID, "@io_bazel_rules_go//stdlib") { + t.Errorf("ID should be prefixed with @io_bazel_rules_go//stdlib :%s", jsonLineStr) + } + if !strings.HasPrefix(result.ExportFile, "__BAZEL_OUTPUT_BASE__") { + t.Errorf("export file should be prefixed with __BAZEL_OUTPUT_BASE__ :%s", jsonLineStr) + } + for _, gofile := range result.GoFiles { + if !strings.HasPrefix(gofile, "__BAZEL_OUTPUT_BASE__/go_sdk") { + t.Errorf("All go files should be prefixed with __BAZEL_OUTPUT_BASE__/go_sdk :%s", jsonLineStr) + } + } + } } From 8b0dde29fb0dd2a7a7b35d6e06a43093f8716bc2 Mon Sep 17 00:00:00 2001 From: Xiaoyang Tan Date: Mon, 16 May 2022 16:20:57 -0700 Subject: [PATCH 09/18] address comments --- go/tools/builders/stdliblist.go | 13 +++++++------ go/tools/builders/stdliblist_test.go | 11 ++++++++--- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/go/tools/builders/stdliblist.go b/go/tools/builders/stdliblist.go index dbc52ac1be..571a2a4b81 100644 --- a/go/tools/builders/stdliblist.go +++ b/go/tools/builders/stdliblist.go @@ -156,14 +156,15 @@ func flatPackageForStd(cloneBase string, pkg *goListPackage) *flatPackage { // If we run "go list" with that GOROOT, this action will fail because those // go:embed directives will refuse to include the symlinks in the sandbox. // -// To work around this, cloneGoRoot creates a copy of external/go_sdk into a new -// cloneBase directory while retaining its path relative to the root directory. +// To work around this, cloneGoRoot creates a copy of a subset of external/go_sdk +// that is sufficient to call "go list" into a new cloneBase directory while +// retaining its path relative to the root directory, e.g. "go list" needs to +// call "compile", which needs "pkg/tool". // So "$OUTPUT_BASE/external/go_sdk" becomes -// {cloneBase}/external/go_sdk", which will be set at GOROOT later. -// This ensures that file paths in the generated JSON are still valid. +// {cloneBase}/external/go_sdk", which will be set at GOROOT later. This ensures +// that file paths in the generated JSON are still valid. // -// cloneGoRoot returns the new GOROOT we should run -// under. +// cloneGoRoot returns the new GOROOT we should run under. func cloneGoRoot(execRoot, relativeGoroot, cloneBase string) (newGoRoot string, err error) { goroot := filepath.Join(execRoot, relativeGoroot) newGoRoot = filepath.Join(cloneBase, relativeGoroot) diff --git a/go/tools/builders/stdliblist_test.go b/go/tools/builders/stdliblist_test.go index 189a0762b3..bcd713be27 100644 --- a/go/tools/builders/stdliblist_test.go +++ b/go/tools/builders/stdliblist_test.go @@ -4,7 +4,7 @@ import ( "bufio" "encoding/json" "fmt" - "io/ioutil" + "os" "path/filepath" "strings" "testing" @@ -14,7 +14,7 @@ import ( func Test_stdliblist(t *testing.T) { testDir := t.TempDir() - f, _ := ioutil.TempFile(testDir, "*") + outJSON := filepath.Join(testDir, "out.json") // test files are at run file directory, but this test is run at // {runfile directory}/bazel.TestWorkspace() @@ -27,7 +27,7 @@ func Test_stdliblist(t *testing.T) { t.Error("failed to find runfiles path") } test_args := []string{ - fmt.Sprintf("-out=%s", f.Name()), + fmt.Sprintf("-out=%s", outJSON), fmt.Sprintf("-sdk=%s", "go_sdk"), fmt.Sprintf("-wd=%s", filepath.Dir(filepath.Clean(runFilesPath))), } @@ -36,6 +36,11 @@ func Test_stdliblist(t *testing.T) { if err != nil { t.Errorf("calling stdliblist got err: %v", err) } + f, err := os.Open(outJSON) + if err != nil { + t.Errorf("cannot open output json: %v", err) + } + defer func() { _ = f.Close() }() scanner := bufio.NewScanner(f) for scanner.Scan() { var result flatPackage From 0c12fc9375fb55427ea103ec683eaf408ca4e2dd Mon Sep 17 00:00:00 2001 From: Xiaoyang Tan Date: Tue, 17 May 2022 23:23:40 -0700 Subject: [PATCH 10/18] works --- go/tools/builders/BUILD.bazel | 5 ++++- go/tools/builders/env.go | 9 +-------- go/tools/builders/stdliblist.go | 19 +++++++++---------- go/tools/builders/stdliblist_test.go | 19 +++++-------------- 4 files changed, 19 insertions(+), 33 deletions(-) diff --git a/go/tools/builders/BUILD.bazel b/go/tools/builders/BUILD.bazel index 192c67993c..762983af13 100644 --- a/go/tools/builders/BUILD.bazel +++ b/go/tools/builders/BUILD.bazel @@ -34,7 +34,10 @@ go_test( "stdliblist_test.go", ], data = ["@go_sdk//:files"], - rundir = ".", + # -sdk in stdliblist needs to be a relative path, otherwise it breaks + # assumptions of cloning go_sdk, thus we need to set up the test in a + # way that go_sdk is under the directory where test is run. + rundir = "..", deps = ["//go/tools/bazel"], ) diff --git a/go/tools/builders/env.go b/go/tools/builders/env.go index 694754c90b..3ed12e9745 100644 --- a/go/tools/builders/env.go +++ b/go/tools/builders/env.go @@ -44,9 +44,6 @@ var ( // See ./README.rst for more information about handling arguments and // environment variables. type env struct { - // wd is the path to the working directory - wd string - // sdk is the path to the Go SDK, which contains tools for the host // platform. This may be different than GOROOT. sdk string @@ -69,12 +66,11 @@ type env struct { // configured with those flags. func envFlags(flags *flag.FlagSet) *env { env := &env{} - flags.StringVar(&env.sdk, "sdk", "", "Path to the Go SDK.") + flags.StringVar(&env.sdk, "sdk", "", "Relative path to the Go SDK.") flags.Var(&tagFlag{}, "tags", "List of build tags considered true.") flags.StringVar(&env.installSuffix, "installsuffix", "", "Standard library under GOROOT/pkg") flags.BoolVar(&env.verbose, "v", false, "Whether subprocess command lines should be printed") flags.BoolVar(&env.shouldPreserveWorkDir, "work", false, "if true, the temporary work directory will be preserved") - flags.StringVar(&env.wd, "wd", ".", "working director default to dot") return env } @@ -125,9 +121,6 @@ func (e *env) goTool(tool string, args ...string) []string { // and additional arguments. func (e *env) goCmd(cmd string, args ...string) []string { exe := filepath.Join(e.sdk, "bin", "go") - if len(e.wd) > 0 { - exe = filepath.Join(e.wd, exe) - } if runtime.GOOS == "windows" { exe += ".exe" diff --git a/go/tools/builders/stdliblist.go b/go/tools/builders/stdliblist.go index 571a2a4b81..46f86d1103 100644 --- a/go/tools/builders/stdliblist.go +++ b/go/tools/builders/stdliblist.go @@ -157,22 +157,22 @@ func flatPackageForStd(cloneBase string, pkg *goListPackage) *flatPackage { // go:embed directives will refuse to include the symlinks in the sandbox. // // To work around this, cloneGoRoot creates a copy of a subset of external/go_sdk -// that is sufficient to call "go list" into a new cloneBase directory while -// retaining its path relative to the root directory, e.g. "go list" needs to -// call "compile", which needs "pkg/tool". -// So "$OUTPUT_BASE/external/go_sdk" becomes +// that is sufficient to call "go list" into a new cloneBase directory, e.g. +// "go list" needs to call "compile", which needs "pkg/tool". +// We also need to retain the same relative path to the root directory, e.g. +// "$OUTPUT_BASE/external/go_sdk" becomes // {cloneBase}/external/go_sdk", which will be set at GOROOT later. This ensures // that file paths in the generated JSON are still valid. // // cloneGoRoot returns the new GOROOT we should run under. -func cloneGoRoot(execRoot, relativeGoroot, cloneBase string) (newGoRoot string, err error) { - goroot := filepath.Join(execRoot, relativeGoroot) - newGoRoot = filepath.Join(cloneBase, relativeGoroot) +func cloneGoRoot(relativeGoRoot, cloneBase string) (newGoRoot string, err error) { + goRoot := abs(relativeGoRoot) + newGoRoot = filepath.Join(cloneBase, relativeGoRoot) if err := os.MkdirAll(newGoRoot, 01755); err != nil { return "", err } - if err := replicate(goroot, newGoRoot, replicatePaths("src", "pkg/tool", "pkg/include")); err != nil { + if err := replicate(goRoot, newGoRoot, replicatePaths("src", "pkg/tool", "pkg/include")); err != nil { return "", err } @@ -198,11 +198,10 @@ func stdliblist(args []string) error { } defer func() { cleanup() }() - cloneGoRoot, err := cloneGoRoot(goenv.wd, goenv.sdk, cloneBase) + cloneGoRoot, err := cloneGoRoot(goenv.sdk, cloneBase) if err != nil { return fmt.Errorf("failed to clone new go root %v", err) } - // Ensure paths are absolute. absPaths := []string{} for _, path := range filepath.SplitList(os.Getenv("PATH")) { diff --git a/go/tools/builders/stdliblist_test.go b/go/tools/builders/stdliblist_test.go index bcd713be27..0cc5b7e488 100644 --- a/go/tools/builders/stdliblist_test.go +++ b/go/tools/builders/stdliblist_test.go @@ -8,31 +8,22 @@ import ( "path/filepath" "strings" "testing" - - "github.com/bazelbuild/rules_go/go/tools/bazel" ) func Test_stdliblist(t *testing.T) { testDir := t.TempDir() outJSON := filepath.Join(testDir, "out.json") - // test files are at run file directory, but this test is run at - // {runfile directory}/bazel.TestWorkspace() - // since -sdk is assumed to be a relative path to execRoot - // (go.sdk.root_file.dirname), thus setting wd to - // {runfile directory} so that go_sdk is discoverable - // {runfile directory} is the parent directory of bazel.RunfilesPath() - runFilesPath, err := bazel.RunfilesPath() - if err != nil { - t.Error("failed to find runfiles path") - } + // test files are at {runfile directory}/go_sdk, + // this test is run at {runfile directory}/{workspace}/../ + // thus go_sdk is the relative path to current working + // directory test_args := []string{ fmt.Sprintf("-out=%s", outJSON), fmt.Sprintf("-sdk=%s", "go_sdk"), - fmt.Sprintf("-wd=%s", filepath.Dir(filepath.Clean(runFilesPath))), } - err = stdliblist(test_args) + err := stdliblist(test_args) if err != nil { t.Errorf("calling stdliblist got err: %v", err) } From d0e87d67409ee350a562bcdfa85a7987ff58aa8f Mon Sep 17 00:00:00 2001 From: Xiaoyang Tan Date: Wed, 18 May 2022 00:18:54 -0700 Subject: [PATCH 11/18] simpler version --- go/tools/builders/BUILD.bazel | 4 ---- go/tools/builders/stdliblist.go | 17 ++++++++--------- go/tools/builders/stdliblist_test.go | 13 +++++++------ 3 files changed, 15 insertions(+), 19 deletions(-) diff --git a/go/tools/builders/BUILD.bazel b/go/tools/builders/BUILD.bazel index 762983af13..23a61dfb93 100644 --- a/go/tools/builders/BUILD.bazel +++ b/go/tools/builders/BUILD.bazel @@ -34,10 +34,6 @@ go_test( "stdliblist_test.go", ], data = ["@go_sdk//:files"], - # -sdk in stdliblist needs to be a relative path, otherwise it breaks - # assumptions of cloning go_sdk, thus we need to set up the test in a - # way that go_sdk is under the directory where test is run. - rundir = "..", deps = ["//go/tools/bazel"], ) diff --git a/go/tools/builders/stdliblist.go b/go/tools/builders/stdliblist.go index 46f86d1103..7d4357e0fc 100644 --- a/go/tools/builders/stdliblist.go +++ b/go/tools/builders/stdliblist.go @@ -164,19 +164,17 @@ func flatPackageForStd(cloneBase string, pkg *goListPackage) *flatPackage { // {cloneBase}/external/go_sdk", which will be set at GOROOT later. This ensures // that file paths in the generated JSON are still valid. // -// cloneGoRoot returns the new GOROOT we should run under. -func cloneGoRoot(relativeGoRoot, cloneBase string) (newGoRoot string, err error) { - goRoot := abs(relativeGoRoot) - newGoRoot = filepath.Join(cloneBase, relativeGoRoot) +// cloneGoRoot replicate goRoot to newGoRoot and returns an error if any. +func cloneGoRoot(goRoot, newGoRoot string) error { if err := os.MkdirAll(newGoRoot, 01755); err != nil { - return "", err + return err } if err := replicate(goRoot, newGoRoot, replicatePaths("src", "pkg/tool", "pkg/include")); err != nil { - return "", err + return err } - return newGoRoot, nil + return nil } // stdliblist runs `go list -json` on the standard library and saves it to a file. @@ -198,7 +196,8 @@ func stdliblist(args []string) error { } defer func() { cleanup() }() - cloneGoRoot, err := cloneGoRoot(goenv.sdk, cloneBase) + newGoRoot := filepath.Join(cloneBase, "go_sdk") + err = cloneGoRoot(abs(goenv.sdk), abs(newGoRoot)) if err != nil { return fmt.Errorf("failed to clone new go root %v", err) } @@ -208,7 +207,7 @@ func stdliblist(args []string) error { absPaths = append(absPaths, abs(path)) } os.Setenv("PATH", strings.Join(absPaths, string(os.PathListSeparator))) - os.Setenv("GOROOT", cloneGoRoot) + os.Setenv("GOROOT", newGoRoot) // Make sure we have an absolute path to the C compiler. // TODO(#1357): also take absolute paths of includes and other paths in flags. os.Setenv("CC", abs(os.Getenv("CC"))) diff --git a/go/tools/builders/stdliblist_test.go b/go/tools/builders/stdliblist_test.go index 0cc5b7e488..af96cee6ff 100644 --- a/go/tools/builders/stdliblist_test.go +++ b/go/tools/builders/stdliblist_test.go @@ -4,6 +4,7 @@ import ( "bufio" "encoding/json" "fmt" + "github.com/bazelbuild/rules_go/go/tools/bazel" "os" "path/filepath" "strings" @@ -14,16 +15,16 @@ func Test_stdliblist(t *testing.T) { testDir := t.TempDir() outJSON := filepath.Join(testDir, "out.json") - // test files are at {runfile directory}/go_sdk, - // this test is run at {runfile directory}/{workspace}/../ - // thus go_sdk is the relative path to current working - // directory + runFilePath, err := bazel.RunfilesPath() + if err != nil { + t.Errorf("cannot file runfile path %v", err) + } test_args := []string{ fmt.Sprintf("-out=%s", outJSON), - fmt.Sprintf("-sdk=%s", "go_sdk"), + fmt.Sprintf("-sdk=%s", abs(filepath.Join(filepath.Dir(runFilePath), "go_sdk"))), } - err := stdliblist(test_args) + err = stdliblist(test_args) if err != nil { t.Errorf("calling stdliblist got err: %v", err) } From 309f5de175504400c552514f6aaaabd0a7c46e9d Mon Sep 17 00:00:00 2001 From: Xiaoyang Tan Date: Wed, 18 May 2022 00:42:24 -0700 Subject: [PATCH 12/18] assume absolute path --- go/tools/builders/stdliblist.go | 2 +- go/tools/builders/stdliblist_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/go/tools/builders/stdliblist.go b/go/tools/builders/stdliblist.go index 7d4357e0fc..63e1bfe82f 100644 --- a/go/tools/builders/stdliblist.go +++ b/go/tools/builders/stdliblist.go @@ -196,7 +196,7 @@ func stdliblist(args []string) error { } defer func() { cleanup() }() - newGoRoot := filepath.Join(cloneBase, "go_sdk") + newGoRoot := filepath.Join(cloneBase, "external/go_sdk") err = cloneGoRoot(abs(goenv.sdk), abs(newGoRoot)) if err != nil { return fmt.Errorf("failed to clone new go root %v", err) diff --git a/go/tools/builders/stdliblist_test.go b/go/tools/builders/stdliblist_test.go index af96cee6ff..2549fbf565 100644 --- a/go/tools/builders/stdliblist_test.go +++ b/go/tools/builders/stdliblist_test.go @@ -47,7 +47,7 @@ func Test_stdliblist(t *testing.T) { t.Errorf("export file should be prefixed with __BAZEL_OUTPUT_BASE__ :%s", jsonLineStr) } for _, gofile := range result.GoFiles { - if !strings.HasPrefix(gofile, "__BAZEL_OUTPUT_BASE__/go_sdk") { + if !strings.HasPrefix(gofile, "__BAZEL_OUTPUT_BASE__/external/go_sdk") { t.Errorf("All go files should be prefixed with __BAZEL_OUTPUT_BASE__/go_sdk :%s", jsonLineStr) } } From 7913ea3ee90d4094bcd2bdd2369be66c3d68a195 Mon Sep 17 00:00:00 2001 From: Xiaoyang Tan Date: Wed, 18 May 2022 01:17:02 -0700 Subject: [PATCH 13/18] test --- go/tools/builders/stdliblist.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/go/tools/builders/stdliblist.go b/go/tools/builders/stdliblist.go index 63e1bfe82f..074bb493e5 100644 --- a/go/tools/builders/stdliblist.go +++ b/go/tools/builders/stdliblist.go @@ -20,6 +20,7 @@ import ( "flag" "fmt" "go/build" + "io/ioutil" "os" "path/filepath" "strings" @@ -190,11 +191,10 @@ func stdliblist(args []string) error { return err } - cloneBase, cleanup, err := goenv.workDir() + cloneBase, err := ioutil.TempDir(goenv.workDirPath, "stdlist-*") if err != nil { return err } - defer func() { cleanup() }() newGoRoot := filepath.Join(cloneBase, "external/go_sdk") err = cloneGoRoot(abs(goenv.sdk), abs(newGoRoot)) From ca25b77a096863f148faa3dfb7ef428cc9f238a5 Mon Sep 17 00:00:00 2001 From: Xiaoyang Tan Date: Wed, 18 May 2022 10:48:30 -0700 Subject: [PATCH 14/18] use external/go_sdk --- go/tools/builders/BUILD.bazel | 2 +- go/tools/builders/stdliblist.go | 7 ++++++- go/tools/builders/stdliblist_test.go | 10 ++-------- 3 files changed, 9 insertions(+), 10 deletions(-) diff --git a/go/tools/builders/BUILD.bazel b/go/tools/builders/BUILD.bazel index 23a61dfb93..4053f5b25d 100644 --- a/go/tools/builders/BUILD.bazel +++ b/go/tools/builders/BUILD.bazel @@ -33,8 +33,8 @@ go_test( "stdliblist.go", "stdliblist_test.go", ], + rundir = ".", data = ["@go_sdk//:files"], - deps = ["//go/tools/bazel"], ) filegroup( diff --git a/go/tools/builders/stdliblist.go b/go/tools/builders/stdliblist.go index 074bb493e5..d045d51bb6 100644 --- a/go/tools/builders/stdliblist.go +++ b/go/tools/builders/stdliblist.go @@ -196,7 +196,12 @@ func stdliblist(args []string) error { return err } - newGoRoot := filepath.Join(cloneBase, "external/go_sdk") + relGoSdk, err := filepath.Rel(".", goenv.sdk) + if err != nil { + return err + } + + newGoRoot := filepath.Join(cloneBase, relGoSdk) err = cloneGoRoot(abs(goenv.sdk), abs(newGoRoot)) if err != nil { return fmt.Errorf("failed to clone new go root %v", err) diff --git a/go/tools/builders/stdliblist_test.go b/go/tools/builders/stdliblist_test.go index 2549fbf565..6eec628361 100644 --- a/go/tools/builders/stdliblist_test.go +++ b/go/tools/builders/stdliblist_test.go @@ -4,7 +4,6 @@ import ( "bufio" "encoding/json" "fmt" - "github.com/bazelbuild/rules_go/go/tools/bazel" "os" "path/filepath" "strings" @@ -15,17 +14,12 @@ func Test_stdliblist(t *testing.T) { testDir := t.TempDir() outJSON := filepath.Join(testDir, "out.json") - runFilePath, err := bazel.RunfilesPath() - if err != nil { - t.Errorf("cannot file runfile path %v", err) - } test_args := []string{ fmt.Sprintf("-out=%s", outJSON), - fmt.Sprintf("-sdk=%s", abs(filepath.Join(filepath.Dir(runFilePath), "go_sdk"))), + fmt.Sprintf("-sdk=%s", "external/go_sdk"), } - err = stdliblist(test_args) - if err != nil { + if err := stdliblist(test_args); err != nil { t.Errorf("calling stdliblist got err: %v", err) } f, err := os.Open(outJSON) From 29449ccf22ce9d64e6eb30d6f1706dfc8a9d81f8 Mon Sep 17 00:00:00 2001 From: Xiaoyang Tan Date: Wed, 18 May 2022 10:55:11 -0700 Subject: [PATCH 15/18] buildifier --- go/tools/builders/BUILD.bazel | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/tools/builders/BUILD.bazel b/go/tools/builders/BUILD.bazel index 4053f5b25d..ec45446d1a 100644 --- a/go/tools/builders/BUILD.bazel +++ b/go/tools/builders/BUILD.bazel @@ -33,8 +33,8 @@ go_test( "stdliblist.go", "stdliblist_test.go", ], - rundir = ".", data = ["@go_sdk//:files"], + rundir = ".", ) filegroup( From 9a858298f89096b7fc90996bdd44a7d813d4438c Mon Sep 17 00:00:00 2001 From: Xiaoyang Tan Date: Wed, 18 May 2022 11:10:32 -0700 Subject: [PATCH 16/18] use workDir --- go/tools/builders/stdliblist.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/go/tools/builders/stdliblist.go b/go/tools/builders/stdliblist.go index d045d51bb6..6367970e9f 100644 --- a/go/tools/builders/stdliblist.go +++ b/go/tools/builders/stdliblist.go @@ -20,7 +20,6 @@ import ( "flag" "fmt" "go/build" - "io/ioutil" "os" "path/filepath" "strings" @@ -191,10 +190,11 @@ func stdliblist(args []string) error { return err } - cloneBase, err := ioutil.TempDir(goenv.workDirPath, "stdlist-*") + cloneBase, cleanup, err := goenv.workDir() if err != nil { return err } + defer func() { cleanup() }() relGoSdk, err := filepath.Rel(".", goenv.sdk) if err != nil { From a9f613ab5a683ce1c44a97ab55163b201108a45b Mon Sep 17 00:00:00 2001 From: Xiaoyang Tan Date: Wed, 18 May 2022 16:27:35 -0700 Subject: [PATCH 17/18] address comments --- go/tools/builders/env.go | 3 +- go/tools/builders/stdliblist.go | 57 +++++++++++----------------- go/tools/builders/stdliblist_test.go | 21 +++++----- 3 files changed, 33 insertions(+), 48 deletions(-) diff --git a/go/tools/builders/env.go b/go/tools/builders/env.go index 3ed12e9745..d05992579e 100644 --- a/go/tools/builders/env.go +++ b/go/tools/builders/env.go @@ -66,7 +66,7 @@ type env struct { // configured with those flags. func envFlags(flags *flag.FlagSet) *env { env := &env{} - flags.StringVar(&env.sdk, "sdk", "", "Relative path to the Go SDK.") + flags.StringVar(&env.sdk, "sdk", "", "Path to the Go SDK.") flags.Var(&tagFlag{}, "tags", "List of build tags considered true.") flags.StringVar(&env.installSuffix, "installsuffix", "", "Standard library under GOROOT/pkg") flags.BoolVar(&env.verbose, "v", false, "Whether subprocess command lines should be printed") @@ -121,7 +121,6 @@ func (e *env) goTool(tool string, args ...string) []string { // and additional arguments. func (e *env) goCmd(cmd string, args ...string) []string { exe := filepath.Join(e.sdk, "bin", "go") - if runtime.GOOS == "windows" { exe += ".exe" } diff --git a/go/tools/builders/stdliblist.go b/go/tools/builders/stdliblist.go index 6367970e9f..dd3ecb049c 100644 --- a/go/tools/builders/stdliblist.go +++ b/go/tools/builders/stdliblist.go @@ -149,34 +149,6 @@ func flatPackageForStd(cloneBase string, pkg *goListPackage) *flatPackage { return newPkg } -// In Go 1.18, the standard library started using go:embed directives. -// When Bazel runs this action, it does so inside a sandbox where GOROOT points -// to an external/go_sdk directory that contains a symlink farm of all files in -// the Go SDK. -// If we run "go list" with that GOROOT, this action will fail because those -// go:embed directives will refuse to include the symlinks in the sandbox. -// -// To work around this, cloneGoRoot creates a copy of a subset of external/go_sdk -// that is sufficient to call "go list" into a new cloneBase directory, e.g. -// "go list" needs to call "compile", which needs "pkg/tool". -// We also need to retain the same relative path to the root directory, e.g. -// "$OUTPUT_BASE/external/go_sdk" becomes -// {cloneBase}/external/go_sdk", which will be set at GOROOT later. This ensures -// that file paths in the generated JSON are still valid. -// -// cloneGoRoot replicate goRoot to newGoRoot and returns an error if any. -func cloneGoRoot(goRoot, newGoRoot string) error { - if err := os.MkdirAll(newGoRoot, 01755); err != nil { - return err - } - - if err := replicate(goRoot, newGoRoot, replicatePaths("src", "pkg/tool", "pkg/include")); err != nil { - return err - } - - return nil -} - // stdliblist runs `go list -json` on the standard library and saves it to a file. func stdliblist(args []string) error { // process the args @@ -190,22 +162,37 @@ func stdliblist(args []string) error { return err } + if filepath.IsAbs(goenv.sdk) { + return fmt.Errorf("-sdk needs to be a relative path, but got %s", goenv.sdk) + } + + // In Go 1.18, the standard library started using go:embed directives. + // When Bazel runs this action, it does so inside a sandbox where GOROOT points + // to an external/go_sdk directory that contains a symlink farm of all files in + // the Go SDK. + // If we run "go list" with that GOROOT, this action will fail because those + // go:embed directives will refuse to include the symlinks in the sandbox. + // + // To work around this, cloneGoRoot creates a copy of a subset of external/go_sdk + // that is sufficient to call "go list" into a new cloneBase directory, e.g. + // "go list" needs to call "compile", which needs "pkg/tool". + // We also need to retain the same relative path to the root directory, e.g. + // "$OUTPUT_BASE/external/go_sdk" becomes + // {cloneBase}/external/go_sdk", which will be set at GOROOT later. This ensures + // that file paths in the generated JSON are still valid. + // + // Here we replicate goRoot(absolute path of goenv.sdk) to newGoRoot. cloneBase, cleanup, err := goenv.workDir() if err != nil { return err } defer func() { cleanup() }() - relGoSdk, err := filepath.Rel(".", goenv.sdk) - if err != nil { + newGoRoot := filepath.Join(cloneBase, goenv.sdk) + if err := replicate(abs(goenv.sdk), abs(newGoRoot), replicatePaths("src", "pkg/tool", "pkg/include")); err != nil { return err } - newGoRoot := filepath.Join(cloneBase, relGoSdk) - err = cloneGoRoot(abs(goenv.sdk), abs(newGoRoot)) - if err != nil { - return fmt.Errorf("failed to clone new go root %v", err) - } // Ensure paths are absolute. absPaths := []string{} for _, path := range filepath.SplitList(os.Getenv("PATH")) { diff --git a/go/tools/builders/stdliblist_test.go b/go/tools/builders/stdliblist_test.go index 6eec628361..b456b0bef0 100644 --- a/go/tools/builders/stdliblist_test.go +++ b/go/tools/builders/stdliblist_test.go @@ -1,7 +1,6 @@ package main import ( - "bufio" "encoding/json" "fmt" "os" @@ -16,7 +15,7 @@ func Test_stdliblist(t *testing.T) { test_args := []string{ fmt.Sprintf("-out=%s", outJSON), - fmt.Sprintf("-sdk=%s", "external/go_sdk"), + "-sdk=external/go_sdk", } if err := stdliblist(test_args); err != nil { @@ -27,22 +26,22 @@ func Test_stdliblist(t *testing.T) { t.Errorf("cannot open output json: %v", err) } defer func() { _ = f.Close() }() - scanner := bufio.NewScanner(f) - for scanner.Scan() { - var result flatPackage - jsonLineStr := scanner.Text() - if err := json.Unmarshal([]byte(jsonLineStr), &result); err != nil { - t.Errorf("cannot parse result line %s \n to goListPackage{}: %v\n", err) + decoder := json.NewDecoder(f) + for decoder.More() { + var result *flatPackage + if err := decoder.Decode(&result); err != nil { + t.Errorf("unable to decode output json: %v\n", err) } + if !strings.HasPrefix(result.ID, "@io_bazel_rules_go//stdlib") { - t.Errorf("ID should be prefixed with @io_bazel_rules_go//stdlib :%s", jsonLineStr) + t.Errorf("ID should be prefixed with @io_bazel_rules_go//stdlib :%v", result) } if !strings.HasPrefix(result.ExportFile, "__BAZEL_OUTPUT_BASE__") { - t.Errorf("export file should be prefixed with __BAZEL_OUTPUT_BASE__ :%s", jsonLineStr) + t.Errorf("export file should be prefixed with __BAZEL_OUTPUT_BASE__ :%v", result) } for _, gofile := range result.GoFiles { if !strings.HasPrefix(gofile, "__BAZEL_OUTPUT_BASE__/external/go_sdk") { - t.Errorf("All go files should be prefixed with __BAZEL_OUTPUT_BASE__/go_sdk :%s", jsonLineStr) + t.Errorf("all go files should be prefixed with __BAZEL_OUTPUT_BASE__/external/go_sdk :%v", result) } } } From e568d43ed81d57aab31f40044cd93b4ee3e59dcb Mon Sep 17 00:00:00 2001 From: Xiaoyang Tan Date: Wed, 18 May 2022 22:22:35 -0700 Subject: [PATCH 18/18] minor renaming --- go/tools/builders/stdliblist.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/go/tools/builders/stdliblist.go b/go/tools/builders/stdliblist.go index dd3ecb049c..3eab29838d 100644 --- a/go/tools/builders/stdliblist.go +++ b/go/tools/builders/stdliblist.go @@ -110,8 +110,8 @@ func stdlibPackageID(importPath string) string { return "@io_bazel_rules_go//stdlib:" + importPath } -// labelledPath replace the cloneBase with output base label -func labelledPath(cloneBase, p string) string { +// outputBasePath replace the cloneBase with output base label +func outputBasePath(cloneBase, p string) string { dir, _ := filepath.Rel(cloneBase, p) return filepath.Join("__BAZEL_OUTPUT_BASE__", dir) } @@ -120,7 +120,7 @@ func labelledPath(cloneBase, p string) string { // paths with the label for all source files in a package func absoluteSourcesPaths(cloneBase, pkgDir string, srcs []string) []string { ret := make([]string, 0, len(srcs)) - pkgDir = labelledPath(cloneBase, pkgDir) + pkgDir = outputBasePath(cloneBase, pkgDir) for _, src := range srcs { ret = append(ret, filepath.Join(pkgDir, src)) } @@ -135,7 +135,7 @@ func flatPackageForStd(cloneBase string, pkg *goListPackage) *flatPackage { ID: stdlibPackageID(pkg.ImportPath), Name: pkg.Name, PkgPath: pkg.ImportPath, - ExportFile: labelledPath(cloneBase, pkg.Target), + ExportFile: outputBasePath(cloneBase, pkg.Target), Imports: map[string]string{}, Standard: pkg.Standard, GoFiles: goFiles,