From 28c7ce63614ec50a1de081685b9fe8797e513f58 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Tue, 21 May 2024 18:54:02 +0200 Subject: [PATCH 1/3] Add support for `include()` in `MODULE.bazel` Previously, `gazelle` wouldn't look into `include`ed segments of module files when collecting the apparent to module name mapping. --- internal/module/BUILD.bazel | 5 +- internal/module/module.go | 75 ++++++++++++++----- tests/bcr/go_mod/.bazelversion | 2 +- tests/bcr/go_mod/MODULE.bazel | 3 +- tests/bcr/go_mod/segments/BUILD.bazel | 0 tests/bcr/go_mod/segments/dir/go.MODULE.bazel | 1 + 6 files changed, 66 insertions(+), 20 deletions(-) create mode 100644 tests/bcr/go_mod/segments/BUILD.bazel create mode 100644 tests/bcr/go_mod/segments/dir/go.MODULE.bazel diff --git a/internal/module/BUILD.bazel b/internal/module/BUILD.bazel index f5ae5e583..9a6a07bc4 100644 --- a/internal/module/BUILD.bazel +++ b/internal/module/BUILD.bazel @@ -5,7 +5,10 @@ go_library( srcs = ["module.go"], importpath = "github.com/bazelbuild/bazel-gazelle/internal/module", visibility = ["//:__subpackages__"], - deps = ["@com_github_bazelbuild_buildtools//build"], + deps = [ + "//label", + "@com_github_bazelbuild_buildtools//build", + ], ) filegroup( diff --git a/internal/module/module.go b/internal/module/module.go index c4411e0cd..22230df83 100644 --- a/internal/module/module.go +++ b/internal/module/module.go @@ -18,9 +18,11 @@ limitations under the License. package module import ( + "fmt" "os" "path/filepath" + "github.com/bazelbuild/bazel-gazelle/label" "github.com/bazelbuild/buildtools/build" ) @@ -29,30 +31,20 @@ import ( // See https://bazel.build/external/module#repository_names_and_strict_deps for more information on // apparent names. func ExtractModuleToApparentNameMapping(repoRoot string) (func(string) string, error) { - moduleFile, err := parseModuleFile(repoRoot) + moduleToApparentName, err := collectApparentNames(repoRoot, "MODULE.bazel") if err != nil { return nil, err } - var moduleToApparentName map[string]string - if moduleFile != nil { - moduleToApparentName = collectApparentNames(moduleFile) - } else { - // If there is no MODULE.bazel file, return a function that always returns the empty string. - // Languages will know to fall back to the WORKSPACE names of repos. - moduleToApparentName = make(map[string]string) - } return func(moduleName string) string { return moduleToApparentName[moduleName] }, nil } -func parseModuleFile(repoRoot string) (*build.File, error) { - path := filepath.Join(repoRoot, "MODULE.bazel") +func parseModuleSegment(repoRoot, relPath string) (*build.File, error) { + path := filepath.Join(repoRoot, relPath) bytes, err := os.ReadFile(path) - if os.IsNotExist(err) { - return nil, nil - } else if err != nil { + if err != nil { return nil, err } return build.ParseModule(path, bytes) @@ -61,11 +53,60 @@ func parseModuleFile(repoRoot string) (*build.File, error) { // Collects the mapping of module names (e.g. "rules_go") to user-configured apparent names (e.g. // "my_rules_go"). See https://bazel.build/external/module#repository_names_and_strict_deps for more // information on apparent names. -func collectApparentNames(m *build.File) map[string]string { +func collectApparentNames(repoRoot, relPath string) (map[string]string, error) { + apparentNames := make(map[string]string) + seenFiles := make(map[string]struct{}) + filesToProcess := []string{relPath} + + for len(filesToProcess) > 0 { + f := filesToProcess[0] + filesToProcess = filesToProcess[1:] + if _, seen := seenFiles[f]; seen { + continue + } + seenFiles[f] = struct{}{} + bf, err := parseModuleSegment(repoRoot, f) + if err != nil { + if f == "MODULE.bazel" && os.IsNotExist(err) { + // If there is no MODULE.bazel file, return an empty map but no error. + // Languages will know to fall back to the WORKSPACE names of repos. + return nil, nil + } + return nil, err + } + names, includeLabels := collectApparentNamesAndIncludes(bf) + for name, apparentName := range names { + apparentNames[name] = apparentName + } + for _, includeLabel := range includeLabels { + var l label.Label + l, err = label.Parse(includeLabel) + if err != nil { + return nil, fmt.Errorf("failed to parse include label %q: %v", includeLabel, err) + } + p := filepath.Join(filepath.FromSlash(l.Pkg), filepath.FromSlash(l.Name)) + filesToProcess = append(filesToProcess, p) + } + } + + return apparentNames, nil +} + +func collectApparentNamesAndIncludes(f *build.File) (map[string]string, []string) { apparentNames := make(map[string]string) + var includeLabels []string - for _, dep := range m.Rules("") { + for _, dep := range f.Rules("") { if dep.Name() == "" { + if ident, ok := dep.Call.X.(*build.Ident); !ok || ident.Name != "include" { + continue + } + if len(dep.Call.List) != 1 { + continue + } + if str, ok := dep.Call.List[0].(*build.StringExpr); ok { + includeLabels = append(includeLabels, str.Value) + } continue } if dep.Kind() != "module" && dep.Kind() != "bazel_dep" { @@ -82,5 +123,5 @@ func collectApparentNames(m *build.File) map[string]string { } } - return apparentNames + return apparentNames, includeLabels } diff --git a/tests/bcr/go_mod/.bazelversion b/tests/bcr/go_mod/.bazelversion index 21c8c7b46..58cbedfa9 100644 --- a/tests/bcr/go_mod/.bazelversion +++ b/tests/bcr/go_mod/.bazelversion @@ -1 +1 @@ -7.1.1 +7.2.0rc1 diff --git a/tests/bcr/go_mod/MODULE.bazel b/tests/bcr/go_mod/MODULE.bazel index c8c2e9656..08bb42f3d 100644 --- a/tests/bcr/go_mod/MODULE.bazel +++ b/tests/bcr/go_mod/MODULE.bazel @@ -14,8 +14,9 @@ local_path_override( path = "test_dep", ) +include("//segments:dir/go.MODULE.bazel") + bazel_dep(name = "protobuf", version = "23.1", repo_name = "my_protobuf") -bazel_dep(name = "rules_go", version = "0.42.0", repo_name = "my_rules_go") bazel_dep(name = "rules_proto", version = "6.0.0-rc2", repo_name = "my_rules_proto") go_sdk = use_extension("@my_rules_go//go:extensions.bzl", "go_sdk") diff --git a/tests/bcr/go_mod/segments/BUILD.bazel b/tests/bcr/go_mod/segments/BUILD.bazel new file mode 100644 index 000000000..e69de29bb diff --git a/tests/bcr/go_mod/segments/dir/go.MODULE.bazel b/tests/bcr/go_mod/segments/dir/go.MODULE.bazel new file mode 100644 index 000000000..ce60dbd71 --- /dev/null +++ b/tests/bcr/go_mod/segments/dir/go.MODULE.bazel @@ -0,0 +1 @@ +bazel_dep(name = "rules_go", version = "0.42.0", repo_name = "my_rules_go") From 540c4b21227c448fce84582b4a5671b5e6b2b711 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Tue, 21 May 2024 19:20:41 +0200 Subject: [PATCH 2/3] Remove segments --- tests/bcr/go_mod/.bazelversion | 2 +- tests/bcr/go_mod/MODULE.bazel | 3 +-- tests/bcr/go_mod/segments/BUILD.bazel | 0 tests/bcr/go_mod/segments/dir/go.MODULE.bazel | 1 - 4 files changed, 2 insertions(+), 4 deletions(-) delete mode 100644 tests/bcr/go_mod/segments/BUILD.bazel delete mode 100644 tests/bcr/go_mod/segments/dir/go.MODULE.bazel diff --git a/tests/bcr/go_mod/.bazelversion b/tests/bcr/go_mod/.bazelversion index 58cbedfa9..21c8c7b46 100644 --- a/tests/bcr/go_mod/.bazelversion +++ b/tests/bcr/go_mod/.bazelversion @@ -1 +1 @@ -7.2.0rc1 +7.1.1 diff --git a/tests/bcr/go_mod/MODULE.bazel b/tests/bcr/go_mod/MODULE.bazel index 08bb42f3d..c8c2e9656 100644 --- a/tests/bcr/go_mod/MODULE.bazel +++ b/tests/bcr/go_mod/MODULE.bazel @@ -14,9 +14,8 @@ local_path_override( path = "test_dep", ) -include("//segments:dir/go.MODULE.bazel") - bazel_dep(name = "protobuf", version = "23.1", repo_name = "my_protobuf") +bazel_dep(name = "rules_go", version = "0.42.0", repo_name = "my_rules_go") bazel_dep(name = "rules_proto", version = "6.0.0-rc2", repo_name = "my_rules_proto") go_sdk = use_extension("@my_rules_go//go:extensions.bzl", "go_sdk") diff --git a/tests/bcr/go_mod/segments/BUILD.bazel b/tests/bcr/go_mod/segments/BUILD.bazel deleted file mode 100644 index e69de29bb..000000000 diff --git a/tests/bcr/go_mod/segments/dir/go.MODULE.bazel b/tests/bcr/go_mod/segments/dir/go.MODULE.bazel deleted file mode 100644 index ce60dbd71..000000000 --- a/tests/bcr/go_mod/segments/dir/go.MODULE.bazel +++ /dev/null @@ -1 +0,0 @@ -bazel_dep(name = "rules_go", version = "0.42.0", repo_name = "my_rules_go") From 29f5f314f75314117af7e05f85c8b4996e359a2b Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Tue, 28 May 2024 22:48:42 +0200 Subject: [PATCH 3/3] Add test and fix bug --- BUILD.bazel | 2 + internal/module/BUILD.bazel | 17 ++++++- internal/module/module.go | 5 +-- internal/module/module_test.go | 44 +++++++++++++++++++ internal/module/testdata/MODULE.bazel | 10 +++++ .../testdata/bazel/dir/deps.MODULE.bazel | 1 + .../module/testdata/bazel/lang.MODULE.bazel | 4 ++ internal/module/testdata/deps.MODULE.bazel | 1 + 8 files changed, 80 insertions(+), 4 deletions(-) create mode 100644 internal/module/module_test.go create mode 100644 internal/module/testdata/MODULE.bazel create mode 100644 internal/module/testdata/bazel/dir/deps.MODULE.bazel create mode 100644 internal/module/testdata/bazel/lang.MODULE.bazel create mode 100644 internal/module/testdata/deps.MODULE.bazel diff --git a/BUILD.bazel b/BUILD.bazel index 5467e221b..e281ea890 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -9,8 +9,10 @@ load("//:def.bzl", "gazelle", "gazelle_binary") # gazelle:exclude .bazelci # gazelle:exclude .bcr # gazelle:exclude .idea +# gazelle:exclude .ijwb # gazelle:exclude .github # gazelle:exclude .vscode +# gazelle:exclude internal/module/testdata # gazelle:go_naming_convention import_alias gazelle( name = "gazelle", diff --git a/internal/module/BUILD.bazel b/internal/module/BUILD.bazel index 9a6a07bc4..0adffa699 100644 --- a/internal/module/BUILD.bazel +++ b/internal/module/BUILD.bazel @@ -1,4 +1,4 @@ -load("@io_bazel_rules_go//go:def.bzl", "go_library") +load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "module", @@ -17,6 +17,7 @@ filegroup( srcs = [ "BUILD.bazel", "module.go", + "module_test.go", ], visibility = ["//visibility:public"], ) @@ -26,3 +27,17 @@ alias( actual = ":module", visibility = ["//:__subpackages__"], ) + +go_test( + name = "module_test", + srcs = ["module_test.go"], + data = glob( + ["testdata/**"], + allow_empty = True, + ), + embed = [":module"], + deps = [ + "@com_github_google_go_cmp//cmp", + "@io_bazel_rules_go//go/runfiles:go_default_library", + ], +) diff --git a/internal/module/module.go b/internal/module/module.go index 22230df83..04e2e612a 100644 --- a/internal/module/module.go +++ b/internal/module/module.go @@ -79,8 +79,7 @@ func collectApparentNames(repoRoot, relPath string) (map[string]string, error) { apparentNames[name] = apparentName } for _, includeLabel := range includeLabels { - var l label.Label - l, err = label.Parse(includeLabel) + l, err := label.Parse(includeLabel) if err != nil { return nil, fmt.Errorf("failed to parse include label %q: %v", includeLabel, err) } @@ -97,7 +96,7 @@ func collectApparentNamesAndIncludes(f *build.File) (map[string]string, []string var includeLabels []string for _, dep := range f.Rules("") { - if dep.Name() == "" { + if dep.ExplicitName() == "" { if ident, ok := dep.Call.X.(*build.Ident); !ok || ident.Name != "include" { continue } diff --git a/internal/module/module_test.go b/internal/module/module_test.go new file mode 100644 index 000000000..a9cee370b --- /dev/null +++ b/internal/module/module_test.go @@ -0,0 +1,44 @@ +package module + +import ( + "path/filepath" + "testing" + + "github.com/bazelbuild/rules_go/go/runfiles" + "github.com/google/go-cmp/cmp" +) + +func TestCollectApparent(t *testing.T) { + moduleFile, err := runfiles.Rlocation("bazel_gazelle/internal/module/testdata/MODULE.bazel") + if err != nil { + t.Fatal(err) + } + + apparentNames, err := collectApparentNames(filepath.Dir(moduleFile), "MODULE.bazel") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + expected := map[string]string{ + "rules_bar": "rules_bar", + "rules_baz": "rules_baz", + "rules_foo": "my_rules_foo", + "rules_lang": "my_rules_lang", + "rules_quz": "rules_quz", + "test_module": "my_test_module", + } + if diff := cmp.Diff(expected, apparentNames); diff != "" { + t.Errorf("unexpected apparent names (-want +got):\n%s", diff) + } +} + +func TestCollectApparent_fileDoesNotExist(t *testing.T) { + _, err := collectApparentNames(t.TempDir(), "MODULE.bazel") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + _, err = collectApparentNames(t.TempDir(), "segment.MODULE.bazel") + if err == nil { + t.Fatalf("expected error, got nil") + } +} diff --git a/internal/module/testdata/MODULE.bazel b/internal/module/testdata/MODULE.bazel new file mode 100644 index 000000000..fd5273061 --- /dev/null +++ b/internal/module/testdata/MODULE.bazel @@ -0,0 +1,10 @@ +module( + name = "test_module", + repo_name = "my_test_module", +) + +bazel_dep(name = "rules_bar", version = "1.2.3") + +include("//bazel:lang.MODULE.bazel") + +bazel_dep(name = "rules_foo", version = "1.2.3", repo_name = "my_rules_foo") diff --git a/internal/module/testdata/bazel/dir/deps.MODULE.bazel b/internal/module/testdata/bazel/dir/deps.MODULE.bazel new file mode 100644 index 000000000..ccafd9582 --- /dev/null +++ b/internal/module/testdata/bazel/dir/deps.MODULE.bazel @@ -0,0 +1 @@ +bazel_dep(name = "rules_quz", version = "0.0.1") diff --git a/internal/module/testdata/bazel/lang.MODULE.bazel b/internal/module/testdata/bazel/lang.MODULE.bazel new file mode 100644 index 000000000..6d8489e4d --- /dev/null +++ b/internal/module/testdata/bazel/lang.MODULE.bazel @@ -0,0 +1,4 @@ +bazel_dep(name = "rules_lang", version = "1.2.3", repo_name = "my_rules_lang") + +include("//:deps.MODULE.bazel") +include("//bazel:dir/deps.MODULE.bazel") diff --git a/internal/module/testdata/deps.MODULE.bazel b/internal/module/testdata/deps.MODULE.bazel new file mode 100644 index 000000000..3c23d33e4 --- /dev/null +++ b/internal/module/testdata/deps.MODULE.bazel @@ -0,0 +1 @@ +bazel_dep(name = "rules_baz", version = "0.0.1")