Skip to content

Commit a1a0b60

Browse files
authored
Fix go_tool_binary non-hermeticity and Go 1.19 incompatibility (#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**
1 parent a54de7c commit a1a0b60

17 files changed

Lines changed: 138 additions & 35 deletions

File tree

‎WORKSPACE‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
workspace(name = "io_bazel_rules_go")
22

33
load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
4-
load("@io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies")
4+
load("@io_bazel_rules_go//go:deps.bzl", "go_download_sdk", "go_register_nogo", "go_register_toolchains", "go_rules_dependencies")
55

66
# Required by toolchains_protoc.
77
http_archive(
@@ -29,6 +29,15 @@ go_rules_dependencies()
2929

3030
go_register_toolchains(version = "1.23.1")
3131

32+
go_download_sdk(
33+
name = "rules_go_internal_compatibility_sdk",
34+
version = "1.19.13",
35+
)
36+
37+
go_register_nogo(
38+
nogo = "@//internal:nogo",
39+
)
40+
3241
http_archive(
3342
name = "rules_proto",
3443
sha256 = "303e86e722a520f6f326a50b41cfc16b98fe6d1955ce46642a5b7a67c11c0f5d",
@@ -137,7 +146,7 @@ grpc_dependencies()
137146

138147
load("@bazel_gazelle//:deps.bzl", "gazelle_dependencies", "go_repository")
139148

140-
gazelle_dependencies()
149+
gazelle_dependencies(go_sdk = "go_sdk")
141150

142151
go_repository(
143152
name = "com_github_google_go_github_v36",

‎go/private/nogo.bzl‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
# limitations under the License.
1414

1515
DEFAULT_NOGO = "@io_bazel_rules_go//:default_nogo"
16-
NOGO_DEFAULT_INCLUDES = ["@@//:__subpackages__"]
16+
NOGO_DEFAULT_INCLUDES = [str(Label("//:__subpackages__"))]
1717
NOGO_DEFAULT_EXCLUDES = []
1818

1919
# repr(Label(...)) does not emit a canonical label literal.

‎go/private/rules/binary.bzl‎

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -464,6 +464,17 @@ go_non_executable_binary = rule(executable = False, **_go_binary_kwargs(
464464
))
465465

466466
def _go_tool_binary_impl(ctx):
467+
# Keep in mind that the actions registered by this rule may not be
468+
# sandboxed, so care must be taken to make them hermetic, for example by
469+
# preventing `go build` from searching for go.mod or downloading a
470+
# different toolchain version.
471+
#
472+
# A side effect of the usage of GO111MODULE below is that the absolute
473+
# path to the sources is included in the buildid, which would make the
474+
# resulting binary non-reproducible. We thus need to blank it out.
475+
# https://github.com/golang/go/blob/583d750fa119d504686c737be6a898994b674b69/src/cmd/go/internal/load/pkg.go#L1764-L1766
476+
# https://github.com/golang/go/blob/583d750fa119d504686c737be6a898994b674b69/src/cmd/go/internal/work/exec.go#L284
477+
467478
sdk = ctx.attr.sdk[GoSDK]
468479
name = ctx.label.name
469480
if sdk.goos == "windows":
@@ -478,7 +489,9 @@ def _go_tool_binary_impl(ctx):
478489
set GOMAXPROCS=1
479490
set GOCACHE=%cd%\\{gotmp}\\gocache
480491
set GOPATH=%cd%"\\{gotmp}\\gopath
481-
{go} build -o {out} -trimpath -ldflags \"{ldflags}\" {srcs}
492+
set GOTOOLCHAIN=local
493+
set GO111MODULE=off
494+
{go} build -o {out} -trimpath -ldflags \"-buildid='' {ldflags}\" {srcs}
482495
set GO_EXIT_CODE=%ERRORLEVEL%
483496
RMDIR /S /Q "{gotmp}"
484497
MKDIR "{gotmp}"
@@ -506,7 +519,16 @@ exit /b %GO_EXIT_CODE%
506519
)
507520
else:
508521
# Note: GOPATH is needed for Go 1.16.
509-
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(
522+
cmd = """
523+
GOTMP=$(mktemp -d)
524+
trap "rm -rf \"$GOTMP\"" EXIT
525+
GOMAXPROCS=1 \
526+
GOCACHE="$GOTMP"/gocache \
527+
GOPATH="$GOTMP"/gopath \
528+
GOTOOLCHAIN=local \
529+
GO111MODULE=off \
530+
{go} build -o {out} -trimpath -ldflags '-buildid="" {ldflags}' {srcs}
531+
""".format(
510532
go = sdk.go.path,
511533
out = out.path,
512534
srcs = " ".join([f.path for f in ctx.files.srcs]),

‎go/runfiles/manifest.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,8 @@ func (m *manifest) open(name string) (fs.File, error) {
138138
// name refers to an actual file or dir listed in the manifest. The
139139
// basename of name may not match the basename of the underlying
140140
// file (e.g. in the case of a root symlink), so patch it.
141-
f, err := os.Open(r)
141+
var f *os.File
142+
f, err = os.Open(r)
142143
if err != nil {
143144
return nil, err
144145
}

‎go/tools/builders/nogo_main.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -223,7 +223,7 @@ func checkPackage(analyzers []*analysis.Analyzer, packagePath string, packageFil
223223
if _, ok := ignoreFilesSet[pkg.fset.Position(f.Pos()).Filename]; ok {
224224
for _, act := range actions {
225225
act.nolint = append(act.nolint, &Range{
226-
from: pkg.fset.Position(f.FileStart),
226+
from: pkg.fset.Position(f.Pos()),
227227
to: pkg.fset.Position(f.End()).Line,
228228
})
229229
}

‎go/tools/releaser/prepare.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,7 @@ func runPrepare(ctx context.Context, stderr io.Writer, args []string) error {
125125
branchName := "release-" + majorMinor[len("v"):]
126126
if !gitBranchExists(ctx, rootDir, branchName) {
127127
if !isMinorRelease {
128-
return fmt.Errorf("release branch %q does not exist locally. Fetch it, set RULES_GO_VERSION, add commits, and run this command again.")
128+
return fmt.Errorf("release branch %q does not exist locally. Fetch it, set RULES_GO_VERSION, add commits, and run this command again.", branchName)
129129
}
130130
if err := checkRulesGoVersion(ctx, rootDir, "HEAD", version); err != nil {
131131
return err
@@ -247,7 +247,7 @@ func checkRulesGoVersion(ctx context.Context, dir, refName, version string) erro
247247
}
248248
rulesGoVersionStr := []byte(fmt.Sprintf(`RULES_GO_VERSION = "%s"`, version[len("v"):]))
249249
if !bytes.Contains(data, rulesGoVersionStr) {
250-
return fmt.Errorf("RULES_GO_VERSION was not set to %q in go/def.bzl. Set it, add commits, and run this command again.")
250+
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)
251251
}
252252
return nil
253253
}

‎go/tools/releaser/upgradedep.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -461,8 +461,8 @@ func upgradeDepDecl(ctx context.Context, gh *githubClient, workDir, name string,
461461
}
462462
patchName := patchLabelValue[len("//third_party:"):]
463463
patchPath := filepath.Join(rootDir, "third_party", patchName)
464-
prevDir := filepath.Join(workDir, name, string('a'+patchIndex))
465-
patchDir := filepath.Join(workDir, name, string('a'+patchIndex+1))
464+
prevDir := filepath.Join(workDir, name, fmt.Sprintf("a%d", patchIndex))
465+
patchDir := filepath.Join(workDir, name, fmt.Sprintf("a%d", patchIndex+1))
466466
var patchCmd []string
467467
for _, c := range comments.Before {
468468
words := strings.Fields(strings.TrimPrefix(c.Token, "#"))
@@ -484,7 +484,7 @@ func upgradeDepDecl(ctx context.Context, gh *githubClient, workDir, name string,
484484
return err
485485
}
486486
}
487-
patch, _ := runForOutput(ctx, filepath.Join(workDir, name), "diff", "-urN", string('a'+patchIndex), string('a'+patchIndex+1))
487+
patch, _ := runForOutput(ctx, filepath.Join(workDir, name), "diff", "-urN", fmt.Sprintf("a%d", patchIndex), fmt.Sprintf("a%d", patchIndex+1))
488488
patch = sanitizePatch(patch)
489489
if err := os.WriteFile(patchPath, patch, 0666); err != nil {
490490
return err

‎internal/BUILD.bazel‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
load("//go:def.bzl", "TOOLS_NOGO", "nogo")
2+
3+
nogo(
4+
name = "nogo",
5+
visibility = ["//visibility:public"],
6+
deps = [
7+
a
8+
for a in TOOLS_NOGO
9+
# Filter out shadow as it triggers on err and is very noisy.
10+
if a != "@org_golang_x_tools//go/analysis/passes/shadow:go_default_library"
11+
],
12+
)
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
load("//go:def.bzl", "go_binary", "go_cross_binary")
2+
3+
go_binary(
4+
name = "hello",
5+
srcs = ["hello.go"],
6+
)
7+
8+
go_cross_binary(
9+
name = "hello_old",
10+
sdk_version = "1.19",
11+
target = ":hello",
12+
)

‎tests/core/compatibility/README.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
# Compatibility
2+
3+
## compatibility_test
4+
5+
Verifies that a simple Go project builds with an older version of Go.

0 commit comments

Comments
 (0)