diff --git a/language/go/config.go b/language/go/config.go index d328650f6..98126b141 100644 --- a/language/go/config.go +++ b/language/go/config.go @@ -80,9 +80,9 @@ type goConfig struct { // goNamingConvention controls the name of generated targets goNamingConvention namingConvention - // goNamingConventionExtern controls the default naming convention for + // goNamingConventionExternal controls the default naming convention for // imports in external repositories with unknown naming conventions. - goNamingConventionExtern namingConvention + goNamingConventionExternal namingConvention // goProtoCompilers is the protocol buffers compiler(s) to use for go code. goProtoCompilers []string @@ -330,7 +330,7 @@ func (*goLang) KnownDirectives() []string { "go_generate_proto", "go_grpc_compilers", "go_naming_convention", - "go_naming_convention_extern", + "go_naming_convention_external", "go_proto_compilers", "go_visibility", "importmap_prefix", @@ -377,8 +377,8 @@ func (*goLang) RegisterFlags(fs *flag.FlagSet, cmd string, c *config.Config) { "go_naming_convention", "controls generated library names. One of (go_default_library, import, import_alias)") fs.Var( - &namingConventionFlag{&gc.goNamingConventionExtern}, - "go_naming_convention_extern", + &namingConventionFlag{&gc.goNamingConventionExternal}, + "go_naming_convention_external", "controls naming convention used when resolving libraries in external repositories with unknown conventions") case "update-repos": @@ -534,9 +534,9 @@ Update io_bazel_rules_go to a newer version in your WORKSPACE file.` log.Print(err) } - case "go_naming_convention_extern": + case "go_naming_convention_external": if nc, err := namingConventionFromString(d.Value); err == nil { - gc.goNamingConventionExtern = nc + gc.goNamingConventionExternal = nc } else { log.Print(err) } diff --git a/language/go/fix.go b/language/go/fix.go index 760e80221..0cadfdc5a 100644 --- a/language/go/fix.go +++ b/language/go/fix.go @@ -40,7 +40,7 @@ func (_ *goLang) Fix(c *config.Config, f *rule.File) { func migrateNamingConvention(c *config.Config, f *rule.File) { // Determine old and new names for go_library and go_test. nc := getGoConfig(c).goNamingConvention - importPath := findImportPath(f) + importPath := InferImportPath(c, f.Pkg) if importPath == "" { return } @@ -69,22 +69,22 @@ func migrateNamingConvention(c *config.Config, f *rule.File) { switch { case r.Name() == libName: haveLib = true - case r.Kind() == "go_library" && r.Name() == migrateLibName: + case r.Kind() == "go_library" && r.Name() == migrateLibName && r.AttrString("importpath") == importPath: haveMigrateLib = true case r.Name() == testName: haveTest = true - case r.Kind() == "go_test" && r.Name() == migrateTestName: + case r.Kind() == "go_test" && r.Name() == migrateTestName && strListAttrContains(r, "embed", ":"+migrateLibName): haveMigrateTest = true } } - haveLibConflict := haveLib && haveMigrateLib - haveTestConflict := haveTest && haveMigrateTest - if haveLibConflict { + if haveLib && haveMigrateLib { log.Printf("%[1]s: Tried to rename %[2]s to %[3]s, but %[3]s already exists.", f.Path, migrateLibName, libName) } - if haveTestConflict { + if haveTest && haveMigrateTest { log.Printf("%[1]s: Tried to rename %[2]s to %[3]s, but %[3]s already exists.", f.Path, migrateTestName, testName) } + shouldMigrateLib := haveMigrateLib && !haveLib + shouldMigrateTest := haveMigrateTest && !haveTest // Rename the targets and stuff in the same file that refers to them. for _, r := range f.Rules { @@ -92,18 +92,18 @@ func migrateNamingConvention(c *config.Config, f *rule.File) { // We'll need to move the metaresolver from resolve.RuleIndex to config.Config so we can access it from here. switch r.Kind() { case "go_binary": - if !haveLibConflict { + if haveMigrateLib && shouldMigrateLib { replaceInStrListAttr(r, "embed", ":"+migrateLibName, ":"+libName) } case "go_library": - if r.Name() == migrateLibName && !haveLibConflict { + if r.Name() == migrateLibName && shouldMigrateLib { r.SetName(libName) } case "go_test": - if r.Name() == migrateTestName && !haveTestConflict { + if r.Name() == migrateTestName && shouldMigrateTest { r.SetName(testName) } - if !haveLibConflict { + if shouldMigrateLib { replaceInStrListAttr(r, "embed", ":"+migrateLibName, ":"+libName) } } @@ -127,34 +127,28 @@ func fileContainsGoBinary(c *config.Config, f *rule.File) bool { return false } -// findImportPath returns the existing import path from the first encountered Go -// rule with the attribute set. -func findImportPath(f *rule.File) string { - for _, r := range f.Rules { - switch r.Kind() { - case "go_binary", "go_library", "go_test": - if ip, ok := r.Attr("importpath").(*bzl.StringExpr); ok && ip.Value != "" { - return ip.Value - } +func replaceInStrListAttr(r *rule.Rule, attr, old, new string) { + items := r.AttrStrings(attr) + changed := false + for i := range items { + if items[i] == old { + changed = true + items[i] = new } } - return "" + if changed { + r.SetAttr(attr, items) + } } -func replaceInStrListAttr(r *rule.Rule, attr, old, new string) { - l := r.AttrStrings(attr) - var shouldAdd = true - var items []string - for _, v := range l { - if v != old { - items = append(items, v) +func strListAttrContains(r *rule.Rule, attr, s string) bool { + items := r.AttrStrings(attr) + for _, item := range items { + if item == s { + return true } - shouldAdd = shouldAdd && v != new } - if shouldAdd { - items = append(items, new) - } - r.SetAttr(attr, items) + return false } // migrateLibraryEmbed converts "library" attributes to "embed" attributes, diff --git a/language/go/fix_test.go b/language/go/fix_test.go index 48f91b514..24e4ce981 100644 --- a/language/go/fix_test.go +++ b/language/go/fix_test.go @@ -16,7 +16,6 @@ limitations under the License. package golang import ( - "fmt" "path/filepath" "testing" @@ -40,7 +39,7 @@ func TestFixFile(t *testing.T) { go_library( name = "go_default_library", srcs = ["foo.go"], - importpath = "foo", + importpath = "example.com/foo", ) go_test( @@ -54,7 +53,7 @@ go_test( go_library( name = "foo", srcs = ["foo.go"], - importpath = "foo", + importpath = "example.com/foo", ) go_test( @@ -75,7 +74,7 @@ go_binary( go_library( name = "go_default_library", - importpath = "foo", + importpath = "example.com/foo", srcs = ["foo.go"], ) @@ -95,7 +94,7 @@ go_binary( go_library( name = "foo_lib", srcs = ["foo.go"], - importpath = "foo", + importpath = "example.com/foo", ) go_test( @@ -113,7 +112,7 @@ load(":build_defs.bzl", "x_binary") go_library( name = "go_default_library", srcs = ["foo.go"], - importpath = "foo", + importpath = "example.com/foo", visibility = ["//visibility:private"], ) @@ -133,7 +132,7 @@ load(":build_defs.bzl", "x_binary") go_library( name = "go_default_library", srcs = ["foo.go"], - importpath = "foo", + importpath = "example.com/foo", visibility = ["//visibility:private"], ) @@ -155,7 +154,7 @@ go_test( go_library( name = "foo", srcs = ["foo.go"], - importpath = "foo", + importpath = "example.com/foo", ) go_test( @@ -169,7 +168,7 @@ go_test( go_library( name = "go_default_library", srcs = ["foo.go"], - importpath = "foo", + importpath = "example.com/foo", ) go_test( @@ -191,7 +190,7 @@ go_binary( go_library( name = "foo_lib", srcs = ["foo.go"], - importpath = "foo", + importpath = "example.com/foo", ) go_test( @@ -210,7 +209,7 @@ go_binary( go_library( name = "go_default_library", srcs = ["foo.go"], - importpath = "foo", + importpath = "example.com/foo", ) go_test( @@ -227,7 +226,7 @@ go_test( go_library( name = "go_default_library", srcs = ["foo.go"], - importpath = "foo", + importpath = "example.com/foo", visibility = ["//visibility:private"], ) @@ -242,7 +241,7 @@ go_test( go_library( name = "foo", srcs = ["foo.go"], - importpath = "foo", + importpath = "example.com/foo", visibility = ["//visibility:private"], ) @@ -260,7 +259,7 @@ go_test( go_library( name = "foo", srcs = ["foo.go"], - importpath = "foo", + importpath = "example.com/foo", ) go_test( @@ -274,7 +273,7 @@ go_test( go_library( name = "go_default_library", srcs = ["foo.go"], - importpath = "foo", + importpath = "example.com/foo", ) go_test( @@ -296,7 +295,7 @@ go_binary( go_library( name = "go_default_library", srcs = ["foo.go"], - importpath = "foo", + importpath = "example.com/foo", ) go_test( @@ -315,7 +314,7 @@ go_binary( go_library( name = "foo_lib", srcs = ["foo.go"], - importpath = "foo", + importpath = "example.com/foo", ) go_test( @@ -332,7 +331,7 @@ go_test( go_library( name = "foo", srcs = ["foo.go"], - importpath = "foo", + importpath = "example.com/foo", ) go_test( @@ -346,7 +345,7 @@ go_test( go_library( name = "foo", srcs = ["foo.go"], - importpath = "foo", + importpath = "example.com/foo", ) go_test( @@ -363,7 +362,7 @@ go_test( go_library( name = "foo", srcs = ["foo.go"], - importpath = "foo", + importpath = "example.com/foo", ) go_test( @@ -377,7 +376,7 @@ go_test( go_library( name = "foo", srcs = ["foo.go"], - importpath = "foo", + importpath = "example.com/foo", ) go_test( @@ -394,7 +393,7 @@ go_test( go_library( name = "foo", srcs = ["foo.go"], - importpath = "foo", + importpath = "example.com/foo", ) go_test( @@ -408,7 +407,7 @@ go_test( go_library( name = "foo", srcs = ["foo.go"], - importpath = "foo", + importpath = "example.com/foo", ) go_test( @@ -425,7 +424,7 @@ go_test( go_library( name = "go_default_library", srcs = ["foo.go"], - importpath = "foo", + importpath = "example.com/foo", visibility = ["//visibility:private"], ) @@ -440,7 +439,7 @@ go_test( go_library( name = "go_default_library", srcs = ["foo.go"], - importpath = "foo", + importpath = "example.com/foo", visibility = ["//visibility:private"], ) @@ -458,7 +457,7 @@ go_test( go_library( name = "foo", srcs = ["foo.go"], - importpath = "foo", + importpath = "example.com/foo", ) go_test( @@ -472,7 +471,7 @@ go_test( go_library( name = "foo", srcs = ["foo.go"], - importpath = "foo", + importpath = "example.com/foo", ) go_test( @@ -861,7 +860,9 @@ go_proto_library(name = "foo_proto") t.Run(tc.desc, func(t *testing.T) { testFix(t, tc, func(f *rule.File) { c, langs, _ := testConfig(t, - fmt.Sprintf("-go_naming_convention=%s", tc.namingConvention)) + "-go_naming_convention="+tc.namingConvention.String(), + "-go_prefix=example.com/foo", + ) c.ShouldFix = true for _, lang := range langs { lang.Fix(c, f) diff --git a/language/go/resolve.go b/language/go/resolve.go index d8b9b39dc..7a61229ec 100644 --- a/language/go/resolve.go +++ b/language/go/resolve.go @@ -278,14 +278,14 @@ func resolveExternal(c *config.Config, rc *repo.RemoteCache, imp string) (label. // user has told us otherwise. nc := gc.repoNamingConvention[repo] if nc == unknownNamingConvention { - if gc.goNamingConventionExtern != unknownNamingConvention { - nc = gc.goNamingConventionExtern + if gc.goNamingConventionExternal != unknownNamingConvention { + nc = gc.goNamingConventionExternal } else { nc = goDefaultLibraryNamingConvention } } else if nc == importAliasNamingConvention { - if gc.goNamingConventionExtern != unknownNamingConvention { - nc = gc.goNamingConventionExtern + if gc.goNamingConventionExternal != unknownNamingConvention { + nc = gc.goNamingConventionExternal } else { nc = gc.goNamingConvention } diff --git a/language/go/resolve_test.go b/language/go/resolve_test.go index 79dd4df7e..29264ff24 100644 --- a/language/go/resolve_test.go +++ b/language/go/resolve_test.go @@ -1084,13 +1084,13 @@ func TestResolveExternal(t *testing.T) { ix.Finish() gl := langs[1].(*goLang) for _, tc := range []struct { - desc, importpath string - repos []repo.Repo - moduleMode bool - namingConvention namingConvention - namingConventionExtern namingConvention - repoNamingConvention map[string]namingConvention - want string + desc, importpath string + repos []repo.Repo + moduleMode bool + namingConvention namingConvention + namingConventionExternal namingConvention + repoNamingConvention map[string]namingConvention + want string }{ { desc: "top", @@ -1149,18 +1149,18 @@ func TestResolveExternal(t *testing.T) { Name: "custom_repo_name", GoPrefix: "example.com/repo", }}, - namingConventionExtern: importNamingConvention, - importpath: "example.com/repo/lib", - want: "@custom_repo_name//lib", + namingConventionExternal: importNamingConvention, + importpath: "example.com/repo/lib", + want: "@custom_repo_name//lib", }, { desc: "custom_repo_naming_convention_extern_default", repos: []repo.Repo{{ Name: "custom_repo_name", GoPrefix: "example.com/repo", }}, - namingConventionExtern: goDefaultLibraryNamingConvention, - importpath: "example.com/repo/lib", - want: "@custom_repo_name//lib:go_default_library", + namingConventionExternal: goDefaultLibraryNamingConvention, + importpath: "example.com/repo/lib", + want: "@custom_repo_name//lib:go_default_library", }, { desc: "qualified", importpath: "example.com/repo.git/lib", @@ -1219,7 +1219,7 @@ func TestResolveExternal(t *testing.T) { t.Run(tc.desc, func(t *testing.T) { gc.moduleMode = tc.moduleMode gc.goNamingConvention = tc.namingConvention - gc.goNamingConventionExtern = tc.namingConventionExtern + gc.goNamingConventionExternal = tc.namingConventionExternal gc.repoNamingConvention = tc.repoNamingConvention rc := testRemoteCache(tc.repos) r := rule.NewRule("go_library", "x")