diff --git a/packages/fileinfo.go b/packages/fileinfo.go index f08804016..145d0a2a3 100644 --- a/packages/fileinfo.go +++ b/packages/fileinfo.go @@ -40,7 +40,6 @@ type fileInfo struct { // This may be read from a package comment (in Go) or a go_package // option (in proto). This field is empty for files that don't specify // an import path. - // TODO(#874): extract from Go files importPath string // category is the type of file, based on extension. diff --git a/packages/fileinfo_go.go b/packages/fileinfo_go.go index 9d343c67b..ec10120ee 100644 --- a/packages/fileinfo_go.go +++ b/packages/fileinfo_go.go @@ -37,6 +37,7 @@ import ( // If the file can't be read, an error will be logged, and partial information // will be returned. // This function is intended to match go/build.Context.Import. +// TODD(#53): extract canonical import path func goFileInfo(c *config.Config, dir, rel, name string) fileInfo { info := fileNameInfo(dir, rel, name) fset := token.NewFileSet() diff --git a/packages/fileinfo_go_test.go b/packages/fileinfo_go_test.go index d9b977f9a..6f52fbaec 100644 --- a/packages/fileinfo_go_test.go +++ b/packages/fileinfo_go_test.go @@ -357,12 +357,16 @@ import "C" if err := ioutil.WriteFile(goFile, content, 0644); err != nil { t.Fatal(err) } - c := &config.Config{RepoRoot: repo} + c := &config.Config{ + RepoRoot: repo, + GoPrefix: "example.com/repo", + } got := buildPackage(c, sub, "sub", []string{"sub.go"}, nil, nil, false) want := &Package{ - Name: "sub", - Dir: sub, - Rel: "sub", + Name: "sub", + Dir: sub, + Rel: "sub", + ImportPath: "example.com/repo/sub", Library: GoTarget{ Sources: PlatformStrings{ Generic: []string{"sub.go"}, diff --git a/packages/package.go b/packages/package.go index d7ed47f3c..7c1b73517 100644 --- a/packages/package.go +++ b/packages/package.go @@ -41,6 +41,9 @@ type Package struct { // Components in Rel are separated with slashes. Rel string + // ImportPath is the string used to import this package in Go. + ImportPath string + Library, Binary, Test, XTest GoTarget Proto ProtoTarget @@ -95,18 +98,6 @@ func (p *Package) IsCommand() bool { return p.Name == "main" } -// ImportPath returns the inferred Go import path for this package. -// TODO(jayconrod): extract canonical import paths from comments on -// package statements. -func (p *Package) ImportPath(c *config.Config) string { - if p.Rel == c.GoPrefixRel { - return c.GoPrefix - } else { - fromPrefixRel := strings.TrimPrefix(p.Rel, c.GoPrefixRel+"/") - return path.Join(c.GoPrefix, fromPrefixRel) - } -} - func (t *GoTarget) HasGo() bool { return t.Sources.HasGo() } @@ -158,6 +149,7 @@ type packageBuilder struct { library, binary, test, xtest goTargetBuilder proto protoTargetBuilder hasTestdata bool + importPath, importPathFile string } type goTargetBuilder struct { @@ -224,6 +216,15 @@ func (pb *packageBuilder) addFile(c *config.Config, info fileInfo, cgo bool) err pb.proto.hasPbGo = true } + if info.importPath != "" { + if pb.importPath == "" { + pb.importPath = info.importPath + pb.importPathFile = info.path + } else if pb.importPath != info.importPath { + return fmt.Errorf("found import comments %q (%s) and %q (%s)", pb.importPath, pb.importPathFile, info.importPath, info.path) + } + } + return nil } @@ -256,11 +257,28 @@ func (pb *packageBuilder) firstGoFile() string { return "" } +func (pb *packageBuilder) inferImportPath(c *config.Config) error { + if pb.importPath != "" { + log.Panic("importPath already set") + } + if pb.rel == c.GoPrefixRel { + if c.GoPrefix == "" { + return fmt.Errorf("in directory %q, prefix is empty, so importpath would be empty for rules. Set a prefix with a '# gazelle:prefix' comment or with -go_prefix on the command line.", pb.dir) + } + pb.importPath = c.GoPrefix + } else { + fromPrefixRel := strings.TrimPrefix(pb.rel, c.GoPrefixRel+"/") + pb.importPath = path.Join(c.GoPrefix, fromPrefixRel) + } + return nil +} + func (pb *packageBuilder) build() *Package { return &Package{ Name: pb.name, Dir: pb.dir, Rel: pb.rel, + ImportPath: pb.importPath, Library: pb.library.build(), Binary: pb.binary.build(), Test: pb.test.build(), diff --git a/packages/walk.go b/packages/walk.go index 161ab2fb8..3659395de 100644 --- a/packages/walk.go +++ b/packages/walk.go @@ -265,14 +265,15 @@ func buildPackage(c *config.Config, dir, rel string, pkgFiles, otherFiles, genFi // or I/O errors. We should keep the file in the srcs list and let the // compiler deal with the error. for _, info := range pkgFilesWithUnknownPackage { - pkg.addFile(c, info, cgo) + if err := pkg.addFile(c, info, cgo); err != nil { + log.Print(err) + } } // Process the other static files. for _, file := range otherFiles { info := otherFileInfo(dir, rel, file) - err = pkg.addFile(c, info, cgo) - if err != nil { + if err := pkg.addFile(c, info, cgo); err != nil { log.Print(err) } } @@ -292,12 +293,17 @@ func buildPackage(c *config.Config, dir, rel string, pkgFiles, otherFiles, genFi continue } info := fileNameInfo(dir, rel, f) - err := pkg.addFile(c, info, cgo) - if err != nil { + if err := pkg.addFile(c, info, cgo); err != nil { log.Print(err) } } + if pkg.importPath == "" { + if err := pkg.inferImportPath(c); err != nil { + log.Print(err) + return nil + } + } return pkg.build() } diff --git a/packages/walk_test.go b/packages/walk_test.go index 7cb518e6c..a7fe2dfb0 100644 --- a/packages/walk_test.go +++ b/packages/walk_test.go @@ -121,7 +121,8 @@ func TestWalkSimple(t *testing.T) { files := []fileSpec{{path: "lib.go", content: "package lib"}} want := []*packages.Package{ { - Name: "lib", + Name: "lib", + ImportPath: "example.com/repo", Library: packages.GoTarget{ Sources: packages.PlatformStrings{ Generic: []string{"lib.go"}, @@ -129,7 +130,7 @@ func TestWalkSimple(t *testing.T) { }, }, } - checkFiles(t, files, "", want) + checkFiles(t, files, "example.com/repo", want) } func TestWalkNested(t *testing.T) { @@ -140,8 +141,9 @@ func TestWalkNested(t *testing.T) { } want := []*packages.Package{ { - Name: "a", - Rel: "a", + Name: "a", + Rel: "a", + ImportPath: "example.com/repo/a", Library: packages.GoTarget{ Sources: packages.PlatformStrings{ Generic: []string{"foo.go"}, @@ -149,8 +151,9 @@ func TestWalkNested(t *testing.T) { }, }, { - Name: "c", - Rel: "b/c", + Name: "c", + Rel: "b/c", + ImportPath: "example.com/repo/b/c", Library: packages.GoTarget{ Sources: packages.PlatformStrings{ Generic: []string{"bar.go"}, @@ -158,8 +161,9 @@ func TestWalkNested(t *testing.T) { }, }, { - Name: "main", - Rel: "b/d", + Name: "main", + Rel: "b/d", + ImportPath: "example.com/repo/b/d", Library: packages.GoTarget{ Sources: packages.PlatformStrings{ Generic: []string{"baz.go"}, @@ -167,7 +171,7 @@ func TestWalkNested(t *testing.T) { }, }, } - checkFiles(t, files, "", want) + checkFiles(t, files, "example.com/repo", want) } func TestProtoOnly(t *testing.T) { @@ -176,8 +180,9 @@ func TestProtoOnly(t *testing.T) { } want := []*packages.Package{ { - Name: "a", - Rel: "a", + Name: "a", + Rel: "a", + ImportPath: "example.com/repo/a", Proto: packages.ProtoTarget{ Sources: packages.PlatformStrings{ Generic: []string{"a.proto"}, @@ -185,7 +190,7 @@ func TestProtoOnly(t *testing.T) { }, }, } - checkFiles(t, files, "", want) + checkFiles(t, files, "example.com/repo", want) } func TestMultiplePackagesWithDefault(t *testing.T) { @@ -195,8 +200,9 @@ func TestMultiplePackagesWithDefault(t *testing.T) { } want := []*packages.Package{ { - Name: "a", - Rel: "a", + Name: "a", + Rel: "a", + ImportPath: "example.com/repo/a", Library: packages.GoTarget{ Sources: packages.PlatformStrings{ Generic: []string{"a.go"}, @@ -204,7 +210,7 @@ func TestMultiplePackagesWithDefault(t *testing.T) { }, }, } - checkFiles(t, files, "", want) + checkFiles(t, files, "example.com/repo", want) } func TestMultiplePackagesWithoutDefault(t *testing.T) { @@ -238,8 +244,9 @@ package a; } want := []*packages.Package{ { - Name: "a", - Rel: "a", + Name: "a", + Rel: "a", + ImportPath: "example.com/repo/a", Proto: packages.ProtoTarget{ Sources: packages.PlatformStrings{ Generic: []string{"a.proto"}, @@ -247,7 +254,7 @@ package a; }, }, } - checkFiles(t, files, "", want) + checkFiles(t, files, "example.com/repo", want) } func TestRootWithPrefix(t *testing.T) { @@ -257,7 +264,8 @@ func TestRootWithPrefix(t *testing.T) { } want := []*packages.Package{ { - Name: "a", + Name: "a", + ImportPath: "example.com/a", Library: packages.GoTarget{ Sources: packages.PlatformStrings{ Generic: []string{"a.go"}, @@ -265,7 +273,7 @@ func TestRootWithPrefix(t *testing.T) { }, }, } - checkFiles(t, files, "github.com/a", want) + checkFiles(t, files, "example.com/a", want) } func TestRootWithoutPrefix(t *testing.T) { @@ -324,6 +332,33 @@ func TestVendorResetsPrefix(t *testing.T) { } } +func TestPrefixEmpty(t *testing.T) { + files := []fileSpec{ + {path: "a.go", content: "package foo"}, + } + want := []*packages.Package{} + checkFiles(t, files, "", want) +} + +func TestProtoImportPath(t *testing.T) { + files := []fileSpec{{ + path: "foo.proto", + content: `syntax = "proto3"; +option go_package = "example.com/repo/foo"; +`, + }} + want := []*packages.Package{{ + Name: "foo", + ImportPath: "example.com/repo/foo", + Proto: packages.ProtoTarget{ + Sources: packages.PlatformStrings{ + Generic: []string{"foo.proto"}, + }, + }, + }} + checkFiles(t, files, "", want) +} + func TestTestdata(t *testing.T) { files := []fileSpec{ {path: "raw/testdata/"}, @@ -339,8 +374,9 @@ func TestTestdata(t *testing.T) { } want := []*packages.Package{ { - Name: "raw", - Rel: "raw", + Name: "raw", + Rel: "raw", + ImportPath: "example.com/repo/raw", Library: packages.GoTarget{ Sources: packages.PlatformStrings{ Generic: []string{"a.go"}, @@ -349,8 +385,9 @@ func TestTestdata(t *testing.T) { HasTestdata: true, }, { - Name: "with_build", - Rel: "with_build", + Name: "with_build", + Rel: "with_build", + ImportPath: "example.com/repo/with_build", Library: packages.GoTarget{ Sources: packages.PlatformStrings{ Generic: []string{"a.go"}, @@ -359,8 +396,9 @@ func TestTestdata(t *testing.T) { HasTestdata: false, }, { - Name: "with_build_bazel", - Rel: "with_build_bazel", + Name: "with_build_bazel", + Rel: "with_build_bazel", + ImportPath: "example.com/repo/with_build_bazel", Library: packages.GoTarget{ Sources: packages.PlatformStrings{ Generic: []string{"a.go"}, @@ -369,8 +407,9 @@ func TestTestdata(t *testing.T) { HasTestdata: false, }, { - Name: "with_build_nested", - Rel: "with_build_nested", + Name: "with_build_nested", + Rel: "with_build_nested", + ImportPath: "example.com/repo/with_build_nested", Library: packages.GoTarget{ Sources: packages.PlatformStrings{ Generic: []string{"a.go"}, @@ -379,8 +418,9 @@ func TestTestdata(t *testing.T) { HasTestdata: false, }, { - Name: "testdata", - Rel: "with_go/testdata", + Name: "testdata", + Rel: "with_go/testdata", + ImportPath: "example.com/repo/with_go/testdata", Library: packages.GoTarget{ Sources: packages.PlatformStrings{ Generic: []string{"a.go"}, @@ -388,8 +428,9 @@ func TestTestdata(t *testing.T) { }, }, { - Name: "with_go", - Rel: "with_go", + Name: "with_go", + Rel: "with_go", + ImportPath: "example.com/repo/with_go", Library: packages.GoTarget{ Sources: packages.PlatformStrings{ Generic: []string{"a.go"}, @@ -398,7 +439,7 @@ func TestTestdata(t *testing.T) { HasTestdata: false, }, } - checkFiles(t, files, "", want) + checkFiles(t, files, "example.com/repo", want) } func TestGenerated(t *testing.T) { @@ -427,8 +468,9 @@ import "github.com/jr_hacker/stuff" } want := []*packages.Package{ { - Name: "foo", - Rel: "gen", + Name: "foo", + Rel: "gen", + ImportPath: "example.com/repo/gen", Library: packages.GoTarget{ Sources: packages.PlatformStrings{ Generic: []string{"bar.go", "baz.go", "foo.go", "y.s"}, @@ -439,7 +481,7 @@ import "github.com/jr_hacker/stuff" }, }, } - checkFiles(t, files, "", want) + checkFiles(t, files, "example.com/repo", want) } func TestGeneratedCgo(t *testing.T) { @@ -470,8 +512,9 @@ import "github.com/jr_hacker/stuff" } want := []*packages.Package{ { - Name: "foo", - Rel: "gen", + Name: "foo", + Rel: "gen", + ImportPath: "example.com/repo/gen", Library: packages.GoTarget{ Sources: packages.PlatformStrings{ Generic: []string{"bar.go", "baz.go", "foo.go", "x.c", "y.s", "z.S"}, @@ -483,7 +526,7 @@ import "github.com/jr_hacker/stuff" }, }, } - checkFiles(t, files, "", want) + checkFiles(t, files, "example.com/repo", want) } func TestExcluded(t *testing.T) { @@ -523,8 +566,9 @@ genrule( } want := []*packages.Package{ { - Name: "exclude", - Rel: "exclude", + Name: "exclude", + Rel: "exclude", + ImportPath: "example.com/repo/exclude", Library: packages.GoTarget{ Sources: packages.PlatformStrings{ Generic: []string{"real.go"}, @@ -532,7 +576,7 @@ genrule( }, }, } - checkFiles(t, files, "", want) + checkFiles(t, files, "example.com/repo", want) } func TestExcludedPbGo(t *testing.T) { @@ -565,8 +609,9 @@ package exclude; } want := []*packages.Package{ { - Name: "exclude", - Rel: "exclude", + Name: "exclude", + Rel: "exclude", + ImportPath: "example.com/repo/exclude", Library: packages.GoTarget{ Sources: packages.PlatformStrings{ Generic: []string{"a.pb.go"}, @@ -580,7 +625,7 @@ package exclude; }, }, } - checkFiles(t, files, "", want) + checkFiles(t, files, "example.com/repo", want) } func TestLegacyProtos(t *testing.T) { @@ -610,8 +655,9 @@ package proto_only;`, } want := []*packages.Package{ { - Name: "have_pbgo", - Rel: "have_pbgo", + Name: "have_pbgo", + Rel: "have_pbgo", + ImportPath: "example.com/repo/have_pbgo", Library: packages.GoTarget{ Sources: packages.PlatformStrings{ Generic: []string{"a.pb.go"}, @@ -624,8 +670,9 @@ package proto_only;`, HasPbGo: true, }, }, { - Name: "no_pbgo", - Rel: "no_pbgo", + Name: "no_pbgo", + Rel: "no_pbgo", + ImportPath: "example.com/repo/no_pbgo", Library: packages.GoTarget{ Sources: packages.PlatformStrings{ Generic: []string{"other.go"}, @@ -639,7 +686,7 @@ package proto_only;`, }, }, } - checkFiles(t, files, "", want) + checkFiles(t, files, "example.com/repo", want) } func TestMalformedBuildFile(t *testing.T) { @@ -648,7 +695,7 @@ func TestMalformedBuildFile(t *testing.T) { {path: "foo.go", content: "package foo"}, } want := []*packages.Package{} - checkFiles(t, files, "", want) + checkFiles(t, files, "example.com/repo", want) } func TestMultipleBuildFiles(t *testing.T) { @@ -658,7 +705,7 @@ func TestMultipleBuildFiles(t *testing.T) { {path: "foo.go", content: "package foo"}, } want := []*packages.Package{} - checkFiles(t, files, "", want) + checkFiles(t, files, "example.com/repo", want) } func TestMalformedGoFile(t *testing.T) { @@ -668,7 +715,8 @@ func TestMalformedGoFile(t *testing.T) { } want := []*packages.Package{ { - Name: "foo", + Name: "foo", + ImportPath: "example.com/repo", Library: packages.GoTarget{ Sources: packages.PlatformStrings{ Generic: []string{"a.go", "b.go"}, @@ -676,5 +724,5 @@ func TestMalformedGoFile(t *testing.T) { }, }, } - checkFiles(t, files, "", want) + checkFiles(t, files, "example.com/repo", want) } diff --git a/rules/generator.go b/rules/generator.go index 5e985b41a..c9075ecda 100644 --- a/rules/generator.go +++ b/rules/generator.go @@ -44,10 +44,6 @@ type Generator struct { // GenerateRules generates a list of rules for targets in "pkg". It also returns // a list of empty rules that may be deleted from an existing file. func (g *Generator) GenerateRules(pkg *packages.Package) (rules []bf.Expr, empty []bf.Expr, err error) { - if imp := pkg.ImportPath(g.c); imp == "" { - return nil, nil, fmt.Errorf("in directory %q, prefix is empty, so importpath would be empty for rules. Set a prefix with a '# gazelle:prefix' comment or with -go_prefix on the command line.", pkg.Rel) - } - var rs []bf.Expr protoLibName, protoRules := g.generateProto(pkg) @@ -123,7 +119,7 @@ func (g *Generator) generateProto(pkg *packages.Package) (string, []bf.Expr) { goProtoAttrs := []KeyValue{ {"name", goProtoName}, {"proto", ":" + protoName}, - {"importpath", pkg.ImportPath(g.c)}, + {"importpath", pkg.ImportPath}, } if pkg.Proto.HasServices { goProtoAttrs = append(goProtoAttrs, KeyValue{"compilers", []string{"@io_bazel_rules_go//proto:go_grpc"}}) @@ -148,7 +144,7 @@ func (g *Generator) generateBin(pkg *packages.Package, library string) bf.Expr { attrs := g.commonAttrs(pkg.Rel, name, visibility, pkg.Binary) // TODO(jayconrod): don't add importpath if it can be inherited from library. // This is blocked by bazelbuild/bazel#3575. - attrs = append(attrs, KeyValue{"importpath", pkg.ImportPath(g.c)}) + attrs = append(attrs, KeyValue{"importpath", pkg.ImportPath}) if library != "" { attrs = append(attrs, KeyValue{"embed", []string{":" + library}}) } @@ -169,7 +165,7 @@ func (g *Generator) generateLib(pkg *packages.Package, goProtoName string) (stri } attrs := g.commonAttrs(pkg.Rel, name, visibility, pkg.Library) - attrs = append(attrs, KeyValue{"importpath", pkg.ImportPath(g.c)}) + attrs = append(attrs, KeyValue{"importpath", pkg.ImportPath}) if goProtoName != "" { attrs = append(attrs, KeyValue{"embed", []string{":" + goProtoName}}) } @@ -209,7 +205,7 @@ func checkInternalVisibility(rel, visibility string) string { func (g *Generator) generateTest(pkg *packages.Package, library string, isXTest bool) bf.Expr { name := g.l.TestLabel(pkg.Rel, isXTest).Name target := pkg.Test - importpath := pkg.ImportPath(g.c) + importpath := pkg.ImportPath if isXTest { target = pkg.XTest importpath += "_test" diff --git a/rules/generator_test.go b/rules/generator_test.go index 201e3c4d9..eda345ec0 100644 --- a/rules/generator_test.go +++ b/rules/generator_test.go @@ -157,24 +157,3 @@ func TestGeneratorEmptyLegacyProto(t *testing.T) { } } } - -func TestErrorWhenImportPathEmpty(t *testing.T) { - repoRoot := filepath.FromSlash("../testdata/repo/lib") - c := testConfig(repoRoot, "") - - var pkg *packages.Package - var oldFile *bf.File - packages.Walk(c, repoRoot, func(rel string, _ *config.Config, p *packages.Package, f *bf.File, _ bool) { - if rel == "" { - pkg = p - oldFile = f - } - }) - - l := resolve.NewLabeler(c) - g := rules.NewGenerator(c, l, oldFile) - _, _, err := g.GenerateRules(pkg) - if err == nil { - t.Error("got success ; want error") - } -}