Fix go_tool_binary non-hermeticity and Go 1.19 incompatibility (bazel-contrib#4167)

**What type of PR is this?**

Bug fix

**What does this PR do? Why is it needed?**

`go_tool_binary` used to non-hermetically pick up rules_go's `go.mod`
file when run unsandboxed, which resulted in it downloading a different
toolchain and messing up version checks.

Also make `nogo_main.go` compatible with Go 1.19 and test with nogo
(which requires fixing a bunch of violations).

**Which issues(s) does this PR fix?**

Fixes #

**Other notes for review**

Author: fmeum

WORKSPACE

@@ -1,7 +1,7 @@
 workspace(name = "io_bazel_rules_go")
 
 load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
-load("@io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies")
+load("@io_bazel_rules_go//go:deps.bzl", "go_download_sdk", "go_register_nogo", "go_register_toolchains", "go_rules_dependencies")
 
 # Required by toolchains_protoc.
 http_archive(
@@ -29,6 +29,15 @@ go_rules_dependencies()
 
 go_register_toolchains(version = "1.23.1")
 
+go_download_sdk(
+    name = "rules_go_internal_compatibility_sdk",
+    version = "1.19.13",
+)
+
+go_register_nogo(
+    nogo = "@//internal:nogo",
+)
+
 http_archive(
     name = "rules_proto",
     sha256 = "303e86e722a520f6f326a50b41cfc16b98fe6d1955ce46642a5b7a67c11c0f5d",
@@ -137,7 +146,7 @@ grpc_dependencies()
 
 load("@bazel_gazelle//:deps.bzl", "gazelle_dependencies", "go_repository")
 
-gazelle_dependencies()
+gazelle_dependencies(go_sdk = "go_sdk")
 
 go_repository(
     name = "com_github_google_go_github_v36",

go/private/nogo.bzl

@@ -13,7 +13,7 @@
 # limitations under the License.
 
 DEFAULT_NOGO = "@io_bazel_rules_go//:default_nogo"
-NOGO_DEFAULT_INCLUDES = ["@@//:__subpackages__"]
+NOGO_DEFAULT_INCLUDES = [str(Label("//:__subpackages__"))]
 NOGO_DEFAULT_EXCLUDES = []
 
 # repr(Label(...)) does not emit a canonical label literal.

go/private/rules/binary.bzl

@@ -464,6 +464,17 @@ go_non_executable_binary = rule(executable = False, **_go_binary_kwargs(
 ))
 
 def _go_tool_binary_impl(ctx):
+    # Keep in mind that the actions registered by this rule may not be
+    # sandboxed, so care must be taken to make them hermetic, for example by
+    # preventing `go build` from searching for go.mod or downloading a
+    # different toolchain version.
+    #
+    # A side effect of the usage of GO111MODULE below is that the absolute
+    # path to the sources is included in the buildid, which would make the
+    # resulting binary non-reproducible. We thus need to blank it out.
+    # https://github.com/golang/go/blob/583d750fa119d504686c737be6a898994b674b69/src/cmd/go/internal/load/pkg.go#L1764-L1766
+    # https://github.com/golang/go/blob/583d750fa119d504686c737be6a898994b674b69/src/cmd/go/internal/work/exec.go#L284
+
     sdk = ctx.attr.sdk[GoSDK]
     name = ctx.label.name
     if sdk.goos == "windows":
@@ -478,7 +489,9 @@ def _go_tool_binary_impl(ctx):
 set GOMAXPROCS=1
 set GOCACHE=%cd%\\{gotmp}\\gocache
 set GOPATH=%cd%"\\{gotmp}\\gopath
-{go} build -o {out} -trimpath -ldflags \"{ldflags}\" {srcs}
+set GOTOOLCHAIN=local
+set GO111MODULE=off
+{go} build -o {out} -trimpath -ldflags \"-buildid='' {ldflags}\" {srcs}
 set GO_EXIT_CODE=%ERRORLEVEL%
 RMDIR /S /Q "{gotmp}"
 MKDIR "{gotmp}"
@@ -506,7 +519,16 @@ exit /b %GO_EXIT_CODE%
         )
     else:
         # Note: GOPATH is needed for Go 1.16.
-        cmd = """GOTMP=$(mktemp -d);trap "rm -rf \"$GOTMP\"" EXIT;GOMAXPROCS=1 GOCACHE="$GOTMP"/gocache GOPATH="$GOTMP"/gopath {go} build -o {out} -trimpath -ldflags '{ldflags}' {srcs}""".format(
+        cmd = """
+GOTMP=$(mktemp -d)
+trap "rm -rf \"$GOTMP\"" EXIT
+GOMAXPROCS=1 \
+GOCACHE="$GOTMP"/gocache \
+GOPATH="$GOTMP"/gopath \
+GOTOOLCHAIN=local \
+GO111MODULE=off \
+{go} build -o {out} -trimpath -ldflags '-buildid="" {ldflags}' {srcs}
+""".format(
             go = sdk.go.path,
             out = out.path,
             srcs = " ".join([f.path for f in ctx.files.srcs]),

go/runfiles/manifest.go

@@ -138,7 +138,8 @@ func (m *manifest) open(name string) (fs.File, error) {
 			// name refers to an actual file or dir listed in the manifest. The
 			// basename of name may not match the basename of the underlying
 			// file (e.g. in the case of a root symlink), so patch it.
-			f, err := os.Open(r)
+			var f *os.File
+			f, err = os.Open(r)
 			if err != nil {
 				return nil, err
 			}

go/tools/builders/nogo_main.go

@@ -223,7 +223,7 @@ func checkPackage(analyzers []*analysis.Analyzer, packagePath string, packageFil
 		if _, ok := ignoreFilesSet[pkg.fset.Position(f.Pos()).Filename]; ok {
 			for _, act := range actions {
 				act.nolint = append(act.nolint, &Range{
-					from: pkg.fset.Position(f.FileStart),
+					from: pkg.fset.Position(f.Pos()),
 					to:   pkg.fset.Position(f.End()).Line,
 				})
 			}

go/tools/releaser/prepare.go

@@ -125,7 +125,7 @@ func runPrepare(ctx context.Context, stderr io.Writer, args []string) error {
 	branchName := "release-" + majorMinor[len("v"):]
 	if !gitBranchExists(ctx, rootDir, branchName) {
 		if !isMinorRelease {
-			return fmt.Errorf("release branch %q does not exist locally. Fetch it, set RULES_GO_VERSION, add commits, and run this command again.")
+			return fmt.Errorf("release branch %q does not exist locally. Fetch it, set RULES_GO_VERSION, add commits, and run this command again.", branchName)
 		}
 		if err := checkRulesGoVersion(ctx, rootDir, "HEAD", version); err != nil {
 			return err
@@ -247,7 +247,7 @@ func checkRulesGoVersion(ctx context.Context, dir, refName, version string) erro
 	}
 	rulesGoVersionStr := []byte(fmt.Sprintf(`RULES_GO_VERSION = "%s"`, version[len("v"):]))
 	if !bytes.Contains(data, rulesGoVersionStr) {
-		return fmt.Errorf("RULES_GO_VERSION was not set to %q in go/def.bzl. Set it, add commits, and run this command again.")
+		return fmt.Errorf("RULES_GO_VERSION was not set to %q in go/def.bzl. Set it, add commits, and run this command again.", version)
 	}
 	return nil
 }

go/tools/releaser/upgradedep.go

@@ -461,8 +461,8 @@ func upgradeDepDecl(ctx context.Context, gh *githubClient, workDir, name string,
 			}
 			patchName := patchLabelValue[len("//third_party:"):]
 			patchPath := filepath.Join(rootDir, "third_party", patchName)
-			prevDir := filepath.Join(workDir, name, string('a'+patchIndex))
-			patchDir := filepath.Join(workDir, name, string('a'+patchIndex+1))
+			prevDir := filepath.Join(workDir, name, fmt.Sprintf("a%d", patchIndex))
+			patchDir := filepath.Join(workDir, name, fmt.Sprintf("a%d", patchIndex+1))
 			var patchCmd []string
 			for _, c := range comments.Before {
 				words := strings.Fields(strings.TrimPrefix(c.Token, "#"))
@@ -484,7 +484,7 @@ func upgradeDepDecl(ctx context.Context, gh *githubClient, workDir, name string,
 					return err
 				}
 			}
-			patch, _ := runForOutput(ctx, filepath.Join(workDir, name), "diff", "-urN", string('a'+patchIndex), string('a'+patchIndex+1))
+			patch, _ := runForOutput(ctx, filepath.Join(workDir, name), "diff", "-urN", fmt.Sprintf("a%d", patchIndex), fmt.Sprintf("a%d", patchIndex+1))
 			patch = sanitizePatch(patch)
 			if err := os.WriteFile(patchPath, patch, 0666); err != nil {
 				return err

internal/BUILD.bazel

@@ -0,0 +1,12 @@
+load("//go:def.bzl", "TOOLS_NOGO", "nogo")
+
+nogo(
+    name = "nogo",
+    visibility = ["//visibility:public"],
+    deps = [
+        a
+        for a in TOOLS_NOGO
+        # Filter out shadow as it triggers on err and is very noisy.
+        if a != "@org_golang_x_tools//go/analysis/passes/shadow:go_default_library"
+    ],
+)

tests/core/compatibility/BUILD.bazel

@@ -0,0 +1,12 @@
+load("//go:def.bzl", "go_binary", "go_cross_binary")
+
+go_binary(
+    name = "hello",
+    srcs = ["hello.go"],
+)
+
+go_cross_binary(
+    name = "hello_old",
+    sdk_version = "1.19",
+    target = ":hello",
+)

tests/core/compatibility/README.md

@@ -0,0 +1,5 @@
+# Compatibility
+
+## compatibility_test
+
+Verifies that a simple Go project builds with an older version of Go.

tests/core/compatibility/hello.go

@@ -0,0 +1,7 @@
+package main
+
+import "fmt"
+
+func main() {
+	fmt.Println("hello")
+}

tests/core/go_proto_library_importpath/importpath_test.go

@@ -16,7 +16,7 @@ func Test(t *testing.T) {
 	var expected int64 = 5
 	if bar.Value.Value != expected {
 		t.Errorf(fmt.Sprintf("Not equal: \n"+
-			"expected: %s\n"+
-			"actual  : %s", expected, bar.Value.Value))
+			"expected: %d\n"+
+			"actual  : %d", expected, bar.Value.Value))
 	}
 }

tests/core/go_test/example_test.go

@@ -4,6 +4,14 @@ import "fmt"
 
 var Expected = "Expected"
 
+// Please the "tests" nogo check.
+var (
+	HelloWorld      string
+	DontTestMe      string
+	TestEmptyOutput string
+	TestQuoting     string
+)
+
 func ExampleHelloWorld() {
 	fmt.Println("Hello Example!")
 	fmt.Println("expected: " + Expected)

tests/core/go_test/filter_test_cases_test.go

@@ -81,7 +81,7 @@ func Test(t *testing.T) {
 			}
 			p, err := bazel_testing.BazelOutput("info", "bazel-testlogs")
 			if err != nil {
-				t.Fatal("could not find testlog root: %s", err)
+				t.Fatalf("could not find testlog root: %s", err)
 			}
 			path := filepath.Join(strings.TrimSpace(string(p)), "filter_test/test.xml")
 			b, err := ioutil.ReadFile(path)

tests/core/go_test/timeout_test.go

@@ -45,7 +45,7 @@ func TestTimeout(t *testing.T) {
 	if err := bazel_testing.RunBazel("test", "//:timeout_test", "--test_timeout=3", "--test_arg=-test.v"); err == nil {
 		t.Fatal("expected bazel test to fail")
 	} else if exitErr, ok := err.(*bazel_testing.StderrExitError); !ok || exitErr.Err.ExitCode() != 3 {
-		t.Fatalf("expected bazel test to fail with exit code 3", err)
+		t.Fatalf("expected bazel test to fail with exit code 3, got %v", err)
 	} else {
 		stderr = string(exitErr.Err.Stderr)
 	}

tests/core/go_test/xmlreport_test.go

@@ -142,7 +142,7 @@ func Test(t *testing.T) {
 
 			p, err := bazel_testing.BazelOutput("info", "bazel-testlogs")
 			if err != nil {
-				t.Fatal("could not find testlog root: %s", err)
+				t.Fatalf("could not find testlog root: %s", err)
 			}
 			path := filepath.Join(strings.TrimSpace(string(p)), "xml_test/test.xml")
 			b, err := ioutil.ReadFile(path)

tests/integration/reproducibility/reproducibility_test.go

@@ -184,6 +184,8 @@ func Test(t *testing.T) {
 	// Build the targets in each directory.
 	var wg sync.WaitGroup
 	wg.Add(len(dirs))
+	var errs []error
+	var mu sync.Mutex
 	for _, dir := range dirs {
 		go func(dir string) {
 			defer wg.Done()
@@ -196,11 +198,19 @@ func Test(t *testing.T) {
 			)
 			cmd.Dir = dir
 			if err := cmd.Run(); err != nil {
-				t.Fatalf("in %s, error running %s: %v", dir, strings.Join(cmd.Args, " "), err)
+				mu.Lock()
+				errs = append(errs, fmt.Errorf("in %s, error running %s: %v", dir, strings.Join(cmd.Args, " "), err))
+				mu.Unlock()
 			}
 		}(dir)
 	}
 	wg.Wait()
+	if len(errs) > 0 {
+		for _, err := range errs {
+			t.Error(err)
+		}
+		t.Fatal("errors building")
+	}
 
 	t.Run("Check Hashes", func(t *testing.T) {
 		// Hash files in each bazel-bin directory.
@@ -232,7 +242,7 @@ func Test(t *testing.T) {
 
 		// Compare dir0 and dir2. They should be different.
 		if err := compareHashes(dirHashes[0], dirHashes[2]); err == nil {
-			t.Fatalf("dir0 and dir2 are the same)", len(dirHashes[0]))
+			t.Fatal("dir0 and dir2 are the same")
 		}
 	})
 
@@ -309,24 +319,29 @@ func copyTree(dstRoot, srcRoot string) error {
 
 		if info.IsDir() {
 			return os.Mkdir(dstPath, 0777)
+		} else {
+			return copyFile(dstPath, srcPath)
 		}
-		r, err := os.Open(srcPath)
-		if err != nil {
-			return nil
-		}
-		defer r.Close()
-		w, err := os.Create(dstPath)
-		if err != nil {
-			return err
-		}
-		defer w.Close()
-		if _, err := io.Copy(w, r); err != nil {
-			return err
-		}
-		return w.Close()
 	})
 }
 
+func copyFile(dstPath, srcPath string) error {
+	r, err := os.Open(srcPath)
+	if err != nil {
+		return nil
+	}
+	defer r.Close()
+	w, err := os.Create(dstPath)
+	if err != nil {
+		return err
+	}
+	defer w.Close()
+	if _, err := io.Copy(w, r); err != nil {
+		return err
+	}
+	return w.Close()
+}
+
 func compareHashes(lhs, rhs []fileHash) error {
 	buf := &bytes.Buffer{}
 	for li, ri := 0, 0; li < len(lhs) || ri < len(rhs); {
@@ -342,6 +357,12 @@ func compareHashes(lhs, rhs []fileHash) error {
 		}
 		if lhs[li].hash != rhs[ri].hash {
 			fmt.Fprintf(buf, "%s is different: %s %s\n", lhs[li].rel, lhs[li].hash, rhs[ri].hash)
+			if err := copyToTestOutputs(lhs[li]); err != nil {
+				fmt.Fprintf(buf, "failed to copy file: %v\n", err)
+			}
+			if err := copyToTestOutputs(rhs[li]); err != nil {
+				fmt.Fprintf(buf, "failed to copy file: %v\n", err)
+			}
 		}
 		li++
 		ri++
@@ -353,7 +374,7 @@ func compareHashes(lhs, rhs []fileHash) error {
 }
 
 type fileHash struct {
-	rel, hash string
+	abs, rel, hash string
 }
 
 func hashFiles(dir string, exclude func(root, path string) bool) ([]fileHash, error) {
@@ -426,7 +447,7 @@ func hashFiles(dir string, exclude func(root, path string) bool) ([]fileHash, er
 		if _, err := io.Copy(h, r); err != nil {
 			return err
 		}
-		hashes = append(hashes, fileHash{rel: rel, hash: hex.EncodeToString(h.Sum(sum[:0]))})
+		hashes = append(hashes, fileHash{abs: path, rel: rel, hash: hex.EncodeToString(h.Sum(sum[:0]))})
 
 		return nil
 	})
@@ -435,3 +456,9 @@ func hashFiles(dir string, exclude func(root, path string) bool) ([]fileHash, er
 	}
 	return hashes, nil
 }
+
+// copyToTestOutputs copies a hashed file to the test outputs directory so that
+// it can be inspected manually under bazel-testlogs, e.g. using diffoscope.
+func copyToTestOutputs(hash fileHash) error {
+	return copyFile(filepath.Join(os.Getenv("TEST_UNDECLARED_OUTPUTS_DIR"), hash.hash), hash.abs)
+}