From 1b7c21494ebfbfc18e7af6548a884cd29de9b593 Mon Sep 17 00:00:00 2001 From: Patrick Scott Date: Fri, 27 Sep 2024 12:58:04 -0400 Subject: [PATCH 1/2] Evaluate build tags as both true and false Gazelle already iterates over all supported os/arch combos to include go files. This is similar but expanded to custom tags. When adding a new build tag, we create a copy with the tag set to true and append to the original. This means that build tag evaluation will grow exponentially. --- Design.rst | 4 ++-- README.rst | 10 ++++---- language/go/config.go | 45 +++++++++++++++++++----------------- language/go/config_test.go | 35 +++++++++------------------- language/go/fileinfo.go | 13 +++++++---- language/go/fileinfo_test.go | 31 ++++++++++++------------- 6 files changed, 67 insertions(+), 71 deletions(-) diff --git a/Design.rst b/Design.rst index 5052fb169..4aa9ccd29 100644 --- a/Design.rst +++ b/Design.rst @@ -107,8 +107,8 @@ Here are a few examples. See the `full list of directives`_. * ``# gazelle:prefix`` - sets the Go import path prefix for the current directory. -* ``# gazelle:build_tags`` - sets the list of build tags which Gazelle considers - to be true on all platforms. +* ``# gazelle:build_tags`` - sets the list of build tags which Gazelle will + defer to Bazel for evaluation. There are a few directives which are not applied to the ``Config`` object but are interpreted directly in packages where they are relevant. diff --git a/README.rst b/README.rst index ba48ffdb6..04b7a3d3b 100644 --- a/README.rst +++ b/README.rst @@ -485,11 +485,12 @@ The following flags are accepted: +-------------------------------------------------------------------+----------------------------------------+ | :flag:`-build_tags tag1,tag2` | | +-------------------------------------------------------------------+----------------------------------------+ -| List of Go build tags Gazelle will consider to be true. Gazelle applies | +| List of Go build tags Gazelle will defer to Bazel for evaluation. Gazelle applies | | constraints when generating Go rules. It assumes certain tags are true on | | certain platforms (for example, ``amd64,linux``). It assumes all Go release | | tags are true (for example, ``go1.8``). It considers other tags to be false | -| (for example, ``ignore``). This flag overrides that behavior. | +| (for example, ``ignore``). This flag allows custom tags to be evaluated by | +| Bazel at build time. | | | | Bazel may still filter sources with these tags. Use | | ``bazel build --define gotags=foo,bar`` to set tags at build time. | @@ -751,11 +752,12 @@ The following directives are recognized: +---------------------------------------------------+----------------------------------------+ | :direc:`# gazelle:build_tags foo,bar` | none | +---------------------------------------------------+----------------------------------------+ -| List of Go build tags Gazelle will consider to be true. Gazelle applies | +| List of Go build tags Gazelle will defer to Bazel for evaluation. Gazelle applies | | constraints when generating Go rules. It assumes certain tags are true on | | certain platforms (for example, ``amd64,linux``). It assumes all Go release | | tags are true (for example, ``go1.8``). It considers other tags to be false | -| (for example, ``ignore``). This flag overrides that behavior. | +| (for example, ``ignore``). This flag allows custom tags to be evaluated by | +| Bazel at build time. | | | | Bazel may still filter sources with these tags. Use | | ``bazel build --define gotags=foo,bar`` to set tags at build time. | diff --git a/language/go/config.go b/language/go/config.go index e23ed5437..19673b4c4 100644 --- a/language/go/config.go +++ b/language/go/config.go @@ -41,6 +41,16 @@ import ( var minimumRulesGoVersion = version.Version{0, 29, 0} +type tagSet map[string]bool + +func (ts tagSet) clone() tagSet { + c := make(tagSet) + for k, v := range ts { + c[k] = v + } + return c +} + // goConfig contains configuration values related to Go rules. type goConfig struct { // The name under which the rules_go repository can be referenced from the @@ -53,7 +63,7 @@ type goConfig struct { // genericTags is a set of tags that Gazelle considers to be true. Set with // -build_tags or # gazelle:build_tags. Some tags, like gc, are always on. - genericTags map[string]bool + genericTags []tagSet // prefix is a prefix of an import path, used to generate importpath // attributes. Set with -go_prefix or # gazelle:prefix. @@ -178,8 +188,10 @@ func newGoConfig() *goConfig { goProtoCompilers: defaultGoProtoCompilers, goGrpcCompilers: defaultGoGrpcCompilers, goGenerateProto: true, + genericTags: []tagSet{ + {"gc": true}, + }, } - gc.preprocessTags() return gc } @@ -189,9 +201,9 @@ func getGoConfig(c *config.Config) *goConfig { func (gc *goConfig) clone() *goConfig { gcCopy := *gc - gcCopy.genericTags = make(map[string]bool) - for k, v := range gc.genericTags { - gcCopy.genericTags[k] = v + gcCopy.genericTags = make([]tagSet, 0, len(gc.genericTags)) + for _, ts := range gc.genericTags { + gcCopy.genericTags = append(gcCopy.genericTags, ts.clone()) } gcCopy.goProtoCompilers = gc.goProtoCompilers[:len(gc.goProtoCompilers):len(gc.goProtoCompilers)] gcCopy.goGrpcCompilers = gc.goGrpcCompilers[:len(gc.goGrpcCompilers):len(gc.goGrpcCompilers)] @@ -199,18 +211,8 @@ func (gc *goConfig) clone() *goConfig { return &gcCopy } -// preprocessTags adds some tags which are on by default before they are -// used to match files. -func (gc *goConfig) preprocessTags() { - if gc.genericTags == nil { - gc.genericTags = make(map[string]bool) - } - gc.genericTags["gc"] = true -} - // setBuildTags sets genericTags by parsing as a comma separated list. An // error will be returned for tags that wouldn't be recognized by "go build". -// preprocessTags should be called before this. func (gc *goConfig) setBuildTags(tags string) error { if tags == "" { return nil @@ -219,7 +221,13 @@ func (gc *goConfig) setBuildTags(tags string) error { if strings.HasPrefix(t, "!") { return fmt.Errorf("build tags can't be negated: %s", t) } - gc.genericTags[t] = true + var newSets []tagSet + for _, ts := range gc.genericTags { + c := ts.clone() + c[t] = true + newSets = append(newSets, c) + } + gc.genericTags = append(gc.genericTags, newSets...) } return nil } @@ -578,11 +586,6 @@ Update io_bazel_rules_go to a newer version in your WORKSPACE file.` for _, d := range f.Directives { switch d.Key { case "build_tags": - if err := gc.setBuildTags(d.Value); err != nil { - log.Print(err) - continue - } - gc.preprocessTags() if err := gc.setBuildTags(d.Value); err != nil { log.Print(err) } diff --git a/language/go/config_test.go b/language/go/config_test.go index e00e81aad..6b3cce54e 100644 --- a/language/go/config_test.go +++ b/language/go/config_test.go @@ -59,6 +59,13 @@ func testConfig(t *testing.T, args ...string) (*config.Config, []language.Langua return c, langs, cexts } +var expectedBuildTags = []tagSet{ + {"gc": true}, + {"gc": true, "foo": true}, + {"gc": true, "bar": true}, + {"gc": true, "foo": true, "bar": true}, +} + func TestCommandLine(t *testing.T) { c, _, _ := testConfig( t, @@ -68,10 +75,8 @@ func TestCommandLine(t *testing.T) { "-external=vendored", "-repo_root=.") gc := getGoConfig(c) - for _, tag := range []string{"foo", "bar", "gc"} { - if !gc.genericTags[tag] { - t.Errorf("expected tag %q to be set", tag) - } + if diff := cmp.Diff(expectedBuildTags, gc.genericTags); diff != "" { + t.Errorf("(-want, +got): %s", diff) } if gc.prefix != "example.com/repo" { t.Errorf(`got prefix %q; want "example.com/repo"`, gc.prefix) @@ -101,10 +106,8 @@ func TestDirectives(t *testing.T) { cext.Configure(c, "test", f) } gc := getGoConfig(c) - for _, tag := range []string{"foo", "bar", "gc"} { - if !gc.genericTags[tag] { - t.Errorf("expected tag %q to be set", tag) - } + if diff := cmp.Diff(expectedBuildTags, gc.genericTags); diff != "" { + t.Errorf("(-want, +got): %s", diff) } if gc.prefix != "y" { t.Errorf(`got prefix %q; want "y"`, gc.prefix) @@ -268,22 +271,6 @@ load("@io_bazel_rules_go//proto:go_proto_library.bzl", "go_proto_library") } } -func TestPreprocessTags(t *testing.T) { - gc := newGoConfig() - expectedTags := []string{"gc"} - for _, tag := range expectedTags { - if !gc.genericTags[tag] { - t.Errorf("tag %q not set", tag) - } - } - unexpectedTags := []string{"x", "cgo", "go1.8", "go1.7"} - for _, tag := range unexpectedTags { - if gc.genericTags[tag] { - t.Errorf("tag %q unexpectedly set", tag) - } - } -} - func TestPrefixFallback(t *testing.T) { c, _, cexts := testConfig(t) for _, tc := range []struct { diff --git a/language/go/fileinfo.go b/language/go/fileinfo.go index 38a95665b..c2c154cfa 100644 --- a/language/go/fileinfo.go +++ b/language/go/fileinfo.go @@ -569,7 +569,7 @@ func checkConstraints(c *config.Config, os, arch, osSuffix, archSuffix string, t } goConf := getGoConfig(c) - checker := func(tag string) bool { + checker := func(tag string, ts tagSet) bool { if isIgnoredTag(tag) { return true } @@ -585,13 +585,18 @@ func checkConstraints(c *config.Config, os, arch, osSuffix, archSuffix string, t return false } return arch == tag - } - return goConf.genericTags[tag] + return ts[tag] } - return tags.eval(checker) && cgoTags.eval(checker) + for _, ts := range goConf.genericTags { + c := func(tag string) bool { return checker(tag, ts) } + if tags.eval(c) && cgoTags.eval(c) { + return true + } + } + return false } // rulesGoSupportsOS returns whether the os tag is recognized by the version of diff --git a/language/go/fileinfo_test.go b/language/go/fileinfo_test.go index b7c5c1d91..6c89ac650 100644 --- a/language/go/fileinfo_test.go +++ b/language/go/fileinfo_test.go @@ -386,7 +386,7 @@ func TestCheckConstraints(t *testing.T) { defer os.RemoveAll(dir) for _, tc := range []struct { desc string - genericTags map[string]bool + genericTags string os, arch, filename, content string want bool }{ @@ -466,12 +466,12 @@ func TestCheckConstraints(t *testing.T) { want: false, }, { desc: "tags all satisfied", - genericTags: map[string]bool{"a": true, "b": true}, + genericTags: "a,b", content: "// +build a,b\n\npackage foo", want: true, }, { desc: "tags some satisfied", - genericTags: map[string]bool{"a": true}, + genericTags: "a", content: "// +build a,b\n\npackage foo", want: false, }, { @@ -480,36 +480,36 @@ func TestCheckConstraints(t *testing.T) { want: true, }, { desc: "tag satisfied negated", - genericTags: map[string]bool{"a": true}, + genericTags: "a", content: "// +build !a\n\npackage foo", - want: false, + want: true, }, { desc: "tag double negative", content: "// +build !!a\n\npackage foo", want: false, }, { desc: "tag group and satisfied", - genericTags: map[string]bool{"foo": true, "bar": true}, + genericTags: "foo,bar", content: "// +build foo,bar\n\npackage foo", want: true, }, { desc: "tag group and unsatisfied", - genericTags: map[string]bool{"foo": true}, + genericTags: "foo", content: "// +build foo,bar\n\npackage foo", want: false, }, { desc: "tag line or satisfied", - genericTags: map[string]bool{"foo": true}, + genericTags: "foo", content: "// +build foo bar\n\npackage foo", want: true, }, { desc: "tag line or unsatisfied", - genericTags: map[string]bool{"foo": true}, + genericTags: "foo", content: "// +build !foo bar\n\npackage foo", - want: false, + want: true, }, { desc: "tag lines and satisfied", - genericTags: map[string]bool{"foo": true, "bar": true}, + genericTags: "foo,bar", content: ` // +build foo // +build bar @@ -518,7 +518,7 @@ package foo`, want: true, }, { desc: "tag lines and unsatisfied", - genericTags: map[string]bool{"foo": true}, + genericTags: "foo", content: ` // +build foo // +build bar @@ -528,7 +528,7 @@ package foo`, }, { desc: "cgo tags satisfied", os: "linux", - genericTags: map[string]bool{"foo": true}, + genericTags: "foo", content: ` // +build foo @@ -581,9 +581,8 @@ import "C" t.Run(tc.desc, func(t *testing.T) { c, _, _ := testConfig(t) gc := getGoConfig(c) - gc.genericTags = tc.genericTags - if gc.genericTags == nil { - gc.genericTags = map[string]bool{"gc": true} + if err := gc.setBuildTags(tc.genericTags); err != nil { + t.Errorf("error setting build tags %q", tc.genericTags) } filename := tc.filename if filename == "" { From b2abf0d63cf239d14e11ac722e96816f6ae1bf62 Mon Sep 17 00:00:00 2001 From: Patrick Scott Date: Tue, 29 Oct 2024 12:47:40 -0400 Subject: [PATCH 2/2] Address PR feedback Specify the map size hint. Refactor the map to use struct instead of bool. --- language/go/config.go | 8 ++++---- language/go/config_test.go | 16 ++++++++++++---- language/go/fileinfo.go | 3 ++- 3 files changed, 18 insertions(+), 9 deletions(-) diff --git a/language/go/config.go b/language/go/config.go index 19673b4c4..c2b120704 100644 --- a/language/go/config.go +++ b/language/go/config.go @@ -41,10 +41,10 @@ import ( var minimumRulesGoVersion = version.Version{0, 29, 0} -type tagSet map[string]bool +type tagSet map[string]struct{} func (ts tagSet) clone() tagSet { - c := make(tagSet) + c := make(tagSet, len(ts)) for k, v := range ts { c[k] = v } @@ -189,7 +189,7 @@ func newGoConfig() *goConfig { goGrpcCompilers: defaultGoGrpcCompilers, goGenerateProto: true, genericTags: []tagSet{ - {"gc": true}, + {"gc": struct{}{}}, }, } return gc @@ -224,7 +224,7 @@ func (gc *goConfig) setBuildTags(tags string) error { var newSets []tagSet for _, ts := range gc.genericTags { c := ts.clone() - c[t] = true + c[t] = struct{}{} newSets = append(newSets, c) } gc.genericTags = append(gc.genericTags, newSets...) diff --git a/language/go/config_test.go b/language/go/config_test.go index 6b3cce54e..3ac54d88e 100644 --- a/language/go/config_test.go +++ b/language/go/config_test.go @@ -59,11 +59,19 @@ func testConfig(t *testing.T, args ...string) (*config.Config, []language.Langua return c, langs, cexts } +func newTagSet(tags ...string) tagSet { + ts := make(tagSet) + for _, t := range tags { + ts[t] = struct{}{} + } + return ts +} + var expectedBuildTags = []tagSet{ - {"gc": true}, - {"gc": true, "foo": true}, - {"gc": true, "bar": true}, - {"gc": true, "foo": true, "bar": true}, + newTagSet("gc"), + newTagSet("gc", "foo"), + newTagSet("gc", "bar"), + newTagSet("gc", "foo", "bar"), } func TestCommandLine(t *testing.T) { diff --git a/language/go/fileinfo.go b/language/go/fileinfo.go index c2c154cfa..1d5d7c924 100644 --- a/language/go/fileinfo.go +++ b/language/go/fileinfo.go @@ -587,7 +587,8 @@ func checkConstraints(c *config.Config, os, arch, osSuffix, archSuffix string, t return arch == tag } - return ts[tag] + _, ok := ts[tag] + return ok } for _, ts := range goConf.genericTags {