diff --git a/go/packages/doc.go b/go/packages/doc.go index 7e2c221b..952bb48d 100644 --- a/go/packages/doc.go +++ b/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: - Dir string - Environ []string +- Add this pass-through option for the underlying query tool: Flags []string - Do away with GOROOT and don't add GOARCH/GOOS: - they are not portable concepts. - The goal is to allow users to express themselves using the conventions + +- Add GOARCH/GOOS? + 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 diff --git a/go/packages/golist.go b/go/packages/golist.go index 0af3a46d..ff19910b 100644 --- a/go/packages/golist.go +++ b/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 { - cmd.Env = append(cmd.Env, "CGO_ENABLED=0") + + if env == nil { + 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 { diff --git a/go/packages/packages.go b/go/packages/packages.go index 2f2bb85b..54267c85 100644 --- a/go/packages/packages.go +++ b/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, - // but they are openable. + // Names are guaranteed to be absolute. // // 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 - // "openable" in the same sense as are Srcs above. + // contains. This includes assembly and C source files. + // 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 } diff --git a/go/packages/packages_test.go b/go/packages/packages_test.go index dc93b98d..2ce0bc2c 100644 --- a/go/packages/packages_test.go +++ b/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 { - t.Errorf("failed to obtain metadata for ad-hoc package (err=%v)", err) + 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: %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()) { - oldcwd, err := os.Getwd() - if err != nil { - t.Fatal(err) - } - dir, err = ioutil.TempDir("", "") +func makeTree(t *testing.T, tree map[string]string) (dir string, cleanup func()) { + 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 { - os.RemoveAll(dir) // ignore errors + return } - } - - if err := os.Chdir(dir); err != nil { - t.Fatalf("chdir: %v", err) + os.RemoveAll(dir) // ignore errors } 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) +}