Skip to content

Commit

Permalink
use fewer build flags when building std or cmd
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
mvdan committed Mar 5, 2023
1 parent 6a8dda9 commit 059c1d6
Show file tree
Hide file tree
Showing 4 changed files with 49 additions and 20 deletions.
22 changes: 15 additions & 7 deletions internal/linker/linker.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import (
"os/exec"
"path/filepath"
"regexp"
"runtime"

"github.com/bluekeyes/go-gitdiff/gitdiff"
"github.com/rogpeppe/go-internal/lockedfile"
Expand All @@ -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)
Expand Down Expand Up @@ -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

Expand Down
6 changes: 1 addition & 5 deletions main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
34 changes: 27 additions & 7 deletions shared.go
Original file line number Diff line number Diff line change
Expand Up @@ -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...)

Expand Down Expand Up @@ -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"))
Expand Down
7 changes: 6 additions & 1 deletion testdata/script/linker.txtar
Original file line number Diff line number Diff line change
@@ -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
Expand Down

0 comments on commit 059c1d6

Please sign in to comment.