Skip to content

Commit

Permalink
Gazelle: add importpath attributes to rules
Browse files Browse the repository at this point in the history
* 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 bazelbuild#721
  • Loading branch information
Jay Conrod committed Aug 29, 2017
1 parent b2abbcf commit ad51f17
Show file tree
Hide file tree
Showing 21 changed files with 82 additions and 114 deletions.
48 changes: 23 additions & 25 deletions go/tools/gazelle/gazelle/integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -156,6 +154,7 @@ go_library(
],
"//conditions:default": [],
}),
importpath = "example.com/foo",
)
`,
},
Expand Down Expand Up @@ -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",
Expand All @@ -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": [
Expand All @@ -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 {
Expand Down Expand Up @@ -279,6 +277,7 @@ go_library(
"pure.go",
],
cgo = True,
importpath = "example.com/foo",
visibility = ["//visibility:default"],
)
Expand All @@ -300,6 +299,7 @@ go_library(
"pure.go",
],
cgo = True,
importpath = "example.com/foo",
visibility = ["//visibility:default"],
)
`,
Expand Down Expand Up @@ -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",
Expand All @@ -342,6 +340,7 @@ cgo_library(
go_library(
name = "go_default_library",
srcs = ["pure.go"],
importpath = "example.com/foo",
visibility = ["//visibility:public"],
)
`,
Expand All @@ -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 {
Expand Down Expand Up @@ -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"],
)
Expand All @@ -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"],
)
Expand All @@ -539,7 +537,7 @@ func TestFlatExternal(t *testing.T) {
gazelle(
name = "gazelle",
prefix = "example.com/repo",
prefix = "example.com/foo",
args = ["-experimental_flat"],
)
`,
Expand Down Expand Up @@ -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"],
Expand All @@ -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",
)
Expand All @@ -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"],
)
Expand All @@ -640,6 +638,7 @@ go_library(
go_library(
name = "c",
srcs = ["c/c.go"],
importpath = "example.com/foo/c",
visibility = ["//visibility:private"],
deps = [
":b",
Expand All @@ -651,6 +650,7 @@ go_library(
go_binary(
name = "c_cmd",
importpath = "example.com/foo/c",
library = ":c",
visibility = ["//visibility:public"],
)
Expand Down Expand Up @@ -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",
Expand All @@ -716,8 +716,6 @@ gazelle(
prefix = "example.com/foo",
)
go_prefix("example.com/foo")
go_library(
name = "foo",
srcs = ["foo.go"],
Expand Down
22 changes: 12 additions & 10 deletions go/tools/gazelle/gazelle/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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...)
Expand Down
13 changes: 7 additions & 6 deletions go/tools/gazelle/merger/merger.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
)

Expand Down
6 changes: 0 additions & 6 deletions go/tools/gazelle/packages/package.go
Original file line number Diff line number Diff line change
Expand Up @@ -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" {
Expand Down
8 changes: 4 additions & 4 deletions go/tools/gazelle/packages/package_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}

Expand All @@ -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)
}
}

Expand Down
25 changes: 9 additions & 16 deletions go/tools/gazelle/rules/generator.go
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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})
}
Expand All @@ -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
Expand Down Expand Up @@ -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})
}
Expand Down
Loading

0 comments on commit ad51f17

Please sign in to comment.