From d61ec66f02b76c26afea95d061ff34f25998ce6d Mon Sep 17 00:00:00 2001 From: Xiaoyang Tan Date: Wed, 18 May 2022 16:27:35 -0700 Subject: [PATCH] address comments --- go/tools/builders/env.go | 2 +- go/tools/builders/stdliblist.go | 57 +++++++++++----------------- go/tools/builders/stdliblist_test.go | 21 +++++----- 3 files changed, 33 insertions(+), 47 deletions(-) diff --git a/go/tools/builders/env.go b/go/tools/builders/env.go index 3ed12e9745..5e3afd5b06 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") 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..0396b6c3f9 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) } } }