go/packages: Options.{Dir,Env} pass-through

This change adds optional Dir string and Env []string options
that are passed through to the build system's metadata query.
As with exec.Cmd, the defaults are inherited from the parent
process.

Options.GOPATH is gone. If the client needs to override
GOPATH, they must use Env, but typically the inherited environment
is correct.

The tests not longer use os.Chdir.

We now guarantee that Package.Srcs are absolute file names.

Added test for Options.Dir and relative patterns.

This is a copy of golang.org/cl/123777, which had a merge conflict.

Change-Id: I301724581f3d8c712ea78fa5ab5cabd909940dca
Reviewed-on: https://go-review.googlesource.com/123777
Run-TryBot: Michael Matloob <matloob@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>

go/packages/doc.go

@@ -244,28 +244,24 @@ application, but not by the metadata query, so, for example:
 
 Questions & Tasks
 
-- Add pass-through options for the underlying query tool:
+- Add this pass-through option for the underlying query tool:
-     Dir     string
-     Environ []string
      Flags   []string
-  Do away with GOROOT and don't add GOARCH/GOOS:
+
-  they are not portable concepts.
+- Add GOARCH/GOOS?
-  The goal is to allow users to express themselves using the conventions
+  They are not portable concepts, but could be made portable.
+  Our goal has been to allow users to express themselves using the conventions
   of the underlying build system: if the build system honors GOARCH
   during a build and during a metadata query, then so should
   applications built atop that query mechanism.
   Conversely, if the target architecture of the build is determined by
-  command-line flags, the application must pass the relevant
+  command-line flags, the application can pass the relevant
   flags through to the build system using a command such as:
     myapp -query_flag="--cpu=amd64" -query_flag="--os=darwin"
+  However, this approach is low-level, unwieldy, and non-portable.
+  GOOS and GOARCH seem important enough to warrant a dedicated option.
 
 - Build tags: where do they fit in?  How does Bazel/Blaze handle them?
 
-- Add an 'IncludeTests bool' option to include tests among the results.
-  This flag is needed to avoid unnecessary dependencies (and, for vgo, downloads).
-  Should it include/skip implied tests? (all tests are implied in go build)
-  Or include/skip all tests?
-
 - How should we handle partial failures such as a mixture of good and
   malformed patterns, existing and non-existent packages, succesful and
   failed builds, import failures, import cycles, and so on, in a call to
@@ -275,7 +271,7 @@ Questions & Tasks
   source file in Srcs to that of the original file, if known, or "" otherwise?
   Or are //line directives and "Generated" comments in those files enough?
 
-- Support bazel/blaze, not just "go list".
+- Support bazel, blaze, and go1.10 list, not just go1.11 list.
 
 - Support a "contains" query: a boolean option would cause the the
   pattern words to be interpreted as filenames, and the query would

go/packages/golist.go

@@ -11,6 +11,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"log"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -25,7 +26,8 @@ type GoTooOldError struct{ error }
 
 // golistPackages uses the "go list" command to expand the
 // pattern words and return metadata for the specified packages.
-func golistPackages(ctx context.Context, gopath string, cgo, export, tests bool, words []string) ([]*Package, error) {
+// dir may be "" and env may be nil, as per os/exec.Command.
+func golistPackages(ctx context.Context, dir string, env []string, cgo, export, tests bool, words []string) ([]*Package, error) {
 	// Fields must match go list;
 	// see $GOROOT/src/cmd/go/internal/load/pkg.go.
 	type jsonPackage struct {
@@ -62,7 +64,7 @@ func golistPackages(ctx context.Context, gopath string, cgo, export, tests bool,
 	// Run "go list" for complete
 	// information on the specified packages.
 
-	buf, err := golist(ctx, gopath, cgo, export, tests, words)
+	buf, err := golist(ctx, dir, env, cgo, export, tests, words)
 	if err != nil {
 		return nil, err
 	}
@@ -113,6 +115,11 @@ func golistPackages(ctx context.Context, gopath string, cgo, export, tests bool,
 			p.GoFiles = nil // ignore fake unsafe.go file
 		}
 
+		// Assume go list emits only absolute paths for Dir.
+		if !filepath.IsAbs(p.Dir) {
+			log.Fatalf("internal error: go list returned non-absolute Package.Dir: %s", p.Dir)
+		}
+
 		export := p.Export
 		if export != "" && !filepath.IsAbs(export) {
 			export = filepath.Join(p.Dir, export)
@@ -176,7 +183,7 @@ func absJoin(dir string, fileses ...[]string) (res []string) {
 }
 
 // golist returns the JSON-encoded result of a "go list args..." query.
-func golist(ctx context.Context, gopath string, cgo, export, tests bool, args []string) (*bytes.Buffer, error) {
+func golist(ctx context.Context, dir string, env []string, cgo, export, tests bool, args []string) (*bytes.Buffer, error) {
 	out := new(bytes.Buffer)
 	cmd := exec.CommandContext(ctx, "go", append([]string{
 		"list",
@@ -188,10 +195,15 @@ func golist(ctx context.Context, gopath string, cgo, export, tests bool, args []
 		"-json",
 		"--",
 	}, args...)...)
-	cmd.Env = append(append([]string(nil), os.Environ()...), "GOPATH="+gopath)
+
-	if !cgo {
+	if env == nil {
-		cmd.Env = append(cmd.Env, "CGO_ENABLED=0")
+		env = os.Environ()
 	}
+	if !cgo {
+		env = append(env, "CGO_ENABLED=0")
+	}
+	cmd.Env = env
+	cmd.Dir = dir
 	cmd.Stdout = out
 	cmd.Stderr = new(bytes.Buffer)
 	if err := cmd.Run(); err != nil {

go/packages/packages.go

@@ -32,14 +32,6 @@ type Options struct {
 	// is equivalent to context.Background().
 	Context context.Context
 
-	// GOPATH is the effective value of the GOPATH environment variable.
-	// If unset, the default is Getenv("GOPATH").
-	//
-	// TODO(adonovan): this is primarily needed for testing, but it
-	// is not a build-system portable concept.
-	// Replace with flags/cwd/environ pass-through.
-	GOPATH string
-
 	// The Tests flag causes the result to include any test packages
 	// implied by the patterns.
 	//
@@ -85,6 +77,15 @@ type Options struct {
 	// bodies to reduce the load on the type-checker.
 	// ParseFile is not used in Metadata mode.
 	ParseFile func(fset *token.FileSet, filename string) (*ast.File, error)
+
+	// Env is a list of environment variables to pass through
+	// to the build system's metadata query tool.
+	// If nil, the current process's environment is used.
+	Env []string
+
+	// Dir is the directory in which to run the build system's metadata query tool.
+	// If "", the current process's working directory is used.
+	Dir string
 }
 
 // Metadata returns the metadata for a set of Go packages,
@@ -215,8 +216,7 @@ type Package struct {
 
 	// Srcs is the list of names of this package's Go
 	// source files as presented to the compiler.
-	// Names aren't guaranteed to be absolute,
+	// Names are guaranteed to be absolute.
-	// but they are openable.
 	//
 	// In Metadata queries, or if DisableCgo is set,
 	// Srcs includes the unmodified source files even
@@ -226,8 +226,8 @@ type Package struct {
 	Srcs []string
 
 	// OtherSrcs is the list of names of non-Go source files that the package
-	// contains. This includes assembly and C source files. The names are
+	// contains. This includes assembly and C source files.
-	// "openable" in the same sense as are Srcs above.
+	// Names are guaranteed to be absolute.
 	OtherSrcs []string
 
 	// Imports maps each import path to its package
@@ -307,17 +307,13 @@ func (ld *loader) load(patterns ...string) ([]*Package, error) {
 		}
 	}
 
-	if ld.GOPATH == "" {
-		ld.GOPATH = os.Getenv("GOPATH")
-	}
-
 	if len(patterns) == 0 {
 		return nil, fmt.Errorf("no packages to load")
 	}
 
 	// Do the metadata query and partial build.
 	// TODO(adonovan): support alternative build systems at this seam.
-	list, err := golistPackages(ld.Context, ld.GOPATH, ld.cgo, ld.mode == typeCheck, ld.Tests, patterns)
+	list, err := golistPackages(ld.Context, ld.Dir, ld.Env, ld.cgo, ld.mode == typeCheck, ld.Tests, patterns)
 	if err != nil {
 		return nil, err
 	}

go/packages/packages_test.go

@@ -32,7 +32,7 @@ import (
 //   that will allow the maintainer to interact with the failing scenario.
 // - vendoring
 // - errors in go-list metadata
-// - all returned file names should be openable
+// - all returned file names are absolute
 // - a foo.test package that cannot be built for some reason (e.g.
 //   import error) will result in a JSON blob with no name and a
 //   nonexistent testmain file in GoFiles. Test that we handle this
@@ -50,7 +50,7 @@ func TestMetadataImportGraph(t *testing.T) {
 	if runtime.GOOS != "linux" {
 		t.Skipf("TODO: skipping on non-Linux; fix this test to run everywhere. golang.org/issue/26387")
 	}
-	tmp, cleanup := enterTree(t, map[string]string{
+	tmp, cleanup := makeTree(t, map[string]string{
 		"src/a/a.go":             `package a; const A = 1`,
 		"src/b/b.go":             `package b; import ("a"; _ "errors"); var B = a.A`,
 		"src/c/c.go":             `package c; import (_ "b"; _ "unsafe")`,
@@ -64,9 +64,8 @@ func TestMetadataImportGraph(t *testing.T) {
 		"src/f/f.go":             `package f`,
 	})
 	defer cleanup()
-	// -- tmp is now the current directory --
 
-	opts := &packages.Options{GOPATH: tmp}
+	opts := &packages.Options{Env: append(os.Environ(), "GOPATH="+tmp)}
 	initial, err := packages.Metadata(opts, "c", "subdir/d", "e")
 	if err != nil {
 		t.Fatal(err)
@@ -178,8 +177,8 @@ func TestMetadataImportGraph(t *testing.T) {
 	}
 
 	// Test an ad-hoc package, analogous to "go run hello.go".
-	if initial, err := packages.Metadata(opts, "src/c/c.go"); len(initial) == 0 {
+	if initial, err := packages.Metadata(opts, filepath.Join(tmp, "src/c/c.go")); len(initial) == 0 {
-		t.Errorf("failed to obtain metadata for ad-hoc package (err=%v)", err)
+		t.Errorf("failed to obtain metadata for ad-hoc package: %s", err)
 	} else {
 		got := fmt.Sprintf("%s %s", initial[0].ID, srcs(initial[0]))
 		if want := "command-line-arguments [c.go]"; got != want {
@@ -203,6 +202,52 @@ func TestMetadataImportGraph(t *testing.T) {
 	}
 }
 
+func TestOptionsDir(t *testing.T) {
+	tmp, cleanup := makeTree(t, map[string]string{
+		"src/a/a.go":   `package a; const Name = "a" `,
+		"src/a/b/b.go": `package b; const Name = "a/b"`,
+		"src/b/b.go":   `package b; const Name = "b"`,
+	})
+	defer cleanup()
+
+	for _, test := range []struct {
+		dir     string
+		pattern string
+		want    string // value of Name constant, or error
+	}{
+		{"", "a", `"a"`},
+		{"", "b", `"b"`},
+		{"", "./a", "packages not found"},
+		{"", "./b", "packages not found"},
+		{filepath.Join(tmp, "/src"), "a", `"a"`},
+		{filepath.Join(tmp, "/src"), "b", `"b"`},
+		{filepath.Join(tmp, "/src"), "./a", `"a"`},
+		{filepath.Join(tmp, "/src"), "./b", `"b"`},
+		{filepath.Join(tmp, "/src/a"), "a", `"a"`},
+		{filepath.Join(tmp, "/src/a"), "b", `"b"`},
+		{filepath.Join(tmp, "/src/a"), "./a", "packages not found"},
+		{filepath.Join(tmp, "/src/a"), "./b", `"a/b"`},
+	} {
+		opts := &packages.Options{
+			Dir: test.dir,
+			Env: append(os.Environ(), "GOPATH="+tmp),
+		}
+		// Use TypeCheck to ensure that files can be opened.
+		initial, err := packages.TypeCheck(opts, test.pattern)
+		var got string
+		if err != nil {
+			got = err.Error()
+		} else {
+			got = constant(initial[0], "Name").Val().String()
+		}
+		if got != test.want {
+			t.Errorf("dir %q, pattern %q: got %s, want %s",
+				test.dir, test.pattern, got, test.want)
+		}
+	}
+
+}
+
 type errCollector struct {
 	mu     sync.Mutex
 	errors []error
@@ -215,7 +260,7 @@ func (ec *errCollector) add(err error) {
 }
 
 func TestTypeCheckOK(t *testing.T) {
-	tmp, cleanup := enterTree(t, map[string]string{
+	tmp, cleanup := makeTree(t, map[string]string{
 		"src/a/a.go": `package a; import "b"; const A = "a" + b.B`,
 		"src/b/b.go": `package b; import "c"; const B = "b" + c.C`,
 		"src/c/c.go": `package c; import "d"; const C = "c" + d.D`,
@@ -223,9 +268,8 @@ func TestTypeCheckOK(t *testing.T) {
 		"src/e/e.go": `package e; const E = "e"`,
 	})
 	defer cleanup()
-	// -- tmp is now the current directory --
 
-	opts := &packages.Options{GOPATH: tmp, Error: func(error) {}}
+	opts := &packages.Options{Env: append(os.Environ(), "GOPATH="+tmp), Error: func(error) {}}
 	initial, err := packages.TypeCheck(opts, "a", "c")
 	if err != nil {
 		t.Fatal(err)
@@ -283,7 +327,7 @@ func TestTypeCheckOK(t *testing.T) {
 	}
 
 	// Check value of constant.
-	aA := all["a"].Type.Scope().Lookup("A").(*types.Const)
+	aA := constant(all["a"], "A")
 	if got, want := fmt.Sprintf("%v %v", aA, aA.Val()), `const a.A untyped string "abcde"`; got != want {
 		t.Errorf("a.A: got %s, want %s", got, want)
 	}
@@ -297,7 +341,7 @@ func TestTypeCheckError(t *testing.T) {
 	// are IllTyped. Package e is not ill-typed, because the user
 	// did not demand its type information (despite it actually
 	// containing a type error).
-	tmp, cleanup := enterTree(t, map[string]string{
+	tmp, cleanup := makeTree(t, map[string]string{
 		"src/a/a.go": `package a; import "b"; const A = "a" + b.B`,
 		"src/b/b.go": `package b; import "c"; const B = "b" + c.C`,
 		"src/c/c.go": `package c; import "d"; const C = "c" + d.D`,
@@ -305,9 +349,8 @@ func TestTypeCheckError(t *testing.T) {
 		"src/e/e.go": `package e; const E = "e" + 1`, // type error
 	})
 	defer cleanup()
-	// -- tmp is now the current directory --
 
-	opts := &packages.Options{GOPATH: tmp, Error: func(error) {}}
+	opts := &packages.Options{Env: append(os.Environ(), "GOPATH="+tmp), Error: func(error) {}}
 	initial, err := packages.TypeCheck(opts, "a", "c")
 	if err != nil {
 		t.Fatal(err)
@@ -356,7 +399,7 @@ func TestTypeCheckError(t *testing.T) {
 	}
 
 	// Check value of constant.
-	aA := all["a"].Type.Scope().Lookup("A").(*types.Const)
+	aA := constant(all["a"], "A")
 	if got, want := aA.String(), `const a.A invalid type`; got != want {
 		t.Errorf("a.A: got %s, want %s", got, want)
 	}
@@ -367,14 +410,13 @@ func TestTypeCheckError(t *testing.T) {
 func TestWholeProgramOverlay(t *testing.T) {
 	type M = map[string]string
 
-	tmp, cleanup := enterTree(t, M{
+	tmp, cleanup := makeTree(t, M{
 		"src/a/a.go": `package a; import "b"; const A = "a" + b.B`,
 		"src/b/b.go": `package b; import "c"; const B = "b" + c.C`,
 		"src/c/c.go": `package c; const C = "c"`,
 		"src/d/d.go": `package d; const D = "d"`,
 	})
 	defer cleanup()
-	// -- tmp is now the current directory --
 
 	for i, test := range []struct {
 		overlay  M
@@ -401,7 +443,7 @@ func TestWholeProgramOverlay(t *testing.T) {
 		}
 		var errs errCollector
 		opts := &packages.Options{
-			GOPATH:    tmp,
+			Env:       append(os.Environ(), "GOPATH="+tmp),
 			Error:     errs.add,
 			ParseFile: parseFile,
 		}
@@ -413,7 +455,7 @@ func TestWholeProgramOverlay(t *testing.T) {
 
 		// Check value of a.A.
 		a := initial[0]
-		got := a.Type.Scope().Lookup("A").(*types.Const).Val().String()
+		got := constant(a, "A").Val().String()
 		if got != test.want {
 			t.Errorf("%d. a.A: got %s, want %s", i, got, test.want)
 		}
@@ -425,7 +467,7 @@ func TestWholeProgramOverlay(t *testing.T) {
 }
 
 func TestWholeProgramImportErrors(t *testing.T) {
-	tmp, cleanup := enterTree(t, map[string]string{
+	tmp, cleanup := makeTree(t, map[string]string{
 		"src/unicycle/unicycle.go": `package unicycle; import _ "unicycle"`,
 		"src/bicycle1/bicycle1.go": `package bicycle1; import _ "bicycle2"`,
 		"src/bicycle2/bicycle2.go": `package bicycle2; import _ "bicycle1"`,
@@ -440,12 +482,14 @@ import (
 )`,
 	})
 	defer cleanup()
-	// -- tmp is now the current directory --
 
-	os.Mkdir("src/empty", 0777) // create an existing but empty package
+	os.Mkdir(filepath.Join(tmp, "src/empty"), 0777) // create an existing but empty package
 
 	var errs2 errCollector
-	opts := &packages.Options{GOPATH: tmp, Error: errs2.add}
+	opts := &packages.Options{
+		Env:   append(os.Environ(), "GOPATH="+tmp),
+		Error: errs2.add,
+	}
 	initial, err := packages.WholeProgram(opts, "root")
 	if err != nil {
 		t.Fatal(err)
@@ -519,8 +563,6 @@ func errorMessages(errors []error) []string {
 }
 
 func srcs(p *packages.Package) (basenames []string) {
-	// Ideally we would show the root-relative portion (e.g. after
-	// src/) but vgo doesn't necessarily have a src/ dir.
 	for i, src := range p.Srcs {
 		if strings.Contains(src, ".cache/go-build") {
 			src = fmt.Sprintf("%d.go", i) // make cache names predictable
@@ -600,35 +642,25 @@ func importGraph(initial []*packages.Package) (string, map[string]*packages.Pack
 
 const skipCleanup = false // for debugging; don't commit 'true'!
 
-// enterTree creates a new temporary directory containing the specified
+// makeTree creates a new temporary directory containing the specified
 // file tree, and chdirs to it. Call the cleanup function to restore the
 // cwd and delete the tree.
-func enterTree(t *testing.T, tree map[string]string) (dir string, cleanup func()) {
+func makeTree(t *testing.T, tree map[string]string) (dir string, cleanup func()) {
-	oldcwd, err := os.Getwd()
+	dir, err := ioutil.TempDir("", "")
-	if err != nil {
-		t.Fatal(err)
-	}
-	dir, err = ioutil.TempDir("", "")
 	if err != nil {
 		t.Fatal(err)
 	}
 
 	cleanup = func() {
-		if err := os.Chdir(oldcwd); err != nil {
-			t.Errorf("cannot restore cwd: %v", err)
-		}
 		if skipCleanup {
 			t.Logf("Skipping cleanup of temp dir: %s", dir)
-		} else {
+			return
-			os.RemoveAll(dir) // ignore errors
 		}
-	}
+		os.RemoveAll(dir) // ignore errors
-
-	if err := os.Chdir(dir); err != nil {
-		t.Fatalf("chdir: %v", err)
 	}
 
 	for name, content := range tree {
+		name := filepath.Join(dir, name)
 		if err := os.MkdirAll(filepath.Dir(name), 0777); err != nil {
 			cleanup()
 			t.Fatal(err)
@@ -640,3 +672,7 @@ func enterTree(t *testing.T, tree map[string]string) (dir string, cleanup func()
 	}
 	return dir, cleanup
 }
+
+func constant(p *packages.Package, name string) *types.Const {
+	return p.Type.Scope().Lookup(name).(*types.Const)
+}