Fix gopackagesdriver for Go 1.18 by replicating stdlib and add test for stdliblist.go #3153

xytan0056 · 2022-05-13T02:00:41Z

What type of PR is this?

This PR addresses comments for #3083

What does this PR do? Why is it needed?
It fixes go list invocations from bazel for go1.18, where std library uses go:embed
see #3083

Which issues(s) does this PR fix?

Fixes #3080

Other notes for review
updated the patch in our repo with this patch

verified that the targets that failed without the patch can build successfully
verified a rebuild of all our repo behaved as expected

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 bazel-contrib#3080

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.

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.

recover tests/core/stdlib/stdlib_files.bzl

xytan0056 · 2022-05-15T04:39:45Z

go/tools/builders/stdliblist.go

+		return "", err
+	}
+
+	if err := replicate(goroot, newGoRoot, replicatePaths("src", "pkg/tool", "pkg/include")); err != nil {


"pkg/tool" is needed because go list would fail without it

Use --sandbox_debug to see verbose messages from the sandbox go: no such tool "compile" go: no such tool "compile" go: no such tool "compile" go: no such tool "compile" go: no such tool "compile" stdliblist: error running subcommand external/go_sdk/bin/go: exit status 2

maybe because the subcommand tool list needs non-empty ToolDir, which is expected to be at pkg/tool

Good finding. Yes, go list needs to call compile. Can you add comments to explain this finding?

linzhp · 2022-05-15T15:00:16Z

go/tools/builders/stdliblist_test.go

+
+func Test_stdliblist(t *testing.T) {
+	testDir := t.TempDir()
+	f, _ := ioutil.TempFile(testDir, "*")


You don't need to create a temp file in a temp dir. outJSON = filepath.Join(testDir, "out.json") should be enough

linzhp · 2022-05-15T15:08:46Z

go/tools/builders/stdliblist_test.go

+	}
+	test_args := []string{
+		fmt.Sprintf("-out=%s", f.Name()),
+		fmt.Sprintf("-sdk=%s", "go_sdk"),


Can you pass the absolute path to go_sdk here? It should be available in $TEST_SRCDIR/external/go_sdk. With the absolute path, you don't need to pass -wd, right?

-sdk passed by go.sdk.root_file.dirname in stdlib.bzl, which is a relative path. See below comment

linzhp · 2022-05-15T15:11:26Z

go/tools/builders/stdliblist_test.go

+	// since -sdk is assumed to be a relative path to execRoot
+	// (go.sdk.root_file.dirname), thus setting wd to


Can make-sdk to take either relative path to current directory (execRoot in bazel action) or an absolute path?

-sdk is passed as relative path in stdlib.bazl, and stdliblist.go has this assumption too.
Also it looks like we can't get absolute path easily #1273

https://github.com/xytan0056/rules_go/blob/8b0dde29fb0dd2a7a7b35d6e06a43093f8716bc2/go/private/actions/stdlib.bzl#L55-L58

I also tried go.root, which is also a relative path

Actually -sdk needs to be a relative path, because during cloning, I'd prefer to retain the same relative path of the original go_sdk, i.e. /old/root/rel_go_sdk/src/package -> /new/root/rel_go_sdk/src/package

It cannot be an absolute path, because if so, the only way to get rel_go_sdk from goroot is filepath.Rel(".", env.go_sdk), but this doesn't work for tests:
consider the following test file structure

/a/b/c/runfiles /a/b/c/runfiles/go_sdk <- data files /a/b/c/runfiles/workspace <- where test is run with rundir being "." in go_test rule

the relative path of go_sdk is ../go_sdk;
Let's make cloneBase := /tmp/123;
Then in order to retain the same relative path ,newGoRoot becomes /tmp/123/../go_sdk, which is /tmp/go_sdk;
Then go list under newGoRoot returns pkg.Dir being /tmp/go_sdk/src/package, which does not share a prefix with cloneBase and thus cannot be replaced with __BAZEL_OUTBASE_LABEL__

func labelledPath(cloneBase, p string) string { // ../go_sdk/src/package dir, _ := filepath.Rel(cloneBase, p) // __BAZEL_OUTPUT_BASE__/../go_sdk/src/package => go_sdk/src/package (wrong) return filepath.Join("__BAZEL_OUTPUT_BASE__", dir) }

The culprit here is .. in the relative path. Instead of maniplulating the path, a simple way is to set rundir=".." in the go_test rule, so that all assumptions naturally fall into places.

Another finding is the relative path has to be external/go_sdk, matching the original, otherwise our internal targets will fail. This is a perfect indicator to use goenv.sdk as relative path.

However if I take "absolute path" approach with hardcoded "external/go_sdk" #3157, it fails reproducibility test.

Why not just cloning from /old/root/external/go_sdk to /new/root/external/go_sdk?

Never mind. xytan0056#1

xytan0056 · 2022-05-18T06:29:47Z

go/tools/builders/BUILD.bazel

+    # -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 = "..",


this is an easy way to avoid having to break assumption of go_sdk to be absolute path.
by setting it this way, the structure will become

/a/b/c/runfiles/ <- this will be where the test is run with rundir being .. go_sdk <- this will be where the data files are workspace <- this will be where the test is run with rundir being .

then naturally go_sdk is the relative path.

xytan0056 · 2022-05-18T18:52:53Z

close in favor of #3157

abhinav and others added 6 commits May 11, 2022 23:47

test/stdlib: Depend on _list_json

6c94802

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.

address comments

41c0139

wip

1de0b73

rebase

bcb289f

recover tests/core/stdlib/stdlib_files.bzl

xytan0056 force-pushed the tanx/stdliblist branch from ece017e to bcb289f Compare May 13, 2022 02:15

buildifier

89e0cf6

xytan0056 force-pushed the tanx/stdliblist branch 5 times, most recently from 4a99e93 to 32b5803 Compare May 13, 2022 23:24

clean up

68b94d1

xytan0056 force-pushed the tanx/stdliblist branch from 32b5803 to 68b94d1 Compare May 13, 2022 23:28

xytan0056 changed the title ~~tanx/stdliblist~~ Fix gopackagesdriver for Go 1.18 by replicating stdlib and add test for stdliblist.go May 13, 2022

linzhp self-assigned this May 14, 2022

xytan0056 commented May 15, 2022

View reviewed changes

linzhp reviewed May 15, 2022

View reviewed changes

address comments

8b0dde2

xytan0056 force-pushed the tanx/stdliblist branch from c62b222 to a94c507 Compare May 18, 2022 06:21

works

0c12fc9

xytan0056 force-pushed the tanx/stdliblist branch from a94c507 to 0c12fc9 Compare May 18, 2022 06:23

xytan0056 commented May 18, 2022

View reviewed changes

xytan0056 mentioned this pull request May 18, 2022

Passing relative path from tests xytan0056/rules_go#1

Closed

xytan0056 closed this May 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix gopackagesdriver for Go 1.18 by replicating stdlib and add test for stdliblist.go #3153

Fix gopackagesdriver for Go 1.18 by replicating stdlib and add test for stdliblist.go #3153

xytan0056 commented May 13, 2022

xytan0056 May 15, 2022 •

edited

Loading

linzhp May 15, 2022

xytan0056 May 16, 2022

linzhp May 15, 2022

xytan0056 May 16, 2022

linzhp May 15, 2022

xytan0056 May 17, 2022

linzhp May 15, 2022

xytan0056 May 16, 2022 •

edited

Loading

xytan0056 May 18, 2022 •

edited

Loading

linzhp May 18, 2022

linzhp May 18, 2022

xytan0056 May 18, 2022

xytan0056 commented May 18, 2022 •

edited

Loading

		// since -sdk is assumed to be a relative path to execRoot
		// (go.sdk.root_file.dirname), thus setting wd to

Fix gopackagesdriver for Go 1.18 by replicating stdlib and add test for stdliblist.go #3153

Fix gopackagesdriver for Go 1.18 by replicating stdlib and add test for stdliblist.go #3153

Conversation

xytan0056 commented May 13, 2022

xytan0056 May 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xytan0056 May 16, 2022 • edited Loading

Choose a reason for hiding this comment

xytan0056 May 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xytan0056 commented May 18, 2022 • edited Loading

xytan0056 May 15, 2022 •

edited

Loading

xytan0056 May 16, 2022 •

edited

Loading

xytan0056 May 18, 2022 •

edited

Loading

xytan0056 commented May 18, 2022 •

edited

Loading