Stop using select in srcs (#152)
Platform-specfic srcs, separated with select expressions, have caused
a few problems. If the goos and goarch attributes of go_binary are
used, the wrong set of sources may be compiled. rules like go_path
will get filtered lists of sources without any way to get the
unfiltered list.
Separating sources is largely unnecessary, since rules_go also filters
sources at execution time. Therefore, this separation is being removed
by Gazelle.
* Rules generated by gazelle will now have flat srcs lists.
* Existing srcs lists will be flattened by "gazelle update" before
merging. Sources that appear multiple times will be de-duplicated,
and comments will be consolidated.
Note that Gazelle still filters sources, it just doesn't generate select
expresions for them anymore. Unbuildable sources (e.g., with
+build ignore) will not appear in any list. Dependencies and flags
will still be platform-specific.
Fixes #134
diff --git a/cmd/gazelle/integration_test.go b/cmd/gazelle/integration_test.go
index f9af161..cff354b 100644
--- a/cmd/gazelle/integration_test.go
+++ b/cmd/gazelle/integration_test.go
@@ -159,10 +159,10 @@
name = "go_default_library",
srcs = select({
"@io_bazel_rules_go//go/platform:linux": [
- # top comment
- "foo.go", # side comment
- # bar comment
- "bar.go",
+ # foo comment
+ "foo.go", # side comment
+ # bar comment
+ "bar.go",
],
"//conditions:default": [],
}),
@@ -198,15 +198,12 @@
go_library(
name = "go_default_library",
- srcs = select({
- "@io_bazel_rules_go//go/platform:linux": [
- # top comment
- # bar comment
- "bar.go",
- "foo.go", # side comment
- ],
- "//conditions:default": [],
- }),
+ srcs = [
+ # bar comment
+ "bar.go",
+ # foo comment
+ "foo.go", # side comment
+ ],
importpath = "example.com/foo",
visibility = ["//visibility:public"],
deps = select({
diff --git a/internal/merger/fix.go b/internal/merger/fix.go
index 1c548c0..969eaa3 100644
--- a/internal/merger/fix.go
+++ b/internal/merger/fix.go
@@ -43,6 +43,7 @@
migrateLibraryEmbed(c, f)
migrateGrpcCompilers(c, f)
removeBinaryImportPath(c, f)
+ flattenSrcs(c, f)
squashCgoLibrary(c, f)
removeLegacyProto(c, f)
}
@@ -346,6 +347,114 @@
f.Stmt = deleteIndices(f.Stmt, deletedIndices)
}
+// flattenSrcs transforms srcs attributes structured as concatenations of
+// lists and selects (generated from PlatformStrings; see
+// extractPlatformStringsExprs for matching details) into a sorted,
+// de-duplicated list. Comments are accumulated and de-duplicated across
+// duplicate expressions.
+func flattenSrcs(c *config.Config, f *bf.File) {
+ for _, stmt := range f.Stmt {
+ call, ok := stmt.(*bf.CallExpr)
+ if !ok {
+ continue
+ }
+ rule := bf.Rule{Call: call}
+ if !isGoRule(rule.Kind()) {
+ continue
+ }
+ oldSrcs := rule.Attr("srcs")
+ if oldSrcs == nil {
+ continue
+ }
+ flatSrcs := flattenSrcsExpr(oldSrcs)
+ rule.SetAttr("srcs", flatSrcs)
+ }
+}
+
+func flattenSrcsExpr(oldSrcs bf.Expr) bf.Expr {
+ oldExprs, err := extractPlatformStringsExprs(oldSrcs)
+ if err != nil {
+ return oldSrcs
+ }
+
+ unique := make(map[string]*bf.StringExpr)
+ type elemComment struct{ elem, com string }
+ seenComments := make(map[elemComment]bool)
+ addElem := func(e bf.Expr) bool {
+ s, ok := e.(*bf.StringExpr)
+ if !ok {
+ return false
+ }
+ sCopy, ok := unique[s.Value]
+ if !ok {
+ // Make a copy of s. We may modify it when we consolidate comments from
+ // duplicate strings. We don't want to modify the original in case this
+ // function fails (due to a later failed pattern match).
+ sCopy = new(bf.StringExpr)
+ *sCopy = *s
+ sCopy.Comments.Before = make([]bf.Comment, 0, len(s.Comments.Before))
+ sCopy.Comments.Suffix = make([]bf.Comment, 0, len(s.Comments.Suffix))
+ unique[s.Value] = sCopy
+ }
+ for _, c := range s.Comment().Before {
+ if key := (elemComment{s.Value, c.Token}); !seenComments[key] {
+ sCopy.Comments.Before = append(sCopy.Comments.Before, c)
+ seenComments[key] = true
+ }
+ }
+ for _, c := range s.Comment().Suffix {
+ if key := (elemComment{s.Value, c.Token}); !seenComments[key] {
+ sCopy.Comments.Suffix = append(sCopy.Comments.Suffix, c)
+ seenComments[key] = true
+ }
+ }
+ return true
+ }
+ addList := func(e bf.Expr) bool {
+ l, ok := e.(*bf.ListExpr)
+ if !ok {
+ return false
+ }
+ for _, elem := range l.List {
+ if !addElem(elem) {
+ return false
+ }
+ }
+ return true
+ }
+ addDict := func(d *bf.DictExpr) bool {
+ for _, kv := range d.List {
+ if !addList(kv.(*bf.KeyValueExpr).Value) {
+ return false
+ }
+ }
+ return true
+ }
+
+ if oldExprs.generic != nil {
+ if !addList(oldExprs.generic) {
+ return oldSrcs
+ }
+ }
+ for _, d := range []*bf.DictExpr{oldExprs.os, oldExprs.arch, oldExprs.platform} {
+ if d == nil {
+ continue
+ }
+ if !addDict(d) {
+ return oldSrcs
+ }
+ }
+
+ sortedExprs := make([]bf.Expr, 0, len(unique))
+ for _, e := range unique {
+ sortedExprs = append(sortedExprs, e)
+ }
+ sort.Slice(sortedExprs, func(i, j int) bool {
+ return sortedExprs[i].(*bf.StringExpr).Value < sortedExprs[j].(*bf.StringExpr).Value
+ })
+ return &bf.ListExpr{List: sortedExprs}
+}
+
// FixLoads removes loads of unused go rules and adds loads of newly used rules.
// This should be called after FixFile and MergeFile, since symbols
// may be introduced that aren't loaded.
diff --git a/internal/merger/fix_test.go b/internal/merger/fix_test.go
index 9b5a901..125777d 100644
--- a/internal/merger/fix_test.go
+++ b/internal/merger/fix_test.go
@@ -123,6 +123,40 @@
)
`,
},
+ // flattenSrcs tests
+ {
+ desc: "flatten srcs",
+ old: `load("@io_bazel_rules_go//go:def.bzl", "go_library")
+
+go_library(
+ name = "go_default_library",
+ srcs = [
+ "gen.go",
+ ] + select({
+ "@io_bazel_rules_go//platform:darwin_amd64": [
+ # darwin
+ "foo.go", # keep
+ ],
+ "@io_bazel_rules_go//platform:linux_amd64": [
+ # linux
+ "foo.go", # keep
+ ],
+ }),
+)
+`,
+ want: `load("@io_bazel_rules_go//go:def.bzl", "go_library")
+
+go_library(
+ name = "go_default_library",
+ srcs = [
+ # darwin
+ # linux
+ "foo.go", # keep
+ "gen.go",
+ ],
+)
+`,
+ },
// squashCgoLibrary tests
{
desc: "no cgo_library",
diff --git a/internal/packages/package.go b/internal/packages/package.go
index 1e38d37..852fb77 100644
--- a/internal/packages/package.go
+++ b/internal/packages/package.go
@@ -129,6 +129,34 @@
return len(ps.Generic) == 0 && len(ps.OS) == 0 && len(ps.Arch) == 0 && len(ps.Platform) == 0
}
+func (ps *PlatformStrings) Flat() []string {
+ unique := make(map[string]struct{})
+ for _, s := range ps.Generic {
+ unique[s] = struct{}{}
+ }
+ for _, ss := range ps.OS {
+ for _, s := range ss {
+ unique[s] = struct{}{}
+ }
+ }
+ for _, ss := range ps.Arch {
+ for _, s := range ss {
+ unique[s] = struct{}{}
+ }
+ }
+ for _, ss := range ps.Platform {
+ for _, s := range ss {
+ unique[s] = struct{}{}
+ }
+ }
+ flat := make([]string, 0, len(unique))
+ for s := range unique {
+ flat = append(flat, s)
+ }
+ sort.Strings(flat)
+ return flat
+}
+
func (ps *PlatformStrings) firstGoFile() string {
for _, f := range ps.Generic {
if strings.HasSuffix(f, ".go") {
diff --git a/internal/rules/generator.go b/internal/rules/generator.go
index 6f15ba8..76e7641 100644
--- a/internal/rules/generator.go
+++ b/internal/rules/generator.go
@@ -223,7 +223,7 @@
func (g *Generator) commonAttrs(pkgRel, name, visibility string, target packages.GoTarget) []KeyValue {
attrs := []KeyValue{{"name", name}}
if !target.Sources.IsEmpty() {
- attrs = append(attrs, KeyValue{"srcs", target.Sources})
+ attrs = append(attrs, KeyValue{"srcs", target.Sources.Flat()})
}
if target.Cgo {
attrs = append(attrs, KeyValue{"cgo", true})
diff --git a/internal/rules/generator_test.go b/internal/rules/generator_test.go
index 4bcc607..2b71ff4 100644
--- a/internal/rules/generator_test.go
+++ b/internal/rules/generator_test.go
@@ -75,36 +75,30 @@
}
for _, dir := range dirs {
- rel, err := filepath.Rel(repoRoot, dir)
- if err != nil {
- t.Fatal(err)
- }
+ rel, _ := filepath.Rel(repoRoot, dir)
+ t.Run(rel, func(t *testing.T) {
+ c, pkg, oldFile := packageFromDir(c, dir)
+ g := rules.NewGenerator(c, l, oldFile)
+ rs, _, err := g.GenerateRules(pkg)
+ if err != nil {
+ t.Fatal(err)
+ }
+ f := &bf.File{Stmt: rs}
+ rules.SortLabels(f)
+ merger.FixLoads(f)
+ got := string(bf.Format(f))
- c, pkg, oldFile := packageFromDir(c, dir)
- if pkg == nil {
- t.Fatalf("%s: no package produced", dir)
- }
- g := rules.NewGenerator(c, l, oldFile)
- rs, _, err := g.GenerateRules(pkg)
- if err != nil {
- t.Fatal(err)
- }
- f := &bf.File{Stmt: rs}
- rules.SortLabels(f)
- merger.FixLoads(f)
- got := string(bf.Format(f))
+ wantPath := filepath.Join(pkg.Dir, "BUILD.want")
+ wantBytes, err := ioutil.ReadFile(wantPath)
+ if err != nil {
+ t.Fatalf("error reading %s: %v", wantPath, err)
+ }
+ want := string(wantBytes)
- wantPath := filepath.Join(pkg.Dir, "BUILD.want")
- wantBytes, err := ioutil.ReadFile(wantPath)
- if err != nil {
- t.Errorf("error reading %s: %v", wantPath, err)
- continue
- }
- want := string(wantBytes)
-
- if got != want {
- t.Errorf("g.Generate(%q, %#v) = %s; want %s", rel, pkg, got, want)
- }
+ if got != want {
+ t.Errorf("g.Generate(%q, %#v) = %s; want %s", rel, pkg, got, want)
+ }
+ })
}
}
diff --git a/internal/rules/testdata/repo/cgolib_with_build_tags/BUILD.want b/internal/rules/testdata/repo/cgolib_with_build_tags/BUILD.want
index eda123d..cf48abb 100644
--- a/internal/rules/testdata/repo/cgolib_with_build_tags/BUILD.want
+++ b/internal/rules/testdata/repo/cgolib_with_build_tags/BUILD.want
@@ -3,66 +3,15 @@
go_library(
name = "go_default_library",
srcs = [
+ "asm_linux.S",
+ "asm_other.S",
"foo.go",
"foo.h",
- ] + select({
- "@io_bazel_rules_go//go/platform:android": [
- "asm_other.S",
- "foo_other.c",
- "pure_other.go",
- ],
- "@io_bazel_rules_go//go/platform:darwin": [
- "asm_other.S",
- "foo_other.c",
- "pure_other.go",
- ],
- "@io_bazel_rules_go//go/platform:dragonfly": [
- "asm_other.S",
- "foo_other.c",
- "pure_other.go",
- ],
- "@io_bazel_rules_go//go/platform:freebsd": [
- "asm_other.S",
- "foo_other.c",
- "pure_other.go",
- ],
- "@io_bazel_rules_go//go/platform:linux": [
- "asm_linux.S",
- "foo_linux.c",
- "pure_linux.go",
- ],
- "@io_bazel_rules_go//go/platform:nacl": [
- "asm_other.S",
- "foo_other.c",
- "pure_other.go",
- ],
- "@io_bazel_rules_go//go/platform:netbsd": [
- "asm_other.S",
- "foo_other.c",
- "pure_other.go",
- ],
- "@io_bazel_rules_go//go/platform:openbsd": [
- "asm_other.S",
- "foo_other.c",
- "pure_other.go",
- ],
- "@io_bazel_rules_go//go/platform:plan9": [
- "asm_other.S",
- "foo_other.c",
- "pure_other.go",
- ],
- "@io_bazel_rules_go//go/platform:solaris": [
- "asm_other.S",
- "foo_other.c",
- "pure_other.go",
- ],
- "@io_bazel_rules_go//go/platform:windows": [
- "asm_other.S",
- "foo_other.c",
- "pure_other.go",
- ],
- "//conditions:default": [],
- }),
+ "foo_linux.c",
+ "foo_other.c",
+ "pure_linux.go",
+ "pure_other.go",
+ ],
_gazelle_imports = [
"example.com/repo/lib",
"fmt",
diff --git a/internal/rules/testdata/repo/gen_and_exclude/BUILD.want b/internal/rules/testdata/repo/gen_and_exclude/BUILD.want
index 226f45f..f6a3ea0 100644
--- a/internal/rules/testdata/repo/gen_and_exclude/BUILD.want
+++ b/internal/rules/testdata/repo/gen_and_exclude/BUILD.want
@@ -4,16 +4,10 @@
name = "go_default_library",
srcs = [
"gen.go",
+ "gen_linux.go",
+ "gen_static_darwin.go",
"static.go",
- ] + select({
- "@io_bazel_rules_go//go/platform:darwin": [
- "gen_static_darwin.go",
- ],
- "@io_bazel_rules_go//go/platform:linux": [
- "gen_linux.go",
- ],
- "//conditions:default": [],
- }),
+ ],
_gazelle_imports = select({
"@io_bazel_rules_go//go/platform:darwin": [
"github.com/jr_hacker/darwin",
diff --git a/internal/rules/testdata/repo/platforms/BUILD.want b/internal/rules/testdata/repo/platforms/BUILD.want
index 9f9a0f7..047e850 100644
--- a/internal/rules/testdata/repo/platforms/BUILD.want
+++ b/internal/rules/testdata/repo/platforms/BUILD.want
@@ -5,31 +5,19 @@
srcs = [
"cgo_generic.c",
"cgo_generic.go",
+ "cgo_linux.c",
+ "cgo_linux.go",
"generic.go",
"no_cgo.go",
"release.go",
- ] + select({
- "@io_bazel_rules_go//go/platform:darwin": [
- "suffix_darwin.go",
- "tag_d.go",
- ],
- "@io_bazel_rules_go//go/platform:linux": [
- "cgo_linux.c",
- "cgo_linux.go",
- "suffix_linux.go",
- "tag_l.go",
- ],
- "//conditions:default": [],
- }) + select({
- "@io_bazel_rules_go//go/platform:amd64": [
- "suffix_amd64.go",
- "tag_a.go",
- ],
- "@io_bazel_rules_go//go/platform:arm": [
- "suffix_arm.go",
- ],
- "//conditions:default": [],
- }),
+ "suffix_amd64.go",
+ "suffix_arm.go",
+ "suffix_darwin.go",
+ "suffix_linux.go",
+ "tag_a.go",
+ "tag_d.go",
+ "tag_l.go",
+ ],
_gazelle_imports = [
"example.com/repo/platforms/generic",
] + select({
@@ -58,10 +46,6 @@
name = "go_default_xtest",
srcs = [
"generic_test.go",
- ] + select({
- "@io_bazel_rules_go//go/platform:linux": [
- "suffix_linux_test.go",
- ],
- "//conditions:default": [],
- }),
+ "suffix_linux_test.go",
+ ],
)
