From 059c1d68e24e3595ecc6328a7d490aa52de72c36 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Mart=C3=AD?= Date: Sat, 4 Mar 2023 18:15:53 +0000 Subject: [PATCH] use fewer build flags when building std or cmd When we use `go list` on the standard library, we need to be careful about what flags are passed from the top-level build command, because some flags are not going to be appropriate. In particular, GOFLAGS=-modfile=... resulted in a failure, reproduced via the GOFLAGS variable added to linker.txtar: go: inconsistent vendoring in /home/mvdan/tip/src: golang.org/x/crypto@v0.5.1-0.20230203195927-310bfa40f1e4: is marked as explicit in vendor/modules.txt, but not explicitly required in go.mod golang.org/x/net@v0.7.0: is marked as explicit in vendor/modules.txt, but not explicitly required in go.mod golang.org/x/sys@v0.5.1-0.20230208141308-4fee21c92339: is marked as explicit in vendor/modules.txt, but not explicitly required in go.mod golang.org/x/text@v0.7.1-0.20230207171107-30dadde3188b: is marked as explicit in vendor/modules.txt, but not explicitly required in go.mod To ignore the vendor directory, use -mod=readonly or -mod=mod. To sync the vendor directory, run: go mod vendor To work around this problem, reset the -mod and -modfile flags when calling "go list" on the standard library, as those are the only two flags which alter how we load the main module in a build. The code which builds a modified cmd/link has a similar problem; it already reset GOOS and GOARCH, but it could similarly run into problems if other env vars like GOFLAGS were set. To be on the safe side, we also disable GOENV and GOEXPERIMENT, which we borrow from Go's bootstrapping commands. --- internal/linker/linker.go | 22 +++++++++++++++------- main.go | 6 +----- shared.go | 34 +++++++++++++++++++++++++++------- testdata/script/linker.txtar | 7 ++++++- 4 files changed, 49 insertions(+), 20 deletions(-) diff --git a/internal/linker/linker.go b/internal/linker/linker.go index 9a87602..0d145cf 100644 --- a/internal/linker/linker.go +++ b/internal/linker/linker.go @@ -16,7 +16,6 @@ import ( "os/exec" "path/filepath" "regexp" - "runtime" "github.com/bluekeyes/go-gitdiff/gitdiff" "github.com/rogpeppe/go-internal/lockedfile" @@ -33,10 +32,8 @@ const ( baseSrcSubdir = "src" ) -var ( - //go:embed patches/*.patch - linkerPatchesFS embed.FS -) +//go:embed patches/*.patch +var linkerPatchesFS embed.FS func loadLinkerPatches() (version string, modFiles map[string]bool, patches [][]byte, err error) { modFiles = make(map[string]bool) @@ -198,8 +195,19 @@ func buildLinker(workingDir string, overlay map[string]string, outputLinkPath st } cmd := exec.Command("go", "build", "-overlay", overlayPath, "-o", outputLinkPath, "cmd/link") - // Explicitly setting GOOS and GOARCH variables prevents conflicts during cross-build - cmd.Env = append(os.Environ(), "GOOS="+runtime.GOOS, "GOARCH="+runtime.GOARCH) + // Ignore any build settings from the environment or GOENV. + // We want to build cmd/link like the rest of the toolchain, + // regardless of what build options are set for the current build. + // + // TODO: a nicer way would be to use the same flags recorded in the current + // cmd/link binary, which can be seen via: + // + // go version -m ~/tip/pkg/tool/linux_amd64/link + // + // and which can be done from Go via debug/buildinfo.ReadFile. + cmd.Env = append(os.Environ(), + "GOENV=off", "GOOS=", "GOARCH=", "GOEXPERIMENT=", "GOFLAGS=", + ) // Building cmd/link is possible from anywhere, but to avoid any possible side effects build in a temp directory cmd.Dir = workingDir diff --git a/main.go b/main.go index c5e10de..20d4c32 100644 --- a/main.go +++ b/main.go @@ -563,11 +563,7 @@ This command wraps "go %s". Below is its help: } } - goArgs := []string{ - command, - "-trimpath", - "-buildvcs=false", - } + goArgs := append([]string{command}, garbleBuildFlags...) // Pass the garble flags down to each toolexec invocation. // This way, all garble processes see the same flag values. diff --git a/shared.go b/shared.go index 87fa08f..bbd8f4f 100644 --- a/shared.go +++ b/shared.go @@ -193,18 +193,38 @@ func (p *listedPackage) obfuscatedImportPath() string { return newPath } +// garbleBuildFlags are always passed to top-level build commands such as +// "go build", "go list", or "go test". +var garbleBuildFlags = []string{"-trimpath", "-buildvcs=false"} + // appendListedPackages gets information about the current package // and all of its dependencies -func appendListedPackages(packages []string, withDeps bool) error { +func appendListedPackages(packages []string, mainBuild bool) error { startTime := time.Now() - // TODO: perhaps include all top-level build flags set by garble, - // including -buildvcs=false. - // They shouldn't affect "go list" here, but might as well be consistent. - args := []string{"list", "-json", "-export", "-compiled", "-trimpath", "-e"} - if withDeps { + args := []string{ + "list", + // Similar flags to what go/packages uses. + "-json", "-export", "-compiled", "-e", + } + if mainBuild { + // When loading the top-level packages we are building, + // we want to transitively load all their dependencies as well. + // That is not the case when loading standard library packages, + // as runtimeLinknamed already contains transitive dependencies. args = append(args, "-deps") } + args = append(args, garbleBuildFlags...) args = append(args, cache.ForwardBuildFlags...) + + if !mainBuild { + // If the top-level build included the -mod or -modfile flags, + // they should be used when loading the top-level packages. + // However, when loading standard library packages, + // using those flags would likely result in an error, + // as the standard library uses its own Go module and vendoring. + args = append(args, "-mod=", "-modfile=") + } + args = append(args, packages...) cmd := exec.Command("go", args...) @@ -282,7 +302,7 @@ func appendListedPackages(packages []string, withDeps bool) error { } if err := cmd.Wait(); err != nil { - return fmt.Errorf("go list error: %v: %s", err, stderr.Bytes()) + return fmt.Errorf("go list error: %v:\nargs: %q\n%s", err, args, stderr.Bytes()) } if len(pkgErrors) > 0 { return errors.New(strings.Join(pkgErrors, "\n")) diff --git a/testdata/script/linker.txtar b/testdata/script/linker.txtar index 2aad6d4..9c0d7ee 100644 --- a/testdata/script/linker.txtar +++ b/testdata/script/linker.txtar @@ -1,7 +1,12 @@ # Past garble versions might not properly patch cmd/link with "git apply" # when running inside a git repository. Skip the extra check with -short. [!short] [exec:git] exec git init -q -[!short] [exec:git] env GARBLE_CACHE_DIR=$WORK/linker-cache +[!short] [exec:git] env GARBLE_CACHE_DIR=${WORK}/linker-cache + +# Any build settings for the main build shouldn't affect building the linker. +# If this flag makes it through when using build commands on std or cmd, +# those commands are likely to fail as std and cmd are their own modules. +env GOFLAGS=-modfile=${WORK}/go.mod garble build exec ./main