Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix gopackagesdriver for Go 1.18 by replicating stdlib #3083

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 43 additions & 3 deletions go/tools/builders/stdliblist.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"bytes"
"encoding/json"
"flag"
"fmt"
"go/build"
"os"
"path/filepath"
Expand Down Expand Up @@ -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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you add args.add("-sdk", go.sdk.root_file.dirname) to here:

args = go.builder_args(go, "stdliblist")
args.add("-out", out)

Then you can get a relative path of goroot from execRoot with goenv.sdk.

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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a bit nervous about writing things to execRoot. It's supposed to contain a symlink tree intended to be read-only. This may have unexpected result if there happen to be a root directory under execRoot. Can you copy it to ioutil.TempDir?

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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need to replicate pkg/tool?

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
Expand All @@ -158,19 +195,22 @@ func stdliblist(args []string) error {
return err
}

execRoot, goroot, err := cloneRoot(".", os.Getenv("GOROOT"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"execRoot" is a special name in Bazel. It means the "The working directory for all actions" (https://bazel.build/docs/output_directories). Because you copied the symlink tree to to a different place, it's no longer the actual execRoot. Please rename it to something like "newBase" or "cloneBase". There are several variables/arguments in this file called "execRoot". After this pull request, they are no longer the actual execroot. Please also renaming all of them.

if 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", 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")))

execRoot := abs(".")

cachePath := abs(*out + ".gocache")
defer os.RemoveAll(cachePath)
os.Setenv("GOCACHE", cachePath)
Expand Down
5 changes: 3 additions & 2 deletions tests/core/stdlib/stdlib_files.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,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,
)]

Expand Down