Repository: ko-build/ko (language: go)

Issue:
Refactor go builder to stop using go/build
In https://github.com/google/ko/pull/303 I added a big warning about GOROOT mismatches between `ko` and `go`. I think this is necessary given how `ko` works, but we should be able to remove that warning if we refactor the go builder a bit.

Historically, `ko` would attempt to [`Import`](https://golang.org/pkg/go/build/#Import) _every single string_ in a document to determine whether or not it should build it. If we succeeded, great. If we failed, just ignore it (i.e. we failed open). That code is [here](https://github.com/google/ko/blob/82cabb40bae577ce3bc016e5939fd85889538e8b/pkg/build/gobuild.go#L252-L273).

Per [rsc](https://github.com/google/ko/issues/106#issuecomment-551929755) this is _bad_. If `ko` embeds knowledge of how to import/build go code in its binary, if that logic changes in `go`, we have a mismatch. This happened with `GOROOT`, which is why we're here.

Unfortunately, shelling out to `go` would be too slow if we needed to do it for every string.

Now that https://github.com/google/ko/pull/281 has landed, we no longer look at every string. We only consider `ko://` strings, and we expect importing/building those targets will always succeed (i.e. we fail closed). Thus, we shouldn't really need to use `go/build.Import`. Shelling out to `go` should be fine, because we only do it a handful of times.

Similarly, we relied on [strange heuristics](https://github.com/google/ko/pull/50#issuecomment-509421373) to avoid [shelling out to `go` to get module info than once](https://github.com/google/ko/blob/82cabb40bae577ce3bc016e5939fd85889538e8b/pkg/build/gobuild.go#L132-L175) or using the `packages` package because it was _slow_ for large numbers of strings.

I think we should revisit some of these decisions, now that we require `ko://` everywhere.

If I remember correctly, we should be able to just use [`packages`](https://pkg.go.dev/golang.org/x/tools/go/packages) for everything (it was build for this express purpose), but we should do some careful benchmarking to make sure we don't slow things down unnecessarily.

I'd like some numbers for tekton, knative, and maybe mink before merging any of this.

cc @mattmoor @ImJasonH thoughts?


Discussion / hints:
> I'd like some numbers for tekton, knative, and maybe mink before merging any of this.
> @ImJasonH thoughts?

+1

What numbers can I provide?
> What numbers

A matrix like this would give me confidence to merge:

cold|warm x before|after x project

We'd need the PR to exist before we test it, though. I don't intend to implement this anytime soon, so if someone else wants to jump on it, feel free.
This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Reopen the issue with `/reopen`. Mark the issue as
fresh by adding the comment `/remove-lifecycle stale`.
i'm slowly taking a look at this 😄 , no promises

Fix patch:
diff --git a/pkg/build/gobuild.go b/pkg/build/gobuild.go
index c62d08d0cb..e22ad91d59 100644
--- a/pkg/build/gobuild.go
+++ b/pkg/build/gobuild.go
@@ -20,7 +20,6 @@ import (
 	"archive/tar"
 	"bytes"
 	"context"
-	"encoding/json"
 	"errors"
 	"fmt"
 	gb "go/build"
@@ -55,10 +54,6 @@ type GetBase func(context.Context, string) (name.Reference, Result, error)
 
 type builder func(context.Context, string, string, v1.Platform, Config) (string, error)
 
-type buildContext interface {
-	Import(path string, srcDir string, mode gb.ImportMode) (*gb.Package, error)
-}
-
 type platformMatcher struct {
 	spec      string
 	platforms []v1.Platform
@@ -71,8 +66,6 @@ type gobuild struct {
 	build                builder
 	disableOptimizations bool
 	buildConfigs         map[string]Config
-	mod                  *modules
-	buildContext         buildContext
 	platformMatcher      *platformMatcher
 	dir                  string
 	labels               map[string]string
@@ -88,8 +81,6 @@ type gobuildOpener struct {
 	build                builder
 	disableOptimizations bool
 	buildConfigs         map[string]Config
-	mod                  *modules
-	buildContext         buildContext
 	platform             string
 	labels               map[string]string
 	dir                  string
@@ -110,90 +101,12 @@ func (gbo *gobuildOpener) Open() (Interface, error) {
 		build:                gbo.build,
 		disableOptimizations: gbo.disableOptimizations,
 		buildConfigs:         gbo.buildConfigs,
-		mod:                  gbo.mod,
-		buildContext:         gbo.buildContext,
 		labels:               gbo.labels,
 		dir:                  gbo.dir,
 		platformMatcher:      matcher,
 	}, nil
 }
 
-// https://golang.org/pkg/cmd/go/internal/modinfo/#ModulePublic
-type modules struct {
-	main *modInfo
-	deps map[string]*modInfo
-}
-
-type modInfo struct {
-	Path string
-	Dir  string
-	Main bool
-}
-
-// moduleInfo returns the module path and module root directory for a project
-// using go modules, otherwise returns nil.
-//
-// Related: https://github.com/golang/go/issues/26504
-func moduleInfo(ctx context.Context, dir string) (*modules, error) {
-	modules := modules{
-		deps: make(map[string]*modInfo),
-	}
-
-	// TODO we read all the output as a single byte array - it may
-	// be possible & more efficient to stream it
-	cmd := exec.CommandContext(ctx, "go", "list", "-mod=readonly", "-json", "-m", "all")
-	cmd.Dir = dir
-	output, err := cmd.Output()
-	if err != nil {
-		return nil, nil
-	}
-
-	dec := json.NewDecoder(bytes.NewReader(output))
-
-	for {
-		var info modInfo
-		err := dec.Decode(&info)
-		if err == io.EOF {
-			// all done
-			break
-		}
-
-		modules.deps[info.Path] = &info
-
-		if info.Main {
-			modules.main = &info
-		}
-
-		if err != nil {
-			return nil, fmt.Errorf("error reading module data %w", err)
-		}
-	}
-
-	if modules.main == nil {
-		return nil, fmt.Errorf("couldn't find main module")
-	}
-
-	return &modules, nil
-}
-
-// getGoroot shells out to `go env GOROOT` to determine
-// the GOROOT for the installed version of go so that we
-// can set it in our buildContext. By default, the GOROOT
-// of our buildContext is set to the GOROOT at install
-// time for `ko`, which means that we break when certain
-// package managers update go or when using a pre-built
-// `ko` binary that expects a different GOROOT.
-//
-// See https://github.com/google/ko/issues/106
-func getGoroot(ctx context.Context, dir string) (string, error) {
-	cmd := exec.CommandContext(ctx, "go", "env", "GOROOT")
-	// It's probably not necessary to set the command working directory here,
-	// but it helps keep everything consistent.
-	cmd.Dir = dir
-	output, err := cmd.Output()
-	return strings.TrimSpace(string(output)), err
-}
-
 // NewGo returns a build.Interface implementation that:
 //  1. builds go binaries named by importpath,
 //  2. containerizes the binary on a suitable base.
@@ -201,36 +114,9 @@ func getGoroot(ctx context.Context, dir string) (string, error) {
 // The `dir` argument is the working directory for executing the `go` tool.
 // If `dir` is empty, the function uses the current process working directory.
 func NewGo(ctx context.Context, dir string, options ...Option) (Interface, error) {
-	// TODO: We could do moduleInfo() and getGoroot() concurrently.
-	module, err := moduleInfo(ctx, dir)
-	if err != nil {
-		return nil, err
-	}
-
-	goroot, err := getGoroot(ctx, dir)
-	if err != nil {
-		// On error, print the output and set goroot to "" to avoid using it later.
-		log.Printf("Unexpected error running \"go env GOROOT\": %v\n%v", err, goroot)
-		goroot = ""
-	} else if goroot == "" {
-		log.Printf(`Unexpected: $(go env GOROOT) == ""`)
-	}
-
-	// If $(go env GOROOT) successfully returns a non-empty string that differs from
-	// the default build context GOROOT, use $(go env GOROOT) instead.
-	bc := gb.Default
-	bc.Dir = dir
-	if goroot != "" && bc.GOROOT != goroot {
-		bc.GOROOT = goroot
-	}
-
 	gbo := &gobuildOpener{
-		build:        build,
-		mod:          module,
-		buildContext: &bc,
-		// dir is set on both buildContext and on gbo. Not ideal, but the
-		// build.Context interface doesn't expose
-		dir: dir,
+		build: build,
+		dir:   dir,
 	}
 
 	for _, option := range options {
@@ -280,39 +166,19 @@ func (g *gobuild) IsSupportedReference(s string) error {
 	if !ref.IsStrict() {
 		return errors.New("importpath does not start with ko://")
 	}
-	p, err := g.importPackage(ref)
+	pkgs, err := packages.Load(&packages.Config{Dir: g.dir, Mode: packages.NeedName}, ref.Path())
 	if err != nil {
-		return err
+		return fmt.Errorf("error loading package from %s: %w", ref.Path(), err)
+	}
+	if len(pkgs) != 1 {
+		return fmt.Errorf("found %d local packages, expected 1", len(pkgs))
 	}
-	if !p.IsCommand() {
+	if pkgs[0].Name != "main" {
 		return errors.New("importpath is not `package main`")
 	}
 	return nil
 }
 
-// importPackage wraps go/build.Import to handle go modules.
-//
-// Note that we will fall back to GOPATH if the project isn't using go modules.
-func (g *gobuild) importPackage(ref reference) (*gb.Package, error) {
-	if g.mod == nil {
-		return g.buildContext.Import(ref.Path(), gb.Default.GOPATH, gb.ImportComment)
-	}
-
-	// If we're inside a go modules project, try to use the module's directory
-	// as our source root to import:
-	// * any strict reference we get
-	// * paths that match module path prefix (they should be in this project)
-	// * relative paths (they should also be in this project)
-	// * path is a module
-
-	_, isDep := g.mod.deps[ref.Path()]
-	if ref.IsStrict() || strings.HasPrefix(ref.Path(), g.mod.main.Path) || gb.IsLocalImport(ref.Path()) || isDep {
-		return g.buildContext.Import(ref.Path(), g.mod.main.Dir, gb.ImportComment)
-	}
-
-	return nil, fmt.Errorf("unmatched importPackage %q with gomodules", ref.String())
-}
-
 func getGoarm(platform v1.Platform) (string, error) {
 	if !strings.HasPrefix(platform.Variant, "v") {
 		return "", fmt.Errorf("strange arm variant: %v", platform.Variant)
@@ -495,11 +361,17 @@ func tarBinary(name, binary string, creationTime v1.Time, platform *v1.Platform)
 }
 
 func (g *gobuild) kodataPath(ref reference) (string, error) {
-	p, err := g.importPackage(ref)
+	pkgs, err := packages.Load(&packages.Config{Dir: g.dir, Mode: packages.NeedFiles}, ref.Path())
 	if err != nil {
-		return "", err
+		return "", fmt.Errorf("error loading package from %s: %w", ref.Path(), err)
+	}
+	if len(pkgs) != 1 {
+		return "", fmt.Errorf("found %d local packages, expected 1", len(pkgs))
+	}
+	if len(pkgs[0].GoFiles) == 0 {
+		return "", fmt.Errorf("package %s contains no Go files", pkgs[0])
 	}
-	return filepath.Join(p.Dir, "kodata"), nil
+	return filepath.Join(filepath.Dir(pkgs[0].GoFiles[0]), "kodata"), nil
 }
 
 // Where kodata lives in the image.
diff --git a/pkg/build/options.go b/pkg/build/options.go
index 60c0fca7c8..443cdd3e54 100644
--- a/pkg/build/options.go
+++ b/pkg/build/options.go
@@ -98,20 +98,3 @@ func withBuilder(b builder) Option {
 		return nil
 	}
 }
-
-// withModulePath is a functional option for overriding the module path for
-// the current ko invocation.
-// This is exposed for testing.
-func withModuleInfo(m *modules) Option {
-	return func(gbo *gobuildOpener) error {
-		gbo.mod = m
-		return nil
-	}
-}
-
-func withBuildContext(b buildContext) Option {
-	return func(gbo *gobuildOpener) error {
-		gbo.buildContext = b
-		return nil
-	}
-}


Test patch:
diff --git a/pkg/build/gobuild_test.go b/pkg/build/gobuild_test.go
index a271364a71..c439b3ba24 100644
--- a/pkg/build/gobuild_test.go
+++ b/pkg/build/gobuild_test.go
@@ -20,7 +20,6 @@ import (
 	"archive/tar"
 	"context"
 	"fmt"
-	gb "go/build"
 	"io"
 	"io/ioutil"
 	"path"
@@ -176,29 +175,8 @@ func TestGoBuildIsSupportedRefWithModules(t *testing.T) {
 		t.Fatalf("random.Image() = %v", err)
 	}
 
-	mods := &modules{
-		main: &modInfo{
-			Path: "github.com/google/ko/test",
-			Dir:  ".",
-		},
-		deps: map[string]*modInfo{
-			"github.com/some/module/cmd": {
-				Path: "github.com/some/module/cmd",
-				Dir:  ".",
-			},
-		},
-	}
-
 	opts := []Option{
 		WithBaseImages(func(context.Context, string) (name.Reference, Result, error) { return baseRef, base, nil }),
-		withModuleInfo(mods),
-		withBuildContext(stubBuildContext{
-			// make all referenced deps commands
-			"github.com/google/ko/test":  &gb.Package{Name: "main"},
-			"github.com/some/module/cmd": &gb.Package{Name: "main"},
-
-			"github.com/google/ko/pkg/build": &gb.Package{Name: "build"},
-		}),
 	}
 
 	ng, err := NewGo(context.Background(), "", opts...)
@@ -208,8 +186,8 @@ func TestGoBuildIsSupportedRefWithModules(t *testing.T) {
 
 	// Supported import paths.
 	for _, importpath := range []string{
-		"ko://github.com/google/ko/test",  // ko can build the test package.
-		"ko://github.com/some/module/cmd", // ko can build commands in dependent modules
+		"ko://github.com/google/ko/test",         // ko can build the test package.
+		"ko://github.com/go-training/helloworld", // ko can build commands in dependent modules
 	} {
 		t.Run(importpath, func(t *testing.T) {
 			if err := ng.IsSupportedReference(importpath); err != nil {
@@ -222,7 +200,7 @@ func TestGoBuildIsSupportedRefWithModules(t *testing.T) {
 	for _, importpath := range []string{
 		"ko://github.com/google/ko/pkg/build",       // not a command.
 		"ko://github.com/google/ko/pkg/nonexistent", // does not exist.
-		"ko://github.com/google/ko",                 // not in this module.
+		"ko://github.com/google/go-github",          // not in this module.
 	} {
 		t.Run(importpath, func(t *testing.T) {
 			if err := ng.IsSupportedReference(importpath); err == nil {
@@ -580,16 +558,6 @@ func validateImage(t *testing.T, img v1.Image, baseLayers int64, creationTime v1
 	})
 }
 
-type stubBuildContext map[string]*gb.Package
-
-func (s stubBuildContext) Import(path string, srcDir string, mode gb.ImportMode) (*gb.Package, error) {
-	p, ok := s[path]
-	if ok {
-		return p, nil
-	}
-	return nil, fmt.Errorf("not found: %s", path)
-}
-
 func TestGoBuild(t *testing.T) {
 	baseLayers := int64(3)
 	base, err := random.Image(1024, baseLayers)