Gazelle: add importpath attributes to rules (#768)

* go_prefix is no longer generated. In a future change, existing
  go_prefix rules will be removed in fix mode.
* All go_library, go_test, and go_binary rules are generated with an
  importpath attribute.
* importpath is now a mergeable attribute; Gazelle will replace
  existing importpath attributes unless they have a keep comment. This
  helps keep libraries up to date after they are moved to a new
  location.

Related #721

Author: jayconrod

go/tools/gazelle/gazelle/integration_test.go

@@ -141,9 +141,7 @@ func TestSelectLabelsSorted(t *testing.T) {
 		{
 			path: "BUILD",
 			content: `
-load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_prefix")
-
-go_prefix("example.com/foo")
+load("@io_bazel_rules_go//go:def.bzl", "go_library")
 
 go_library(
     name = "go_default_library",
@@ -156,6 +154,7 @@ go_library(
         ],
         "//conditions:default": [],
     }),
+    importpath = "example.com/foo",
 )
 `,
 		},
@@ -183,9 +182,7 @@ package foo
 		{path: "outer/outer.go", content: "package outer"},
 		{path: "outer/inner/inner.go", content: "package inner"},
 	})
-	want := `load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_prefix")
-
-go_prefix("example.com/foo")
+	want := `load("@io_bazel_rules_go//go:def.bzl", "go_library")
 
 go_library(
     name = "go_default_library",
@@ -198,6 +195,7 @@ go_library(
         ],
         "//conditions:default": [],
     }),
+    importpath = "example.com/foo",
     visibility = ["//visibility:public"],
     deps = select({
         "@io_bazel_rules_go//go/platform:linux_amd64": [
@@ -213,7 +211,7 @@ go_library(
 		t.Fatal(err)
 	}
 
-	if err := runGazelle(dir, nil); err != nil {
+	if err := runGazelle(dir, []string{"-go_prefix", "example.com/foo"}); err != nil {
 		t.Fatal(err)
 	}
 	if got, err := ioutil.ReadFile(filepath.Join(dir, "BUILD")); err != nil {
@@ -279,6 +277,7 @@ go_library(
         "pure.go",
     ],
     cgo = True,
+    importpath = "example.com/foo",
     visibility = ["//visibility:default"],
 )
 
@@ -300,6 +299,7 @@ go_library(
         "pure.go",
     ],
     cgo = True,
+    importpath = "example.com/foo",
     visibility = ["//visibility:default"],
 )
 `,
@@ -330,9 +330,7 @@ func TestFixUnlinkedCgoLibrary(t *testing.T) {
 		{path: "WORKSPACE"},
 		{
 			path: "BUILD",
-			content: `load("@io_bazel_rules_go//go:def.bzl", "cgo_library", "go_library", "go_prefix")
-
-go_prefix("example.com/foo")
+			content: `load("@io_bazel_rules_go//go:def.bzl", "cgo_library", "go_library")
 
 cgo_library(
     name = "cgo_default_library",
@@ -342,6 +340,7 @@ cgo_library(
 go_library(
     name = "go_default_library",
     srcs = ["pure.go"],
+    importpath = "example.com/foo",
     visibility = ["//visibility:public"],
 )
 `,
@@ -356,17 +355,16 @@ go_library(
 		t.Fatal(err)
 	}
 
-	want := `load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_prefix")
-
-go_prefix("example.com/foo")
+	want := `load("@io_bazel_rules_go//go:def.bzl", "go_library")
 
 go_library(
     name = "go_default_library",
     srcs = ["pure.go"],
+    importpath = "example.com/foo",
     visibility = ["//visibility:public"],
 )
 `
-	if err := runGazelle(dir, []string{"fix"}); err != nil {
+	if err := runGazelle(dir, []string{"fix", "-go_prefix", "example.com/foo"}); err != nil {
 		t.Fatal(err)
 	}
 	if got, err := ioutil.ReadFile(filepath.Join(dir, "BUILD")); err != nil {
@@ -504,13 +502,12 @@ import _ "golang.org/x/baz"
 	checkFiles(t, dir, []fileSpec{
 		{
 			path: config.DefaultValidBuildFileNames[0],
-			content: `load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_prefix")
-
-go_prefix("example.com/foo")
+			content: `load("@io_bazel_rules_go//go:def.bzl", "go_library")
 
 go_library(
     name = "go_default_library",
     srcs = ["a.go"],
+    importpath = "example.com/foo",
     visibility = ["//visibility:public"],
     deps = ["//vendor/golang.org/x/bar:go_default_library"],
 )
@@ -522,6 +519,7 @@ go_library(
 go_library(
     name = "go_default_library",
     srcs = ["bar.go"],
+    importpath = "golang.org/x/bar",
     visibility = ["//visibility:public"],
     deps = ["//vendor/golang.org/x/baz:go_default_library"],
 )
@@ -539,7 +537,7 @@ func TestFlatExternal(t *testing.T) {
 
 gazelle(
     name = "gazelle",
-    prefix = "example.com/repo",
+    prefix = "example.com/foo",
     args = ["-experimental_flat"],
 )
 `,
@@ -590,16 +588,14 @@ import (
 	checkFiles(t, dir, []fileSpec{
 		{
 			path: config.DefaultValidBuildFileNames[0],
-			content: `load("@io_bazel_rules_go//go:def.bzl", "gazelle", "go_binary", "go_library", "go_prefix", "go_test")
+			content: `load("@io_bazel_rules_go//go:def.bzl", "gazelle", "go_binary", "go_library", "go_test")
 
 gazelle(
     name = "gazelle",
     args = ["-experimental_flat"],
-    prefix = "example.com/repo",
+    prefix = "example.com/foo",
 )
 
-go_prefix("example.com/foo")
-
 go_library(
     name = "foo",
     srcs = ["a.go"],
@@ -618,6 +614,7 @@ go_test(
     name = "b_test",
     srcs = ["b/b_test.go"],
     data = glob(["b/testdata/**"]),
+    importpath = "example.com/foo/b",
     library = ":b",
     rundir = "b",
 )
@@ -626,6 +623,7 @@ go_test(
     name = "b_xtest",
     srcs = ["b/b_x_test.go"],
     data = glob(["b/testdata/**"]),
+    importpath = "example.com/foo/b_test",
     rundir = "b",
     deps = [":b"],
 )
@@ -640,6 +638,7 @@ go_library(
 go_library(
     name = "c",
     srcs = ["c/c.go"],
+    importpath = "example.com/foo/c",
     visibility = ["//visibility:private"],
     deps = [
         ":b",
@@ -651,6 +650,7 @@ go_library(
 
 go_binary(
     name = "c_cmd",
+    importpath = "example.com/foo/c",
     library = ":c",
     visibility = ["//visibility:public"],
 )
@@ -707,7 +707,7 @@ import _ "github.com/jr_hacker/stuff/a/b"
 	checkFiles(t, dir, []fileSpec{
 		{
 			path: config.DefaultValidBuildFileNames[0],
-			content: `load("@io_bazel_rules_go//go:def.bzl", "gazelle", "go_library", "go_prefix")
+			content: `load("@io_bazel_rules_go//go:def.bzl", "gazelle", "go_library")
 
 gazelle(
     name = "gazelle",
@@ -716,8 +716,6 @@ gazelle(
     prefix = "example.com/foo",
 )
 
-go_prefix("example.com/foo")
-
 go_library(
     name = "foo",
     srcs = ["foo.go"],

go/tools/gazelle/gazelle/main.go

@@ -128,17 +128,20 @@ func (v *hierarchicalVisitor) finish() {
 		return
 	}
 
-	// We did not process a package at the repository root. We need to put
-	// a go_prefix rule there, even if there are no .go files in that directory.
-	oldFile, err := loadBuildFile(v.c, v.c.RepoRoot)
-	if err != nil && !os.IsNotExist(err) {
+	// We did not process a package at the repository root. We need to create
+	// a build file if none exists.
+	for _, base := range v.c.ValidBuildFileNames {
+		p := filepath.Join(v.c.RepoRoot, base)
+		if _, err := os.Stat(p); err == nil || !os.IsNotExist(err) {
+			return
+		}
+	}
+	p := filepath.Join(v.c.RepoRoot, v.c.DefaultBuildFileName())
+	if f, err := os.Create(p); err != nil {
 		log.Print(err)
+	} else {
+		f.Close()
 	}
-
-	pkg := &packages.Package{Dir: v.c.RepoRoot}
-	g := rules.NewGenerator(v.c, v.r, v.l, "", oldFile)
-	genFile := g.Generate(pkg)
-	v.mergeAndEmit(genFile, oldFile)
 }
 
 // flatVisitor generates and updates a single build file that contains rules
@@ -178,7 +181,6 @@ func (v *flatVisitor) finish() {
 	sort.Strings(packageNames)
 
 	genFile.Stmt = append(genFile.Stmt, nil) // reserve space for load
-	genFile.Stmt = append(genFile.Stmt, g.GeneratePrefix())
 	for _, name := range packageNames {
 		rs := v.rules[name]
 		genFile.Stmt = append(genFile.Stmt, rs...)

go/tools/gazelle/merger/merger.go

@@ -32,12 +32,13 @@ const (
 
 var (
 	mergeableFields = map[string]bool{
-		"cgo":       true,
-		"clinkopts": true,
-		"copts":     true,
-		"deps":      true,
-		"library":   true,
-		"srcs":      true,
+		"cgo":        true,
+		"clinkopts":  true,
+		"copts":      true,
+		"deps":       true,
+		"importpath": true,
+		"library":    true,
+		"srcs":       true,
 	}
 )
 

go/tools/gazelle/packages/package.go

@@ -81,19 +81,13 @@ func (p *Package) HasGo() bool {
 // ImportPath returns the inferred Go import path for this package. This
 // is determined as follows:
 //
-// * If there is no library target or if Name is "main", "" is returned.
-//   This indicates the library cannot be imported.
 // * If "vendor" is a component in p.Rel, everything after the last "vendor"
 //   component is returned.
 // * Otherwise, prefix joined with Rel is returned.
 //
 // TODO(jayconrod): extract canonical import paths from comments on
 // package statements.
 func (p *Package) ImportPath(prefix string) string {
-	if !p.Library.HasGo() || p.IsCommand() {
-		return ""
-	}
-
 	components := strings.Split(p.Rel, "/")
 	for i := len(components) - 1; i >= 0; i-- {
 		if components[i] == "vendor" {

go/tools/gazelle/packages/package_test.go

@@ -67,8 +67,8 @@ func TestImportPathNoLib(t *testing.T) {
 		Name: "bar",
 		Rel:  "foo/bar",
 	}
-	if got := pkg.ImportPath("example.com/repo"); got != "" {
-		t.Errorf(`got %q; want ""`, got)
+	if got, want := pkg.ImportPath("example.com/repo"), "example.com/repo/foo/bar"; got != want {
+		t.Errorf(`got %q; want %q`, got, want)
 	}
 }
 
@@ -82,8 +82,8 @@ func TestImportPathCmd(t *testing.T) {
 			},
 		},
 	}
-	if got := pkg.ImportPath("example.com/repo"); got != "" {
-		t.Errorf(`got %q; want ""`, got)
+	if got, want := pkg.ImportPath("example.com/repo"), "example.com/repo/foo/bar"; got != want {
+		t.Errorf(`got %q; want %q`, got, want)
 	}
 }
 

go/tools/gazelle/rules/generator.go

@@ -38,10 +38,6 @@ type Generator interface {
 	// in this interface.
 	Generate(pkg *packages.Package) *bf.File
 
-	// GeneratePrefix generates a go_prefix rule. This should be in the
-	// top-level build file for the repository.
-	GeneratePrefix() bf.Expr
-
 	// GenerateRules generates a list of rules for targets in "pkg".
 	GenerateRules(pkg *packages.Package) []bf.Expr
 
@@ -72,18 +68,11 @@ func (g *generator) Generate(pkg *packages.Package) *bf.File {
 		Path: filepath.Join(pkg.Dir, g.c.DefaultBuildFileName()),
 	}
 	f.Stmt = append(f.Stmt, nil) // reserve space for load
-	if pkg.Rel == "" {
-		f.Stmt = append(f.Stmt, g.GeneratePrefix())
-	}
 	f.Stmt = append(f.Stmt, g.GenerateRules(pkg)...)
 	f.Stmt[0] = g.GenerateLoad(f.Stmt[1:])
 	return f
 }
 
-func (g *generator) GeneratePrefix() bf.Expr {
-	return newRule("go_prefix", []interface{}{g.c.GoPrefix}, nil)
-}
-
 func (g *generator) GenerateRules(pkg *packages.Package) []bf.Expr {
 	var rules []bf.Expr
 	library, r := g.generateLib(pkg)
@@ -150,6 +139,9 @@ func (g *generator) generateBin(pkg *packages.Package, library string) bf.Expr {
 	name := g.l.BinaryLabel(pkg.Rel).Name
 	visibility := checkInternalVisibility(pkg.Rel, "//visibility:public")
 	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.GoPrefix)})
 	if library != "" {
 		attrs = append(attrs, keyvalue{"library", ":" + library})
 	}
@@ -170,11 +162,7 @@ func (g *generator) generateLib(pkg *packages.Package) (string, bf.Expr) {
 	}
 
 	attrs := g.commonAttrs(pkg.Rel, name, visibility, pkg.Library)
-	if !pkg.IsCommand() && g.c.StructureMode == config.FlatMode {
-		// TODO(jayconrod): add importpath attributes outside of flat mode after
-		// we have verified it works correctly.
-		attrs = append(attrs, keyvalue{"importpath", pkg.ImportPath(g.c.GoPrefix)})
-	}
+	attrs = append(attrs, keyvalue{"importpath", pkg.ImportPath(g.c.GoPrefix)})
 
 	rule := newRule("go_library", nil, attrs)
 	return name, rule
@@ -224,14 +212,19 @@ func (g *generator) filegroup(pkg *packages.Package) bf.Expr {
 
 func (g *generator) generateTest(pkg *packages.Package, library string, isXTest bool) bf.Expr {
 	target := pkg.Test
+	importpath := pkg.ImportPath(g.c.GoPrefix)
 	if isXTest {
 		target = pkg.XTest
+		importpath += "_test"
 	}
 	if !target.HasGo() {
 		return nil
 	}
 	name := g.l.TestLabel(pkg.Rel, isXTest).Name
 	attrs := g.commonAttrs(pkg.Rel, name, "", target)
+	// 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", importpath})
 	if library != "" {
 		attrs = append(attrs, keyvalue{"library", ":" + library})
 	}