From d920aec08bd5bfbbc28cf8ef174426abe82ae44a Mon Sep 17 00:00:00 2001 From: Alex Martani Date: Thu, 10 Sep 2026 16:44:48 -0700 Subject: [PATCH 01/18] fix(gazelle): merge pytest conftest annotations deterministically --- gazelle/docs/annotations.md | 10 ++ gazelle/python/BUILD.bazel | 4 + gazelle/python/parser.go | 19 +++- gazelle/python/parser_test.go | 92 +++++++++++++++++++ .../BUILD.in | 2 + .../BUILD.out | 2 + .../README.md | 4 + .../WORKSPACE | 1 + .../conftest.py | 1 + .../false_test.py | 1 + .../test.yaml | 5 + .../true_test.py | 1 + news/gazelle-pytest-conftest.fixed.md | 2 + 13 files changed, 143 insertions(+), 1 deletion(-) create mode 100644 gazelle/python/parser_test.go create mode 100644 gazelle/python/testdata/annotation_include_pytest_conftest_conflict/BUILD.in create mode 100644 gazelle/python/testdata/annotation_include_pytest_conftest_conflict/BUILD.out create mode 100644 gazelle/python/testdata/annotation_include_pytest_conftest_conflict/README.md create mode 100644 gazelle/python/testdata/annotation_include_pytest_conftest_conflict/WORKSPACE create mode 100644 gazelle/python/testdata/annotation_include_pytest_conftest_conflict/conftest.py create mode 100644 gazelle/python/testdata/annotation_include_pytest_conftest_conflict/false_test.py create mode 100644 gazelle/python/testdata/annotation_include_pytest_conftest_conflict/test.yaml create mode 100644 gazelle/python/testdata/annotation_include_pytest_conftest_conflict/true_test.py create mode 100644 news/gazelle-pytest-conftest.fixed.md diff --git a/gazelle/docs/annotations.md b/gazelle/docs/annotations.md index b3f06b9991..05ac4a6daa 100644 --- a/gazelle/docs/annotations.md +++ b/gazelle/docs/annotations.md @@ -198,3 +198,13 @@ py_test( ``` See {gh-issue}`3076` for more information. + +When a `py_test` has multiple source files, the annotation may be omitted from +some files. If multiple source files set the annotation, they must all set it to +the same value; Gazelle reports an error if the values conflict. + +:::{versionchanged} VERSION_NEXT_PATCH +For multi-source `py_test` targets, annotations in different source files must +agree. An annotation in one source file is no longer overwritten by an unset +value in another source file. +::: diff --git a/gazelle/python/BUILD.bazel b/gazelle/python/BUILD.bazel index 1ffa2890e1..bd6679572f 100644 --- a/gazelle/python/BUILD.bazel +++ b/gazelle/python/BUILD.bazel @@ -120,10 +120,14 @@ go_test( name = "default_test", srcs = [ "file_parser_test.go", + "parser_test.go", "std_modules_test.go", ], embed = [":python"], deps = [ + "@com_github_emirpasic_gods//sets/treeset:go_default_library", + "@com_github_emirpasic_gods//utils:go_default_library", "@com_github_stretchr_testify//assert", + "@com_github_stretchr_testify//require", ], ) diff --git a/gazelle/python/parser.go b/gazelle/python/parser.go index 3d0dbe7a5f..bead1848b1 100644 --- a/gazelle/python/parser.go +++ b/gazelle/python/parser.go @@ -92,6 +92,8 @@ func (p *python3Parser) parse(pyFilenames *treeset.Set) (*treeset.Set, map[strin mainModules := make(map[string]*treeset.Set, len(chRes)) allAnnotations := new(annotations) allAnnotations.ignore = make(map[string]struct{}) + var includesPytestConftest bool + var excludesPytestConftest bool for res := range chRes { if res.HasMain { mainModules[res.FileName] = treeset.NewWith(moduleComparator) @@ -125,9 +127,24 @@ func (p *python3Parser) parse(pyFilenames *treeset.Set) (*treeset.Set, map[strin allAnnotations.ignore[k] = v } allAnnotations.includeDeps = append(allAnnotations.includeDeps, annotations.includeDeps...) - allAnnotations.includePytestConftest = annotations.includePytestConftest + if annotations.includePytestConftest != nil { + if *annotations.includePytestConftest { + includesPytestConftest = true + } else { + excludesPytestConftest = true + } + } } + if includesPytestConftest && excludesPytestConftest { + return nil, nil, nil, fmt.Errorf( + "conflicting values for the %q annotation across Python source files", + annotationKindIncludePytestConftest, + ) + } + if includesPytestConftest || excludesPytestConftest { + allAnnotations.includePytestConftest = &includesPytestConftest + } allAnnotations.includeDeps = removeDupesFromStringTreeSetSlice(allAnnotations.includeDeps) return modules, mainModules, allAnnotations, nil diff --git a/gazelle/python/parser_test.go b/gazelle/python/parser_test.go new file mode 100644 index 0000000000..9aff397fd2 --- /dev/null +++ b/gazelle/python/parser_test.go @@ -0,0 +1,92 @@ +package python + +import ( + "os" + "path/filepath" + "testing" + + "github.com/emirpasic/gods/sets/treeset" + godsutils "github.com/emirpasic/gods/utils" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestParseIncludePytestConftestAnnotations(t *testing.T) { + t.Parallel() + + boolPointer := func(value bool) *bool { + return &value + } + tests := []struct { + name string + contents []string + expected *bool + expectErr string + }{ + { + name: "all unset", + contents: []string{"", ""}, + }, + { + name: "false and unset", + contents: []string{"# gazelle:include_pytest_conftest false", ""}, + expected: boolPointer(false), + }, + { + name: "true and unset", + contents: []string{"", "# gazelle:include_pytest_conftest true"}, + expected: boolPointer(true), + }, + { + name: "matching false values", + contents: []string{ + "# gazelle:include_pytest_conftest false", + "# gazelle:include_pytest_conftest false", + "", + }, + expected: boolPointer(false), + }, + { + name: "matching true values", + contents: []string{ + "# gazelle:include_pytest_conftest true", + "", + "# gazelle:include_pytest_conftest true", + }, + expected: boolPointer(true), + }, + { + name: "conflicting values", + contents: []string{ + "# gazelle:include_pytest_conftest false", + "", + "# gazelle:include_pytest_conftest true", + }, + expectErr: "conflicting values for the \"include_pytest_conftest\" annotation " + + "across Python source files", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + repoRoot := t.TempDir() + filenames := treeset.NewWith(godsutils.StringComparator) + for index, contents := range test.contents { + filename := string(rune('a'+index)) + "_test.py" + require.NoError(t, os.WriteFile(filepath.Join(repoRoot, filename), []byte(contents), 0o600)) + filenames.Add(filename) + } + + parser := newPython3Parser(repoRoot, "", func(string) bool { return false }) + _, _, annotations, err := parser.parse(filenames) + if test.expectErr != "" { + assert.EqualError(t, err, test.expectErr) + return + } + + require.NoError(t, err) + assert.Equal(t, test.expected, annotations.includePytestConftest) + }) + } +} diff --git a/gazelle/python/testdata/annotation_include_pytest_conftest_conflict/BUILD.in b/gazelle/python/testdata/annotation_include_pytest_conftest_conflict/BUILD.in new file mode 100644 index 0000000000..786a959d7b --- /dev/null +++ b/gazelle/python/testdata/annotation_include_pytest_conftest_conflict/BUILD.in @@ -0,0 +1,2 @@ +# gazelle:python_generation_mode package +# gazelle:python_generation_mode_per_package_require_test_entry_point false diff --git a/gazelle/python/testdata/annotation_include_pytest_conftest_conflict/BUILD.out b/gazelle/python/testdata/annotation_include_pytest_conftest_conflict/BUILD.out new file mode 100644 index 0000000000..786a959d7b --- /dev/null +++ b/gazelle/python/testdata/annotation_include_pytest_conftest_conflict/BUILD.out @@ -0,0 +1,2 @@ +# gazelle:python_generation_mode package +# gazelle:python_generation_mode_per_package_require_test_entry_point false diff --git a/gazelle/python/testdata/annotation_include_pytest_conftest_conflict/README.md b/gazelle/python/testdata/annotation_include_pytest_conftest_conflict/README.md new file mode 100644 index 0000000000..97317ffca7 --- /dev/null +++ b/gazelle/python/testdata/annotation_include_pytest_conftest_conflict/README.md @@ -0,0 +1,4 @@ +# Conflicting `include_pytest_conftest` annotations + +This test case asserts that Gazelle fails when source files in the same +`py_test` set `include_pytest_conftest` to conflicting values. diff --git a/gazelle/python/testdata/annotation_include_pytest_conftest_conflict/WORKSPACE b/gazelle/python/testdata/annotation_include_pytest_conftest_conflict/WORKSPACE new file mode 100644 index 0000000000..faff6af87a --- /dev/null +++ b/gazelle/python/testdata/annotation_include_pytest_conftest_conflict/WORKSPACE @@ -0,0 +1 @@ +# This is a Bazel workspace for the Gazelle test data. diff --git a/gazelle/python/testdata/annotation_include_pytest_conftest_conflict/conftest.py b/gazelle/python/testdata/annotation_include_pytest_conftest_conflict/conftest.py new file mode 100644 index 0000000000..8b13789179 --- /dev/null +++ b/gazelle/python/testdata/annotation_include_pytest_conftest_conflict/conftest.py @@ -0,0 +1 @@ + diff --git a/gazelle/python/testdata/annotation_include_pytest_conftest_conflict/false_test.py b/gazelle/python/testdata/annotation_include_pytest_conftest_conflict/false_test.py new file mode 100644 index 0000000000..ba71a2818b --- /dev/null +++ b/gazelle/python/testdata/annotation_include_pytest_conftest_conflict/false_test.py @@ -0,0 +1 @@ +# gazelle:include_pytest_conftest false diff --git a/gazelle/python/testdata/annotation_include_pytest_conftest_conflict/test.yaml b/gazelle/python/testdata/annotation_include_pytest_conftest_conflict/test.yaml new file mode 100644 index 0000000000..780d2148d7 --- /dev/null +++ b/gazelle/python/testdata/annotation_include_pytest_conftest_conflict/test.yaml @@ -0,0 +1,5 @@ +--- +expect: + exit_code: 1 + stderr: | + gazelle: ERROR: conflicting values for the "include_pytest_conftest" annotation across Python source files diff --git a/gazelle/python/testdata/annotation_include_pytest_conftest_conflict/true_test.py b/gazelle/python/testdata/annotation_include_pytest_conftest_conflict/true_test.py new file mode 100644 index 0000000000..b2d10359da --- /dev/null +++ b/gazelle/python/testdata/annotation_include_pytest_conftest_conflict/true_test.py @@ -0,0 +1 @@ +# gazelle:include_pytest_conftest true diff --git a/news/gazelle-pytest-conftest.fixed.md b/news/gazelle-pytest-conftest.fixed.md new file mode 100644 index 0000000000..532e13012f --- /dev/null +++ b/news/gazelle-pytest-conftest.fixed.md @@ -0,0 +1,2 @@ +(gazelle) Made `include_pytest_conftest` annotations deterministic for +multi-source tests and report conflicting explicit values. From 7500bb377456e1c03453a780688b41f30195b868 Mon Sep 17 00:00:00 2001 From: Alex Martani Date: Sat, 2 May 2026 10:20:25 -0700 Subject: [PATCH 02/18] test(gazelle) add tests for preserving existing targets --- .../BUILD.in | 10 +++++ .../BUILD.out | 30 +++++++++++++ .../README.md | 8 ++++ .../WORKSPACE | 1 + .../__init__.py | 0 .../bar.py | 1 + .../bar_test.py | 1 + .../baz.py | 1 + .../foo.py | 1 + .../qux.py | 1 + .../test.yaml | 1 + .../BUILD.in | 12 ++++++ .../BUILD.out | 33 +++++++++++++++ .../README.md | 8 ++++ .../WORKSPACE | 1 + .../__init__.py | 0 .../bar.py | 1 + .../bar_test.py | 1 + .../baz.py | 1 + .../foo.py | 0 .../foo_test.py | 0 .../test.yaml | 1 + .../BUILD.in | 12 ++++++ .../BUILD.out | 26 ++++++++++++ .../README.md | 8 ++++ .../WORKSPACE | 1 + .../bar.py | 4 ++ .../baz.py | 1 + .../foo.py | 1 + .../test.yaml | 1 + .../BUILD.in | 26 ++++++++++++ .../BUILD.out | 42 +++++++++++++++++++ .../README.md | 9 ++++ .../WORKSPACE | 1 + .../__init__.py | 0 .../bar.py | 1 + .../bar_test.py | 1 + .../baz.py | 1 + .../foo.py | 0 .../foo_test.py | 1 + .../test.yaml | 1 + .../simple_binary_with_library/BUILD.in | 2 +- .../simple_binary_with_library/BUILD.out | 3 +- .../simple_binary_with_library/README.md | 4 ++ 44 files changed, 257 insertions(+), 2 deletions(-) create mode 100644 gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/BUILD.in create mode 100644 gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/BUILD.out create mode 100644 gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/README.md create mode 100644 gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/WORKSPACE create mode 100644 gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/__init__.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/bar.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/bar_test.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/baz.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/foo.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/qux.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/test.yaml create mode 100644 gazelle/python/testdata/per_file_respect_existing_multiple_srcs/BUILD.in create mode 100644 gazelle/python/testdata/per_file_respect_existing_multiple_srcs/BUILD.out create mode 100644 gazelle/python/testdata/per_file_respect_existing_multiple_srcs/README.md create mode 100644 gazelle/python/testdata/per_file_respect_existing_multiple_srcs/WORKSPACE create mode 100644 gazelle/python/testdata/per_file_respect_existing_multiple_srcs/__init__.py create mode 100644 gazelle/python/testdata/per_file_respect_existing_multiple_srcs/bar.py create mode 100644 gazelle/python/testdata/per_file_respect_existing_multiple_srcs/bar_test.py create mode 100644 gazelle/python/testdata/per_file_respect_existing_multiple_srcs/baz.py create mode 100644 gazelle/python/testdata/per_file_respect_existing_multiple_srcs/foo.py create mode 100644 gazelle/python/testdata/per_file_respect_existing_multiple_srcs/foo_test.py create mode 100644 gazelle/python/testdata/per_file_respect_existing_multiple_srcs/test.yaml create mode 100644 gazelle/python/testdata/per_file_respect_existing_with_main_module/BUILD.in create mode 100644 gazelle/python/testdata/per_file_respect_existing_with_main_module/BUILD.out create mode 100644 gazelle/python/testdata/per_file_respect_existing_with_main_module/README.md create mode 100644 gazelle/python/testdata/per_file_respect_existing_with_main_module/WORKSPACE create mode 100644 gazelle/python/testdata/per_file_respect_existing_with_main_module/bar.py create mode 100644 gazelle/python/testdata/per_file_respect_existing_with_main_module/baz.py create mode 100644 gazelle/python/testdata/per_file_respect_existing_with_main_module/foo.py create mode 100644 gazelle/python/testdata/per_file_respect_existing_with_main_module/test.yaml create mode 100644 gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/BUILD.in create mode 100644 gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/BUILD.out create mode 100644 gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/README.md create mode 100644 gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/WORKSPACE create mode 100644 gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/__init__.py create mode 100644 gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/bar.py create mode 100644 gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/bar_test.py create mode 100644 gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/baz.py create mode 100644 gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/foo.py create mode 100644 gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/foo_test.py create mode 100644 gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/test.yaml diff --git a/gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/BUILD.in b/gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/BUILD.in new file mode 100644 index 0000000000..5b4869909e --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/BUILD.in @@ -0,0 +1,10 @@ +load("@rules_python//python:defs.bzl", "py_library") + +# Gazelle should preserve this custom source group and prune missing sources. +# Unclaimed sources should still go into the generated package target. +py_library( + name = "custom", + srcs = ["bar.py", "baz.py", "removed.py"], + visibility = ["//visibility:private"], + tags = ["keep_me"], +) diff --git a/gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/BUILD.out b/gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/BUILD.out new file mode 100644 index 0000000000..5dbd988d50 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/BUILD.out @@ -0,0 +1,30 @@ +load("@rules_python//python:defs.bzl", "py_library", "py_test") + +# Gazelle should preserve this custom source group and prune missing sources. +# Unclaimed sources should still go into the generated package target. +py_library( + name = "custom", + srcs = [ + "bar.py", + "baz.py", + ], + tags = ["keep_me"], + visibility = ["//visibility:private"], + deps = [":package_mode_respect_existing_multiple_srcs"], +) + +py_library( + name = "package_mode_respect_existing_multiple_srcs", + srcs = [ + "__init__.py", + "foo.py", + "qux.py", + ], + visibility = ["//:__subpackages__"], +) + +py_test( + name = "bar_test", + srcs = ["bar_test.py"], + deps = [":custom"], +) diff --git a/gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/README.md b/gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/README.md new file mode 100644 index 0000000000..79d828073d --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/README.md @@ -0,0 +1,8 @@ +# Package Mode With Existing Target Spanning Multiple Files + +This test verifies that default package generation preserves a non-standard +`py_library` that already owns multiple sources. + +Gazelle should prune sources that no longer exist, keep the target's +non-generated attributes, add generated dependencies, and put unclaimed sources +in the generated package target. diff --git a/gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/WORKSPACE b/gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/WORKSPACE new file mode 100644 index 0000000000..faff6af87a --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/WORKSPACE @@ -0,0 +1 @@ +# This is a Bazel workspace for the Gazelle test data. diff --git a/gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/__init__.py b/gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/bar.py b/gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/bar.py new file mode 100644 index 0000000000..ddf557475a --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/bar.py @@ -0,0 +1 @@ +import foo diff --git a/gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/bar_test.py b/gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/bar_test.py new file mode 100644 index 0000000000..b6b8723822 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/bar_test.py @@ -0,0 +1 @@ +import bar diff --git a/gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/baz.py b/gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/baz.py new file mode 100644 index 0000000000..ddf557475a --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/baz.py @@ -0,0 +1 @@ +import foo diff --git a/gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/foo.py b/gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/foo.py new file mode 100644 index 0000000000..6b58ff30a8 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/foo.py @@ -0,0 +1 @@ +# For test purposes only. diff --git a/gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/qux.py b/gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/qux.py new file mode 100644 index 0000000000..6b58ff30a8 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/qux.py @@ -0,0 +1 @@ +# For test purposes only. diff --git a/gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/test.yaml b/gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/test.yaml new file mode 100644 index 0000000000..ed97d539c0 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_multiple_srcs/test.yaml @@ -0,0 +1 @@ +--- diff --git a/gazelle/python/testdata/per_file_respect_existing_multiple_srcs/BUILD.in b/gazelle/python/testdata/per_file_respect_existing_multiple_srcs/BUILD.in new file mode 100644 index 0000000000..55883023d1 --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_multiple_srcs/BUILD.in @@ -0,0 +1,12 @@ +load("@rules_python//python:defs.bzl", "py_library") + +# gazelle:python_generation_mode file + +# Gazelle should preserve this custom source group and prune missing sources. +# Unclaimed sources should still get generated per-file targets. +py_library( + name = "custom", + srcs = ["bar.py", "baz.py", "removed.py"], + visibility = ["//visibility:private"], + tags = ["cant_touch_this"], +) diff --git a/gazelle/python/testdata/per_file_respect_existing_multiple_srcs/BUILD.out b/gazelle/python/testdata/per_file_respect_existing_multiple_srcs/BUILD.out new file mode 100644 index 0000000000..a0e9ce97c1 --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_multiple_srcs/BUILD.out @@ -0,0 +1,33 @@ +load("@rules_python//python:defs.bzl", "py_library", "py_test") + +# gazelle:python_generation_mode file + +# Gazelle should preserve this custom source group and prune missing sources. +# Unclaimed sources should still get generated per-file targets. +py_library( + name = "custom", + srcs = [ + "bar.py", + "baz.py", + ], + tags = ["cant_touch_this"], + visibility = ["//visibility:private"], + deps = [":foo"], +) + +py_library( + name = "foo", + srcs = ["foo.py"], + visibility = ["//:__subpackages__"], +) + +py_test( + name = "bar_test", + srcs = ["bar_test.py"], + deps = [":custom"], +) + +py_test( + name = "foo_test", + srcs = ["foo_test.py"], +) diff --git a/gazelle/python/testdata/per_file_respect_existing_multiple_srcs/README.md b/gazelle/python/testdata/per_file_respect_existing_multiple_srcs/README.md new file mode 100644 index 0000000000..1998e70a6e --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_multiple_srcs/README.md @@ -0,0 +1,8 @@ +# Per-File Generation With Existing Target Spanning Multiple Files + +This test verifies that per-file generation preserves a non-standard +`py_library` that already owns multiple sources. + +Gazelle should prune sources that no longer exist, keep the target's +non-generated attributes, add generated dependencies, and create per-file +targets only for unclaimed files. diff --git a/gazelle/python/testdata/per_file_respect_existing_multiple_srcs/WORKSPACE b/gazelle/python/testdata/per_file_respect_existing_multiple_srcs/WORKSPACE new file mode 100644 index 0000000000..faff6af87a --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_multiple_srcs/WORKSPACE @@ -0,0 +1 @@ +# This is a Bazel workspace for the Gazelle test data. diff --git a/gazelle/python/testdata/per_file_respect_existing_multiple_srcs/__init__.py b/gazelle/python/testdata/per_file_respect_existing_multiple_srcs/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/gazelle/python/testdata/per_file_respect_existing_multiple_srcs/bar.py b/gazelle/python/testdata/per_file_respect_existing_multiple_srcs/bar.py new file mode 100644 index 0000000000..6b58ff30a8 --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_multiple_srcs/bar.py @@ -0,0 +1 @@ +# For test purposes only. diff --git a/gazelle/python/testdata/per_file_respect_existing_multiple_srcs/bar_test.py b/gazelle/python/testdata/per_file_respect_existing_multiple_srcs/bar_test.py new file mode 100644 index 0000000000..b6b8723822 --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_multiple_srcs/bar_test.py @@ -0,0 +1 @@ +import bar diff --git a/gazelle/python/testdata/per_file_respect_existing_multiple_srcs/baz.py b/gazelle/python/testdata/per_file_respect_existing_multiple_srcs/baz.py new file mode 100644 index 0000000000..ddf557475a --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_multiple_srcs/baz.py @@ -0,0 +1 @@ +import foo diff --git a/gazelle/python/testdata/per_file_respect_existing_multiple_srcs/foo.py b/gazelle/python/testdata/per_file_respect_existing_multiple_srcs/foo.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/gazelle/python/testdata/per_file_respect_existing_multiple_srcs/foo_test.py b/gazelle/python/testdata/per_file_respect_existing_multiple_srcs/foo_test.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/gazelle/python/testdata/per_file_respect_existing_multiple_srcs/test.yaml b/gazelle/python/testdata/per_file_respect_existing_multiple_srcs/test.yaml new file mode 100644 index 0000000000..ed97d539c0 --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_multiple_srcs/test.yaml @@ -0,0 +1 @@ +--- diff --git a/gazelle/python/testdata/per_file_respect_existing_with_main_module/BUILD.in b/gazelle/python/testdata/per_file_respect_existing_with_main_module/BUILD.in new file mode 100644 index 0000000000..1433adebec --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_with_main_module/BUILD.in @@ -0,0 +1,12 @@ +load("@rules_python//python:defs.bzl", "py_library") + +# gazelle:python_generation_mode file + +# Gazelle should extract bar.py into a py_binary because it has a main module. +# The preserved library should keep only its remaining valid sources. +py_library( + name = "custom", + srcs = ["bar.py", "baz.py"], + visibility = ["//visibility:private"], + tags = ["keep_me"], +) diff --git a/gazelle/python/testdata/per_file_respect_existing_with_main_module/BUILD.out b/gazelle/python/testdata/per_file_respect_existing_with_main_module/BUILD.out new file mode 100644 index 0000000000..5cdb2621d1 --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_with_main_module/BUILD.out @@ -0,0 +1,26 @@ +load("@rules_python//python:defs.bzl", "py_binary", "py_library") + +# gazelle:python_generation_mode file + +# Gazelle should extract bar.py into a py_binary because it has a main module. +# The preserved library should keep only its remaining valid sources. +py_library( + name = "custom", + srcs = ["baz.py"], + tags = ["keep_me"], + visibility = ["//visibility:private"], + deps = [":foo"], +) + +py_binary( + name = "bar", + srcs = ["bar.py"], + visibility = ["//:__subpackages__"], + deps = [":foo"], +) + +py_library( + name = "foo", + srcs = ["foo.py"], + visibility = ["//:__subpackages__"], +) diff --git a/gazelle/python/testdata/per_file_respect_existing_with_main_module/README.md b/gazelle/python/testdata/per_file_respect_existing_with_main_module/README.md new file mode 100644 index 0000000000..0f3df6ae4e --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_with_main_module/README.md @@ -0,0 +1,8 @@ +# Per-File Generation With Preserved Target Containing a Main Module + +This test verifies that per-file generation still extracts a `py_binary` when a +preserved target contains a source with `if __name__ == "__main__":`. + +Gazelle should remove the main-module source from the preserved `py_library`, +keep the remaining sources and non-generated attributes, and generate the +matching `py_binary`. diff --git a/gazelle/python/testdata/per_file_respect_existing_with_main_module/WORKSPACE b/gazelle/python/testdata/per_file_respect_existing_with_main_module/WORKSPACE new file mode 100644 index 0000000000..faff6af87a --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_with_main_module/WORKSPACE @@ -0,0 +1 @@ +# This is a Bazel workspace for the Gazelle test data. diff --git a/gazelle/python/testdata/per_file_respect_existing_with_main_module/bar.py b/gazelle/python/testdata/per_file_respect_existing_with_main_module/bar.py new file mode 100644 index 0000000000..6b2c4bbce6 --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_with_main_module/bar.py @@ -0,0 +1,4 @@ +import foo + +if __name__ == "__main__": + pass diff --git a/gazelle/python/testdata/per_file_respect_existing_with_main_module/baz.py b/gazelle/python/testdata/per_file_respect_existing_with_main_module/baz.py new file mode 100644 index 0000000000..ddf557475a --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_with_main_module/baz.py @@ -0,0 +1 @@ +import foo diff --git a/gazelle/python/testdata/per_file_respect_existing_with_main_module/foo.py b/gazelle/python/testdata/per_file_respect_existing_with_main_module/foo.py new file mode 100644 index 0000000000..6b58ff30a8 --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_with_main_module/foo.py @@ -0,0 +1 @@ +# For test purposes only. diff --git a/gazelle/python/testdata/per_file_respect_existing_with_main_module/test.yaml b/gazelle/python/testdata/per_file_respect_existing_with_main_module/test.yaml new file mode 100644 index 0000000000..ed97d539c0 --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_with_main_module/test.yaml @@ -0,0 +1 @@ +--- diff --git a/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/BUILD.in b/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/BUILD.in new file mode 100644 index 0000000000..a39f90e6c4 --- /dev/null +++ b/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/BUILD.in @@ -0,0 +1,26 @@ +load("@rules_python//python:defs.bzl", "py_library") + +# gazelle:python_generation_mode project + +# This target already matches the generated project package and should remain +# unchanged. +py_library( + name = "__init__", + srcs = ["__init__.py"], + visibility = ["//visibility:private"], +) + +# Gazelle should preserve this custom source group and prune missing sources. +py_library( + name = "custom", + srcs = ["bar.py", "baz.py", "removed.py"], + visibility = ["//visibility:private"], + tags = ["cant_touch_this"], +) + + +# Gazelle should preserve this custom test target and add generated deps. +py_test( + name = "foo_test", + srcs = ["foo_test.py"], +) diff --git a/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/BUILD.out b/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/BUILD.out new file mode 100644 index 0000000000..25ab8b9e93 --- /dev/null +++ b/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/BUILD.out @@ -0,0 +1,42 @@ +load("@rules_python//python:defs.bzl", "py_library", "py_test") + +# gazelle:python_generation_mode project + +# This target already matches the generated project package and should remain +# unchanged. +py_library( + name = "__init__", + srcs = ["__init__.py"], + visibility = ["//visibility:private"], +) + +# Gazelle should preserve this custom source group and prune missing sources. +py_library( + name = "custom", + srcs = [ + "bar.py", + "baz.py", + ], + tags = ["cant_touch_this"], + visibility = ["//visibility:private"], + deps = [":project_generation_mode_respect_existing_multiple_srcs"], +) + +# Gazelle should preserve this custom test target and add generated deps. +py_test( + name = "foo_test", + srcs = ["foo_test.py"], + deps = [":project_generation_mode_respect_existing_multiple_srcs"], +) + +py_library( + name = "project_generation_mode_respect_existing_multiple_srcs", + srcs = ["foo.py"], + visibility = ["//:__subpackages__"], +) + +py_test( + name = "project_generation_mode_respect_existing_multiple_srcs_test", + srcs = ["bar_test.py"], + deps = [":custom"], +) diff --git a/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/README.md b/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/README.md new file mode 100644 index 0000000000..98a129559a --- /dev/null +++ b/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/README.md @@ -0,0 +1,9 @@ +# Project Generation With Existing Target Spanning Multiple Files + +This test verifies that project generation preserves existing non-standard +`py_library` and `py_test` targets while still generating project-wide targets +for unclaimed sources. + +Gazelle should prune sources that no longer exist, keep non-generated +attributes, add generated dependencies, and leave the existing `__init__` target +unchanged. diff --git a/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/WORKSPACE b/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/WORKSPACE new file mode 100644 index 0000000000..faff6af87a --- /dev/null +++ b/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/WORKSPACE @@ -0,0 +1 @@ +# This is a Bazel workspace for the Gazelle test data. diff --git a/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/__init__.py b/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/bar.py b/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/bar.py new file mode 100644 index 0000000000..6b58ff30a8 --- /dev/null +++ b/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/bar.py @@ -0,0 +1 @@ +# For test purposes only. diff --git a/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/bar_test.py b/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/bar_test.py new file mode 100644 index 0000000000..b6b8723822 --- /dev/null +++ b/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/bar_test.py @@ -0,0 +1 @@ +import bar diff --git a/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/baz.py b/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/baz.py new file mode 100644 index 0000000000..ddf557475a --- /dev/null +++ b/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/baz.py @@ -0,0 +1 @@ +import foo diff --git a/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/foo.py b/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/foo.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/foo_test.py b/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/foo_test.py new file mode 100644 index 0000000000..ddf557475a --- /dev/null +++ b/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/foo_test.py @@ -0,0 +1 @@ +import foo diff --git a/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/test.yaml b/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/test.yaml new file mode 100644 index 0000000000..ed97d539c0 --- /dev/null +++ b/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/test.yaml @@ -0,0 +1 @@ +--- diff --git a/gazelle/python/testdata/simple_binary_with_library/BUILD.in b/gazelle/python/testdata/simple_binary_with_library/BUILD.in index b60e84f17e..1df483ec35 100644 --- a/gazelle/python/testdata/simple_binary_with_library/BUILD.in +++ b/gazelle/python/testdata/simple_binary_with_library/BUILD.in @@ -9,7 +9,7 @@ py_library( ], ) -# This target should be kept unmodified by Gazelle. +# Gazelle should preserve this custom target and keep bar.py in both libraries. py_library( name = "custom", srcs = [ diff --git a/gazelle/python/testdata/simple_binary_with_library/BUILD.out b/gazelle/python/testdata/simple_binary_with_library/BUILD.out index eddc15cacd..d7a04ade3e 100644 --- a/gazelle/python/testdata/simple_binary_with_library/BUILD.out +++ b/gazelle/python/testdata/simple_binary_with_library/BUILD.out @@ -10,12 +10,13 @@ py_library( visibility = ["//:__subpackages__"], ) -# This target should be kept unmodified by Gazelle. +# Gazelle should preserve this custom target and keep bar.py in both libraries. py_library( name = "custom", srcs = [ "bar.py", ], + visibility = ["//:__subpackages__"], ) py_binary( diff --git a/gazelle/python/testdata/simple_binary_with_library/README.md b/gazelle/python/testdata/simple_binary_with_library/README.md index cfc81a3581..ea2686122c 100644 --- a/gazelle/python/testdata/simple_binary_with_library/README.md +++ b/gazelle/python/testdata/simple_binary_with_library/README.md @@ -2,3 +2,7 @@ This test case asserts that a simple `py_binary` is generated as expected referencing a `py_library`. + +The existing custom `py_library` shares `bar.py` with the generated package +library. Gazelle should preserve that shared source in both targets while adding +generated attributes such as `visibility`. From e9b806f358ab5cb83dc599ab9d04a6ee68e80927 Mon Sep 17 00:00:00 2001 From: Alex Martani Date: Sat, 2 May 2026 10:43:08 -0700 Subject: [PATCH 03/18] fix(gazelle): preserve existing Python source targets Teach the Python generator to account for existing source-group rules so custom targets keep ownership of their sources while stale files are pruned and dependencies are resolved. --- gazelle/python/generate.go | 157 +++++++++++++++++++++++++++++++++++-- 1 file changed, 152 insertions(+), 5 deletions(-) diff --git a/gazelle/python/generate.go b/gazelle/python/generate.go index 90e06a1546..771f101814 100644 --- a/gazelle/python/generate.go +++ b/gazelle/python/generate.go @@ -48,6 +48,12 @@ var ( buildFilenames = []string{"BUILD", "BUILD.bazel"} ) +type existingPythonSourceRule struct { + kind string + name string + srcs *treeset.Set +} + // Returns the mapped kind, or kind if no mapping is configured with the map_kind directive. func getMappedKind(c *config.Config, kind string) string { if mapped, ok := c.KindMap[kind]; ok { @@ -74,6 +80,93 @@ func matchesAnyGlob(s string, globs []string) bool { return false } +func isTargetSrc(src string) bool { + return strings.HasPrefix(src, "@") || strings.HasPrefix(src, "//") || strings.HasPrefix(src, ":") +} + +func collectExistingPythonSourceRules(c *config.Config, file *rule.File, kind string, knownSrcs map[string]struct{}) []existingPythonSourceRule { + if file == nil { + return nil + } + + var sourceRules []existingPythonSourceRule + for _, existingRule := range file.Rules { + if !kindMatches(c, existingRule, kind) { + continue + } + + srcs := existingRule.AttrStrings("srcs") + if len(srcs) == 0 { + continue + } + + validSrcs := treeset.NewWith(godsutils.StringComparator) + skip := false + for _, src := range srcs { + if isTargetSrc(src) || filepath.Ext(src) != ".py" { + skip = true + break + } + if _, ok := knownSrcs[src]; ok { + validSrcs.Add(src) + } + } + if skip { + continue + } + if validSrcs.Empty() { + continue + } + + sourceRules = append(sourceRules, existingPythonSourceRule{ + kind: kind, + name: existingRule.Name(), + srcs: validSrcs, + }) + } + return sourceRules +} + +func addSetValuesToMap(srcs *treeset.Set, dst map[string]struct{}) { + it := srcs.Iterator() + for it.Next() { + dst[it.Value().(string)] = struct{}{} + } +} + +func removeClaimedSrcs(rules []existingPythonSourceRule, srcSets ...*treeset.Set) { + for _, sourceRule := range rules { + it := sourceRule.srcs.Iterator() + for it.Next() { + src := it.Value().(string) + for _, srcSet := range srcSets { + srcSet.Remove(src) + } + } + } +} + +func filterExistingPythonSourceRules(rules []existingPythonSourceRule, shouldKeep func(existingPythonSourceRule) bool) []existingPythonSourceRule { + filtered := make([]existingPythonSourceRule, 0, len(rules)) + for _, sourceRule := range rules { + if shouldKeep(sourceRule) { + filtered = append(filtered, sourceRule) + } + } + return filtered +} + +func sourceRuleMatchesGeneratedPerFileName(sourceRule existingPythonSourceRule) bool { + it := sourceRule.srcs.Iterator() + for it.Next() { + src := it.Value().(string) + if sourceRule.name == strings.TrimSuffix(filepath.Base(src), ".py") { + return true + } + } + return false +} + // findConftestPaths returns package paths containing conftest.py, from currentPkg // up through ancestors, stopping at module root. func findConftestPaths(repoRoot, currentPkg, pythonProjectRoot string, includeAncestorConftest bool) []string { @@ -271,6 +364,51 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes autoIncludeInit = cfg.PerFileGenerationIncludeInit() && hasInit && hasPopulatedInit } + knownPySrcs := make(map[string]struct{}) + addSetValuesToMap(pyLibraryFilenames, knownPySrcs) + addSetValuesToMap(pyTestFilenames, knownPySrcs) + for _, src := range []struct { + name string + present bool + }{ + {pyBinaryEntrypointFilename, hasPyBinaryEntryPointFile}, + {pyTestEntrypointFilename, hasPyTestEntryPointFile}, + {conftestFilename, hasConftestFile}, + } { + if src.present { + knownPySrcs[src.name] = struct{}{} + } + } + + existingPyLibraries := collectExistingPythonSourceRules(args.Config, args.File, pyLibraryKind, knownPySrcs) + existingPyTests := collectExistingPythonSourceRules(args.Config, args.File, pyTestKind, knownPySrcs) + existingPyLibraries = filterExistingPythonSourceRules( + existingPyLibraries, + func(sourceRule existingPythonSourceRule) bool { + if !cfg.PerFileGeneration() { + return sourceRule.name != cfg.RenderLibraryName(packageName) + } + return !sourceRuleMatchesGeneratedPerFileName(sourceRule) + }, + ) + existingPyTests = filterExistingPythonSourceRules( + existingPyTests, + func(sourceRule existingPythonSourceRule) bool { + if !cfg.PerFileGeneration() { + return sourceRule.name != cfg.RenderTestName(packageName) + } + return !sourceRuleMatchesGeneratedPerFileName(sourceRule) + }, + ) + claimingPyLibraries := filterExistingPythonSourceRules( + existingPyLibraries, + func(sourceRule existingPythonSourceRule) bool { + return sourceRule.srcs.Size() > 1 || cfg.CoarseGrainedGeneration() + }, + ) + removeClaimedSrcs(claimingPyLibraries, pyLibraryFilenames, pyTestFilenames) + removeClaimedSrcs(existingPyTests, pyLibraryFilenames, pyTestFilenames) + appendPyLibrary := func(srcs *treeset.Set, pyLibraryTargetName string) { allDeps, mainModules, annotations, err := parser.parse(srcs) for name := range mainModules { @@ -380,6 +518,10 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes } } + for _, existingPyLibrary := range existingPyLibraries { + appendPyLibrary(existingPyLibrary.srcs, existingPyLibrary.name) + } + if cfg.PerFileGeneration() { pyLibraryFilenames.Each(func(index int, filename interface{}) { pyLibraryTargetName := strings.TrimSuffix(filepath.Base(filename.(string)), ".py") @@ -503,6 +645,15 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes setAnnotations(*annotations). generateImportsAttribute() } + + for _, existingPyTest := range existingPyTests { + if existingPyTest.srcs.Empty() { + result.Empty = append(result.Empty, rule.NewRule(existingPyTest.kind, existingPyTest.name)) + continue + } + pyTestTargets = append(pyTestTargets, newPyTestTargetBuilder(existingPyTest.srcs, existingPyTest.name)) + } + if (!cfg.PerPackageGenerationRequireTestEntryPoint() || hasPyTestEntryPointFile || hasPyTestEntryPointTarget || cfg.CoarseGrainedGeneration()) && !cfg.PerFileGeneration() { // Create one py_test target per package if hasPyTestEntryPointFile { @@ -603,10 +754,6 @@ func (py *Python) getRulesWithInvalidSrcs(args language.GenerateArgs, validFiles for _, file := range args.RegularFiles { allFilesMap[file] = struct{}{} } - - isTarget := func(src string) bool { - return strings.HasPrefix(src, "@") || strings.HasPrefix(src, "//") || strings.HasPrefix(src, ":") - } for _, existingRule := range args.File.Rules { var matchedKind string var filesMap map[string]struct{} @@ -629,7 +776,7 @@ func (py *Python) getRulesWithInvalidSrcs(args language.GenerateArgs, validFiles } var hasValidSrcs bool for _, src := range srcs { - if isTarget(src) { + if isTargetSrc(src) { hasValidSrcs = true break } From a2ad17c4e33a76b2e3a7f2d00f383e4c47c99bd8 Mon Sep 17 00:00:00 2001 From: Alex Martani Date: Fri, 14 Aug 2026 21:09:14 -0700 Subject: [PATCH 04/18] fix(gazelle): don't drop unmanaged srcs or duplicate py_binary targets Preserving existing py_library/py_test targets introduced three defects, each covered by a new testdata case. Sources listed by a preserved target were pruned unless Gazelle itself would have generated them, but that set excludes files hidden by python_ignore_files, gazelle:exclude, and subdirectories in per-package mode. Since srcs is mergeable, those sources were deleted from the user's BUILD file even though they exist on disk. Pruning now keys off whether the file exists; membership in the managed set only decides whether the rule is adopted at all, so a rule built entirely from ignored sources stays untouched as before. A preserved target with a single source does not claim it, so appendPyLibrary saw the same file twice and extracted its main module into two identical py_binary targets, which Gazelle rejected with "multiple rules found with label". Main modules are now extracted once per source file. Extracting a main module in per-file mode also removed __init__.py from the target, on the assumption that the caller had just added it via python_generation_mode_per_file_include_init. For a preserved target that had listed __init__.py by hand this emptied its srcs and deleted it. Whether __init__.py was auto-included is now passed explicitly. Co-Authored-By: Claude Opus 5 (1M context) --- gazelle/python/generate.go | 70 +++++++++++++++---- .../BUILD.in | 10 +++ .../BUILD.out | 29 ++++++++ .../README.md | 9 +++ .../WORKSPACE | 1 + .../__init__.py | 0 .../cli.py | 4 ++ .../foo.py | 1 + .../test.yaml | 1 + .../BUILD.in | 17 +++++ .../BUILD.out | 24 +++++++ .../README.md | 8 +++ .../WORKSPACE | 1 + .../bar.py | 1 + .../excluded.py | 1 + .../foo.py | 1 + .../ignored.py | 1 + .../test.yaml | 1 + .../BUILD.in | 15 ++++ .../BUILD.out | 33 +++++++++ .../README.md | 13 ++++ .../WORKSPACE | 1 + .../__init__.py | 1 + .../cli.py | 4 ++ .../foo.py | 1 + .../test.yaml | 1 + 26 files changed, 237 insertions(+), 12 deletions(-) create mode 100644 gazelle/python/testdata/package_mode_respect_existing_single_src_main_module/BUILD.in create mode 100644 gazelle/python/testdata/package_mode_respect_existing_single_src_main_module/BUILD.out create mode 100644 gazelle/python/testdata/package_mode_respect_existing_single_src_main_module/README.md create mode 100644 gazelle/python/testdata/package_mode_respect_existing_single_src_main_module/WORKSPACE create mode 100644 gazelle/python/testdata/package_mode_respect_existing_single_src_main_module/__init__.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_single_src_main_module/cli.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_single_src_main_module/foo.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_single_src_main_module/test.yaml create mode 100644 gazelle/python/testdata/package_mode_respect_existing_unmanaged_srcs/BUILD.in create mode 100644 gazelle/python/testdata/package_mode_respect_existing_unmanaged_srcs/BUILD.out create mode 100644 gazelle/python/testdata/package_mode_respect_existing_unmanaged_srcs/README.md create mode 100644 gazelle/python/testdata/package_mode_respect_existing_unmanaged_srcs/WORKSPACE create mode 100644 gazelle/python/testdata/package_mode_respect_existing_unmanaged_srcs/bar.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_unmanaged_srcs/excluded.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_unmanaged_srcs/foo.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_unmanaged_srcs/ignored.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_unmanaged_srcs/test.yaml create mode 100644 gazelle/python/testdata/per_file_respect_existing_with_init_and_main_module/BUILD.in create mode 100644 gazelle/python/testdata/per_file_respect_existing_with_init_and_main_module/BUILD.out create mode 100644 gazelle/python/testdata/per_file_respect_existing_with_init_and_main_module/README.md create mode 100644 gazelle/python/testdata/per_file_respect_existing_with_init_and_main_module/WORKSPACE create mode 100644 gazelle/python/testdata/per_file_respect_existing_with_init_and_main_module/__init__.py create mode 100644 gazelle/python/testdata/per_file_respect_existing_with_init_and_main_module/cli.py create mode 100644 gazelle/python/testdata/per_file_respect_existing_with_init_and_main_module/foo.py create mode 100644 gazelle/python/testdata/per_file_respect_existing_with_init_and_main_module/test.yaml diff --git a/gazelle/python/generate.go b/gazelle/python/generate.go index 771f101814..44dd45e76c 100644 --- a/gazelle/python/generate.go +++ b/gazelle/python/generate.go @@ -84,14 +84,36 @@ func isTargetSrc(src string) bool { return strings.HasPrefix(src, "@") || strings.HasPrefix(src, "//") || strings.HasPrefix(src, ":") } -func collectExistingPythonSourceRules(c *config.Config, file *rule.File, kind string, knownSrcs map[string]struct{}) []existingPythonSourceRule { - if file == nil { +// collectExistingPythonSourceRules returns the rules of the canonical kind +// `kind` that Gazelle should regenerate in place rather than replace. knownSrcs +// holds the source files Gazelle would itself put in a generated target's srcs. +// +// A rule is only adopted if at least one of its srcs is in knownSrcs; a rule +// built entirely from sources Gazelle was told to leave alone is left alone too. +// Once adopted, srcs that exist but are absent from knownSrcs are still kept: +// python_ignore_files, gazelle:exclude and subdirectory sources are hidden from +// generation, which must not cause Gazelle to delete them from a hand-written +// target. Only srcs that no longer exist are pruned. +func collectExistingPythonSourceRules(args language.GenerateArgs, kind string, knownSrcs map[string]struct{}) []existingPythonSourceRule { + if args.File == nil { return nil } + genFiles := make(map[string]struct{}, len(args.GenFiles)) + for _, f := range args.GenFiles { + genFiles[f] = struct{}{} + } + srcExists := func(src string) bool { + if _, ok := genFiles[src]; ok { + return true + } + _, err := os.Stat(filepath.Join(args.Dir, src)) + return err == nil + } + var sourceRules []existingPythonSourceRule - for _, existingRule := range file.Rules { - if !kindMatches(c, existingRule, kind) { + for _, existingRule := range args.File.Rules { + if !kindMatches(args.Config, existingRule, kind) { continue } @@ -102,19 +124,23 @@ func collectExistingPythonSourceRules(c *config.Config, file *rule.File, kind st validSrcs := treeset.NewWith(godsutils.StringComparator) skip := false + hasKnownSrc := false for _, src := range srcs { if isTargetSrc(src) || filepath.Ext(src) != ".py" { skip = true break } if _, ok := knownSrcs[src]; ok { + hasKnownSrc = true + validSrcs.Add(src) + } else if srcExists(src) { validSrcs.Add(src) } } if skip { continue } - if validSrcs.Empty() { + if !hasKnownSrc { continue } @@ -364,6 +390,12 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes autoIncludeInit = cfg.PerFileGenerationIncludeInit() && hasInit && hasPopulatedInit } + // knownPySrcs is the set of source files Gazelle manages in this package, i.e. + // the ones it would put in a generated target's srcs. It is narrower than "the + // .py files that exist here": files hidden by python_ignore_files or + // gazelle:exclude, and subdirectory files in per-package mode, are absent. + // The entrypoints and conftest.py are diverted out of pyLibraryFilenames and + // pyTestFilenames by the scan above, so they are added back explicitly. knownPySrcs := make(map[string]struct{}) addSetValuesToMap(pyLibraryFilenames, knownPySrcs) addSetValuesToMap(pyTestFilenames, knownPySrcs) @@ -380,8 +412,8 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes } } - existingPyLibraries := collectExistingPythonSourceRules(args.Config, args.File, pyLibraryKind, knownPySrcs) - existingPyTests := collectExistingPythonSourceRules(args.Config, args.File, pyTestKind, knownPySrcs) + existingPyLibraries := collectExistingPythonSourceRules(args, pyLibraryKind, knownPySrcs) + existingPyTests := collectExistingPythonSourceRules(args, pyTestKind, knownPySrcs) existingPyLibraries = filterExistingPythonSourceRules( existingPyLibraries, func(sourceRule existingPythonSourceRule) bool { @@ -409,7 +441,16 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes removeClaimedSrcs(claimingPyLibraries, pyLibraryFilenames, pyTestFilenames) removeClaimedSrcs(existingPyTests, pyLibraryFilenames, pyTestFilenames) - appendPyLibrary := func(srcs *treeset.Set, pyLibraryTargetName string) { + // extractedMainModules tracks the main modules that already have a generated + // py_binary target. A source file can be owned by both a preserved target and + // a generated one, in which case appendPyLibrary sees it twice and would + // otherwise emit a duplicate py_binary for it. + extractedMainModules := make(map[string]struct{}) + + // autoIncludedInit reports whether the caller added pyLibraryEntrypointFilename + // to srcs itself, rather than it being a source the user listed by hand. Only + // in the former case may it be removed again when a main module is extracted. + appendPyLibrary := func(srcs *treeset.Set, pyLibraryTargetName string, autoIncludedInit bool) { allDeps, mainModules, annotations, err := parser.parse(srcs) for name := range mainModules { validFilesMap[name] = struct{}{} @@ -429,7 +470,7 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes if cfg.PerFileGeneration() { srcs.Remove(name) // Also remove the __init__.py that was added earlier. - if autoIncludeInit { + if autoIncludedInit { srcs.Remove(pyLibraryEntrypointFilename) } } @@ -437,6 +478,11 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes sort.Strings(mainFileNames) for _, filename := range mainFileNames { + if _, ok := extractedMainModules[filename]; ok { + continue + } + extractedMainModules[filename] = struct{}{} + pyBinaryTargetName := strings.TrimSuffix(filepath.Base(filename), ".py") if err := ensureNoCollision(args.Config, args.File, pyBinaryTargetName, pyBinaryKind); err != nil { fqTarget := label.New("", args.Rel, pyBinaryTargetName) @@ -519,7 +565,7 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes } for _, existingPyLibrary := range existingPyLibraries { - appendPyLibrary(existingPyLibrary.srcs, existingPyLibrary.name) + appendPyLibrary(existingPyLibrary.srcs, existingPyLibrary.name, false) } if cfg.PerFileGeneration() { @@ -532,10 +578,10 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes if autoIncludeInit { srcs.Add(pyLibraryEntrypointFilename) } - appendPyLibrary(srcs, pyLibraryTargetName) + appendPyLibrary(srcs, pyLibraryTargetName, autoIncludeInit) }) } else { - appendPyLibrary(pyLibraryFilenames, cfg.RenderLibraryName(packageName)) + appendPyLibrary(pyLibraryFilenames, cfg.RenderLibraryName(packageName), false) } if hasPyBinaryEntryPointFile { diff --git a/gazelle/python/testdata/package_mode_respect_existing_single_src_main_module/BUILD.in b/gazelle/python/testdata/package_mode_respect_existing_single_src_main_module/BUILD.in new file mode 100644 index 0000000000..11866a140a --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_single_src_main_module/BUILD.in @@ -0,0 +1,10 @@ +load("@rules_python//python:defs.bzl", "py_library") + +# Gazelle should preserve this custom target. Because it has a single source, +# cli.py stays in the generated package target too, but only one py_binary +# should be generated for it. +py_library( + name = "custom", + srcs = ["cli.py"], + tags = ["keep_me"], +) diff --git a/gazelle/python/testdata/package_mode_respect_existing_single_src_main_module/BUILD.out b/gazelle/python/testdata/package_mode_respect_existing_single_src_main_module/BUILD.out new file mode 100644 index 0000000000..b11c1e365b --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_single_src_main_module/BUILD.out @@ -0,0 +1,29 @@ +load("@rules_python//python:defs.bzl", "py_binary", "py_library") + +# Gazelle should preserve this custom target. Because it has a single source, +# cli.py stays in the generated package target too, but only one py_binary +# should be generated for it. +py_library( + name = "custom", + srcs = ["cli.py"], + tags = ["keep_me"], + visibility = ["//:__subpackages__"], + deps = [":package_mode_respect_existing_single_src_main_module"], +) + +py_binary( + name = "cli", + srcs = ["cli.py"], + visibility = ["//:__subpackages__"], + deps = [":package_mode_respect_existing_single_src_main_module"], +) + +py_library( + name = "package_mode_respect_existing_single_src_main_module", + srcs = [ + "__init__.py", + "cli.py", + "foo.py", + ], + visibility = ["//:__subpackages__"], +) diff --git a/gazelle/python/testdata/package_mode_respect_existing_single_src_main_module/README.md b/gazelle/python/testdata/package_mode_respect_existing_single_src_main_module/README.md new file mode 100644 index 0000000000..d3d69e3675 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_single_src_main_module/README.md @@ -0,0 +1,9 @@ +# Package Mode With Preserved Single-Source Target Containing a Main Module + +This test verifies that a main module owned by a preserved target yields exactly +one `py_binary`. + +A preserved target with a single source does not claim it, so the source is also +part of the generated package target. Gazelle must still extract the main module +only once instead of emitting a duplicate `py_binary` for each target that owns +the source. diff --git a/gazelle/python/testdata/package_mode_respect_existing_single_src_main_module/WORKSPACE b/gazelle/python/testdata/package_mode_respect_existing_single_src_main_module/WORKSPACE new file mode 100644 index 0000000000..faff6af87a --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_single_src_main_module/WORKSPACE @@ -0,0 +1 @@ +# This is a Bazel workspace for the Gazelle test data. diff --git a/gazelle/python/testdata/package_mode_respect_existing_single_src_main_module/__init__.py b/gazelle/python/testdata/package_mode_respect_existing_single_src_main_module/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/gazelle/python/testdata/package_mode_respect_existing_single_src_main_module/cli.py b/gazelle/python/testdata/package_mode_respect_existing_single_src_main_module/cli.py new file mode 100644 index 0000000000..07cb4bb282 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_single_src_main_module/cli.py @@ -0,0 +1,4 @@ +import foo + +if __name__ == "__main__": + print(foo) diff --git a/gazelle/python/testdata/package_mode_respect_existing_single_src_main_module/foo.py b/gazelle/python/testdata/package_mode_respect_existing_single_src_main_module/foo.py new file mode 100644 index 0000000000..6b58ff30a8 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_single_src_main_module/foo.py @@ -0,0 +1 @@ +# For test purposes only. diff --git a/gazelle/python/testdata/package_mode_respect_existing_single_src_main_module/test.yaml b/gazelle/python/testdata/package_mode_respect_existing_single_src_main_module/test.yaml new file mode 100644 index 0000000000..ed97d539c0 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_single_src_main_module/test.yaml @@ -0,0 +1 @@ +--- diff --git a/gazelle/python/testdata/package_mode_respect_existing_unmanaged_srcs/BUILD.in b/gazelle/python/testdata/package_mode_respect_existing_unmanaged_srcs/BUILD.in new file mode 100644 index 0000000000..88b41a2764 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_unmanaged_srcs/BUILD.in @@ -0,0 +1,17 @@ +load("@rules_python//python:defs.bzl", "py_library") + +# gazelle:python_ignore_files ignored.py +# gazelle:exclude excluded.py + +# Gazelle should keep excluded.py and ignored.py because both exist on disk, +# and prune only removed.py. +py_library( + name = "custom", + srcs = [ + "excluded.py", + "foo.py", + "ignored.py", + "removed.py", + ], + tags = ["keep_me"], +) diff --git a/gazelle/python/testdata/package_mode_respect_existing_unmanaged_srcs/BUILD.out b/gazelle/python/testdata/package_mode_respect_existing_unmanaged_srcs/BUILD.out new file mode 100644 index 0000000000..64ba0815c6 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_unmanaged_srcs/BUILD.out @@ -0,0 +1,24 @@ +load("@rules_python//python:defs.bzl", "py_library") + +# gazelle:python_ignore_files ignored.py +# gazelle:exclude excluded.py + +# Gazelle should keep excluded.py and ignored.py because both exist on disk, +# and prune only removed.py. +py_library( + name = "custom", + srcs = [ + "excluded.py", + "foo.py", + "ignored.py", + ], + tags = ["keep_me"], + visibility = ["//:__subpackages__"], +) + +py_library( + name = "package_mode_respect_existing_unmanaged_srcs", + srcs = ["bar.py"], + visibility = ["//:__subpackages__"], + deps = [":custom"], +) diff --git a/gazelle/python/testdata/package_mode_respect_existing_unmanaged_srcs/README.md b/gazelle/python/testdata/package_mode_respect_existing_unmanaged_srcs/README.md new file mode 100644 index 0000000000..64277b0991 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_unmanaged_srcs/README.md @@ -0,0 +1,8 @@ +# Package Mode With Preserved Target Owning Unmanaged Sources + +This test verifies that Gazelle only prunes sources that do not exist on disk. + +`ignored.py` and `excluded.py` exist but are hidden from generation by +`python_ignore_files` and `gazelle:exclude`. Those directives suppress +generation; they must not cause Gazelle to delete the sources from a +hand-written target. Only `removed.py`, which does not exist, is pruned. diff --git a/gazelle/python/testdata/package_mode_respect_existing_unmanaged_srcs/WORKSPACE b/gazelle/python/testdata/package_mode_respect_existing_unmanaged_srcs/WORKSPACE new file mode 100644 index 0000000000..faff6af87a --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_unmanaged_srcs/WORKSPACE @@ -0,0 +1 @@ +# This is a Bazel workspace for the Gazelle test data. diff --git a/gazelle/python/testdata/package_mode_respect_existing_unmanaged_srcs/bar.py b/gazelle/python/testdata/package_mode_respect_existing_unmanaged_srcs/bar.py new file mode 100644 index 0000000000..ddf557475a --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_unmanaged_srcs/bar.py @@ -0,0 +1 @@ +import foo diff --git a/gazelle/python/testdata/package_mode_respect_existing_unmanaged_srcs/excluded.py b/gazelle/python/testdata/package_mode_respect_existing_unmanaged_srcs/excluded.py new file mode 100644 index 0000000000..6b58ff30a8 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_unmanaged_srcs/excluded.py @@ -0,0 +1 @@ +# For test purposes only. diff --git a/gazelle/python/testdata/package_mode_respect_existing_unmanaged_srcs/foo.py b/gazelle/python/testdata/package_mode_respect_existing_unmanaged_srcs/foo.py new file mode 100644 index 0000000000..6b58ff30a8 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_unmanaged_srcs/foo.py @@ -0,0 +1 @@ +# For test purposes only. diff --git a/gazelle/python/testdata/package_mode_respect_existing_unmanaged_srcs/ignored.py b/gazelle/python/testdata/package_mode_respect_existing_unmanaged_srcs/ignored.py new file mode 100644 index 0000000000..6b58ff30a8 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_unmanaged_srcs/ignored.py @@ -0,0 +1 @@ +# For test purposes only. diff --git a/gazelle/python/testdata/package_mode_respect_existing_unmanaged_srcs/test.yaml b/gazelle/python/testdata/package_mode_respect_existing_unmanaged_srcs/test.yaml new file mode 100644 index 0000000000..ed97d539c0 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_unmanaged_srcs/test.yaml @@ -0,0 +1 @@ +--- diff --git a/gazelle/python/testdata/per_file_respect_existing_with_init_and_main_module/BUILD.in b/gazelle/python/testdata/per_file_respect_existing_with_init_and_main_module/BUILD.in new file mode 100644 index 0000000000..5ce4362590 --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_with_init_and_main_module/BUILD.in @@ -0,0 +1,15 @@ +load("@rules_python//python:defs.bzl", "py_library") + +# gazelle:python_generation_mode file +# gazelle:python_generation_mode_per_file_include_init true + +# Gazelle should extract cli.py into a py_binary but keep __init__.py, which was +# written by hand rather than added by python_generation_mode_per_file_include_init. +py_library( + name = "custom", + srcs = [ + "__init__.py", + "cli.py", + ], + tags = ["keep_me"], +) diff --git a/gazelle/python/testdata/per_file_respect_existing_with_init_and_main_module/BUILD.out b/gazelle/python/testdata/per_file_respect_existing_with_init_and_main_module/BUILD.out new file mode 100644 index 0000000000..ae20706ff0 --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_with_init_and_main_module/BUILD.out @@ -0,0 +1,33 @@ +load("@rules_python//python:defs.bzl", "py_binary", "py_library") + +# gazelle:python_generation_mode file +# gazelle:python_generation_mode_per_file_include_init true + +# Gazelle should extract cli.py into a py_binary but keep __init__.py, which was +# written by hand rather than added by python_generation_mode_per_file_include_init. +py_library( + name = "custom", + srcs = ["__init__.py"], + tags = ["keep_me"], + visibility = ["//:__subpackages__"], + deps = [":foo"], +) + +py_binary( + name = "cli", + srcs = [ + "__init__.py", + "cli.py", + ], + visibility = ["//:__subpackages__"], + deps = [":foo"], +) + +py_library( + name = "foo", + srcs = [ + "__init__.py", + "foo.py", + ], + visibility = ["//:__subpackages__"], +) diff --git a/gazelle/python/testdata/per_file_respect_existing_with_init_and_main_module/README.md b/gazelle/python/testdata/per_file_respect_existing_with_init_and_main_module/README.md new file mode 100644 index 0000000000..12b1953699 --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_with_init_and_main_module/README.md @@ -0,0 +1,13 @@ +# Per-File Generation With Preserved Target Owning `__init__.py` and a Main Module + +This test verifies that extracting a main module from a preserved target does not +also drop a hand-written `__init__.py` source. + +With `python_generation_mode_per_file_include_init`, Gazelle adds `__init__.py` +to the per-file targets it generates. It must not remove `__init__.py` from a +preserved target that listed it explicitly, which would empty the target's srcs +and delete it. + +The preserved target keeps a dependency on `:foo` even though `__init__.py` does +not import it: dependencies are resolved from the target's sources before the +main module is extracted, so `cli.py`'s imports are still attributed to it. diff --git a/gazelle/python/testdata/per_file_respect_existing_with_init_and_main_module/WORKSPACE b/gazelle/python/testdata/per_file_respect_existing_with_init_and_main_module/WORKSPACE new file mode 100644 index 0000000000..faff6af87a --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_with_init_and_main_module/WORKSPACE @@ -0,0 +1 @@ +# This is a Bazel workspace for the Gazelle test data. diff --git a/gazelle/python/testdata/per_file_respect_existing_with_init_and_main_module/__init__.py b/gazelle/python/testdata/per_file_respect_existing_with_init_and_main_module/__init__.py new file mode 100644 index 0000000000..769f462dfd --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_with_init_and_main_module/__init__.py @@ -0,0 +1 @@ +BAR = "baz" diff --git a/gazelle/python/testdata/per_file_respect_existing_with_init_and_main_module/cli.py b/gazelle/python/testdata/per_file_respect_existing_with_init_and_main_module/cli.py new file mode 100644 index 0000000000..07cb4bb282 --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_with_init_and_main_module/cli.py @@ -0,0 +1,4 @@ +import foo + +if __name__ == "__main__": + print(foo) diff --git a/gazelle/python/testdata/per_file_respect_existing_with_init_and_main_module/foo.py b/gazelle/python/testdata/per_file_respect_existing_with_init_and_main_module/foo.py new file mode 100644 index 0000000000..6b58ff30a8 --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_with_init_and_main_module/foo.py @@ -0,0 +1 @@ +# For test purposes only. diff --git a/gazelle/python/testdata/per_file_respect_existing_with_init_and_main_module/test.yaml b/gazelle/python/testdata/per_file_respect_existing_with_init_and_main_module/test.yaml new file mode 100644 index 0000000000..ed97d539c0 --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_with_init_and_main_module/test.yaml @@ -0,0 +1 @@ +--- From 1c00a775b0e6d4c372576056078fc5b8611eeca3 Mon Sep 17 00:00:00 2001 From: Alex Martani Date: Fri, 14 Aug 2026 21:09:54 -0700 Subject: [PATCH 05/18] refactor(gazelle): drop unreachable empty-srcs branch for preserved tests collectExistingPythonSourceRules never returns a rule with empty srcs, and nothing mutates a preserved py_test's srcs between collection and use, so the branch emitting an empty rule could not be reached. A preserved test whose sources have all disappeared is not collected in the first place, and getRulesWithInvalidSrcs already deletes it. Removing the branch leaves existingPythonSourceRule.kind unread. The field was also misleading: it held the canonical kind rather than the kind written in the BUILD file, so under map_kind the empty rule it built would not have matched the rule it was meant to delete. Co-Authored-By: Claude Opus 5 (1M context) --- gazelle/python/generate.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/gazelle/python/generate.go b/gazelle/python/generate.go index 44dd45e76c..283572f964 100644 --- a/gazelle/python/generate.go +++ b/gazelle/python/generate.go @@ -49,7 +49,6 @@ var ( ) type existingPythonSourceRule struct { - kind string name string srcs *treeset.Set } @@ -145,7 +144,6 @@ func collectExistingPythonSourceRules(args language.GenerateArgs, kind string, k } sourceRules = append(sourceRules, existingPythonSourceRule{ - kind: kind, name: existingRule.Name(), srcs: validSrcs, }) @@ -693,10 +691,6 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes } for _, existingPyTest := range existingPyTests { - if existingPyTest.srcs.Empty() { - result.Empty = append(result.Empty, rule.NewRule(existingPyTest.kind, existingPyTest.name)) - continue - } pyTestTargets = append(pyTestTargets, newPyTestTargetBuilder(existingPyTest.srcs, existingPyTest.name)) } From 6281f898e8758c6feae32784a521592fd5f7f684 Mon Sep 17 00:00:00 2001 From: Alex Martani Date: Fri, 14 Aug 2026 21:10:29 -0700 Subject: [PATCH 06/18] docs(gazelle): correct comment on preserved __init__ target in project mode The comment claimed the __init__ target matches the generated project package, but that target is named after the package. __init__ is preserved like any other hand-written target; it appears unchanged because project mode makes it claim __init__.py and regenerating it yields identical content. The old wording implied name-matching targets are left alone, which is the opposite of what happens: they are the ones excluded from preservation. Co-Authored-By: Claude Opus 5 (1M context) --- .../BUILD.in | 5 +++-- .../BUILD.out | 5 +++-- .../README.md | 9 +++++++-- 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/BUILD.in b/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/BUILD.in index a39f90e6c4..6fb7544b59 100644 --- a/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/BUILD.in +++ b/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/BUILD.in @@ -2,8 +2,9 @@ load("@rules_python//python:defs.bzl", "py_library") # gazelle:python_generation_mode project -# This target already matches the generated project package and should remain -# unchanged. +# In project mode a preserved target claims its sources even when it has only +# one, so __init__.py is left out of the generated project library below. +# Regenerating this target yields identical content, so it appears unchanged. py_library( name = "__init__", srcs = ["__init__.py"], diff --git a/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/BUILD.out b/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/BUILD.out index 25ab8b9e93..f220eba0a1 100644 --- a/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/BUILD.out +++ b/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/BUILD.out @@ -2,8 +2,9 @@ load("@rules_python//python:defs.bzl", "py_library", "py_test") # gazelle:python_generation_mode project -# This target already matches the generated project package and should remain -# unchanged. +# In project mode a preserved target claims its sources even when it has only +# one, so __init__.py is left out of the generated project library below. +# Regenerating this target yields identical content, so it appears unchanged. py_library( name = "__init__", srcs = ["__init__.py"], diff --git a/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/README.md b/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/README.md index 98a129559a..d0269ba85e 100644 --- a/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/README.md +++ b/gazelle/python/testdata/project_generation_mode_respect_existing_multiple_srcs/README.md @@ -5,5 +5,10 @@ This test verifies that project generation preserves existing non-standard for unclaimed sources. Gazelle should prune sources that no longer exist, keep non-generated -attributes, add generated dependencies, and leave the existing `__init__` target -unchanged. +attributes, and add generated dependencies. + +Unlike the other generation modes, project mode has a single generated library +for the whole tree, so every preserved target claims its sources even when it +has only one. That is why `__init__.py` is absent from the generated project +library, and why the existing `__init__` target appears unchanged: regenerating +it produces identical content. From 51cb56231ce81421084664417067924666df798b Mon Sep 17 00:00:00 2001 From: Alex Martani Date: Fri, 14 Aug 2026 21:21:28 -0700 Subject: [PATCH 07/18] docs: track open review findings for target preservation in TODO.md Records the review findings for the py_library/py_test preservation feature that are not addressed on this branch, with line references, the mechanism that makes each one reachable, and the decisions already taken so they are not relitigated. Co-Authored-By: Claude Opus 5 (1M context) --- TODO.md | 209 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 209 insertions(+) create mode 100644 TODO.md diff --git a/TODO.md b/TODO.md new file mode 100644 index 0000000000..d1d19cefb4 --- /dev/null +++ b/TODO.md @@ -0,0 +1,209 @@ +# Open follow-ups: preserving existing Python source targets + +Review findings for the `py_library` / `py_test` preservation feature that are +**not** addressed on this branch. Line references are against +`gazelle/python/generate.go` as of the last commit on +`martani/preserve-existing-targets-2`. + +Background on why any of this is destructive: putting a hand-written target into +`result.Gen` moves it from *unmanaged* to *managed*. `srcs` is in +`MergeableAttrs` and `deps` / `pyi_deps` / `pyi_srcs` are in `ResolveAttrs` +(`gazelle/python/kinds.go:57-74` for `py_library`, `:81-98` for `py_test`), and +`rule.MergeRules` drops any value in the existing list that is not in the +generated set unless it carries a `# keep` comment. Attributes present in the +generated rule but absent from the existing one are copied in unconditionally, +regardless of mergeability. + +## Explicitly decided, do not reopen + +- **Pruning hand-written `deps`.** Preserved targets participate in the resolve + phase, so a `deps` entry not derivable from an `import` statement (e.g. an + `importlib` plugin, a `//third_party/...` runtime dep) is deleted. This is + intended behavior. It still needs a release note and a documented `# keep` + story — see "Release notes" and "Documentation" below. + +## Correctness + +### 1. `visibility` is injected into previously-unmanaged targets + +`generate.go:548` (`addVisibility(visibility)` on the `py_library` built by +`appendPyLibrary`). + +`visibility` is not in `MergeableAttrs`, but `MergeRules` copies attributes that +are absent from the existing rule, and there is no `# keep` path for an absent +attribute. A hand-written target relying on Bazel's default private visibility +silently becomes visible across the subtree. + +Already enshrined in +`gazelle/python/testdata/simple_binary_with_library/BUILD.out`, whose comment +changed from "This target should be kept unmodified by Gazelle" to "Gazelle +should preserve this custom target". Decide whether preserved targets should get +`visibility` at all; if not, only apply it to targets Gazelle created. Either +way, keep a case in the suite asserting that Gazelle does not rewrite a target +it is not managing — that guarantee currently has no test. + +### 2. Preserved and generated targets can form a dependency cycle + +Once a preserved target claims its sources, imports can point both ways: +`custom`'s `bar.py` imports `foo` so `custom` gets `deps = [":pkg"]`, and if +`pkg`'s `foo.py` imports `bar` then `pkg` gets `deps = [":custom"]`. Bazel +rejects the cycle. Before the feature this was a self-import inside one target +and produced no edge. + +Not reachable in the current fixtures, which are arranged so imports only flow +from the preserved target to the generated one. Needs a decision (detect and +warn? refuse to claim?) and a test either way. + +### 3. Entrypoints and `conftest.py` in a preserved rule cause double ownership + +`generate.go:397-413` adds `__main__.py`, `__test__.py` and `conftest.py` to +`knownPySrcs`, but they are never members of `pyLibraryFilenames` / +`pyTestFilenames` — the scan at `generate.go:260-279` routes them to dedicated +branches. So `removeClaimedSrcs` cannot remove them, and the `py_binary`, +`conftest` and package-level `py_test` targets are still generated from the same +file. Note `generate.go:702` adds `__test__.py` to `pyTestFilenames` *after* +claiming has run. + +Probably best handled by declining to preserve a rule that lists an entrypoint or +`conftest.py`, leaving it untouched as before the feature. + +### 4. The claiming threshold is computed on the pruned source set + +`generate.go:433-440`: `sourceRule.srcs.Size() > 1 || cfg.CoarseGrainedGeneration()`, +where `srcs` has already had nonexistent entries removed. So +`srcs = ["bar.py", "baz.py"]` claims its sources but +`srcs = ["bar.py", "deleted.py"]` does not — deleting an unrelated file flips a +target between claiming and non-claiming. Claiming should not depend on +filesystem state this way. + +Dropping the single-source carve-out entirely would resolve this and simplify the +feature, at the cost of changing `simple_binary_with_library` (`bar.py` would +leave the generated package library). Worth evaluating against +`testdata/dont_rename_target` and `testdata/invalid_imported_module/foo`. + +### 5. Per-file name collisions are not checked + +`sourceRuleMatchesGeneratedPerFileName` (`generate.go:183-192`) compares the rule +name only against the basenames of *its own* srcs. In file mode, +`py_library(name = "qux", srcs = ["bar.py", "baz.py"])` alongside an unclaimed +`qux.py` puts two `py_library(name = "qux")` rules into `result.Gen`; +`ensureNoCollision` cannot catch it because both are the same kind. Same gap for +`py_test` via the filter at `generate.go:424-432`. + +Fix by collecting the names Gazelle will generate before the filters run and +rejecting existing rules whose name is in that set. + +### 6. Stale `deps` inherited from an extracted main module + +`generate.go:452` parses `srcs` at the top of `appendPyLibrary`, before +`srcs.Remove(name)` strips main modules further down. A preserved target +therefore keeps dependencies contributed by a source it no longer owns. + +Pinned in +`testdata/per_file_respect_existing_with_init_and_main_module/BUILD.out`, where +`custom` has `deps = [":foo"]` although its only remaining src (`__init__.py`) +imports nothing. Fixing it needs per-source dep attribution rather than the +aggregated `allDeps`. + +### 7. A preserved target whose sources are all main modules is still deleted + +The `__init__.py` trigger for this path is fixed, but the underlying shape +remains: in file mode a preserved target whose every src is a main module has its +`srcs` emptied, then `generate.go:517-529` finds the same-named existing rule, +sets `generateEmptyLibrary`, and the built rule is `IsEmpty` — so the +hand-written target is deleted rather than preserved. + +## Release notes + +`CONTRIBUTING.md:176-191` requires a news fragment per change; this branch has +none. Add `news/.changed.md`. It must call out that Gazelle now manages +`srcs` and `deps` on previously hand-written `py_library` / `py_test` targets, +that `deps` not derivable from imports will be removed, and that `# keep` is the +escape hatch. + +## Documentation + +`gazelle/docs/installation_and_usage.md:166-169` still says "all source files are +collected into the `srcs` of the `py_library`", which is no longer true — sources +claimed by a preserved target are excluded. The `### Tests` section +(`:178`) should state that existing `py_test` targets are preserved and always +claim their srcs, unlike libraries. + +Also document, since none of it is written down anywhere: + +- Which attributes Gazelle takes over on a preserved target (`srcs`, `deps`) and + which it leaves alone (`tags`, and `visibility` when already present). +- That `# keep` is the only way to protect a value. +- That there is no directive to opt out of preservation. `gazelle/docs/directives.md` + is unchanged by this branch; consider whether an opt-out is needed. + +## Code comments + +`generate.go` documents 9 of its 12 pre-existing package-level functions, and the +three that it doesn't are self-describing. These new declarations still have no +doc comment: + +- `existingPythonSourceRule` (`:51`) +- `isTargetSrc` (`:82`) — worth noting the polarity differs by call site: a label + src disqualifies a rule from preservation (`:128`) but marks it *valid* in + `getRulesWithInvalidSrcs` (`:819`). +- `addSetValuesToMap` (`:154`), `removeClaimedSrcs` (`:161`), + `filterExistingPythonSourceRules` (`:173`), + `sourceRuleMatchesGeneratedPerFileName` (`:183`) + +`removeClaimedSrcs` is the place to define "claim", which is load-bearing +vocabulary used nowhere else. The policy block at `generate.go:415-442` also needs +the *why* for two rules a reader cannot derive: why a single-source library does +not claim while a `py_test` always does, and why targets whose name matches a +generated name are excluded from preservation. + +## Test gaps + +Ranked. None of these exist today. + +1. `map_kind` / `alias_kind` with a target name that is not the generated name — + the preservation path is entirely untested for renamed kinds. The three + existing cases (`respect_alias_kind`, `respect_kind_mapping`, + `respect_alias_and_map_kind`) all use names the filters exclude. +2. A preserved `py_test` in file mode, and the collision shape where a preserved + `py_test` named `foo_test` has `srcs = ["bar_test.py"]`. +3. `srcs = glob([...])`: `AttrStrings` returns nothing, so the rule is skipped and + its files are also swept into generated targets. Probably the right + conservative behavior, but it is silent and unasserted — and `glob` is the + commonest hand-written form. +4. Label and non-`.py` srcs (`srcs = ["a.py", ":generated.py"]`, + `srcs = ["a.py", "schema.json"]`), which disqualify the whole rule. +5. Project mode with a preserved target listing subdirectory sources, and any case + in a subpackage — all preservation fixtures sit at the workspace root + (`args.Rel == ""`), so `imports` rendering is never exercised. +6. Custom `python_library_naming_convention` / + `python_test_naming_convention`, which feed the "is this the generated + target?" filters via `RenderLibraryName` / `RenderTestName`. +7. A test-pattern file inside a preserved library's srcs — `knownPySrcs` merges + library and test filenames, so a preserved `py_library` can claim `foo_test.py` + and suppress the generated `py_test`. + +## Diagnostics + +The preservation code emits no log output at all. Every branch that declines to +preserve a rule (`generate.go:120-122` no srcs, `:139-141` the `skip` bail-out +decided at `:128` for a label or non-`.py` src, `:142-144` no managed src) leaves +Gazelle generating a competing target over the same sources, which is exactly +what the user needs to know. Consider one log line per declined rule. + +Separately, `log.Fatalf` at `generate.go:457` and `:668` names no target, and the +same file is now parsed twice (once for the preserved target, once for the +generated one), so the message does not identify which target failed. The +`mainModules` loop at `:453-455` also runs before the `err` check at `:456`. + +## Cosmetic + +- `testdata/per_file_respect_existing_multiple_srcs/BUILD.in:11` and + `testdata/project_generation_mode_respect_existing_multiple_srcs/BUILD.in:19` + indent `tags` with a literal tab; neighbouring lines use four spaces. +- `testdata/project_generation_mode_respect_existing_multiple_srcs/BUILD.in` + has a stray double blank line, and its `load` statement omits `py_test` + although the file uses it. +- New testdata READMEs use Title Case headings; the dominant convention across + the other ~80 cases is sentence case. +- The new `test.yaml` files omit the license header that most existing ones carry. From cb35b03b3b9cb201267ea19c67dcccadf8b90d06 Mon Sep 17 00:00:00 2001 From: Alex Martani Date: Thu, 10 Sep 2026 16:19:01 -0700 Subject: [PATCH 08/18] fix(gazelle): harden preservation of existing Python targets Preserved source targets could gain visibility, collide with generated targets, retain stale dependencies after main-module extraction, or be incorrectly claimed or deleted for edge-case source sets. Exclude targets Gazelle must generate itself and sources requiring dedicated handling. Base claiming on declared sources, preserve visibility, and recompute dependencies after source extraction. Document the management semantics and cover the edge cases with integration fixtures. --- TODO.md | 209 +++++------------- gazelle/docs/installation_and_usage.md | 44 +++- gazelle/python/generate.go | 164 +++++++++++--- .../BUILD.in | 12 + .../BUILD.out | 22 ++ .../README.md | 9 + .../WORKSPACE | 1 + .../bar.py | 1 + .../baz.py | 1 + .../conftest.py | 1 + .../foo.py | 1 + .../test.yaml | 1 + .../BUILD.in | 13 ++ .../BUILD.out | 38 ++++ .../README.md | 9 + .../WORKSPACE | 1 + .../__main__.py | 1 + .../bar.py | 1 + .../conftest.py | 1 + .../foo.py | 1 + .../test.yaml | 1 + .../BUILD.in | 12 + .../BUILD.out | 16 ++ .../README.md | 10 + .../WORKSPACE | 1 + .../bar.py | 1 + .../foo.py | 1 + .../test.yaml | 1 + .../BUILD.out | 1 - .../BUILD.out | 1 - .../BUILD.in | 14 ++ .../BUILD.out | 34 +++ .../README.md | 10 + .../WORKSPACE | 1 + .../a.py | 4 + .../b.py | 4 + .../foo.py | 1 + .../test.yaml | 1 + .../BUILD.in | 13 ++ .../BUILD.out | 31 +++ .../README.md | 15 ++ .../WORKSPACE | 1 + .../bar.py | 1 + .../baz.py | 1 + .../foo.py | 1 + .../qux.py | 1 + .../test.yaml | 1 + .../BUILD.out | 2 - .../README.md | 6 +- .../simple_binary_with_library/BUILD.in | 2 +- .../simple_binary_with_library/BUILD.out | 3 +- .../simple_binary_with_library/README.md | 6 +- ...reserve_existing_python_targets.changed.md | 8 + 53 files changed, 531 insertions(+), 206 deletions(-) create mode 100644 gazelle/python/testdata/package_mode_respect_existing_conftest_name_collision/BUILD.in create mode 100644 gazelle/python/testdata/package_mode_respect_existing_conftest_name_collision/BUILD.out create mode 100644 gazelle/python/testdata/package_mode_respect_existing_conftest_name_collision/README.md create mode 100644 gazelle/python/testdata/package_mode_respect_existing_conftest_name_collision/WORKSPACE create mode 100644 gazelle/python/testdata/package_mode_respect_existing_conftest_name_collision/bar.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_conftest_name_collision/baz.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_conftest_name_collision/conftest.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_conftest_name_collision/foo.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_conftest_name_collision/test.yaml create mode 100644 gazelle/python/testdata/package_mode_respect_existing_entrypoint_srcs/BUILD.in create mode 100644 gazelle/python/testdata/package_mode_respect_existing_entrypoint_srcs/BUILD.out create mode 100644 gazelle/python/testdata/package_mode_respect_existing_entrypoint_srcs/README.md create mode 100644 gazelle/python/testdata/package_mode_respect_existing_entrypoint_srcs/WORKSPACE create mode 100644 gazelle/python/testdata/package_mode_respect_existing_entrypoint_srcs/__main__.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_entrypoint_srcs/bar.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_entrypoint_srcs/conftest.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_entrypoint_srcs/foo.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_entrypoint_srcs/test.yaml create mode 100644 gazelle/python/testdata/package_mode_respect_existing_pruned_src_count/BUILD.in create mode 100644 gazelle/python/testdata/package_mode_respect_existing_pruned_src_count/BUILD.out create mode 100644 gazelle/python/testdata/package_mode_respect_existing_pruned_src_count/README.md create mode 100644 gazelle/python/testdata/package_mode_respect_existing_pruned_src_count/WORKSPACE create mode 100644 gazelle/python/testdata/package_mode_respect_existing_pruned_src_count/bar.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_pruned_src_count/foo.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_pruned_src_count/test.yaml create mode 100644 gazelle/python/testdata/per_file_respect_existing_all_main_modules/BUILD.in create mode 100644 gazelle/python/testdata/per_file_respect_existing_all_main_modules/BUILD.out create mode 100644 gazelle/python/testdata/per_file_respect_existing_all_main_modules/README.md create mode 100644 gazelle/python/testdata/per_file_respect_existing_all_main_modules/WORKSPACE create mode 100644 gazelle/python/testdata/per_file_respect_existing_all_main_modules/a.py create mode 100644 gazelle/python/testdata/per_file_respect_existing_all_main_modules/b.py create mode 100644 gazelle/python/testdata/per_file_respect_existing_all_main_modules/foo.py create mode 100644 gazelle/python/testdata/per_file_respect_existing_all_main_modules/test.yaml create mode 100644 gazelle/python/testdata/per_file_respect_existing_generated_name_collision/BUILD.in create mode 100644 gazelle/python/testdata/per_file_respect_existing_generated_name_collision/BUILD.out create mode 100644 gazelle/python/testdata/per_file_respect_existing_generated_name_collision/README.md create mode 100644 gazelle/python/testdata/per_file_respect_existing_generated_name_collision/WORKSPACE create mode 100644 gazelle/python/testdata/per_file_respect_existing_generated_name_collision/bar.py create mode 100644 gazelle/python/testdata/per_file_respect_existing_generated_name_collision/baz.py create mode 100644 gazelle/python/testdata/per_file_respect_existing_generated_name_collision/foo.py create mode 100644 gazelle/python/testdata/per_file_respect_existing_generated_name_collision/qux.py create mode 100644 gazelle/python/testdata/per_file_respect_existing_generated_name_collision/test.yaml create mode 100644 news/preserve_existing_python_targets.changed.md diff --git a/TODO.md b/TODO.md index d1d19cefb4..6d9c8b9e59 100644 --- a/TODO.md +++ b/TODO.md @@ -1,9 +1,8 @@ -# Open follow-ups: preserving existing Python source targets +# Review follow-ups: preserving existing Python source targets -Review findings for the `py_library` / `py_test` preservation feature that are -**not** addressed on this branch. Line references are against -`gazelle/python/generate.go` as of the last commit on -`martani/preserve-existing-targets-2`. +Status of review findings for the `py_library` / `py_test` preservation +feature. Line references are against `gazelle/python/generate.go` as of the +last commit on `martani/preserve-existing-targets-2`. Background on why any of this is destructive: putting a hand-written target into `result.Gen` moves it from *unmanaged* to *managed*. `srcs` is in @@ -19,30 +18,35 @@ regardless of mergeability. - **Pruning hand-written `deps`.** Preserved targets participate in the resolve phase, so a `deps` entry not derivable from an `import` statement (e.g. an `importlib` plugin, a `//third_party/...` runtime dep) is deleted. This is - intended behavior. It still needs a release note and a documented `# keep` - story — see "Release notes" and "Documentation" below. + intended behavior and is covered by the release note and user documentation. + +## Fixed + +- **`visibility` injected into previously-unmanaged targets** — preserved + targets no longer get `addVisibility`, since `MergeRules` copies a + non-mergeable attribute the existing rule does not set and offers no `# keep` + for it. +- **Entrypoints and `conftest.py` in a preserved rule** — a rule listing + `__main__.py`, `__test__.py` or `conftest.py` is no longer adopted. +- **The claiming threshold** is now taken from the declared src count rather + than the pruned set, so deleting an unrelated file cannot flip a target + between claiming and not claiming. +- **Name collisions with generated targets** — the names Gazelle will generate + are collected before claiming and existing rules with one of those names are + not adopted. This covers the per-file names, the package library/test names, + the `py_binary` name, `conftest`, and a `py_binary` extracted from a + preserved target's own main module. Without it the two rules merged and + orphaned the sources of whichever lost. +- **A preserved target whose sources are all main modules** is now left as + written instead of emptied and deleted. +- **Stale `deps` inherited from an extracted main module** — dependencies are + recomputed after main modules are removed from `srcs`. +- **User documentation and release notes** now describe eligibility, claiming, + managed attributes, `# keep`, exclusions, and the possible dependency cycle. ## Correctness -### 1. `visibility` is injected into previously-unmanaged targets - -`generate.go:548` (`addVisibility(visibility)` on the `py_library` built by -`appendPyLibrary`). - -`visibility` is not in `MergeableAttrs`, but `MergeRules` copies attributes that -are absent from the existing rule, and there is no `# keep` path for an absent -attribute. A hand-written target relying on Bazel's default private visibility -silently becomes visible across the subtree. - -Already enshrined in -`gazelle/python/testdata/simple_binary_with_library/BUILD.out`, whose comment -changed from "This target should be kept unmodified by Gazelle" to "Gazelle -should preserve this custom target". Decide whether preserved targets should get -`visibility` at all; if not, only apply it to targets Gazelle created. Either -way, keep a case in the suite asserting that Gazelle does not rewrite a target -it is not managing — that guarantee currently has no test. - -### 2. Preserved and generated targets can form a dependency cycle +### 1. Preserved and generated targets can form a dependency cycle Once a preserved target claims its sources, imports can point both ways: `custom`'s `bar.py` imports `foo` so `custom` gets `deps = [":pkg"]`, and if @@ -51,150 +55,48 @@ rejects the cycle. Before the feature this was a self-import inside one target and produced no edge. Not reachable in the current fixtures, which are arranged so imports only flow -from the preserved target to the generated one. Needs a decision (detect and -warn? refuse to claim?) and a test either way. - -### 3. Entrypoints and `conftest.py` in a preserved rule cause double ownership - -`generate.go:397-413` adds `__main__.py`, `__test__.py` and `conftest.py` to -`knownPySrcs`, but they are never members of `pyLibraryFilenames` / -`pyTestFilenames` — the scan at `generate.go:260-279` routes them to dedicated -branches. So `removeClaimedSrcs` cannot remove them, and the `py_binary`, -`conftest` and package-level `py_test` targets are still generated from the same -file. Note `generate.go:702` adds `__test__.py` to `pyTestFilenames` *after* -claiming has run. - -Probably best handled by declining to preserve a rule that lists an entrypoint or -`conftest.py`, leaving it untouched as before the feature. - -### 4. The claiming threshold is computed on the pruned source set - -`generate.go:433-440`: `sourceRule.srcs.Size() > 1 || cfg.CoarseGrainedGeneration()`, -where `srcs` has already had nonexistent entries removed. So -`srcs = ["bar.py", "baz.py"]` claims its sources but -`srcs = ["bar.py", "deleted.py"]` does not — deleting an unrelated file flips a -target between claiming and non-claiming. Claiming should not depend on -filesystem state this way. - -Dropping the single-source carve-out entirely would resolve this and simplify the -feature, at the cost of changing `simple_binary_with_library` (`bar.py` would -leave the generated package library). Worth evaluating against -`testdata/dont_rename_target` and `testdata/invalid_imported_module/foo`. - -### 5. Per-file name collisions are not checked - -`sourceRuleMatchesGeneratedPerFileName` (`generate.go:183-192`) compares the rule -name only against the basenames of *its own* srcs. In file mode, -`py_library(name = "qux", srcs = ["bar.py", "baz.py"])` alongside an unclaimed -`qux.py` puts two `py_library(name = "qux")` rules into `result.Gen`; -`ensureNoCollision` cannot catch it because both are the same kind. Same gap for -`py_test` via the filter at `generate.go:424-432`. - -Fix by collecting the names Gazelle will generate before the filters run and -rejecting existing rules whose name is in that set. - -### 6. Stale `deps` inherited from an extracted main module - -`generate.go:452` parses `srcs` at the top of `appendPyLibrary`, before -`srcs.Remove(name)` strips main modules further down. A preserved target -therefore keeps dependencies contributed by a source it no longer owns. - -Pinned in -`testdata/per_file_respect_existing_with_init_and_main_module/BUILD.out`, where -`custom` has `deps = [":foo"]` although its only remaining src (`__init__.py`) -imports nothing. Fixing it needs per-source dep attribution rather than the -aggregated `allDeps`. - -### 7. A preserved target whose sources are all main modules is still deleted - -The `__init__.py` trigger for this path is fixed, but the underlying shape -remains: in file mode a preserved target whose every src is a main module has its -`srcs` emptied, then `generate.go:517-529` finds the same-named existing rule, -sets `generateEmptyLibrary`, and the built rule is `IsEmpty` — so the -hand-written target is deleted rather than preserved. - -## Release notes - -`CONTRIBUTING.md:176-191` requires a news fragment per change; this branch has -none. Add `news/.changed.md`. It must call out that Gazelle now manages -`srcs` and `deps` on previously hand-written `py_library` / `py_test` targets, -that `deps` not derivable from imports will be removed, and that `# keep` is the -escape hatch. - -## Documentation - -`gazelle/docs/installation_and_usage.md:166-169` still says "all source files are -collected into the `srcs` of the `py_library`", which is no longer true — sources -claimed by a preserved target are excluded. The `### Tests` section -(`:178`) should state that existing `py_test` targets are preserved and always -claim their srcs, unlike libraries. - -Also document, since none of it is written down anywhere: - -- Which attributes Gazelle takes over on a preserved target (`srcs`, `deps`) and - which it leaves alone (`tags`, and `visibility` when already present). -- That `# keep` is the only way to protect a value. -- That there is no directive to opt out of preservation. `gazelle/docs/directives.md` - is unchanged by this branch; consider whether an opt-out is needed. - -## Code comments - -`generate.go` documents 9 of its 12 pre-existing package-level functions, and the -three that it doesn't are self-describing. These new declarations still have no -doc comment: - -- `existingPythonSourceRule` (`:51`) -- `isTargetSrc` (`:82`) — worth noting the polarity differs by call site: a label - src disqualifies a rule from preservation (`:128`) but marks it *valid* in - `getRulesWithInvalidSrcs` (`:819`). -- `addSetValuesToMap` (`:154`), `removeClaimedSrcs` (`:161`), - `filterExistingPythonSourceRules` (`:173`), - `sourceRuleMatchesGeneratedPerFileName` (`:183`) - -`removeClaimedSrcs` is the place to define "claim", which is load-bearing -vocabulary used nowhere else. The policy block at `generate.go:415-442` also needs -the *why* for two rules a reader cannot derive: why a single-source library does -not claim while a `py_test` always does, and why targets whose name matches a -generated name are excluded from preservation. +from the preserved target to the generated one. Detecting it properly needs the +resolve phase, which runs after generation, so this is likely a documentation +item rather than something to block on. It is also already reachable in file +mode without the feature. ## Test gaps Ranked. None of these exist today. -1. `map_kind` / `alias_kind` with a target name that is not the generated name — - the preservation path is entirely untested for renamed kinds. The three - existing cases (`respect_alias_kind`, `respect_kind_mapping`, +1. `map_kind` / `alias_kind` with a target name that is not the generated + name — the preservation path is entirely untested for renamed kinds. The + three existing cases (`respect_alias_kind`, `respect_kind_mapping`, `respect_alias_and_map_kind`) all use names the filters exclude. -2. A preserved `py_test` in file mode, and the collision shape where a preserved - `py_test` named `foo_test` has `srcs = ["bar_test.py"]`. -3. `srcs = glob([...])`: `AttrStrings` returns nothing, so the rule is skipped and - its files are also swept into generated targets. Probably the right +2. A preserved `py_test` in file mode. +3. `srcs = glob([...])`: `AttrStrings` returns nothing, so the rule is skipped + and its files are also swept into generated targets. Probably the right conservative behavior, but it is silent and unasserted — and `glob` is the commonest hand-written form. 4. Label and non-`.py` srcs (`srcs = ["a.py", ":generated.py"]`, `srcs = ["a.py", "schema.json"]`), which disqualify the whole rule. -5. Project mode with a preserved target listing subdirectory sources, and any case - in a subpackage — all preservation fixtures sit at the workspace root +5. Project mode with a preserved target listing subdirectory sources, and any + case in a subpackage — all preservation fixtures sit at the workspace root (`args.Rel == ""`), so `imports` rendering is never exercised. 6. Custom `python_library_naming_convention` / `python_test_naming_convention`, which feed the "is this the generated target?" filters via `RenderLibraryName` / `RenderTestName`. -7. A test-pattern file inside a preserved library's srcs — `knownPySrcs` merges - library and test filenames, so a preserved `py_library` can claim `foo_test.py` - and suppress the generated `py_test`. +7. A test-pattern file inside a preserved library's srcs — `knownPySrcs` + merges library and test filenames, so a preserved `py_library` can claim + `foo_test.py` and suppress the generated `py_test`. ## Diagnostics The preservation code emits no log output at all. Every branch that declines to -preserve a rule (`generate.go:120-122` no srcs, `:139-141` the `skip` bail-out -decided at `:128` for a label or non-`.py` src, `:142-144` no managed src) leaves -Gazelle generating a competing target over the same sources, which is exactly -what the user needs to know. Consider one log line per declined rule. +preserve a rule (no srcs, a label or non-`.py` src, an entrypoint or +`conftest.py` src, no managed src, a name Gazelle generates) leaves Gazelle +generating a competing target over the same sources, which is exactly what the +user needs to know. Consider one log line per declined rule. -Separately, `log.Fatalf` at `generate.go:457` and `:668` names no target, and the -same file is now parsed twice (once for the preserved target, once for the -generated one), so the message does not identify which target failed. The -`mainModules` loop at `:453-455` also runs before the `err` check at `:456`. +Separately, the `log.Fatalf` calls in `appendPyLibrary` and +`newPyTestTargetBuilder` name no target, and the same file is now parsed twice +(once for the preserved target, once for the generated one), so the message does +not identify which target failed. ## Cosmetic @@ -204,6 +106,5 @@ generated one), so the message does not identify which target failed. The - `testdata/project_generation_mode_respect_existing_multiple_srcs/BUILD.in` has a stray double blank line, and its `load` statement omits `py_test` although the file uses it. -- New testdata READMEs use Title Case headings; the dominant convention across - the other ~80 cases is sentence case. -- The new `test.yaml` files omit the license header that most existing ones carry. +- The testdata READMEs added by the first preservation commit use Title Case + headings; the dominant convention across the other ~80 cases is sentence case. diff --git a/gazelle/docs/installation_and_usage.md b/gazelle/docs/installation_and_usage.md index 16bccded02..baec6845b8 100644 --- a/gazelle/docs/installation_and_usage.md +++ b/gazelle/docs/installation_and_usage.md @@ -161,8 +161,8 @@ you edit Python code, and it should update your `BUILD` files correctly. ### Libraries Python source files are those ending in `.py` that are not matched as a test -file via the {term}`# gazelle:python_test_file_pattern value` directive. By default, -python source files are all `*.py` files except for `*_test.py` and +file via the {term}`# gazelle:python_test_file_pattern value` directive. By +default, python source files are all `*.py` files except for `*_test.py` and `test_*.py`. First, we look for the nearest ancestor `BUILD(.bazel)` file starting from @@ -170,8 +170,9 @@ the folder containing the Python source file. + In `package` generation mode, if there is no {bzl:obj}`py_library` in this `BUILD(.bazel)` file, one is created using the package name as the target's - name. This makes it the default target in the package. Next, all source - files are collected into the `srcs` of the {bzl:obj}`py_library`. + name. This makes it the default target in the package. Next, source files not + claimed by another target are collected into the `srcs` of the + {bzl:obj}`py_library`. + In `project` generation mode, all source files in subdirectories (that don't have `BUILD(.bazel)` files) are also collected. + In `file` generation mode, each python source file is given its own target. @@ -205,6 +206,41 @@ py_test( You can control the naming convention for test targets using the {term}`# gazelle:python_test_naming_convention value` directive. +### Existing source targets + +Gazelle regenerates eligible hand-written {bzl:obj}`py_library` and +{bzl:obj}`py_test` targets in place. A target is eligible when its `srcs` is a +non-empty list of relative `.py` paths and at least one of those paths is a +source Gazelle manages. Targets are excluded from this behavior when: + +- Their name is one Gazelle will generate. +- Their `srcs` attribute uses `glob()`, a label, or a non-`.py` file. +- Their `srcs` contains `__main__.py`, `__test__.py`, or `conftest.py`, which + Gazelle handles with dedicated targets. + +For an eligible target, Gazelle updates `srcs` and the dependency attributes +`deps`, `pyi_deps`, and `pyi_srcs`. It may add `imports` when required by the +configured Python root. Other attributes, including `visibility` and `tags`, +remain unchanged. A `# keep` comment on a value prevents Gazelle from removing +that value. A `# keep` comment above the rule prevents Gazelle from changing the +rule at all. + +An eligible {bzl:obj}`py_test` always claims its sources, which keeps those +sources out of generated test and library targets. A {bzl:obj}`py_library` with +more than one declared source also claims its sources. A library with one +declared source continues to share it with the generated library, except in +`project` generation mode, where it claims the source. + +After sources are divided between preserved and generated targets, imports can +produce dependencies in both directions and therefore a Bazel dependency cycle. +If this happens, reorganize the sources or use `# keep` above the existing rule +to opt it out of preservation. + +:::{versionchanged} VERSION_NEXT_FEATURE +Eligible existing {bzl:obj}`py_library` and {bzl:obj}`py_test` targets are now +regenerated in place. +::: + ### Binaries diff --git a/gazelle/python/generate.go b/gazelle/python/generate.go index 283572f964..6b45e091e8 100644 --- a/gazelle/python/generate.go +++ b/gazelle/python/generate.go @@ -48,9 +48,17 @@ var ( buildFilenames = []string{"BUILD", "BUILD.bazel"} ) +// existingPythonSourceRule is a hand-written rule that Gazelle regenerates in +// place instead of replacing. type existingPythonSourceRule struct { name string + // srcs are the rule's srcs with the entries that no longer exist pruned. srcs *treeset.Set + // declaredSrcCount is the number of srcs the rule lists in the BUILD file, + // before pruning. Decisions about how a rule is treated are made on this + // count so that they reflect only what the user wrote: deleting an unrelated + // file must not change how Gazelle handles the rule. + declaredSrcCount int } // Returns the mapped kind, or kind if no mapping is configured with the map_kind directive. @@ -79,6 +87,7 @@ func matchesAnyGlob(s string, globs []string) bool { return false } +// isTargetSrc reports whether src is a label rather than a file path. func isTargetSrc(src string) bool { return strings.HasPrefix(src, "@") || strings.HasPrefix(src, "//") || strings.HasPrefix(src, ":") } @@ -93,6 +102,10 @@ func isTargetSrc(src string) bool { // python_ignore_files, gazelle:exclude and subdirectory sources are hidden from // generation, which must not cause Gazelle to delete them from a hand-written // target. Only srcs that no longer exist are pruned. +// +// A rule listing an entrypoint or conftest.py is never adopted: those sources +// have dedicated targets that Gazelle always generates, so adopting the rule +// would leave two targets owning the same file. func collectExistingPythonSourceRules(args language.GenerateArgs, kind string, knownSrcs map[string]struct{}) []existingPythonSourceRule { if args.File == nil { return nil @@ -129,6 +142,12 @@ func collectExistingPythonSourceRules(args language.GenerateArgs, kind string, k skip = true break } + if src == pyBinaryEntrypointFilename || + src == pyTestEntrypointFilename || + src == conftestFilename { + skip = true + break + } if _, ok := knownSrcs[src]; ok { hasKnownSrc = true validSrcs.Add(src) @@ -144,13 +163,15 @@ func collectExistingPythonSourceRules(args language.GenerateArgs, kind string, k } sourceRules = append(sourceRules, existingPythonSourceRule{ - name: existingRule.Name(), - srcs: validSrcs, + name: existingRule.Name(), + srcs: validSrcs, + declaredSrcCount: len(srcs), }) } return sourceRules } +// addSetValuesToMap copies every value in srcs into dst. func addSetValuesToMap(srcs *treeset.Set, dst map[string]struct{}) { it := srcs.Iterator() for it.Next() { @@ -158,6 +179,9 @@ func addSetValuesToMap(srcs *treeset.Set, dst map[string]struct{}) { } } +// removeClaimedSrcs removes sources owned by rules from the generated source +// sets. A preserved rule claims a source when it becomes its sole generated +// owner rather than sharing it with another target Gazelle generates. func removeClaimedSrcs(rules []existingPythonSourceRule, srcSets ...*treeset.Set) { for _, sourceRule := range rules { it := sourceRule.srcs.Iterator() @@ -170,7 +194,11 @@ func removeClaimedSrcs(rules []existingPythonSourceRule, srcSets ...*treeset.Set } } -func filterExistingPythonSourceRules(rules []existingPythonSourceRule, shouldKeep func(existingPythonSourceRule) bool) []existingPythonSourceRule { +// filterExistingPythonSourceRules returns the rules accepted by shouldKeep. +func filterExistingPythonSourceRules( + rules []existingPythonSourceRule, + shouldKeep func(existingPythonSourceRule) bool, +) []existingPythonSourceRule { filtered := make([]existingPythonSourceRule, 0, len(rules)) for _, sourceRule := range rules { if shouldKeep(sourceRule) { @@ -180,15 +208,14 @@ func filterExistingPythonSourceRules(rules []existingPythonSourceRule, shouldKee return filtered } -func sourceRuleMatchesGeneratedPerFileName(sourceRule existingPythonSourceRule) bool { - it := sourceRule.srcs.Iterator() +// addTargetNamesForSrcs records the per-file target name Gazelle derives from +// each of srcs. +func addTargetNamesForSrcs(srcs *treeset.Set, dst map[string]struct{}) { + it := srcs.Iterator() for it.Next() { src := it.Value().(string) - if sourceRule.name == strings.TrimSuffix(filepath.Base(src), ".py") { - return true - } + dst[strings.TrimSuffix(filepath.Base(src), ".py")] = struct{}{} } - return false } // findConftestPaths returns package paths containing conftest.py, from currentPkg @@ -410,30 +437,49 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes } } + // generatedTargetNames holds the names of the targets Gazelle generates in + // this package. An existing rule with one of those names is not a + // hand-written target to adopt, it is the target Gazelle would have + // generated anyway. Adopting it would put two rules with the same name into + // result.Gen, where they merge into one and silently orphan the sources of + // whichever rule lost. This is computed before claiming so that it still + // covers the per-file names of the sources an adopted rule takes over. + generatedTargetNames := make(map[string]struct{}) + if cfg.PerFileGeneration() { + addTargetNamesForSrcs(pyLibraryFilenames, generatedTargetNames) + addTargetNamesForSrcs(pyTestFilenames, generatedTargetNames) + } else { + generatedTargetNames[cfg.RenderLibraryName(packageName)] = struct{}{} + generatedTargetNames[cfg.RenderTestName(packageName)] = struct{}{} + } + if hasPyBinaryEntryPointFile { + generatedTargetNames[cfg.RenderBinaryName(packageName)] = struct{}{} + } + if hasConftestFile { + generatedTargetNames[conftestTargetname] = struct{}{} + } + isNotGeneratedTargetName := func(sourceRule existingPythonSourceRule) bool { + _, isGenerated := generatedTargetNames[sourceRule.name] + return !isGenerated + } + existingPyLibraries := collectExistingPythonSourceRules(args, pyLibraryKind, knownPySrcs) existingPyTests := collectExistingPythonSourceRules(args, pyTestKind, knownPySrcs) existingPyLibraries = filterExistingPythonSourceRules( existingPyLibraries, - func(sourceRule existingPythonSourceRule) bool { - if !cfg.PerFileGeneration() { - return sourceRule.name != cfg.RenderLibraryName(packageName) - } - return !sourceRuleMatchesGeneratedPerFileName(sourceRule) - }, - ) - existingPyTests = filterExistingPythonSourceRules( - existingPyTests, - func(sourceRule existingPythonSourceRule) bool { - if !cfg.PerFileGeneration() { - return sourceRule.name != cfg.RenderTestName(packageName) - } - return !sourceRuleMatchesGeneratedPerFileName(sourceRule) - }, + isNotGeneratedTargetName, ) + existingPyTests = filterExistingPythonSourceRules(existingPyTests, isNotGeneratedTargetName) + // A library that owns a single source does not claim it: the source stays in + // the generated target as well, which is what users of the long-standing + // "extra target over one file" pattern expect. Coarse-grained generation has + // a single library for the whole tree, so there claiming is unconditional or + // the source would be owned twice. A py_test always claims, because a source + // pulled into two test targets is executed twice. claimingPyLibraries := filterExistingPythonSourceRules( existingPyLibraries, func(sourceRule existingPythonSourceRule) bool { - return sourceRule.srcs.Size() > 1 || cfg.CoarseGrainedGeneration() + return sourceRule.declaredSrcCount > 1 || cfg.CoarseGrainedGeneration() }, ) removeClaimedSrcs(claimingPyLibraries, pyLibraryFilenames, pyTestFilenames) @@ -448,25 +494,40 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes // autoIncludedInit reports whether the caller added pyLibraryEntrypointFilename // to srcs itself, rather than it being a source the user listed by hand. Only // in the former case may it be removed again when a main module is extracted. - appendPyLibrary := func(srcs *treeset.Set, pyLibraryTargetName string, autoIncludedInit bool) { + // + // isPreserved reports whether the target is an existing hand-written one being + // regenerated in place, as opposed to one Gazelle created. + appendPyLibrary := func( + srcs *treeset.Set, + pyLibraryTargetName string, + autoIncludedInit, isPreserved bool, + ) { allDeps, mainModules, annotations, err := parser.parse(srcs) - for name := range mainModules { - validFilesMap[name] = struct{}{} - } if err != nil { log.Fatalf("ERROR: %v\n", err) } + for name := range mainModules { + validFilesMap[name] = struct{}{} + } + srcsChanged := false if !hasPyBinaryEntryPointFile { // Creating one py_binary target per main module when __main__.py doesn't exist. mainFileNames := make([]string, 0, len(mainModules)) for name := range mainModules { + // A py_binary named after the target it would be extracted from + // cannot be generated: both rules would land in result.Gen under + // the same name and merge into one. + if isPreserved && strings.TrimSuffix(filepath.Base(name), ".py") == pyLibraryTargetName { + continue + } mainFileNames = append(mainFileNames, name) // Remove the file from srcs if we're doing per-file library generation so // that we don't also generate a py_library target for it. if cfg.PerFileGeneration() { srcs.Remove(name) + srcsChanged = true // Also remove the __init__.py that was added earlier. if autoIncludedInit { srcs.Remove(pyLibraryEntrypointFilename) @@ -515,6 +576,12 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes // If we're doing per-file generation, srcs could be empty at this point, meaning we shouldn't make a py_library. // If there is already a package named py_library target before, we should generate an empty py_library. if srcs.Empty() { + // Leave a preserved target exactly as it was written instead. Falling + // through would build an empty rule, which Gazelle reports as removable + // and so deletes a hand-written target. + if isPreserved { + return + } if args.File == nil { return } @@ -529,6 +596,16 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes } } + if srcsChanged { + // The dependencies above were derived from the srcs the target had + // before the main modules were extracted. Recompute them so the target + // does not keep dependencies contributed by a source it no longer owns. + allDeps, _, annotations, err = parser.parse(srcs) + if err != nil { + log.Fatalf("ERROR: %v\n", err) + } + } + // Add any sibling .pyi files to pyi_srcs pyiSrcs, _ := getPyiFilenames(srcs, cfg.GeneratePyiSrcs(), args.Dir) @@ -544,15 +621,30 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes collisionErrors.Add(err) } - pyLibrary := newTargetBuilder(pyLibraryKind, pyLibraryTargetName, pythonProjectRoot, args.Rel, pyFileNames, cfg.ResolveSiblingImports()). - addVisibility(visibility). + pyLibraryBuilder := newTargetBuilder( + pyLibraryKind, + pyLibraryTargetName, + pythonProjectRoot, + args.Rel, + pyFileNames, + cfg.ResolveSiblingImports(), + ). addSrcs(srcs). addPyiSrcs(pyiSrcs). addModuleDependencies(allDeps). addResolvedDependencies(annotations.includeDeps). generateImportsAttribute(). - setAnnotations(*annotations). - build() + setAnnotations(*annotations) + + // visibility is not a mergeable attribute, so rule.MergeRules copies it + // into an existing rule that does not set one and offers no '# keep' to + // prevent that. Injecting it would silently widen a hand-written target + // that relies on Bazel's default private visibility. + if !isPreserved { + pyLibraryBuilder.addVisibility(visibility) + } + + pyLibrary := pyLibraryBuilder.build() if pyLibrary.IsEmpty(py.Kinds()[pyLibrary.Kind()]) { result.Empty = append(result.Empty, pyLibrary) @@ -563,7 +655,7 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes } for _, existingPyLibrary := range existingPyLibraries { - appendPyLibrary(existingPyLibrary.srcs, existingPyLibrary.name, false) + appendPyLibrary(existingPyLibrary.srcs, existingPyLibrary.name, false, true) } if cfg.PerFileGeneration() { @@ -576,10 +668,10 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes if autoIncludeInit { srcs.Add(pyLibraryEntrypointFilename) } - appendPyLibrary(srcs, pyLibraryTargetName, autoIncludeInit) + appendPyLibrary(srcs, pyLibraryTargetName, autoIncludeInit, false) }) } else { - appendPyLibrary(pyLibraryFilenames, cfg.RenderLibraryName(packageName), false) + appendPyLibrary(pyLibraryFilenames, cfg.RenderLibraryName(packageName), false, false) } if hasPyBinaryEntryPointFile { diff --git a/gazelle/python/testdata/package_mode_respect_existing_conftest_name_collision/BUILD.in b/gazelle/python/testdata/package_mode_respect_existing_conftest_name_collision/BUILD.in new file mode 100644 index 0000000000..3ce356c497 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_conftest_name_collision/BUILD.in @@ -0,0 +1,12 @@ +load("@rules_python//python:defs.bzl", "py_library") + +# "conftest" is the name Gazelle generates for conftest.py, so this target is +# not adopted. +py_library( + name = "conftest", + srcs = [ + "bar.py", + "baz.py", + ], + tags = ["keep_me"], +) diff --git a/gazelle/python/testdata/package_mode_respect_existing_conftest_name_collision/BUILD.out b/gazelle/python/testdata/package_mode_respect_existing_conftest_name_collision/BUILD.out new file mode 100644 index 0000000000..2a59062431 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_conftest_name_collision/BUILD.out @@ -0,0 +1,22 @@ +load("@rules_python//python:defs.bzl", "py_library") + +# "conftest" is the name Gazelle generates for conftest.py, so this target is +# not adopted. +py_library( + name = "conftest", + testonly = True, + srcs = ["conftest.py"], + tags = ["keep_me"], + visibility = ["//:__subpackages__"], + deps = [":package_mode_respect_existing_conftest_name_collision"], +) + +py_library( + name = "package_mode_respect_existing_conftest_name_collision", + srcs = [ + "bar.py", + "baz.py", + "foo.py", + ], + visibility = ["//:__subpackages__"], +) diff --git a/gazelle/python/testdata/package_mode_respect_existing_conftest_name_collision/README.md b/gazelle/python/testdata/package_mode_respect_existing_conftest_name_collision/README.md new file mode 100644 index 0000000000..7ceabe97d3 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_conftest_name_collision/README.md @@ -0,0 +1,9 @@ +# Package mode with an existing target named after the generated conftest + +This test verifies that the generated-name check covers target names Gazelle +derives from something other than the generation mode. + +`conftest` is the name of the target Gazelle generates for `conftest.py`, so the +existing `conftest` is not adopted. Were it adopted, it would claim `bar.py` and +`baz.py` and the generated `conftest` would then merge over it, dropping both +sources from the build. diff --git a/gazelle/python/testdata/package_mode_respect_existing_conftest_name_collision/WORKSPACE b/gazelle/python/testdata/package_mode_respect_existing_conftest_name_collision/WORKSPACE new file mode 100644 index 0000000000..faff6af87a --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_conftest_name_collision/WORKSPACE @@ -0,0 +1 @@ +# This is a Bazel workspace for the Gazelle test data. diff --git a/gazelle/python/testdata/package_mode_respect_existing_conftest_name_collision/bar.py b/gazelle/python/testdata/package_mode_respect_existing_conftest_name_collision/bar.py new file mode 100644 index 0000000000..6b58ff30a8 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_conftest_name_collision/bar.py @@ -0,0 +1 @@ +# For test purposes only. diff --git a/gazelle/python/testdata/package_mode_respect_existing_conftest_name_collision/baz.py b/gazelle/python/testdata/package_mode_respect_existing_conftest_name_collision/baz.py new file mode 100644 index 0000000000..6b58ff30a8 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_conftest_name_collision/baz.py @@ -0,0 +1 @@ +# For test purposes only. diff --git a/gazelle/python/testdata/package_mode_respect_existing_conftest_name_collision/conftest.py b/gazelle/python/testdata/package_mode_respect_existing_conftest_name_collision/conftest.py new file mode 100644 index 0000000000..ddf557475a --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_conftest_name_collision/conftest.py @@ -0,0 +1 @@ +import foo diff --git a/gazelle/python/testdata/package_mode_respect_existing_conftest_name_collision/foo.py b/gazelle/python/testdata/package_mode_respect_existing_conftest_name_collision/foo.py new file mode 100644 index 0000000000..6b58ff30a8 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_conftest_name_collision/foo.py @@ -0,0 +1 @@ +# For test purposes only. diff --git a/gazelle/python/testdata/package_mode_respect_existing_conftest_name_collision/test.yaml b/gazelle/python/testdata/package_mode_respect_existing_conftest_name_collision/test.yaml new file mode 100644 index 0000000000..ed97d539c0 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_conftest_name_collision/test.yaml @@ -0,0 +1 @@ +--- diff --git a/gazelle/python/testdata/package_mode_respect_existing_entrypoint_srcs/BUILD.in b/gazelle/python/testdata/package_mode_respect_existing_entrypoint_srcs/BUILD.in new file mode 100644 index 0000000000..18ae0770f5 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_entrypoint_srcs/BUILD.in @@ -0,0 +1,13 @@ +load("@rules_python//python:defs.bzl", "py_library") + +# Gazelle should leave this target alone: it lists sources that always get their +# own generated targets. +py_library( + name = "custom", + srcs = [ + "__main__.py", + "bar.py", + "conftest.py", + ], + tags = ["keep_me"], +) diff --git a/gazelle/python/testdata/package_mode_respect_existing_entrypoint_srcs/BUILD.out b/gazelle/python/testdata/package_mode_respect_existing_entrypoint_srcs/BUILD.out new file mode 100644 index 0000000000..4dfca9dc91 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_entrypoint_srcs/BUILD.out @@ -0,0 +1,38 @@ +load("@rules_python//python:defs.bzl", "py_binary", "py_library") + +# Gazelle should leave this target alone: it lists sources that always get their +# own generated targets. +py_library( + name = "custom", + srcs = [ + "__main__.py", + "bar.py", + "conftest.py", + ], + tags = ["keep_me"], +) + +py_library( + name = "package_mode_respect_existing_entrypoint_srcs", + srcs = [ + "bar.py", + "foo.py", + ], + visibility = ["//:__subpackages__"], +) + +py_binary( + name = "package_mode_respect_existing_entrypoint_srcs_bin", + srcs = ["__main__.py"], + main = "__main__.py", + visibility = ["//:__subpackages__"], + deps = [":package_mode_respect_existing_entrypoint_srcs"], +) + +py_library( + name = "conftest", + testonly = True, + srcs = ["conftest.py"], + visibility = ["//:__subpackages__"], + deps = [":package_mode_respect_existing_entrypoint_srcs"], +) diff --git a/gazelle/python/testdata/package_mode_respect_existing_entrypoint_srcs/README.md b/gazelle/python/testdata/package_mode_respect_existing_entrypoint_srcs/README.md new file mode 100644 index 0000000000..ef86083c99 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_entrypoint_srcs/README.md @@ -0,0 +1,9 @@ +# Package mode with an existing target owning entrypoint sources + +This test verifies that a rule listing an entrypoint or `conftest.py` is left +untouched rather than preserved. + +`__main__.py` and `conftest.py` always get their own generated targets, and +Gazelle has no way to hand them over to another target. Regenerating `custom` +in place would therefore leave two targets owning each of those sources, so +`custom` keeps all three of its sources and gains no generated attributes. diff --git a/gazelle/python/testdata/package_mode_respect_existing_entrypoint_srcs/WORKSPACE b/gazelle/python/testdata/package_mode_respect_existing_entrypoint_srcs/WORKSPACE new file mode 100644 index 0000000000..faff6af87a --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_entrypoint_srcs/WORKSPACE @@ -0,0 +1 @@ +# This is a Bazel workspace for the Gazelle test data. diff --git a/gazelle/python/testdata/package_mode_respect_existing_entrypoint_srcs/__main__.py b/gazelle/python/testdata/package_mode_respect_existing_entrypoint_srcs/__main__.py new file mode 100644 index 0000000000..ddf557475a --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_entrypoint_srcs/__main__.py @@ -0,0 +1 @@ +import foo diff --git a/gazelle/python/testdata/package_mode_respect_existing_entrypoint_srcs/bar.py b/gazelle/python/testdata/package_mode_respect_existing_entrypoint_srcs/bar.py new file mode 100644 index 0000000000..6b58ff30a8 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_entrypoint_srcs/bar.py @@ -0,0 +1 @@ +# For test purposes only. diff --git a/gazelle/python/testdata/package_mode_respect_existing_entrypoint_srcs/conftest.py b/gazelle/python/testdata/package_mode_respect_existing_entrypoint_srcs/conftest.py new file mode 100644 index 0000000000..ddf557475a --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_entrypoint_srcs/conftest.py @@ -0,0 +1 @@ +import foo diff --git a/gazelle/python/testdata/package_mode_respect_existing_entrypoint_srcs/foo.py b/gazelle/python/testdata/package_mode_respect_existing_entrypoint_srcs/foo.py new file mode 100644 index 0000000000..6b58ff30a8 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_entrypoint_srcs/foo.py @@ -0,0 +1 @@ +# For test purposes only. diff --git a/gazelle/python/testdata/package_mode_respect_existing_entrypoint_srcs/test.yaml b/gazelle/python/testdata/package_mode_respect_existing_entrypoint_srcs/test.yaml new file mode 100644 index 0000000000..ed97d539c0 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_entrypoint_srcs/test.yaml @@ -0,0 +1 @@ +--- diff --git a/gazelle/python/testdata/package_mode_respect_existing_pruned_src_count/BUILD.in b/gazelle/python/testdata/package_mode_respect_existing_pruned_src_count/BUILD.in new file mode 100644 index 0000000000..d097d1c0a1 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_pruned_src_count/BUILD.in @@ -0,0 +1,12 @@ +load("@rules_python//python:defs.bzl", "py_library") + +# Two sources are declared, so this target claims bar.py even though removed.py +# no longer exists. +py_library( + name = "custom", + srcs = [ + "bar.py", + "removed.py", + ], + tags = ["keep_me"], +) diff --git a/gazelle/python/testdata/package_mode_respect_existing_pruned_src_count/BUILD.out b/gazelle/python/testdata/package_mode_respect_existing_pruned_src_count/BUILD.out new file mode 100644 index 0000000000..96dd47c8fb --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_pruned_src_count/BUILD.out @@ -0,0 +1,16 @@ +load("@rules_python//python:defs.bzl", "py_library") + +# Two sources are declared, so this target claims bar.py even though removed.py +# no longer exists. +py_library( + name = "custom", + srcs = ["bar.py"], + tags = ["keep_me"], + deps = [":package_mode_respect_existing_pruned_src_count"], +) + +py_library( + name = "package_mode_respect_existing_pruned_src_count", + srcs = ["foo.py"], + visibility = ["//:__subpackages__"], +) diff --git a/gazelle/python/testdata/package_mode_respect_existing_pruned_src_count/README.md b/gazelle/python/testdata/package_mode_respect_existing_pruned_src_count/README.md new file mode 100644 index 0000000000..e06ecdd77e --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_pruned_src_count/README.md @@ -0,0 +1,10 @@ +# Package mode with a preserved target whose declared sources are pruned + +This test verifies that whether a preserved library claims its sources depends +only on what the BUILD file declares. + +A library owning a single source does not claim it, but the count is taken from +the srcs the user wrote rather than from the srcs that survive pruning. `custom` +declares two sources, so it claims `bar.py` even though `removed.py` no longer +exists, and `bar.py` stays out of the generated package library. Deleting an +unrelated file must not flip a target between claiming and not claiming. diff --git a/gazelle/python/testdata/package_mode_respect_existing_pruned_src_count/WORKSPACE b/gazelle/python/testdata/package_mode_respect_existing_pruned_src_count/WORKSPACE new file mode 100644 index 0000000000..faff6af87a --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_pruned_src_count/WORKSPACE @@ -0,0 +1 @@ +# This is a Bazel workspace for the Gazelle test data. diff --git a/gazelle/python/testdata/package_mode_respect_existing_pruned_src_count/bar.py b/gazelle/python/testdata/package_mode_respect_existing_pruned_src_count/bar.py new file mode 100644 index 0000000000..ddf557475a --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_pruned_src_count/bar.py @@ -0,0 +1 @@ +import foo diff --git a/gazelle/python/testdata/package_mode_respect_existing_pruned_src_count/foo.py b/gazelle/python/testdata/package_mode_respect_existing_pruned_src_count/foo.py new file mode 100644 index 0000000000..6b58ff30a8 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_pruned_src_count/foo.py @@ -0,0 +1 @@ +# For test purposes only. diff --git a/gazelle/python/testdata/package_mode_respect_existing_pruned_src_count/test.yaml b/gazelle/python/testdata/package_mode_respect_existing_pruned_src_count/test.yaml new file mode 100644 index 0000000000..ed97d539c0 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_pruned_src_count/test.yaml @@ -0,0 +1 @@ +--- diff --git a/gazelle/python/testdata/package_mode_respect_existing_single_src_main_module/BUILD.out b/gazelle/python/testdata/package_mode_respect_existing_single_src_main_module/BUILD.out index b11c1e365b..c97cc86df0 100644 --- a/gazelle/python/testdata/package_mode_respect_existing_single_src_main_module/BUILD.out +++ b/gazelle/python/testdata/package_mode_respect_existing_single_src_main_module/BUILD.out @@ -7,7 +7,6 @@ py_library( name = "custom", srcs = ["cli.py"], tags = ["keep_me"], - visibility = ["//:__subpackages__"], deps = [":package_mode_respect_existing_single_src_main_module"], ) diff --git a/gazelle/python/testdata/package_mode_respect_existing_unmanaged_srcs/BUILD.out b/gazelle/python/testdata/package_mode_respect_existing_unmanaged_srcs/BUILD.out index 64ba0815c6..c64647c907 100644 --- a/gazelle/python/testdata/package_mode_respect_existing_unmanaged_srcs/BUILD.out +++ b/gazelle/python/testdata/package_mode_respect_existing_unmanaged_srcs/BUILD.out @@ -13,7 +13,6 @@ py_library( "ignored.py", ], tags = ["keep_me"], - visibility = ["//:__subpackages__"], ) py_library( diff --git a/gazelle/python/testdata/per_file_respect_existing_all_main_modules/BUILD.in b/gazelle/python/testdata/per_file_respect_existing_all_main_modules/BUILD.in new file mode 100644 index 0000000000..5b194e8ec8 --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_all_main_modules/BUILD.in @@ -0,0 +1,14 @@ +load("@rules_python//python:defs.bzl", "py_library") + +# gazelle:python_generation_mode file + +# Every source is a main module, so this target must be left alone rather than +# emptied and deleted. +py_library( + name = "custom", + srcs = [ + "a.py", + "b.py", + ], + tags = ["keep_me"], +) diff --git a/gazelle/python/testdata/per_file_respect_existing_all_main_modules/BUILD.out b/gazelle/python/testdata/per_file_respect_existing_all_main_modules/BUILD.out new file mode 100644 index 0000000000..2590d4e158 --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_all_main_modules/BUILD.out @@ -0,0 +1,34 @@ +load("@rules_python//python:defs.bzl", "py_binary", "py_library") + +# gazelle:python_generation_mode file + +# Every source is a main module, so this target must be left alone rather than +# emptied and deleted. +py_library( + name = "custom", + srcs = [ + "a.py", + "b.py", + ], + tags = ["keep_me"], +) + +py_binary( + name = "a", + srcs = ["a.py"], + visibility = ["//:__subpackages__"], + deps = [":foo"], +) + +py_binary( + name = "b", + srcs = ["b.py"], + visibility = ["//:__subpackages__"], + deps = [":foo"], +) + +py_library( + name = "foo", + srcs = ["foo.py"], + visibility = ["//:__subpackages__"], +) diff --git a/gazelle/python/testdata/per_file_respect_existing_all_main_modules/README.md b/gazelle/python/testdata/per_file_respect_existing_all_main_modules/README.md new file mode 100644 index 0000000000..24f56aa828 --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_all_main_modules/README.md @@ -0,0 +1,10 @@ +# Per-file generation with a preserved target of only main modules + +This test verifies that a preserved target is never deleted by having its +sources extracted out from under it. + +In per-file generation a main module is removed from the library's srcs so that +only the `py_binary` owns it. Here every source is a main module, so the target +would be left with empty srcs, and an empty rule is one Gazelle reports as +removable. Rather than delete a hand-written target, Gazelle leaves it exactly +as written and still generates the two `py_binary` targets. diff --git a/gazelle/python/testdata/per_file_respect_existing_all_main_modules/WORKSPACE b/gazelle/python/testdata/per_file_respect_existing_all_main_modules/WORKSPACE new file mode 100644 index 0000000000..faff6af87a --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_all_main_modules/WORKSPACE @@ -0,0 +1 @@ +# This is a Bazel workspace for the Gazelle test data. diff --git a/gazelle/python/testdata/per_file_respect_existing_all_main_modules/a.py b/gazelle/python/testdata/per_file_respect_existing_all_main_modules/a.py new file mode 100644 index 0000000000..07cb4bb282 --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_all_main_modules/a.py @@ -0,0 +1,4 @@ +import foo + +if __name__ == "__main__": + print(foo) diff --git a/gazelle/python/testdata/per_file_respect_existing_all_main_modules/b.py b/gazelle/python/testdata/per_file_respect_existing_all_main_modules/b.py new file mode 100644 index 0000000000..07cb4bb282 --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_all_main_modules/b.py @@ -0,0 +1,4 @@ +import foo + +if __name__ == "__main__": + print(foo) diff --git a/gazelle/python/testdata/per_file_respect_existing_all_main_modules/foo.py b/gazelle/python/testdata/per_file_respect_existing_all_main_modules/foo.py new file mode 100644 index 0000000000..6b58ff30a8 --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_all_main_modules/foo.py @@ -0,0 +1 @@ +# For test purposes only. diff --git a/gazelle/python/testdata/per_file_respect_existing_all_main_modules/test.yaml b/gazelle/python/testdata/per_file_respect_existing_all_main_modules/test.yaml new file mode 100644 index 0000000000..ed97d539c0 --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_all_main_modules/test.yaml @@ -0,0 +1 @@ +--- diff --git a/gazelle/python/testdata/per_file_respect_existing_generated_name_collision/BUILD.in b/gazelle/python/testdata/per_file_respect_existing_generated_name_collision/BUILD.in new file mode 100644 index 0000000000..002acd3b9c --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_generated_name_collision/BUILD.in @@ -0,0 +1,13 @@ +load("@rules_python//python:defs.bzl", "py_library") + +# gazelle:python_generation_mode file + +# "qux" is the name Gazelle generates for qux.py, so this target is not adopted. +py_library( + name = "qux", + srcs = [ + "bar.py", + "baz.py", + ], + tags = ["keep_me"], +) diff --git a/gazelle/python/testdata/per_file_respect_existing_generated_name_collision/BUILD.out b/gazelle/python/testdata/per_file_respect_existing_generated_name_collision/BUILD.out new file mode 100644 index 0000000000..3d7e45756b --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_generated_name_collision/BUILD.out @@ -0,0 +1,31 @@ +load("@rules_python//python:defs.bzl", "py_library") + +# gazelle:python_generation_mode file + +# "qux" is the name Gazelle generates for qux.py, so this target is not adopted. +py_library( + name = "qux", + srcs = ["qux.py"], + tags = ["keep_me"], + visibility = ["//:__subpackages__"], + deps = [":bar"], +) + +py_library( + name = "bar", + srcs = ["bar.py"], + visibility = ["//:__subpackages__"], + deps = [":foo"], +) + +py_library( + name = "baz", + srcs = ["baz.py"], + visibility = ["//:__subpackages__"], +) + +py_library( + name = "foo", + srcs = ["foo.py"], + visibility = ["//:__subpackages__"], +) diff --git a/gazelle/python/testdata/per_file_respect_existing_generated_name_collision/README.md b/gazelle/python/testdata/per_file_respect_existing_generated_name_collision/README.md new file mode 100644 index 0000000000..b3e994f86d --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_generated_name_collision/README.md @@ -0,0 +1,15 @@ +# Per-file generation with an existing target named after a generated one + +This test verifies that Gazelle refuses to adopt an existing target whose name +is one it generates itself. + +`qux` is the per-file target name for `qux.py`, so the existing `qux` is not a +hand-written target to regenerate in place. Adopting it would let it claim +`bar.py` and `baz.py`, suppressing the per-file targets for them, and the +generated `qux` would then merge over it and drop both sources -- leaving no +target that owns them. + +Instead the existing rule is left out of preservation and `bar.py` and `baz.py` +get their own per-file targets. Gazelle still merges the generated `qux` over +the existing rule of that name, which is its long-standing behavior for any +rule whose name matches a generated target. diff --git a/gazelle/python/testdata/per_file_respect_existing_generated_name_collision/WORKSPACE b/gazelle/python/testdata/per_file_respect_existing_generated_name_collision/WORKSPACE new file mode 100644 index 0000000000..faff6af87a --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_generated_name_collision/WORKSPACE @@ -0,0 +1 @@ +# This is a Bazel workspace for the Gazelle test data. diff --git a/gazelle/python/testdata/per_file_respect_existing_generated_name_collision/bar.py b/gazelle/python/testdata/per_file_respect_existing_generated_name_collision/bar.py new file mode 100644 index 0000000000..ddf557475a --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_generated_name_collision/bar.py @@ -0,0 +1 @@ +import foo diff --git a/gazelle/python/testdata/per_file_respect_existing_generated_name_collision/baz.py b/gazelle/python/testdata/per_file_respect_existing_generated_name_collision/baz.py new file mode 100644 index 0000000000..6b58ff30a8 --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_generated_name_collision/baz.py @@ -0,0 +1 @@ +# For test purposes only. diff --git a/gazelle/python/testdata/per_file_respect_existing_generated_name_collision/foo.py b/gazelle/python/testdata/per_file_respect_existing_generated_name_collision/foo.py new file mode 100644 index 0000000000..6b58ff30a8 --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_generated_name_collision/foo.py @@ -0,0 +1 @@ +# For test purposes only. diff --git a/gazelle/python/testdata/per_file_respect_existing_generated_name_collision/qux.py b/gazelle/python/testdata/per_file_respect_existing_generated_name_collision/qux.py new file mode 100644 index 0000000000..b6b8723822 --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_generated_name_collision/qux.py @@ -0,0 +1 @@ +import bar diff --git a/gazelle/python/testdata/per_file_respect_existing_generated_name_collision/test.yaml b/gazelle/python/testdata/per_file_respect_existing_generated_name_collision/test.yaml new file mode 100644 index 0000000000..ed97d539c0 --- /dev/null +++ b/gazelle/python/testdata/per_file_respect_existing_generated_name_collision/test.yaml @@ -0,0 +1 @@ +--- diff --git a/gazelle/python/testdata/per_file_respect_existing_with_init_and_main_module/BUILD.out b/gazelle/python/testdata/per_file_respect_existing_with_init_and_main_module/BUILD.out index ae20706ff0..978dd2894f 100644 --- a/gazelle/python/testdata/per_file_respect_existing_with_init_and_main_module/BUILD.out +++ b/gazelle/python/testdata/per_file_respect_existing_with_init_and_main_module/BUILD.out @@ -9,8 +9,6 @@ py_library( name = "custom", srcs = ["__init__.py"], tags = ["keep_me"], - visibility = ["//:__subpackages__"], - deps = [":foo"], ) py_binary( diff --git a/gazelle/python/testdata/per_file_respect_existing_with_init_and_main_module/README.md b/gazelle/python/testdata/per_file_respect_existing_with_init_and_main_module/README.md index 12b1953699..c65040c1b4 100644 --- a/gazelle/python/testdata/per_file_respect_existing_with_init_and_main_module/README.md +++ b/gazelle/python/testdata/per_file_respect_existing_with_init_and_main_module/README.md @@ -8,6 +8,6 @@ to the per-file targets it generates. It must not remove `__init__.py` from a preserved target that listed it explicitly, which would empty the target's srcs and delete it. -The preserved target keeps a dependency on `:foo` even though `__init__.py` does -not import it: dependencies are resolved from the target's sources before the -main module is extracted, so `cli.py`'s imports are still attributed to it. +The preserved target gets no dependency on `:foo`: `cli.py` imports `foo`, but +dependencies are recomputed after the main module is extracted, so an import +from a source the target no longer owns does not survive in its `deps`. diff --git a/gazelle/python/testdata/simple_binary_with_library/BUILD.in b/gazelle/python/testdata/simple_binary_with_library/BUILD.in index 1df483ec35..1b3869119f 100644 --- a/gazelle/python/testdata/simple_binary_with_library/BUILD.in +++ b/gazelle/python/testdata/simple_binary_with_library/BUILD.in @@ -9,7 +9,7 @@ py_library( ], ) -# Gazelle should preserve this custom target and keep bar.py in both libraries. +# Gazelle should keep this custom target unmodified, with bar.py in both libraries. py_library( name = "custom", srcs = [ diff --git a/gazelle/python/testdata/simple_binary_with_library/BUILD.out b/gazelle/python/testdata/simple_binary_with_library/BUILD.out index d7a04ade3e..e97dc02b4f 100644 --- a/gazelle/python/testdata/simple_binary_with_library/BUILD.out +++ b/gazelle/python/testdata/simple_binary_with_library/BUILD.out @@ -10,13 +10,12 @@ py_library( visibility = ["//:__subpackages__"], ) -# Gazelle should preserve this custom target and keep bar.py in both libraries. +# Gazelle should keep this custom target unmodified, with bar.py in both libraries. py_library( name = "custom", srcs = [ "bar.py", ], - visibility = ["//:__subpackages__"], ) py_binary( diff --git a/gazelle/python/testdata/simple_binary_with_library/README.md b/gazelle/python/testdata/simple_binary_with_library/README.md index ea2686122c..c6d8af7f85 100644 --- a/gazelle/python/testdata/simple_binary_with_library/README.md +++ b/gazelle/python/testdata/simple_binary_with_library/README.md @@ -4,5 +4,7 @@ This test case asserts that a simple `py_binary` is generated as expected referencing a `py_library`. The existing custom `py_library` shares `bar.py` with the generated package -library. Gazelle should preserve that shared source in both targets while adding -generated attributes such as `visibility`. +library. Gazelle should preserve that shared source in both targets and leave +the custom target otherwise untouched: it gets no `visibility`, because +injecting one would widen a hand-written target that relies on the default +private visibility. diff --git a/news/preserve_existing_python_targets.changed.md b/news/preserve_existing_python_targets.changed.md new file mode 100644 index 0000000000..61efa61372 --- /dev/null +++ b/news/preserve_existing_python_targets.changed.md @@ -0,0 +1,8 @@ +(gazelle) Existing hand-written {obj}`py_library` and {obj}`py_test` targets +that own sources Gazelle manages can now be regenerated in place. Gazelle +manages their `srcs` and dependency attributes, and usually keeps their sources +out of generated targets; a single-source library remains shared outside +project mode. Dependencies not derived from imports are removed unless marked +with `# keep`. Add `# keep` above the rule to opt out. Rules named after a +generated target, and rules listing `__main__.py`, `__test__.py`, or +`conftest.py`, are excluded from this preservation behavior. From e0d791d3f84a5818e236e8bb383482e4b902bdbc Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 11 Sep 2026 00:33:37 +0000 Subject: [PATCH 09/18] fix(gazelle): preserve split package libraries Co-authored-by: Alex Martani --- gazelle/docs/installation_and_usage.md | 6 +- gazelle/python/generate.go | 99 +++++++++++++++---- .../BUILD.in | 29 ++++++ .../BUILD.out | 30 ++++++ .../README.md | 6 ++ .../WORKSPACE | 1 + .../__init__.py | 0 .../aux.py | 1 + .../bar.py | 1 + .../baz.py | 1 + .../foo.py | 1 + .../qux.py | 1 + .../test.yaml | 1 + 13 files changed, 159 insertions(+), 18 deletions(-) create mode 100644 gazelle/python/testdata/package_mode_respect_existing_split_package_library/BUILD.in create mode 100644 gazelle/python/testdata/package_mode_respect_existing_split_package_library/BUILD.out create mode 100644 gazelle/python/testdata/package_mode_respect_existing_split_package_library/README.md create mode 100644 gazelle/python/testdata/package_mode_respect_existing_split_package_library/WORKSPACE create mode 100644 gazelle/python/testdata/package_mode_respect_existing_split_package_library/__init__.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_split_package_library/aux.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_split_package_library/bar.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_split_package_library/baz.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_split_package_library/foo.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_split_package_library/qux.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_split_package_library/test.yaml diff --git a/gazelle/docs/installation_and_usage.md b/gazelle/docs/installation_and_usage.md index baec6845b8..499895b86d 100644 --- a/gazelle/docs/installation_and_usage.md +++ b/gazelle/docs/installation_and_usage.md @@ -213,7 +213,11 @@ Gazelle regenerates eligible hand-written {bzl:obj}`py_library` and non-empty list of relative `.py` paths and at least one of those paths is a source Gazelle manages. Targets are excluded from this behavior when: -- Their name is one Gazelle will generate. +- In file generation mode, their name is one Gazelle will generate for a + source file in the package. In package generation mode, their name is the + generated package library or test target unless the package uses a split + layout: a package library alongside per-file libraries with disjoint + sources. - Their `srcs` attribute uses `glob()`, a label, or a non-`.py` file. - Their `srcs` contains `__main__.py`, `__test__.py`, or `conftest.py`, which Gazelle handles with dedicated targets. diff --git a/gazelle/python/generate.go b/gazelle/python/generate.go index 6b45e091e8..cd94d41c9d 100644 --- a/gazelle/python/generate.go +++ b/gazelle/python/generate.go @@ -208,6 +208,44 @@ func filterExistingPythonSourceRules( return filtered } +// existingRulesShareSrcs reports whether two preserved rules list the same +// source file. +func existingRulesShareSrcs(a, b existingPythonSourceRule) bool { + it := a.srcs.Iterator() + for it.Next() { + if b.srcs.Contains(it.Value()) { + return true + } + } + return false +} + +// hasSplitPackageLibraryLayout reports whether the package already defines the +// generated package library name alongside other preserved libraries whose +// sources are disjoint from it. This is the layout where per-file libraries own +// individual modules and the package library owns the remainder. +func hasSplitPackageLibraryLayout(packageLibraryName string, rules []existingPythonSourceRule) bool { + var packageLibrary *existingPythonSourceRule + for i := range rules { + if rules[i].name == packageLibraryName { + packageLibrary = &rules[i] + break + } + } + if packageLibrary == nil || len(rules) < 2 { + return false + } + for _, other := range rules { + if other.name == packageLibraryName { + continue + } + if existingRulesShareSrcs(*packageLibrary, other) { + return false + } + } + return true +} + // addTargetNamesForSrcs records the per-file target name Gazelle derives from // each of srcs. func addTargetNamesForSrcs(srcs *treeset.Set, dst map[string]struct{}) { @@ -437,19 +475,27 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes } } - // generatedTargetNames holds the names of the targets Gazelle generates in - // this package. An existing rule with one of those names is not a - // hand-written target to adopt, it is the target Gazelle would have - // generated anyway. Adopting it would put two rules with the same name into - // result.Gen, where they merge into one and silently orphan the sources of - // whichever rule lost. This is computed before claiming so that it still - // covers the per-file names of the sources an adopted rule takes over. + // generatedTargetNames holds per-file target names Gazelle generates in this + // package. In file mode, an existing rule with one of those names is not a + // hand-written target to adopt: adopting it would let it claim sources that + // belong in other per-file targets, and the generated rule of the same name + // would merge over it and drop them. Package- and project-level library and + // test names are intentionally absent: those targets are regenerated in + // place. Dedicated binary and conftest target names are always excluded. + packageLibraryName := cfg.RenderLibraryName(packageName) + existingPyLibraries := collectExistingPythonSourceRules(args, pyLibraryKind, knownPySrcs) + existingPyTests := collectExistingPythonSourceRules(args, pyTestKind, knownPySrcs) + splitPackageLibraryLayout := false + if !cfg.PerFileGeneration() { + splitPackageLibraryLayout = hasSplitPackageLibraryLayout(packageLibraryName, existingPyLibraries) + } + generatedTargetNames := make(map[string]struct{}) if cfg.PerFileGeneration() { addTargetNamesForSrcs(pyLibraryFilenames, generatedTargetNames) addTargetNamesForSrcs(pyTestFilenames, generatedTargetNames) - } else { - generatedTargetNames[cfg.RenderLibraryName(packageName)] = struct{}{} + } else if !splitPackageLibraryLayout { + generatedTargetNames[packageLibraryName] = struct{}{} generatedTargetNames[cfg.RenderTestName(packageName)] = struct{}{} } if hasPyBinaryEntryPointFile { @@ -463,23 +509,31 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes return !isGenerated } - existingPyLibraries := collectExistingPythonSourceRules(args, pyLibraryKind, knownPySrcs) - existingPyTests := collectExistingPythonSourceRules(args, pyTestKind, knownPySrcs) existingPyLibraries = filterExistingPythonSourceRules( existingPyLibraries, isNotGeneratedTargetName, ) existingPyTests = filterExistingPythonSourceRules(existingPyTests, isNotGeneratedTargetName) + hasPreservedPackageLibrary := splitPackageLibraryLayout // A library that owns a single source does not claim it: the source stays in // the generated target as well, which is what users of the long-standing // "extra target over one file" pattern expect. Coarse-grained generation has // a single library for the whole tree, so there claiming is unconditional or - // the source would be owned twice. A py_test always claims, because a source - // pulled into two test targets is executed twice. + // the source would be owned twice. When a hand-written target already uses + // the package library name alongside per-file libraries, every other + // preserved library claims its sources so the generated package library does + // not duplicate them. A py_test always claims, because a source pulled into + // two test targets is executed twice. claimingPyLibraries := filterExistingPythonSourceRules( existingPyLibraries, func(sourceRule existingPythonSourceRule) bool { - return sourceRule.declaredSrcCount > 1 || cfg.CoarseGrainedGeneration() + if sourceRule.declaredSrcCount > 1 || cfg.CoarseGrainedGeneration() { + return true + } + if hasPreservedPackageLibrary && sourceRule.name != packageLibraryName { + return true + } + return false }, ) removeClaimedSrcs(claimingPyLibraries, pyLibraryFilenames, pyTestFilenames) @@ -655,7 +709,18 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes } for _, existingPyLibrary := range existingPyLibraries { - appendPyLibrary(existingPyLibrary.srcs, existingPyLibrary.name, false, true) + srcs := existingPyLibrary.srcs + if existingPyLibrary.name == packageLibraryName { + mergedSrcs := treeset.NewWith(godsutils.StringComparator) + srcs.Each(func(index int, filename interface{}) { + mergedSrcs.Add(filename) + }) + pyLibraryFilenames.Each(func(index int, filename interface{}) { + mergedSrcs.Add(filename) + }) + srcs = mergedSrcs + } + appendPyLibrary(srcs, existingPyLibrary.name, false, true) } if cfg.PerFileGeneration() { @@ -670,8 +735,8 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes } appendPyLibrary(srcs, pyLibraryTargetName, autoIncludeInit, false) }) - } else { - appendPyLibrary(pyLibraryFilenames, cfg.RenderLibraryName(packageName), false, false) + } else if !hasPreservedPackageLibrary { + appendPyLibrary(pyLibraryFilenames, packageLibraryName, false, false) } if hasPyBinaryEntryPointFile { diff --git a/gazelle/python/testdata/package_mode_respect_existing_split_package_library/BUILD.in b/gazelle/python/testdata/package_mode_respect_existing_split_package_library/BUILD.in new file mode 100644 index 0000000000..6726d93651 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_split_package_library/BUILD.in @@ -0,0 +1,29 @@ +load("@rules_python//python:defs.bzl", "py_library") + +py_library( + name = "foo", + srcs = ["foo.py"], + tags = ["keep_foo"], +) + +py_library( + name = "bar", + srcs = ["bar.py"], + tags = ["keep_bar"], +) + +py_library( + name = "baz", + srcs = ["baz.py"], + tags = ["keep_baz"], +) + +# Uses the package library name Gazelle generates in package mode. +py_library( + name = "package_mode_respect_existing_split_package_library", + srcs = [ + "aux.py", + "qux.py", + ], + tags = ["keep_pkg"], +) diff --git a/gazelle/python/testdata/package_mode_respect_existing_split_package_library/BUILD.out b/gazelle/python/testdata/package_mode_respect_existing_split_package_library/BUILD.out new file mode 100644 index 0000000000..9e458620de --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_split_package_library/BUILD.out @@ -0,0 +1,30 @@ +load("@rules_python//python:defs.bzl", "py_library") + +py_library( + name = "foo", + srcs = ["foo.py"], + tags = ["keep_foo"], +) + +py_library( + name = "bar", + srcs = ["bar.py"], + tags = ["keep_bar"], +) + +py_library( + name = "baz", + srcs = ["baz.py"], + tags = ["keep_baz"], +) + +# Uses the package library name Gazelle generates in package mode. +py_library( + name = "package_mode_respect_existing_split_package_library", + srcs = [ + "__init__.py", + "aux.py", + "qux.py", + ], + tags = ["keep_pkg"], +) diff --git a/gazelle/python/testdata/package_mode_respect_existing_split_package_library/README.md b/gazelle/python/testdata/package_mode_respect_existing_split_package_library/README.md new file mode 100644 index 0000000000..0a6fb2db27 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_split_package_library/README.md @@ -0,0 +1,6 @@ +# Package mode with a split package library and per-file libraries + +This test verifies that a hand-written target using the package library name +is preserved and regenerated in place alongside per-file `py_library` targets. +Sources owned by the per-file targets must not also appear in the generated +package library. diff --git a/gazelle/python/testdata/package_mode_respect_existing_split_package_library/WORKSPACE b/gazelle/python/testdata/package_mode_respect_existing_split_package_library/WORKSPACE new file mode 100644 index 0000000000..faff6af87a --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_split_package_library/WORKSPACE @@ -0,0 +1 @@ +# This is a Bazel workspace for the Gazelle test data. diff --git a/gazelle/python/testdata/package_mode_respect_existing_split_package_library/__init__.py b/gazelle/python/testdata/package_mode_respect_existing_split_package_library/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/gazelle/python/testdata/package_mode_respect_existing_split_package_library/aux.py b/gazelle/python/testdata/package_mode_respect_existing_split_package_library/aux.py new file mode 100644 index 0000000000..22a23c4c1c --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_split_package_library/aux.py @@ -0,0 +1 @@ +v = 5 diff --git a/gazelle/python/testdata/package_mode_respect_existing_split_package_library/bar.py b/gazelle/python/testdata/package_mode_respect_existing_split_package_library/bar.py new file mode 100644 index 0000000000..47643d4d30 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_split_package_library/bar.py @@ -0,0 +1 @@ +y = 2 diff --git a/gazelle/python/testdata/package_mode_respect_existing_split_package_library/baz.py b/gazelle/python/testdata/package_mode_respect_existing_split_package_library/baz.py new file mode 100644 index 0000000000..b67ca6a5d8 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_split_package_library/baz.py @@ -0,0 +1 @@ +z = 3 diff --git a/gazelle/python/testdata/package_mode_respect_existing_split_package_library/foo.py b/gazelle/python/testdata/package_mode_respect_existing_split_package_library/foo.py new file mode 100644 index 0000000000..7d4290a117 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_split_package_library/foo.py @@ -0,0 +1 @@ +x = 1 diff --git a/gazelle/python/testdata/package_mode_respect_existing_split_package_library/qux.py b/gazelle/python/testdata/package_mode_respect_existing_split_package_library/qux.py new file mode 100644 index 0000000000..6e912b5abc --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_split_package_library/qux.py @@ -0,0 +1 @@ +w = 4 diff --git a/gazelle/python/testdata/package_mode_respect_existing_split_package_library/test.yaml b/gazelle/python/testdata/package_mode_respect_existing_split_package_library/test.yaml new file mode 100644 index 0000000000..ed97d539c0 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_split_package_library/test.yaml @@ -0,0 +1 @@ +--- From c4355434a5e6cb45f65217ceab10e5b61a94bd87 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 11 Sep 2026 00:45:20 +0000 Subject: [PATCH 10/18] fix(gazelle): preserve excluded init package targets Co-authored-by: Alex Martani --- gazelle/python/BUILD.bazel | 4 + gazelle/python/generate.go | 80 +++++++++ gazelle/python/generate_test.go | 165 ++++++++++++++++++ .../BUILD.in | 17 ++ .../BUILD.out | 18 ++ .../README.md | 5 + .../WORKSPACE | 1 + .../excluded_only.py | 1 + .../foo.py | 1 + .../test.yaml | 1 + .../BUILD.in | 17 ++ .../BUILD.out | 17 ++ .../README.md | 7 + .../WORKSPACE | 1 + .../__init__.py | 0 .../foo.py | 2 + .../test.yaml | 1 + 17 files changed, 338 insertions(+) create mode 100644 gazelle/python/generate_test.go create mode 100644 gazelle/python/testdata/package_mode_respect_existing_excluded_only_target/BUILD.in create mode 100644 gazelle/python/testdata/package_mode_respect_existing_excluded_only_target/BUILD.out create mode 100644 gazelle/python/testdata/package_mode_respect_existing_excluded_only_target/README.md create mode 100644 gazelle/python/testdata/package_mode_respect_existing_excluded_only_target/WORKSPACE create mode 100644 gazelle/python/testdata/package_mode_respect_existing_excluded_only_target/excluded_only.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_excluded_only_target/foo.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_excluded_only_target/test.yaml create mode 100644 gazelle/python/testdata/package_mode_respect_existing_split_package_library_excluded_init/BUILD.in create mode 100644 gazelle/python/testdata/package_mode_respect_existing_split_package_library_excluded_init/BUILD.out create mode 100644 gazelle/python/testdata/package_mode_respect_existing_split_package_library_excluded_init/README.md create mode 100644 gazelle/python/testdata/package_mode_respect_existing_split_package_library_excluded_init/WORKSPACE create mode 100644 gazelle/python/testdata/package_mode_respect_existing_split_package_library_excluded_init/__init__.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_split_package_library_excluded_init/foo.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_split_package_library_excluded_init/test.yaml diff --git a/gazelle/python/BUILD.bazel b/gazelle/python/BUILD.bazel index bd6679572f..2b81d46f5e 100644 --- a/gazelle/python/BUILD.bazel +++ b/gazelle/python/BUILD.bazel @@ -120,11 +120,15 @@ go_test( name = "default_test", srcs = [ "file_parser_test.go", + "generate_test.go", "parser_test.go", "std_modules_test.go", ], embed = [":python"], deps = [ + "@bazel_gazelle//config:go_default_library", + "@bazel_gazelle//language:go_default_library", + "@bazel_gazelle//rule:go_default_library", "@com_github_emirpasic_gods//sets/treeset:go_default_library", "@com_github_emirpasic_gods//utils:go_default_library", "@com_github_stretchr_testify//assert", diff --git a/gazelle/python/generate.go b/gazelle/python/generate.go index cd94d41c9d..02acc8c2b8 100644 --- a/gazelle/python/generate.go +++ b/gazelle/python/generate.go @@ -246,6 +246,70 @@ func hasSplitPackageLibraryLayout(packageLibraryName string, rules []existingPyt return true } +// adoptExcludedInitOnlyPackageLibraryForSplitLayout appends the hand-written +// package library when it lists only an excluded __init__.py. That target is +// otherwise not adopted because none of its srcs are Gazelle-managed, but it +// is still the package aggregate in a split layout and must be preserved so +// Gazelle does not emit a competing package-level library. +func adoptExcludedInitOnlyPackageLibraryForSplitLayout( + args language.GenerateArgs, + kind string, + packageLibraryName string, + knownSrcs map[string]struct{}, + rules []existingPythonSourceRule, +) []existingPythonSourceRule { + if args.File == nil || len(rules) == 0 { + return rules + } + for _, sourceRule := range rules { + if sourceRule.name == packageLibraryName { + return rules + } + } + + genFiles := make(map[string]struct{}, len(args.GenFiles)) + for _, f := range args.GenFiles { + genFiles[f] = struct{}{} + } + srcExists := func(src string) bool { + if _, ok := genFiles[src]; ok { + return true + } + _, err := os.Stat(filepath.Join(args.Dir, src)) + return err == nil + } + + for _, existingRule := range args.File.Rules { + if existingRule.Name() != packageLibraryName || !kindMatches(args.Config, existingRule, kind) { + continue + } + srcs := existingRule.AttrStrings("srcs") + if len(srcs) != 1 || srcs[0] != pyLibraryEntrypointFilename { + return rules + } + if _, ok := knownSrcs[pyLibraryEntrypointFilename]; ok { + return rules + } + if !srcExists(pyLibraryEntrypointFilename) { + return rules + } + + validSrcs := treeset.NewWith(godsutils.StringComparator, pyLibraryEntrypointFilename) + candidate := existingPythonSourceRule{ + name: packageLibraryName, + srcs: validSrcs, + declaredSrcCount: 1, + } + for _, other := range rules { + if existingRulesShareSrcs(candidate, other) { + return rules + } + } + return append(rules, candidate) + } + return rules +} + // addTargetNamesForSrcs records the per-file target name Gazelle derives from // each of srcs. func addTargetNamesForSrcs(srcs *treeset.Set, dst map[string]struct{}) { @@ -484,6 +548,15 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes // place. Dedicated binary and conftest target names are always excluded. packageLibraryName := cfg.RenderLibraryName(packageName) existingPyLibraries := collectExistingPythonSourceRules(args, pyLibraryKind, knownPySrcs) + if !cfg.PerFileGeneration() { + existingPyLibraries = adoptExcludedInitOnlyPackageLibraryForSplitLayout( + args, + pyLibraryKind, + packageLibraryName, + knownPySrcs, + existingPyLibraries, + ) + } existingPyTests := collectExistingPythonSourceRules(args, pyTestKind, knownPySrcs) splitPackageLibraryLayout := false if !cfg.PerFileGeneration() { @@ -981,6 +1054,13 @@ func (py *Python) getRulesWithInvalidSrcs(args language.GenerateArgs, validFiles hasValidSrcs = true break } + // Sources hidden from generation via gazelle:exclude or + // python_ignore_files are absent from filesMap but may still be + // listed in a hand-written target that Gazelle leaves unmanaged. + if _, err := os.Stat(filepath.Join(args.Dir, src)); err == nil { + hasValidSrcs = true + break + } } if !hasValidSrcs { invalidRules = append(invalidRules, newTargetBuilder(matchedKind, existingRule.Name(), "", "", nil, false).build()) diff --git a/gazelle/python/generate_test.go b/gazelle/python/generate_test.go new file mode 100644 index 0000000000..60940c7e64 --- /dev/null +++ b/gazelle/python/generate_test.go @@ -0,0 +1,165 @@ +package python + +import ( + "os" + "path/filepath" + "testing" + + "github.com/bazelbuild/bazel-gazelle/config" + "github.com/bazelbuild/bazel-gazelle/language" + "github.com/bazelbuild/bazel-gazelle/rule" + "github.com/emirpasic/gods/sets/treeset" + godsutils "github.com/emirpasic/gods/utils" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func newTestBuildFile(rules ...*rule.Rule) *rule.File { + return &rule.File{ + Path: "BUILD.bazel", + Rules: rules, + } +} + +func newPyLibraryRule(name string, srcs []string) *rule.Rule { + r := rule.NewRule("py_library", name) + r.SetAttr("srcs", srcs) + return r +} + +func TestCollectExistingPythonSourceRulesSkipsExcludedOnly(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "excluded_only.py"), []byte(""), 0o600)) + + args := language.GenerateArgs{ + Dir: dir, + File: newTestBuildFile(newPyLibraryRule("excluded_only", []string{"excluded_only.py"})), + Config: &config.Config{}, + } + knownSrcs := map[string]struct{}{"foo.py": {}} + + rules := collectExistingPythonSourceRules(args, pyLibraryKind, knownSrcs) + assert.Empty(t, rules) +} + +func TestCollectExistingPythonSourceRulesKeepsExcludedAlongsideKnown(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "foo.py"), []byte(""), 0o600)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "excluded_only.py"), []byte(""), 0o600)) + + args := language.GenerateArgs{ + Dir: dir, + File: newTestBuildFile(newPyLibraryRule("custom", []string{ + "foo.py", + "excluded_only.py", + })), + Config: &config.Config{}, + } + knownSrcs := map[string]struct{}{"foo.py": {}} + + rules := collectExistingPythonSourceRules(args, pyLibraryKind, knownSrcs) + require.Len(t, rules, 1) + assert.Equal(t, "custom", rules[0].name) + assert.Equal(t, 2, rules[0].srcs.Size()) +} + +func TestAdoptExcludedInitOnlyPackageLibraryForSplitLayout(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "__init__.py"), []byte(""), 0o600)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "foo.py"), []byte(""), 0o600)) + + packageLibraryName := "pkg" + args := language.GenerateArgs{ + Dir: dir, + File: newTestBuildFile( + newPyLibraryRule("foo", []string{"foo.py"}), + newPyLibraryRule(packageLibraryName, []string{pyLibraryEntrypointFilename}), + ), + Config: &config.Config{}, + } + knownSrcs := map[string]struct{}{"foo.py": {}} + + adopted := adoptExcludedInitOnlyPackageLibraryForSplitLayout( + args, + pyLibraryKind, + packageLibraryName, + knownSrcs, + collectExistingPythonSourceRules(args, pyLibraryKind, knownSrcs), + ) + require.Len(t, adopted, 2) + assert.True(t, hasSplitPackageLibraryLayout(packageLibraryName, adopted)) + + packageRule := adopted[1] + assert.Equal(t, packageLibraryName, packageRule.name) + assert.Equal(t, 1, packageRule.srcs.Size()) + assert.True(t, packageRule.srcs.Contains(pyLibraryEntrypointFilename)) +} + +func TestAdoptExcludedInitOnlyPackageLibraryIgnoredWithoutOtherLibraries(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "__init__.py"), []byte(""), 0o600)) + + packageLibraryName := "pkg" + args := language.GenerateArgs{ + Dir: dir, + File: newTestBuildFile(newPyLibraryRule(packageLibraryName, []string{pyLibraryEntrypointFilename})), + Config: &config.Config{}, + } + + adopted := adoptExcludedInitOnlyPackageLibraryForSplitLayout( + args, + pyLibraryKind, + packageLibraryName, + map[string]struct{}{}, + nil, + ) + assert.Nil(t, adopted) +} + +func TestGetRulesWithInvalidSrcsKeepsExcludedSourcesOnDisk(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "__init__.py"), []byte(""), 0o600)) + + buildFile := newTestBuildFile(newPyLibraryRule("pkg", []string{pyLibraryEntrypointFilename})) + args := language.GenerateArgs{ + Dir: dir, + File: buildFile, + Config: &config.Config{}, + RegularFiles: []string{"foo.py"}, + } + + py := &Python{} + invalid := py.getRulesWithInvalidSrcs(args, map[string]struct{}{}) + assert.Empty(t, invalid) +} + +func TestHasSplitPackageLibraryLayout(t *testing.T) { + t.Parallel() + + packageLibraryName := "pkg" + fooSrcs := treeset.NewWith(godsutils.StringComparator, "foo.py") + pkgSrcs := treeset.NewWith(godsutils.StringComparator, "aux.py") + + rules := []existingPythonSourceRule{ + {name: "foo", srcs: fooSrcs, declaredSrcCount: 1}, + {name: packageLibraryName, srcs: pkgSrcs, declaredSrcCount: 1}, + } + assert.True(t, hasSplitPackageLibraryLayout(packageLibraryName, rules)) + + overlapPkg := treeset.NewWith(godsutils.StringComparator, "foo.py", "aux.py") + overlapRules := []existingPythonSourceRule{ + {name: "foo", srcs: fooSrcs, declaredSrcCount: 1}, + {name: packageLibraryName, srcs: overlapPkg, declaredSrcCount: 2}, + } + assert.False(t, hasSplitPackageLibraryLayout(packageLibraryName, overlapRules)) +} diff --git a/gazelle/python/testdata/package_mode_respect_existing_excluded_only_target/BUILD.in b/gazelle/python/testdata/package_mode_respect_existing_excluded_only_target/BUILD.in new file mode 100644 index 0000000000..51aab3e64f --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_excluded_only_target/BUILD.in @@ -0,0 +1,17 @@ +# gazelle:exclude excluded_only.py +# gazelle:python_generation_mode package + +load("@rules_python//python:defs.bzl", "py_library") + +# Entirely excluded sources: Gazelle must leave this target unmanaged. +py_library( + name = "excluded_only", + srcs = ["excluded_only.py"], + tags = ["keep_excluded"], +) + +py_library( + name = "foo", + srcs = ["foo.py"], + tags = ["keep_foo"], +) diff --git a/gazelle/python/testdata/package_mode_respect_existing_excluded_only_target/BUILD.out b/gazelle/python/testdata/package_mode_respect_existing_excluded_only_target/BUILD.out new file mode 100644 index 0000000000..a330e0bf56 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_excluded_only_target/BUILD.out @@ -0,0 +1,18 @@ +# gazelle:exclude excluded_only.py +# gazelle:python_generation_mode package + +load("@rules_python//python:defs.bzl", "py_library") + +# Entirely excluded sources: Gazelle must leave this target unmanaged. +py_library( + name = "excluded_only", + srcs = ["excluded_only.py"], + tags = ["keep_excluded"], +) + +py_library( + name = "foo", + srcs = ["foo.py"], + tags = ["keep_foo"], + visibility = ["//:__subpackages__"], +) diff --git a/gazelle/python/testdata/package_mode_respect_existing_excluded_only_target/README.md b/gazelle/python/testdata/package_mode_respect_existing_excluded_only_target/README.md new file mode 100644 index 0000000000..c9a8fbb81e --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_excluded_only_target/README.md @@ -0,0 +1,5 @@ +# Package mode with an excluded-only hand-written target + +This test verifies that a `py_library` listing only `gazelle:exclude` sources +stays unmanaged while Gazelle still adopts other targets that own +Gazelle-managed modules. diff --git a/gazelle/python/testdata/package_mode_respect_existing_excluded_only_target/WORKSPACE b/gazelle/python/testdata/package_mode_respect_existing_excluded_only_target/WORKSPACE new file mode 100644 index 0000000000..faff6af87a --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_excluded_only_target/WORKSPACE @@ -0,0 +1 @@ +# This is a Bazel workspace for the Gazelle test data. diff --git a/gazelle/python/testdata/package_mode_respect_existing_excluded_only_target/excluded_only.py b/gazelle/python/testdata/package_mode_respect_existing_excluded_only_target/excluded_only.py new file mode 100644 index 0000000000..6b58ff30a8 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_excluded_only_target/excluded_only.py @@ -0,0 +1 @@ +# For test purposes only. diff --git a/gazelle/python/testdata/package_mode_respect_existing_excluded_only_target/foo.py b/gazelle/python/testdata/package_mode_respect_existing_excluded_only_target/foo.py new file mode 100644 index 0000000000..6b58ff30a8 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_excluded_only_target/foo.py @@ -0,0 +1 @@ +# For test purposes only. diff --git a/gazelle/python/testdata/package_mode_respect_existing_excluded_only_target/test.yaml b/gazelle/python/testdata/package_mode_respect_existing_excluded_only_target/test.yaml new file mode 100644 index 0000000000..ed97d539c0 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_excluded_only_target/test.yaml @@ -0,0 +1 @@ +--- diff --git a/gazelle/python/testdata/package_mode_respect_existing_split_package_library_excluded_init/BUILD.in b/gazelle/python/testdata/package_mode_respect_existing_split_package_library_excluded_init/BUILD.in new file mode 100644 index 0000000000..226d6f1b38 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_split_package_library_excluded_init/BUILD.in @@ -0,0 +1,17 @@ +# gazelle:exclude **/__init__.py +# gazelle:python_generation_mode package + +load("@rules_python//python:defs.bzl", "py_library") + +py_library( + name = "foo", + srcs = ["foo.py"], + tags = ["keep_foo"], +) + +# Package library name Gazelle generates in package mode; only excluded __init__.py. +py_library( + name = "package_mode_respect_existing_split_package_library_excluded_init", + srcs = ["__init__.py"], + tags = ["keep_pkg"], +) diff --git a/gazelle/python/testdata/package_mode_respect_existing_split_package_library_excluded_init/BUILD.out b/gazelle/python/testdata/package_mode_respect_existing_split_package_library_excluded_init/BUILD.out new file mode 100644 index 0000000000..226d6f1b38 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_split_package_library_excluded_init/BUILD.out @@ -0,0 +1,17 @@ +# gazelle:exclude **/__init__.py +# gazelle:python_generation_mode package + +load("@rules_python//python:defs.bzl", "py_library") + +py_library( + name = "foo", + srcs = ["foo.py"], + tags = ["keep_foo"], +) + +# Package library name Gazelle generates in package mode; only excluded __init__.py. +py_library( + name = "package_mode_respect_existing_split_package_library_excluded_init", + srcs = ["__init__.py"], + tags = ["keep_pkg"], +) diff --git a/gazelle/python/testdata/package_mode_respect_existing_split_package_library_excluded_init/README.md b/gazelle/python/testdata/package_mode_respect_existing_split_package_library_excluded_init/README.md new file mode 100644 index 0000000000..962dea1174 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_split_package_library_excluded_init/README.md @@ -0,0 +1,7 @@ +# Package mode with excluded `__init__.py` and a split package library + +This test verifies that a hand-written package library owning only an excluded +`__init__.py` is still preserved alongside per-file `py_library` targets. The +parent `gazelle:exclude **/__init__.py` pattern matches Benchling monolith +packages where init-only package libraries must not cause Gazelle to regenerate +a competing package-level target over the remaining modules. diff --git a/gazelle/python/testdata/package_mode_respect_existing_split_package_library_excluded_init/WORKSPACE b/gazelle/python/testdata/package_mode_respect_existing_split_package_library_excluded_init/WORKSPACE new file mode 100644 index 0000000000..faff6af87a --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_split_package_library_excluded_init/WORKSPACE @@ -0,0 +1 @@ +# This is a Bazel workspace for the Gazelle test data. diff --git a/gazelle/python/testdata/package_mode_respect_existing_split_package_library_excluded_init/__init__.py b/gazelle/python/testdata/package_mode_respect_existing_split_package_library_excluded_init/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/gazelle/python/testdata/package_mode_respect_existing_split_package_library_excluded_init/foo.py b/gazelle/python/testdata/package_mode_respect_existing_split_package_library_excluded_init/foo.py new file mode 100644 index 0000000000..cf68624419 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_split_package_library_excluded_init/foo.py @@ -0,0 +1,2 @@ +def foo(): + return "foo" diff --git a/gazelle/python/testdata/package_mode_respect_existing_split_package_library_excluded_init/test.yaml b/gazelle/python/testdata/package_mode_respect_existing_split_package_library_excluded_init/test.yaml new file mode 100644 index 0000000000..ed97d539c0 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_split_package_library_excluded_init/test.yaml @@ -0,0 +1 @@ +--- From 1c4de76c6a1dd6b40baf40954cbccec384fa44ec Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 11 Sep 2026 00:49:48 +0000 Subject: [PATCH 11/18] fix(gazelle): preserve empty package aggregates Co-authored-by: Alex Martani --- gazelle/python/generate.go | 57 +++++++++++- gazelle/python/generate_test.go | 88 +++++++++++++++++++ .../BUILD.in | 21 +++++ .../BUILD.out | 21 +++++ .../WORKSPACE | 1 + .../bar.py | 1 + .../foo.py | 1 + .../test.yaml | 1 + 8 files changed, 190 insertions(+), 1 deletion(-) create mode 100644 gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate/BUILD.in create mode 100644 gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate/BUILD.out create mode 100644 gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate/WORKSPACE create mode 100644 gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate/bar.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate/foo.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate/test.yaml diff --git a/gazelle/python/generate.go b/gazelle/python/generate.go index 02acc8c2b8..c10f896ec4 100644 --- a/gazelle/python/generate.go +++ b/gazelle/python/generate.go @@ -310,6 +310,55 @@ func adoptExcludedInitOnlyPackageLibraryForSplitLayout( return rules } +// adoptEmptyAggregatePackageLibraryForSplitLayout appends the hand-written +// package library when it omits srcs entirely. That target is otherwise not +// adopted because collectExistingPythonSourceRules ignores rules without srcs, +// but it is still the deps-only package aggregate in a split layout and must +// be preserved so Gazelle does not emit a competing package-level library. +// Only applies when every other adopted library is a single-module target. +func adoptEmptyAggregatePackageLibraryForSplitLayout( + args language.GenerateArgs, + kind string, + packageLibraryName string, + rules []existingPythonSourceRule, +) []existingPythonSourceRule { + if args.File == nil || len(rules) == 0 { + return rules + } + for _, sourceRule := range rules { + if sourceRule.name == packageLibraryName { + return rules + } + } + for _, other := range rules { + if other.declaredSrcCount != 1 { + return rules + } + } + + for _, existingRule := range args.File.Rules { + if existingRule.Name() != packageLibraryName || !kindMatches(args.Config, existingRule, kind) { + continue + } + if len(existingRule.AttrStrings("srcs")) != 0 { + return rules + } + + candidate := existingPythonSourceRule{ + name: packageLibraryName, + srcs: treeset.NewWith(godsutils.StringComparator), + declaredSrcCount: 0, + } + for _, other := range rules { + if existingRulesShareSrcs(candidate, other) { + return rules + } + } + return append(rules, candidate) + } + return rules +} + // addTargetNamesForSrcs records the per-file target name Gazelle derives from // each of srcs. func addTargetNamesForSrcs(srcs *treeset.Set, dst map[string]struct{}) { @@ -556,6 +605,12 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes knownPySrcs, existingPyLibraries, ) + existingPyLibraries = adoptEmptyAggregatePackageLibraryForSplitLayout( + args, + pyLibraryKind, + packageLibraryName, + existingPyLibraries, + ) } existingPyTests := collectExistingPythonSourceRules(args, pyTestKind, knownPySrcs) splitPackageLibraryLayout := false @@ -783,7 +838,7 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes for _, existingPyLibrary := range existingPyLibraries { srcs := existingPyLibrary.srcs - if existingPyLibrary.name == packageLibraryName { + if existingPyLibrary.name == packageLibraryName && existingPyLibrary.declaredSrcCount > 0 { mergedSrcs := treeset.NewWith(godsutils.StringComparator) srcs.Each(func(index int, filename interface{}) { mergedSrcs.Add(filename) diff --git a/gazelle/python/generate_test.go b/gazelle/python/generate_test.go index 60940c7e64..2ee7797fca 100644 --- a/gazelle/python/generate_test.go +++ b/gazelle/python/generate_test.go @@ -163,3 +163,91 @@ func TestHasSplitPackageLibraryLayout(t *testing.T) { } assert.False(t, hasSplitPackageLibraryLayout(packageLibraryName, overlapRules)) } + +func TestAdoptEmptyAggregatePackageLibraryForSplitLayout(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "foo.py"), []byte(""), 0o600)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "bar.py"), []byte(""), 0o600)) + + packageLibraryName := "pkg" + args := language.GenerateArgs{ + Dir: dir, + File: newTestBuildFile( + rule.NewRule("py_library", packageLibraryName), + newPyLibraryRule("foo", []string{"foo.py"}), + newPyLibraryRule("bar", []string{"bar.py"}), + ), + Config: &config.Config{}, + } + knownSrcs := map[string]struct{}{"foo.py": {}, "bar.py": {}} + + adopted := adoptEmptyAggregatePackageLibraryForSplitLayout( + args, + pyLibraryKind, + packageLibraryName, + collectExistingPythonSourceRules(args, pyLibraryKind, knownSrcs), + ) + require.Len(t, adopted, 3) + assert.True(t, hasSplitPackageLibraryLayout(packageLibraryName, adopted)) + + var packageRule *existingPythonSourceRule + for i := range adopted { + if adopted[i].name == packageLibraryName { + packageRule = &adopted[i] + break + } + } + require.NotNil(t, packageRule) + assert.Equal(t, 0, packageRule.srcs.Size()) + assert.Equal(t, 0, packageRule.declaredSrcCount) +} + +func TestAdoptEmptyAggregatePackageLibraryIgnoredWithoutPerFileLibraries(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + packageLibraryName := "pkg" + args := language.GenerateArgs{ + Dir: dir, + File: newTestBuildFile(rule.NewRule("py_library", packageLibraryName)), + Config: &config.Config{}, + } + + adopted := adoptEmptyAggregatePackageLibraryForSplitLayout( + args, + pyLibraryKind, + packageLibraryName, + nil, + ) + assert.Nil(t, adopted) +} + +func TestAdoptEmptyAggregatePackageLibraryIgnoredWithMultiSrcLibrary(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "foo.py"), []byte(""), 0o600)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "bar.py"), []byte(""), 0o600)) + + packageLibraryName := "pkg" + args := language.GenerateArgs{ + Dir: dir, + File: newTestBuildFile( + rule.NewRule("py_library", packageLibraryName), + newPyLibraryRule("custom", []string{"foo.py", "bar.py"}), + ), + Config: &config.Config{}, + } + knownSrcs := map[string]struct{}{"foo.py": {}, "bar.py": {}} + + adopted := adoptEmptyAggregatePackageLibraryForSplitLayout( + args, + pyLibraryKind, + packageLibraryName, + collectExistingPythonSourceRules(args, pyLibraryKind, knownSrcs), + ) + require.Len(t, adopted, 1) + assert.Equal(t, "custom", adopted[0].name) +} diff --git a/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate/BUILD.in b/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate/BUILD.in new file mode 100644 index 0000000000..2491f1169a --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate/BUILD.in @@ -0,0 +1,21 @@ +load("@rules_python//python:defs.bzl", "py_library") + +py_library( + name = "foo", + srcs = ["foo.py"], + tags = ["keep_foo"], +) + +py_library( + name = "bar", + srcs = ["bar.py"], + tags = ["keep_bar"], +) + +# Package library name Gazelle generates in package mode; deps-only aggregate. +py_library( + name = "package_mode_respect_existing_split_package_library_empty_aggregate", + deps = [":foo"], + tags = ["keep_pkg"], + visibility = ["//visibility:public"], +) diff --git a/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate/BUILD.out b/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate/BUILD.out new file mode 100644 index 0000000000..2491f1169a --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate/BUILD.out @@ -0,0 +1,21 @@ +load("@rules_python//python:defs.bzl", "py_library") + +py_library( + name = "foo", + srcs = ["foo.py"], + tags = ["keep_foo"], +) + +py_library( + name = "bar", + srcs = ["bar.py"], + tags = ["keep_bar"], +) + +# Package library name Gazelle generates in package mode; deps-only aggregate. +py_library( + name = "package_mode_respect_existing_split_package_library_empty_aggregate", + deps = [":foo"], + tags = ["keep_pkg"], + visibility = ["//visibility:public"], +) diff --git a/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate/WORKSPACE b/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate/WORKSPACE new file mode 100644 index 0000000000..faff6af87a --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate/WORKSPACE @@ -0,0 +1 @@ +# This is a Bazel workspace for the Gazelle test data. diff --git a/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate/bar.py b/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate/bar.py new file mode 100644 index 0000000000..d45ce085dc --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate/bar.py @@ -0,0 +1 @@ +"""bar module.""" diff --git a/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate/foo.py b/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate/foo.py new file mode 100644 index 0000000000..5a47c3032b --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate/foo.py @@ -0,0 +1 @@ +"""foo module.""" diff --git a/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate/test.yaml b/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate/test.yaml new file mode 100644 index 0000000000..ed97d539c0 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate/test.yaml @@ -0,0 +1 @@ +--- From f1f9f2c3c072693b5dc00408a61c941134a67069 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 11 Sep 2026 00:58:50 +0000 Subject: [PATCH 12/18] fix(gazelle): preserve all-per-file package layouts Co-authored-by: Alex Martani --- gazelle/python/generate.go | 124 +++++++++++++++++- gazelle/python/generate_test.go | 74 ++++++++++- .../BUILD.in | 15 +++ .../README.md | 5 + .../WORKSPACE | 1 + .../config.py | 1 + .../email_handlers.py | 1 + .../test.yaml | 1 + .../BUILD.out | 2 +- 9 files changed, 214 insertions(+), 10 deletions(-) create mode 100644 gazelle/python/testdata/package_mode_respect_existing_all_per_file_no_aggregate/BUILD.in create mode 100644 gazelle/python/testdata/package_mode_respect_existing_all_per_file_no_aggregate/README.md create mode 100644 gazelle/python/testdata/package_mode_respect_existing_all_per_file_no_aggregate/WORKSPACE create mode 100644 gazelle/python/testdata/package_mode_respect_existing_all_per_file_no_aggregate/config.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_all_per_file_no_aggregate/email_handlers.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_all_per_file_no_aggregate/test.yaml diff --git a/gazelle/python/generate.go b/gazelle/python/generate.go index c10f896ec4..fa0b000d3a 100644 --- a/gazelle/python/generate.go +++ b/gazelle/python/generate.go @@ -246,6 +246,48 @@ func hasSplitPackageLibraryLayout(packageLibraryName string, rules []existingPyt return true } +// hasAllPerFileLibrariesLayout reports whether every Gazelle-managed library +// source in the package is owned by a distinct single-module preserved target +// and no target uses the generated package library name. This is the layout +// where Gazelle must not emit a package-level library and every per-file +// library must claim its source. +func hasAllPerFileLibrariesLayout( + packageLibraryName string, + rules []existingPythonSourceRule, + libraryFilenames *treeset.Set, +) bool { + if libraryFilenames == nil || libraryFilenames.Empty() { + return false + } + for _, sourceRule := range rules { + if sourceRule.name == packageLibraryName { + return false + } + } + if len(rules) < 2 { + return false + } + for _, sourceRule := range rules { + if sourceRule.declaredSrcCount != 1 { + return false + } + } + covered := make(map[string]struct{}) + for _, sourceRule := range rules { + it := sourceRule.srcs.Iterator() + for it.Next() { + covered[it.Value().(string)] = struct{}{} + } + } + it := libraryFilenames.Iterator() + for it.Next() { + if _, ok := covered[it.Value().(string)]; !ok { + return false + } + } + return true +} + // adoptExcludedInitOnlyPackageLibraryForSplitLayout appends the hand-written // package library when it lists only an excluded __init__.py. That target is // otherwise not adopted because none of its srcs are Gazelle-managed, but it @@ -616,6 +658,13 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes splitPackageLibraryLayout := false if !cfg.PerFileGeneration() { splitPackageLibraryLayout = hasSplitPackageLibraryLayout(packageLibraryName, existingPyLibraries) + if !splitPackageLibraryLayout { + splitPackageLibraryLayout = hasAllPerFileLibrariesLayout( + packageLibraryName, + existingPyLibraries, + pyLibraryFilenames, + ) + } } generatedTargetNames := make(map[string]struct{}) @@ -837,6 +886,11 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes } for _, existingPyLibrary := range existingPyLibraries { + if existingPyLibrary.name == packageLibraryName && + existingPyLibrary.declaredSrcCount == 0 && + existingPyLibrary.srcs.Empty() { + continue + } srcs := existingPyLibrary.srcs if existingPyLibrary.name == packageLibraryName && existingPyLibrary.declaredSrcCount > 0 { mergedSrcs := treeset.NewWith(godsutils.StringComparator) @@ -1059,12 +1113,59 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes return result } +// ruleListsGazelleManagedSrc reports whether any src is one Gazelle would place +// in a generated target for this package. +func ruleListsGazelleManagedSrc(srcs []string, managed map[string]struct{}) bool { + for _, src := range srcs { + if isTargetSrc(src) || filepath.Ext(src) != ".py" { + continue + } + if _, ok := managed[src]; ok { + return true + } + } + return false +} + +// isExcludedInitOnlyPackageLibrarySrcOnDisk reports whether src is the only +// source of the hand-written package library and exists on disk but is hidden +// from Gazelle generation (for example via gazelle:exclude). +func isExcludedInitOnlyPackageLibrarySrcOnDisk( + args language.GenerateArgs, + packageLibraryName string, + existingRule *rule.Rule, + src string, +) bool { + if !kindMatches(args.Config, existingRule, pyLibraryKind) { + return false + } + if existingRule.Name() != packageLibraryName || src != pyLibraryEntrypointFilename { + return false + } + srcs := existingRule.AttrStrings("srcs") + if len(srcs) != 1 { + return false + } + _, err := os.Stat(filepath.Join(args.Dir, src)) + return err == nil +} + // getRulesWithInvalidSrcs checks existing Python rules in the BUILD file and return the rules with invalid source files. // Invalid source files are files that do not exist or not a target. func (py *Python) getRulesWithInvalidSrcs(args language.GenerateArgs, validFilesMap map[string]struct{}) (invalidRules []*rule.Rule) { if args.File == nil { return } + packageLibraryName := filepath.Base(args.Dir) + if args.Config != nil { + if raw, ok := args.Config.Exts[languageName]; ok && raw != nil { + cfg := raw.(pythonconfig.Configs)[args.Rel] + if cfg != nil { + packageLibraryName = cfg.RenderLibraryName(packageLibraryName) + } + } + } + for _, file := range args.GenFiles { validFilesMap[file] = struct{}{} } @@ -1109,14 +1210,29 @@ func (py *Python) getRulesWithInvalidSrcs(args language.GenerateArgs, validFiles hasValidSrcs = true break } - // Sources hidden from generation via gazelle:exclude or - // python_ignore_files are absent from filesMap but may still be - // listed in a hand-written target that Gazelle leaves unmanaged. - if _, err := os.Stat(filepath.Join(args.Dir, src)); err == nil { + if isExcludedInitOnlyPackageLibrarySrcOnDisk( + args, + packageLibraryName, + existingRule, + src, + ) { hasValidSrcs = true break } } + if !hasValidSrcs && matchedKind != pyBinaryKind && + !ruleListsGazelleManagedSrc(srcs, validFilesMap) { + for _, src := range srcs { + if isTargetSrc(src) { + hasValidSrcs = true + break + } + if _, err := os.Stat(filepath.Join(args.Dir, src)); err == nil { + hasValidSrcs = true + break + } + } + } if !hasValidSrcs { invalidRules = append(invalidRules, newTargetBuilder(matchedKind, existingRule.Name(), "", "", nil, false).build()) } diff --git a/gazelle/python/generate_test.go b/gazelle/python/generate_test.go index 2ee7797fca..422098fb98 100644 --- a/gazelle/python/generate_test.go +++ b/gazelle/python/generate_test.go @@ -12,6 +12,8 @@ import ( godsutils "github.com/emirpasic/gods/utils" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/bazel-contrib/rules_python/gazelle/pythonconfig" ) func newTestBuildFile(rules ...*rule.Rule) *rule.File { @@ -127,15 +129,22 @@ func TestAdoptExcludedInitOnlyPackageLibraryIgnoredWithoutOtherLibraries(t *test func TestGetRulesWithInvalidSrcsKeepsExcludedSourcesOnDisk(t *testing.T) { t.Parallel() - dir := t.TempDir() + dir := filepath.Join(t.TempDir(), "pkg") + require.NoError(t, os.MkdirAll(dir, 0o755)) require.NoError(t, os.WriteFile(filepath.Join(dir, "__init__.py"), []byte(""), 0o600)) buildFile := newTestBuildFile(newPyLibraryRule("pkg", []string{pyLibraryEntrypointFilename})) + pkgConfig := pythonconfig.New(dir, "") args := language.GenerateArgs{ - Dir: dir, - File: buildFile, - Config: &config.Config{}, - RegularFiles: []string{"foo.py"}, + Dir: dir, + Rel: "pkg", + File: buildFile, + Config: &config.Config{ + Exts: map[string]interface{}{ + "py": pythonconfig.Configs{"pkg": pkgConfig}, + }, + }, + RegularFiles: []string{"foo.py"}, } py := &Python{} @@ -164,6 +173,34 @@ func TestHasSplitPackageLibraryLayout(t *testing.T) { assert.False(t, hasSplitPackageLibraryLayout(packageLibraryName, overlapRules)) } +func TestEmptyAggregateFixtureSplitLayout(t *testing.T) { + t.Parallel() + + packageLibraryName := "package_mode_respect_existing_split_package_library_empty_aggregate" + dir := filepath.Join(t.TempDir(), packageLibraryName) + require.NoError(t, os.MkdirAll(dir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "foo.py"), []byte(""), 0o600)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "bar.py"), []byte(""), 0o600)) + + pkgRule := rule.NewRule("py_library", packageLibraryName) + pkgRule.SetAttr("deps", []string{":foo"}) + args := language.GenerateArgs{ + Dir: dir, + File: newTestBuildFile( + newPyLibraryRule("foo", []string{"foo.py"}), + newPyLibraryRule("bar", []string{"bar.py"}), + pkgRule, + ), + Config: &config.Config{}, + } + knownSrcs := map[string]struct{}{"foo.py": {}, "bar.py": {}} + + rules := collectExistingPythonSourceRules(args, pyLibraryKind, knownSrcs) + rules = adoptEmptyAggregatePackageLibraryForSplitLayout(args, pyLibraryKind, packageLibraryName, rules) + require.Len(t, rules, 3) + assert.True(t, hasSplitPackageLibraryLayout(packageLibraryName, rules)) +} + func TestAdoptEmptyAggregatePackageLibraryForSplitLayout(t *testing.T) { t.Parallel() @@ -224,6 +261,33 @@ func TestAdoptEmptyAggregatePackageLibraryIgnoredWithoutPerFileLibraries(t *test assert.Nil(t, adopted) } +func TestHasAllPerFileLibrariesLayout(t *testing.T) { + t.Parallel() + + packageLibraryName := "pkg" + fooSrcs := treeset.NewWith(godsutils.StringComparator, "foo.py") + barSrcs := treeset.NewWith(godsutils.StringComparator, "bar.py") + libraryFilenames := treeset.NewWith(godsutils.StringComparator, "foo.py", "bar.py") + + rules := []existingPythonSourceRule{ + {name: "foo", srcs: fooSrcs, declaredSrcCount: 1}, + {name: "bar", srcs: barSrcs, declaredSrcCount: 1}, + } + assert.True(t, hasAllPerFileLibrariesLayout(packageLibraryName, rules, libraryFilenames)) + + withPackageLib := append(rules, existingPythonSourceRule{ + name: packageLibraryName, + srcs: treeset.NewWith(godsutils.StringComparator), + declaredSrcCount: 0, + }) + assert.False(t, hasAllPerFileLibrariesLayout(packageLibraryName, withPackageLib, libraryFilenames)) + + multiSrc := []existingPythonSourceRule{ + {name: "custom", srcs: libraryFilenames, declaredSrcCount: 2}, + } + assert.False(t, hasAllPerFileLibrariesLayout(packageLibraryName, multiSrc, libraryFilenames)) +} + func TestAdoptEmptyAggregatePackageLibraryIgnoredWithMultiSrcLibrary(t *testing.T) { t.Parallel() diff --git a/gazelle/python/testdata/package_mode_respect_existing_all_per_file_no_aggregate/BUILD.in b/gazelle/python/testdata/package_mode_respect_existing_all_per_file_no_aggregate/BUILD.in new file mode 100644 index 0000000000..02e4320ee3 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_all_per_file_no_aggregate/BUILD.in @@ -0,0 +1,15 @@ +# gazelle:python_generation_mode package + +load("@rules_python//python:defs.bzl", "py_library") + +py_library( + name = "config", + srcs = ["config.py"], + tags = ["keep_config"], +) + +py_library( + name = "email_handlers", + srcs = ["email_handlers.py"], + tags = ["keep_email_handlers"], +) diff --git a/gazelle/python/testdata/package_mode_respect_existing_all_per_file_no_aggregate/README.md b/gazelle/python/testdata/package_mode_respect_existing_all_per_file_no_aggregate/README.md new file mode 100644 index 0000000000..f6fda140c4 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_all_per_file_no_aggregate/README.md @@ -0,0 +1,5 @@ +# Package mode with only per-file libraries and no package aggregate + +This test verifies that Gazelle does not emit a package-level `py_library` +when every module is already owned by a single-source preserved target, and +that those targets claim their sources so nothing is duplicated. diff --git a/gazelle/python/testdata/package_mode_respect_existing_all_per_file_no_aggregate/WORKSPACE b/gazelle/python/testdata/package_mode_respect_existing_all_per_file_no_aggregate/WORKSPACE new file mode 100644 index 0000000000..faff6af87a --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_all_per_file_no_aggregate/WORKSPACE @@ -0,0 +1 @@ +# This is a Bazel workspace for the Gazelle test data. diff --git a/gazelle/python/testdata/package_mode_respect_existing_all_per_file_no_aggregate/config.py b/gazelle/python/testdata/package_mode_respect_existing_all_per_file_no_aggregate/config.py new file mode 100644 index 0000000000..da7ff7c7be --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_all_per_file_no_aggregate/config.py @@ -0,0 +1 @@ +"""Config module.""" diff --git a/gazelle/python/testdata/package_mode_respect_existing_all_per_file_no_aggregate/email_handlers.py b/gazelle/python/testdata/package_mode_respect_existing_all_per_file_no_aggregate/email_handlers.py new file mode 100644 index 0000000000..36e1bddd71 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_all_per_file_no_aggregate/email_handlers.py @@ -0,0 +1 @@ +"""Email handlers module.""" diff --git a/gazelle/python/testdata/package_mode_respect_existing_all_per_file_no_aggregate/test.yaml b/gazelle/python/testdata/package_mode_respect_existing_all_per_file_no_aggregate/test.yaml new file mode 100644 index 0000000000..ed97d539c0 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_all_per_file_no_aggregate/test.yaml @@ -0,0 +1 @@ +--- diff --git a/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate/BUILD.out b/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate/BUILD.out index 2491f1169a..b5b2622cd0 100644 --- a/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate/BUILD.out +++ b/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate/BUILD.out @@ -15,7 +15,7 @@ py_library( # Package library name Gazelle generates in package mode; deps-only aggregate. py_library( name = "package_mode_respect_existing_split_package_library_empty_aggregate", - deps = [":foo"], tags = ["keep_pkg"], visibility = ["//visibility:public"], + deps = [":foo"], ) From c78a02b013afe0af1cd903ca860dfd91d34bae03 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 11 Sep 2026 01:03:42 +0000 Subject: [PATCH 13/18] fix(gazelle): support explicit source ownership Co-authored-by: Alex Martani --- gazelle/python/generate.go | 27 +++++++-------- gazelle/python/generate_test.go | 34 ++++++++++++++++--- .../testdata/dont_rename_target/BUILD.out | 1 - .../BUILD.out | 15 ++++++++ .../README.md | 4 +-- .../BUILD.out | 1 - .../BUILD.in | 18 ++++++++++ .../BUILD.out | 18 ++++++++++ .../README.md | 5 +++ .../WORKSPACE | 1 + .../auth.py | 0 .../foo.py | 0 .../oauth2.py | 0 .../test.yaml | 1 + .../BUILD.in | 9 +++++ .../BUILD.out | 9 +++++ .../README.md | 5 +++ .../WORKSPACE | 1 + .../only.py | 0 .../test.yaml | 1 + 20 files changed, 128 insertions(+), 22 deletions(-) create mode 100644 gazelle/python/testdata/package_mode_respect_existing_all_per_file_no_aggregate/BUILD.out create mode 100644 gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_mixed_srcs/BUILD.in create mode 100644 gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_mixed_srcs/BUILD.out create mode 100644 gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_mixed_srcs/README.md create mode 100644 gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_mixed_srcs/WORKSPACE create mode 100644 gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_mixed_srcs/auth.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_mixed_srcs/foo.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_mixed_srcs/oauth2.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_mixed_srcs/test.yaml create mode 100644 gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_single_target/BUILD.in create mode 100644 gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_single_target/BUILD.out create mode 100644 gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_single_target/README.md create mode 100644 gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_single_target/WORKSPACE create mode 100644 gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_single_target/only.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_single_target/test.yaml diff --git a/gazelle/python/generate.go b/gazelle/python/generate.go index fa0b000d3a..7af70f942d 100644 --- a/gazelle/python/generate.go +++ b/gazelle/python/generate.go @@ -246,17 +246,17 @@ func hasSplitPackageLibraryLayout(packageLibraryName string, rules []existingPyt return true } -// hasAllPerFileLibrariesLayout reports whether every Gazelle-managed library -// source in the package is owned by a distinct single-module preserved target -// and no target uses the generated package library name. This is the layout -// where Gazelle must not emit a package-level library and every per-file -// library must claim its source. -func hasAllPerFileLibrariesLayout( +// hasExplicitSourceOwnershipLayout reports whether preserved non-package +// libraries collectively own every Gazelle-managed library source without +// overlapping each other and without using the generated package library name. +// When true, Gazelle must not emit a package-level library and every preserved +// library must claim its sources, regardless of per-target source counts. +func hasExplicitSourceOwnershipLayout( packageLibraryName string, rules []existingPythonSourceRule, libraryFilenames *treeset.Set, ) bool { - if libraryFilenames == nil || libraryFilenames.Empty() { + if libraryFilenames == nil || libraryFilenames.Empty() || len(rules) == 0 { return false } for _, sourceRule := range rules { @@ -264,12 +264,11 @@ func hasAllPerFileLibrariesLayout( return false } } - if len(rules) < 2 { - return false - } - for _, sourceRule := range rules { - if sourceRule.declaredSrcCount != 1 { - return false + for i := range rules { + for j := i + 1; j < len(rules); j++ { + if existingRulesShareSrcs(rules[i], rules[j]) { + return false + } } } covered := make(map[string]struct{}) @@ -659,7 +658,7 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes if !cfg.PerFileGeneration() { splitPackageLibraryLayout = hasSplitPackageLibraryLayout(packageLibraryName, existingPyLibraries) if !splitPackageLibraryLayout { - splitPackageLibraryLayout = hasAllPerFileLibrariesLayout( + splitPackageLibraryLayout = hasExplicitSourceOwnershipLayout( packageLibraryName, existingPyLibraries, pyLibraryFilenames, diff --git a/gazelle/python/generate_test.go b/gazelle/python/generate_test.go index 422098fb98..8fa843ca34 100644 --- a/gazelle/python/generate_test.go +++ b/gazelle/python/generate_test.go @@ -261,7 +261,7 @@ func TestAdoptEmptyAggregatePackageLibraryIgnoredWithoutPerFileLibraries(t *test assert.Nil(t, adopted) } -func TestHasAllPerFileLibrariesLayout(t *testing.T) { +func TestHasExplicitSourceOwnershipLayout(t *testing.T) { t.Parallel() packageLibraryName := "pkg" @@ -273,19 +273,45 @@ func TestHasAllPerFileLibrariesLayout(t *testing.T) { {name: "foo", srcs: fooSrcs, declaredSrcCount: 1}, {name: "bar", srcs: barSrcs, declaredSrcCount: 1}, } - assert.True(t, hasAllPerFileLibrariesLayout(packageLibraryName, rules, libraryFilenames)) + assert.True(t, hasExplicitSourceOwnershipLayout(packageLibraryName, rules, libraryFilenames)) withPackageLib := append(rules, existingPythonSourceRule{ name: packageLibraryName, srcs: treeset.NewWith(godsutils.StringComparator), declaredSrcCount: 0, }) - assert.False(t, hasAllPerFileLibrariesLayout(packageLibraryName, withPackageLib, libraryFilenames)) + assert.False(t, hasExplicitSourceOwnershipLayout(packageLibraryName, withPackageLib, libraryFilenames)) multiSrc := []existingPythonSourceRule{ {name: "custom", srcs: libraryFilenames, declaredSrcCount: 2}, } - assert.False(t, hasAllPerFileLibrariesLayout(packageLibraryName, multiSrc, libraryFilenames)) + assert.True(t, hasExplicitSourceOwnershipLayout(packageLibraryName, multiSrc, libraryFilenames)) + + authSrcs := treeset.NewWith(godsutils.StringComparator, "auth.py", "oauth2.py") + mixedFilenames := treeset.NewWith(godsutils.StringComparator, "foo.py", "auth.py", "oauth2.py") + mixed := []existingPythonSourceRule{ + {name: "foo", srcs: fooSrcs, declaredSrcCount: 1}, + {name: "auth", srcs: authSrcs, declaredSrcCount: 2}, + } + assert.True(t, hasExplicitSourceOwnershipLayout(packageLibraryName, mixed, mixedFilenames)) + + onlySrcs := treeset.NewWith(godsutils.StringComparator, "only.py") + singleTarget := []existingPythonSourceRule{ + {name: "only", srcs: onlySrcs, declaredSrcCount: 1}, + } + singleFilenames := treeset.NewWith(godsutils.StringComparator, "only.py") + assert.True(t, hasExplicitSourceOwnershipLayout(packageLibraryName, singleTarget, singleFilenames)) + + overlap := []existingPythonSourceRule{ + {name: "a", srcs: fooSrcs, declaredSrcCount: 1}, + {name: "b", srcs: libraryFilenames, declaredSrcCount: 2}, + } + assert.False(t, hasExplicitSourceOwnershipLayout(packageLibraryName, overlap, libraryFilenames)) + + partial := []existingPythonSourceRule{ + {name: "foo", srcs: fooSrcs, declaredSrcCount: 1}, + } + assert.False(t, hasExplicitSourceOwnershipLayout(packageLibraryName, partial, libraryFilenames)) } func TestAdoptEmptyAggregatePackageLibraryIgnoredWithMultiSrcLibrary(t *testing.T) { diff --git a/gazelle/python/testdata/dont_rename_target/BUILD.out b/gazelle/python/testdata/dont_rename_target/BUILD.out index 62772e30b5..e9bc0e6e29 100644 --- a/gazelle/python/testdata/dont_rename_target/BUILD.out +++ b/gazelle/python/testdata/dont_rename_target/BUILD.out @@ -3,5 +3,4 @@ load("@rules_python//python:defs.bzl", "py_library") py_library( name = "my_custom_target", srcs = ["__init__.py"], - visibility = ["//:__subpackages__"], ) diff --git a/gazelle/python/testdata/package_mode_respect_existing_all_per_file_no_aggregate/BUILD.out b/gazelle/python/testdata/package_mode_respect_existing_all_per_file_no_aggregate/BUILD.out new file mode 100644 index 0000000000..02e4320ee3 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_all_per_file_no_aggregate/BUILD.out @@ -0,0 +1,15 @@ +# gazelle:python_generation_mode package + +load("@rules_python//python:defs.bzl", "py_library") + +py_library( + name = "config", + srcs = ["config.py"], + tags = ["keep_config"], +) + +py_library( + name = "email_handlers", + srcs = ["email_handlers.py"], + tags = ["keep_email_handlers"], +) diff --git a/gazelle/python/testdata/package_mode_respect_existing_all_per_file_no_aggregate/README.md b/gazelle/python/testdata/package_mode_respect_existing_all_per_file_no_aggregate/README.md index f6fda140c4..d780e180b9 100644 --- a/gazelle/python/testdata/package_mode_respect_existing_all_per_file_no_aggregate/README.md +++ b/gazelle/python/testdata/package_mode_respect_existing_all_per_file_no_aggregate/README.md @@ -1,5 +1,5 @@ # Package mode with only per-file libraries and no package aggregate This test verifies that Gazelle does not emit a package-level `py_library` -when every module is already owned by a single-source preserved target, and -that those targets claim their sources so nothing is duplicated. +when every module is already owned by preserved targets with disjoint sources, +and that those targets claim their sources so nothing is duplicated. diff --git a/gazelle/python/testdata/package_mode_respect_existing_excluded_only_target/BUILD.out b/gazelle/python/testdata/package_mode_respect_existing_excluded_only_target/BUILD.out index a330e0bf56..51aab3e64f 100644 --- a/gazelle/python/testdata/package_mode_respect_existing_excluded_only_target/BUILD.out +++ b/gazelle/python/testdata/package_mode_respect_existing_excluded_only_target/BUILD.out @@ -14,5 +14,4 @@ py_library( name = "foo", srcs = ["foo.py"], tags = ["keep_foo"], - visibility = ["//:__subpackages__"], ) diff --git a/gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_mixed_srcs/BUILD.in b/gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_mixed_srcs/BUILD.in new file mode 100644 index 0000000000..1b4fcb3aeb --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_mixed_srcs/BUILD.in @@ -0,0 +1,18 @@ +# gazelle:python_generation_mode package + +load("@rules_python//python:defs.bzl", "py_library") + +py_library( + name = "foo", + srcs = ["foo.py"], + tags = ["keep_foo"], +) + +py_library( + name = "auth", + srcs = [ + "auth.py", + "oauth2.py", + ], + tags = ["keep_auth"], +) diff --git a/gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_mixed_srcs/BUILD.out b/gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_mixed_srcs/BUILD.out new file mode 100644 index 0000000000..1b4fcb3aeb --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_mixed_srcs/BUILD.out @@ -0,0 +1,18 @@ +# gazelle:python_generation_mode package + +load("@rules_python//python:defs.bzl", "py_library") + +py_library( + name = "foo", + srcs = ["foo.py"], + tags = ["keep_foo"], +) + +py_library( + name = "auth", + srcs = [ + "auth.py", + "oauth2.py", + ], + tags = ["keep_auth"], +) diff --git a/gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_mixed_srcs/README.md b/gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_mixed_srcs/README.md new file mode 100644 index 0000000000..7890d121a0 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_mixed_srcs/README.md @@ -0,0 +1,5 @@ +# Package mode with explicit source ownership (mixed single- and multi-source) + +This test verifies that Gazelle does not emit a package-level `py_library` +when preserved libraries collectively own every module without overlap, including +a multi-source target alongside single-source targets. diff --git a/gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_mixed_srcs/WORKSPACE b/gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_mixed_srcs/WORKSPACE new file mode 100644 index 0000000000..faff6af87a --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_mixed_srcs/WORKSPACE @@ -0,0 +1 @@ +# This is a Bazel workspace for the Gazelle test data. diff --git a/gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_mixed_srcs/auth.py b/gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_mixed_srcs/auth.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_mixed_srcs/foo.py b/gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_mixed_srcs/foo.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_mixed_srcs/oauth2.py b/gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_mixed_srcs/oauth2.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_mixed_srcs/test.yaml b/gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_mixed_srcs/test.yaml new file mode 100644 index 0000000000..ed97d539c0 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_mixed_srcs/test.yaml @@ -0,0 +1 @@ +--- diff --git a/gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_single_target/BUILD.in b/gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_single_target/BUILD.in new file mode 100644 index 0000000000..a8e9dba292 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_single_target/BUILD.in @@ -0,0 +1,9 @@ +# gazelle:python_generation_mode package + +load("@rules_python//python:defs.bzl", "py_library") + +py_library( + name = "only", + srcs = ["only.py"], + tags = ["keep_only"], +) diff --git a/gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_single_target/BUILD.out b/gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_single_target/BUILD.out new file mode 100644 index 0000000000..a8e9dba292 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_single_target/BUILD.out @@ -0,0 +1,9 @@ +# gazelle:python_generation_mode package + +load("@rules_python//python:defs.bzl", "py_library") + +py_library( + name = "only", + srcs = ["only.py"], + tags = ["keep_only"], +) diff --git a/gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_single_target/README.md b/gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_single_target/README.md new file mode 100644 index 0000000000..92d2b7329e --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_single_target/README.md @@ -0,0 +1,5 @@ +# Package mode with a single preserved library owning the only module + +This test verifies that one preserved `py_library` covering the package's only +Gazelle-managed source is enough to suppress the generated package aggregate and +claim that source. diff --git a/gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_single_target/WORKSPACE b/gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_single_target/WORKSPACE new file mode 100644 index 0000000000..faff6af87a --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_single_target/WORKSPACE @@ -0,0 +1 @@ +# This is a Bazel workspace for the Gazelle test data. diff --git a/gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_single_target/only.py b/gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_single_target/only.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_single_target/test.yaml b/gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_single_target/test.yaml new file mode 100644 index 0000000000..ed97d539c0 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_explicit_ownership_single_target/test.yaml @@ -0,0 +1 @@ +--- From 85db4e9c397d6a7c622ff647b2d3ec3ebae718eb Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 11 Sep 2026 01:09:59 +0000 Subject: [PATCH 14/18] fix(gazelle): preserve file-mode package init targets Co-authored-by: Alex Martani --- gazelle/python/BUILD.bazel | 2 + gazelle/python/generate.go | 4 +- gazelle/python/resolve_test.go | 69 +++++++++++++++++++ .../README.md | 9 +++ .../WORKSPACE | 1 + .../test.yaml | 15 ++++ .../util/BUILD.in | 2 + .../util/BUILD.out | 2 + .../util/events/BUILD.in | 13 ++++ .../util/events/BUILD.out | 22 ++++++ .../util/events/__init__.py | 1 + .../util/events/datadog.py | 1 + .../util/events/user.py | 1 + 13 files changed, 141 insertions(+), 1 deletion(-) create mode 100644 gazelle/python/resolve_test.go create mode 100644 gazelle/python/testdata/file_mode_inherited_init_package_and_per_file/README.md create mode 100644 gazelle/python/testdata/file_mode_inherited_init_package_and_per_file/WORKSPACE create mode 100644 gazelle/python/testdata/file_mode_inherited_init_package_and_per_file/test.yaml create mode 100644 gazelle/python/testdata/file_mode_inherited_init_package_and_per_file/util/BUILD.in create mode 100644 gazelle/python/testdata/file_mode_inherited_init_package_and_per_file/util/BUILD.out create mode 100644 gazelle/python/testdata/file_mode_inherited_init_package_and_per_file/util/events/BUILD.in create mode 100644 gazelle/python/testdata/file_mode_inherited_init_package_and_per_file/util/events/BUILD.out create mode 100644 gazelle/python/testdata/file_mode_inherited_init_package_and_per_file/util/events/__init__.py create mode 100644 gazelle/python/testdata/file_mode_inherited_init_package_and_per_file/util/events/datadog.py create mode 100644 gazelle/python/testdata/file_mode_inherited_init_package_and_per_file/util/events/user.py diff --git a/gazelle/python/BUILD.bazel b/gazelle/python/BUILD.bazel index 2b81d46f5e..03d0794fd2 100644 --- a/gazelle/python/BUILD.bazel +++ b/gazelle/python/BUILD.bazel @@ -122,12 +122,14 @@ go_test( "file_parser_test.go", "generate_test.go", "parser_test.go", + "resolve_test.go", "std_modules_test.go", ], embed = [":python"], deps = [ "@bazel_gazelle//config:go_default_library", "@bazel_gazelle//language:go_default_library", + "@bazel_gazelle//resolve:go_default_library", "@bazel_gazelle//rule:go_default_library", "@com_github_emirpasic_gods//sets/treeset:go_default_library", "@com_github_emirpasic_gods//utils:go_default_library", diff --git a/gazelle/python/generate.go b/gazelle/python/generate.go index 7af70f942d..74914b984a 100644 --- a/gazelle/python/generate.go +++ b/gazelle/python/generate.go @@ -891,7 +891,9 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes continue } srcs := existingPyLibrary.srcs - if existingPyLibrary.name == packageLibraryName && existingPyLibrary.declaredSrcCount > 0 { + if existingPyLibrary.name == packageLibraryName && + existingPyLibrary.declaredSrcCount > 0 && + !cfg.PerFileGeneration() { mergedSrcs := treeset.NewWith(godsutils.StringComparator) srcs.Each(func(index int, filename interface{}) { mergedSrcs.Add(filename) diff --git a/gazelle/python/resolve_test.go b/gazelle/python/resolve_test.go new file mode 100644 index 0000000000..5a8a090571 --- /dev/null +++ b/gazelle/python/resolve_test.go @@ -0,0 +1,69 @@ +package python + +import ( + "testing" + + "github.com/bazelbuild/bazel-gazelle/config" + "github.com/bazelbuild/bazel-gazelle/resolve" + "github.com/bazelbuild/bazel-gazelle/rule" + "github.com/stretchr/testify/assert" + + "github.com/bazel-contrib/rules_python/gazelle/pythonconfig" +) + +func TestImportsPerFileGenerationInitPackageTargetDoesNotIndexSiblingModules(t *testing.T) { + t.Parallel() + + fileModeCfg := &pythonconfig.Config{} + fileModeCfg.SetPerFileGeneration(true) + c := &config.Config{ + Exts: map[string]interface{}{ + languageName: pythonconfig.Configs{ + "util/events": fileModeCfg, + }, + }, + } + f := &rule.File{Pkg: "util/events"} + + resolver := &Resolver{} + eventsRule := rule.NewRule("py_library", "events") + eventsRule.SetAttr("srcs", []string{"__init__.py"}) + datadogRule := rule.NewRule("py_library", "datadog") + datadogRule.SetAttr("srcs", []string{"datadog.py"}) + + eventsImports := resolver.Imports(c, eventsRule, f) + datadogImports := resolver.Imports(c, datadogRule, f) + + assert.Equal(t, []string{"util.events"}, importImps(eventsImports)) + assert.Equal(t, []string{"util.events.datadog"}, importImps(datadogImports)) +} + +func TestImportsMergedPackageLibraryIndexesUnclaimedModules(t *testing.T) { + t.Parallel() + + packageModeCfg := &pythonconfig.Config{} + packageModeCfg.SetPerFileGeneration(false) + c := &config.Config{ + Exts: map[string]interface{}{ + languageName: pythonconfig.Configs{ + "pkg": packageModeCfg, + }, + }, + } + f := &rule.File{Pkg: "pkg"} + + resolver := &Resolver{} + pkgRule := rule.NewRule("py_library", "pkg") + pkgRule.SetAttr("srcs", []string{"__init__.py", "datadog.py"}) + + imports := resolver.Imports(c, pkgRule, f) + assert.Equal(t, []string{"pkg", "pkg.datadog"}, importImps(imports)) +} + +func importImps(specs []resolve.ImportSpec) []string { + out := make([]string, len(specs)) + for i, spec := range specs { + out[i] = spec.Imp + } + return out +} diff --git a/gazelle/python/testdata/file_mode_inherited_init_package_and_per_file/README.md b/gazelle/python/testdata/file_mode_inherited_init_package_and_per_file/README.md new file mode 100644 index 0000000000..634c610452 --- /dev/null +++ b/gazelle/python/testdata/file_mode_inherited_init_package_and_per_file/README.md @@ -0,0 +1,9 @@ +# File mode with inherited config, init package library, and per-file targets + +Parent `util/BUILD` sets `gazelle:python_generation_mode file`. The child +`util/events` package has no local mode directive, a hand-written package +library named after the directory (`events`) that owns only `__init__.py`, and a +separate per-file library for `datadog.py`. + +Gazelle must not merge unclaimed per-file sources into the package library in +file mode; doing so duplicates import specs with the per-file targets. diff --git a/gazelle/python/testdata/file_mode_inherited_init_package_and_per_file/WORKSPACE b/gazelle/python/testdata/file_mode_inherited_init_package_and_per_file/WORKSPACE new file mode 100644 index 0000000000..faff6af87a --- /dev/null +++ b/gazelle/python/testdata/file_mode_inherited_init_package_and_per_file/WORKSPACE @@ -0,0 +1 @@ +# This is a Bazel workspace for the Gazelle test data. diff --git a/gazelle/python/testdata/file_mode_inherited_init_package_and_per_file/test.yaml b/gazelle/python/testdata/file_mode_inherited_init_package_and_per_file/test.yaml new file mode 100644 index 0000000000..fcea77710f --- /dev/null +++ b/gazelle/python/testdata/file_mode_inherited_init_package_and_per_file/test.yaml @@ -0,0 +1,15 @@ +# Copyright 2023 The Bazel Authors. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +--- diff --git a/gazelle/python/testdata/file_mode_inherited_init_package_and_per_file/util/BUILD.in b/gazelle/python/testdata/file_mode_inherited_init_package_and_per_file/util/BUILD.in new file mode 100644 index 0000000000..7afa751466 --- /dev/null +++ b/gazelle/python/testdata/file_mode_inherited_init_package_and_per_file/util/BUILD.in @@ -0,0 +1,2 @@ +# gazelle:python_generation_mode file +# gazelle:exclude __init__.py diff --git a/gazelle/python/testdata/file_mode_inherited_init_package_and_per_file/util/BUILD.out b/gazelle/python/testdata/file_mode_inherited_init_package_and_per_file/util/BUILD.out new file mode 100644 index 0000000000..7afa751466 --- /dev/null +++ b/gazelle/python/testdata/file_mode_inherited_init_package_and_per_file/util/BUILD.out @@ -0,0 +1,2 @@ +# gazelle:python_generation_mode file +# gazelle:exclude __init__.py diff --git a/gazelle/python/testdata/file_mode_inherited_init_package_and_per_file/util/events/BUILD.in b/gazelle/python/testdata/file_mode_inherited_init_package_and_per_file/util/events/BUILD.in new file mode 100644 index 0000000000..6c19ed1893 --- /dev/null +++ b/gazelle/python/testdata/file_mode_inherited_init_package_and_per_file/util/events/BUILD.in @@ -0,0 +1,13 @@ +load("@rules_python//python:defs.bzl", "py_library") + +py_library( + name = "events", + srcs = ["__init__.py"], + tags = ["keep_events"], +) + +py_library( + name = "datadog", + srcs = ["datadog.py"], + tags = ["keep_datadog"], +) diff --git a/gazelle/python/testdata/file_mode_inherited_init_package_and_per_file/util/events/BUILD.out b/gazelle/python/testdata/file_mode_inherited_init_package_and_per_file/util/events/BUILD.out new file mode 100644 index 0000000000..975896dc65 --- /dev/null +++ b/gazelle/python/testdata/file_mode_inherited_init_package_and_per_file/util/events/BUILD.out @@ -0,0 +1,22 @@ +load("@rules_python//python:defs.bzl", "py_library") + +py_library( + name = "events", + srcs = ["__init__.py"], + tags = ["keep_events"], + visibility = ["//:__subpackages__"], +) + +py_library( + name = "datadog", + srcs = ["datadog.py"], + tags = ["keep_datadog"], + visibility = ["//:__subpackages__"], +) + +py_library( + name = "user", + srcs = ["user.py"], + visibility = ["//:__subpackages__"], + deps = [":datadog"], +) diff --git a/gazelle/python/testdata/file_mode_inherited_init_package_and_per_file/util/events/__init__.py b/gazelle/python/testdata/file_mode_inherited_init_package_and_per_file/util/events/__init__.py new file mode 100644 index 0000000000..ba1055549d --- /dev/null +++ b/gazelle/python/testdata/file_mode_inherited_init_package_and_per_file/util/events/__init__.py @@ -0,0 +1 @@ +"""Events package.""" diff --git a/gazelle/python/testdata/file_mode_inherited_init_package_and_per_file/util/events/datadog.py b/gazelle/python/testdata/file_mode_inherited_init_package_and_per_file/util/events/datadog.py new file mode 100644 index 0000000000..729c944fe0 --- /dev/null +++ b/gazelle/python/testdata/file_mode_inherited_init_package_and_per_file/util/events/datadog.py @@ -0,0 +1 @@ +"""Datadog helpers for events.""" diff --git a/gazelle/python/testdata/file_mode_inherited_init_package_and_per_file/util/events/user.py b/gazelle/python/testdata/file_mode_inherited_init_package_and_per_file/util/events/user.py new file mode 100644 index 0000000000..0bc684bb58 --- /dev/null +++ b/gazelle/python/testdata/file_mode_inherited_init_package_and_per_file/util/events/user.py @@ -0,0 +1 @@ +from util.events import datadog From be1ade6f9810f3dce628737d298cce13f0fceb20 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 11 Sep 2026 01:17:12 +0000 Subject: [PATCH 15/18] fix(gazelle): ignore empty visibility labels Co-authored-by: Alex Martani --- gazelle/python/target.go | 5 ++++ .../BUILD.in | 2 ++ .../BUILD.out | 14 +++++++++++ .../README.md | 7 ++++++ .../WORKSPACE | 1 + .../foo.py | 1 + .../test.yaml | 1 + gazelle/pythonconfig/BUILD.bazel | 6 ++++- gazelle/pythonconfig/pythonconfig.go | 24 ++++++++++++++++++- .../pythonconfig_visibility_test.go | 24 +++++++++++++++++++ 10 files changed, 83 insertions(+), 2 deletions(-) create mode 100644 gazelle/python/testdata/per_file_default_visibility_trailing_comma/BUILD.in create mode 100644 gazelle/python/testdata/per_file_default_visibility_trailing_comma/BUILD.out create mode 100644 gazelle/python/testdata/per_file_default_visibility_trailing_comma/README.md create mode 100644 gazelle/python/testdata/per_file_default_visibility_trailing_comma/WORKSPACE create mode 100644 gazelle/python/testdata/per_file_default_visibility_trailing_comma/foo.py create mode 100644 gazelle/python/testdata/per_file_default_visibility_trailing_comma/test.yaml create mode 100644 gazelle/pythonconfig/pythonconfig_visibility_test.go diff --git a/gazelle/python/target.go b/gazelle/python/target.go index c7009a6a84..1cbf8eaea9 100644 --- a/gazelle/python/target.go +++ b/gazelle/python/target.go @@ -16,6 +16,7 @@ package python import ( "path/filepath" + "strings" "github.com/bazelbuild/bazel-gazelle/config" "github.com/bazelbuild/bazel-gazelle/rule" @@ -134,6 +135,10 @@ func (t *targetBuilder) addResolvedDependencies(deps []string) *targetBuilder { // addVisibility adds visibility labels to the target. func (t *targetBuilder) addVisibility(visibility []string) *targetBuilder { for _, item := range visibility { + item = strings.TrimSpace(item) + if item == "" { + continue + } t.visibility.Add(item) } return t diff --git a/gazelle/python/testdata/per_file_default_visibility_trailing_comma/BUILD.in b/gazelle/python/testdata/per_file_default_visibility_trailing_comma/BUILD.in new file mode 100644 index 0000000000..a1519f3ac8 --- /dev/null +++ b/gazelle/python/testdata/per_file_default_visibility_trailing_comma/BUILD.in @@ -0,0 +1,2 @@ +# gazelle:python_generation_mode file +# gazelle:python_default_visibility //benchling:__subpackages__,//scripts:__subpackages__,//tests:__subpackages__, diff --git a/gazelle/python/testdata/per_file_default_visibility_trailing_comma/BUILD.out b/gazelle/python/testdata/per_file_default_visibility_trailing_comma/BUILD.out new file mode 100644 index 0000000000..e74acea2aa --- /dev/null +++ b/gazelle/python/testdata/per_file_default_visibility_trailing_comma/BUILD.out @@ -0,0 +1,14 @@ +load("@rules_python//python:defs.bzl", "py_library") + +# gazelle:python_generation_mode file +# gazelle:python_default_visibility //benchling:__subpackages__,//scripts:__subpackages__,//tests:__subpackages__, + +py_library( + name = "foo", + srcs = ["foo.py"], + visibility = [ + "//benchling:__subpackages__", + "//scripts:__subpackages__", + "//tests:__subpackages__", + ], +) diff --git a/gazelle/python/testdata/per_file_default_visibility_trailing_comma/README.md b/gazelle/python/testdata/per_file_default_visibility_trailing_comma/README.md new file mode 100644 index 0000000000..a1c03df0f5 --- /dev/null +++ b/gazelle/python/testdata/per_file_default_visibility_trailing_comma/README.md @@ -0,0 +1,7 @@ +# Default visibility with a trailing comma in file generation mode + +This test verifies that a `python_default_visibility` directive whose value +ends with a comma does not emit an empty visibility label on generated +per-file targets. That empty label is what Bazel lint rejects as an invalid +`""` label when a new per-file target is emitted after splitting a preserved +multi-source library. diff --git a/gazelle/python/testdata/per_file_default_visibility_trailing_comma/WORKSPACE b/gazelle/python/testdata/per_file_default_visibility_trailing_comma/WORKSPACE new file mode 100644 index 0000000000..faff6af87a --- /dev/null +++ b/gazelle/python/testdata/per_file_default_visibility_trailing_comma/WORKSPACE @@ -0,0 +1 @@ +# This is a Bazel workspace for the Gazelle test data. diff --git a/gazelle/python/testdata/per_file_default_visibility_trailing_comma/foo.py b/gazelle/python/testdata/per_file_default_visibility_trailing_comma/foo.py new file mode 100644 index 0000000000..5fca550345 --- /dev/null +++ b/gazelle/python/testdata/per_file_default_visibility_trailing_comma/foo.py @@ -0,0 +1 @@ +"""Module for trailing-comma visibility regression.""" diff --git a/gazelle/python/testdata/per_file_default_visibility_trailing_comma/test.yaml b/gazelle/python/testdata/per_file_default_visibility_trailing_comma/test.yaml new file mode 100644 index 0000000000..ed97d539c0 --- /dev/null +++ b/gazelle/python/testdata/per_file_default_visibility_trailing_comma/test.yaml @@ -0,0 +1 @@ +--- diff --git a/gazelle/pythonconfig/BUILD.bazel b/gazelle/pythonconfig/BUILD.bazel index a82a7f19ca..c1db45f40f 100644 --- a/gazelle/pythonconfig/BUILD.bazel +++ b/gazelle/pythonconfig/BUILD.bazel @@ -17,8 +17,12 @@ go_library( go_test( name = "pythonconfig_test", - srcs = ["pythonconfig_test.go"], + srcs = [ + "pythonconfig_test.go", + "pythonconfig_visibility_test.go", + ], embed = [":pythonconfig"], + deps = ["@com_github_stretchr_testify//assert"], ) filegroup( diff --git a/gazelle/pythonconfig/pythonconfig.go b/gazelle/pythonconfig/pythonconfig.go index c88d59abcf..18c5213cf6 100644 --- a/gazelle/pythonconfig/pythonconfig.go +++ b/gazelle/pythonconfig/pythonconfig.go @@ -537,8 +537,30 @@ func (c *Config) RenderProtoName(protoName string) string { return strings.ReplaceAll(c.protoNamingConvention, protoNameNamingConventionSubstitution, strings.TrimSuffix(protoName, "_proto")) } +// nonEmptyVisibilityLabels returns labels with empty and whitespace-only entries +// removed. Comma-separated python_default_visibility directives may include a +// trailing comma, which strings.Split turns into an empty label. +func nonEmptyVisibilityLabels(labels []string) []string { + if len(labels) == 0 { + return labels + } + filtered := make([]string, 0, len(labels)) + for _, label := range labels { + label = strings.TrimSpace(label) + if label == "" { + continue + } + filtered = append(filtered, label) + } + return filtered +} + // AppendVisibility adds additional items to the target's visibility. func (c *Config) AppendVisibility(visibility string) { + visibility = strings.TrimSpace(visibility) + if visibility == "" { + return + } c.visibility = append(c.visibility, visibility) } @@ -549,7 +571,7 @@ func (c *Config) Visibility() []string { // SetDefaultVisibility sets the default visibility of the target. func (c *Config) SetDefaultVisibility(visibility []string) { - c.defaultVisibility = visibility + c.defaultVisibility = nonEmptyVisibilityLabels(visibility) } // DefaultVisibilty returns the target's default visibility. diff --git a/gazelle/pythonconfig/pythonconfig_visibility_test.go b/gazelle/pythonconfig/pythonconfig_visibility_test.go new file mode 100644 index 0000000000..7e74aabae1 --- /dev/null +++ b/gazelle/pythonconfig/pythonconfig_visibility_test.go @@ -0,0 +1,24 @@ +package pythonconfig + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestNonEmptyVisibilityLabels(t *testing.T) { + assert.Equal(t, []string{"//a:b", "//c:d"}, nonEmptyVisibilityLabels([]string{ + "//a:b", + "", + " //c:d ", + "", + })) + assert.Nil(t, nonEmptyVisibilityLabels(nil)) + assert.Empty(t, nonEmptyVisibilityLabels([]string{"", " "})) +} + +func TestSetDefaultVisibilityDropsEmptyLabels(t *testing.T) { + cfg := New("", "") + cfg.SetDefaultVisibility([]string{"//benchling:__subpackages__", ""}) + assert.Equal(t, []string{"//benchling:__subpackages__"}, cfg.Visibility()) +} From d214fe61430f59a77a2eedf0722db26ea1aab650 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 11 Sep 2026 01:49:18 +0000 Subject: [PATCH 16/18] fix(gazelle): remove stale package umbrellas Co-authored-by: Alex Martani --- gazelle/python/generate.go | 76 +++++++++++++++++-- gazelle/python/generate_test.go | 19 +---- .../BUILD.in | 16 ++++ .../BUILD.out | 9 +++ .../WORKSPACE | 1 + .../nested/BUILD.in | 8 ++ .../nested/BUILD.out | 9 +++ .../nested/helper.py | 0 .../test.yaml | 1 + .../widget.py | 0 .../BUILD.in | 19 +++++ .../BUILD.out | 16 ++++ .../WORKSPACE | 1 + .../consumer.py | 5 ++ .../test.yaml | 1 + .../widget.py | 2 + .../BUILD.out | 8 -- .../BUILD.in | 21 +++++ .../BUILD.out | 21 +++++ .../WORKSPACE | 1 + .../bar.py | 0 .../foo.py | 0 .../test.yaml | 1 + 23 files changed, 206 insertions(+), 29 deletions(-) create mode 100644 gazelle/python/testdata/file_mode_remove_stale_sourceless_umbrella/BUILD.in create mode 100644 gazelle/python/testdata/file_mode_remove_stale_sourceless_umbrella/BUILD.out create mode 100644 gazelle/python/testdata/file_mode_remove_stale_sourceless_umbrella/WORKSPACE create mode 100644 gazelle/python/testdata/file_mode_remove_stale_sourceless_umbrella/nested/BUILD.in create mode 100644 gazelle/python/testdata/file_mode_remove_stale_sourceless_umbrella/nested/BUILD.out create mode 100644 gazelle/python/testdata/file_mode_remove_stale_sourceless_umbrella/nested/helper.py create mode 100644 gazelle/python/testdata/file_mode_remove_stale_sourceless_umbrella/test.yaml create mode 100644 gazelle/python/testdata/file_mode_remove_stale_sourceless_umbrella/widget.py create mode 100644 gazelle/python/testdata/file_mode_remove_stale_umbrella_stale_dep/BUILD.in create mode 100644 gazelle/python/testdata/file_mode_remove_stale_umbrella_stale_dep/BUILD.out create mode 100644 gazelle/python/testdata/file_mode_remove_stale_umbrella_stale_dep/WORKSPACE create mode 100644 gazelle/python/testdata/file_mode_remove_stale_umbrella_stale_dep/consumer.py create mode 100644 gazelle/python/testdata/file_mode_remove_stale_umbrella_stale_dep/test.yaml create mode 100644 gazelle/python/testdata/file_mode_remove_stale_umbrella_stale_dep/widget.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate_keep/BUILD.in create mode 100644 gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate_keep/BUILD.out create mode 100644 gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate_keep/WORKSPACE create mode 100644 gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate_keep/bar.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate_keep/foo.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate_keep/test.yaml diff --git a/gazelle/python/generate.go b/gazelle/python/generate.go index 74914b984a..e90eb92980 100644 --- a/gazelle/python/generate.go +++ b/gazelle/python/generate.go @@ -352,11 +352,10 @@ func adoptExcludedInitOnlyPackageLibraryForSplitLayout( } // adoptEmptyAggregatePackageLibraryForSplitLayout appends the hand-written -// package library when it omits srcs entirely. That target is otherwise not -// adopted because collectExistingPythonSourceRules ignores rules without srcs, -// but it is still the deps-only package aggregate in a split layout and must -// be preserved so Gazelle does not emit a competing package-level library. -// Only applies when every other adopted library is a single-module target. +// package library when it omits srcs entirely and is marked with "# keep". +// Without "# keep", such targets are stale deps-only aggregates and are removed +// instead. Only applies when every other adopted library is a single-module +// target. func adoptEmptyAggregatePackageLibraryForSplitLayout( args language.GenerateArgs, kind string, @@ -384,6 +383,9 @@ func adoptEmptyAggregatePackageLibraryForSplitLayout( if len(existingRule.AttrStrings("srcs")) != 0 { return rules } + if !existingRule.ShouldKeep() { + return rules + } candidate := existingPythonSourceRule{ name: packageLibraryName, @@ -817,9 +819,22 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes } generateEmptyLibrary := false for _, r := range args.File.Rules { - if r.Name() == pyLibraryTargetName && kindMatches(args.Config, r, pyLibraryKind) { + if r.Name() != pyLibraryTargetName || !kindMatches(args.Config, r, pyLibraryKind) { + continue + } + if r.ShouldKeep() { generateEmptyLibrary = true + break } + result.Empty = append(result.Empty, newTargetBuilder( + pyLibraryKind, + pyLibraryTargetName, + pythonProjectRoot, + args.Rel, + pyFileNames, + cfg.ResolveSiblingImports(), + ).build()) + return } if !generateEmptyLibrary { return @@ -1103,6 +1118,7 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes } emptyRules := py.getRulesWithInvalidSrcs(args, validFilesMap) result.Empty = append(result.Empty, emptyRules...) + result.Empty = append(result.Empty, getStaleSourcelessPyLibraryRules(args, packageLibraryName, generatedTargetNames, result.Empty)...) if !collisionErrors.Empty() { it := collisionErrors.Iterator() for it.Next() { @@ -1114,6 +1130,54 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes return result } +// getStaleSourcelessPyLibraryRules returns deps-only py_library targets with no +// srcs that Gazelle should delete. Hand-written re-export umbrellas must use +// a "# keep" suffix comment to opt out. +func getStaleSourcelessPyLibraryRules( + args language.GenerateArgs, + packageLibraryName string, + generatedTargetNames map[string]struct{}, + alreadyScheduled []*rule.Rule, +) []*rule.Rule { + if args.File == nil { + return nil + } + alreadyEmpty := make(map[string]struct{}, len(alreadyScheduled)) + for _, r := range alreadyScheduled { + alreadyEmpty[r.Name()] = struct{}{} + } + var stale []*rule.Rule + for _, existingRule := range args.File.Rules { + if !kindMatches(args.Config, existingRule, pyLibraryKind) { + continue + } + if existingRule.Name() != packageLibraryName { + continue + } + if existingRule.ShouldKeep() { + continue + } + if len(existingRule.AttrStrings("srcs")) != 0 { + continue + } + if _, isGenerated := generatedTargetNames[existingRule.Name()]; isGenerated { + continue + } + if _, dup := alreadyEmpty[existingRule.Name()]; dup { + continue + } + stale = append(stale, newTargetBuilder( + pyLibraryKind, + existingRule.Name(), + "", + "", + nil, + false, + ).build()) + } + return stale +} + // ruleListsGazelleManagedSrc reports whether any src is one Gazelle would place // in a generated target for this package. func ruleListsGazelleManagedSrc(srcs []string, managed map[string]struct{}) bool { diff --git a/gazelle/python/generate_test.go b/gazelle/python/generate_test.go index 8fa843ca34..af20ef8b9f 100644 --- a/gazelle/python/generate_test.go +++ b/gazelle/python/generate_test.go @@ -197,8 +197,8 @@ func TestEmptyAggregateFixtureSplitLayout(t *testing.T) { rules := collectExistingPythonSourceRules(args, pyLibraryKind, knownSrcs) rules = adoptEmptyAggregatePackageLibraryForSplitLayout(args, pyLibraryKind, packageLibraryName, rules) - require.Len(t, rules, 3) - assert.True(t, hasSplitPackageLibraryLayout(packageLibraryName, rules)) + require.Len(t, rules, 2) + assert.False(t, hasSplitPackageLibraryLayout(packageLibraryName, rules)) } func TestAdoptEmptyAggregatePackageLibraryForSplitLayout(t *testing.T) { @@ -226,19 +226,8 @@ func TestAdoptEmptyAggregatePackageLibraryForSplitLayout(t *testing.T) { packageLibraryName, collectExistingPythonSourceRules(args, pyLibraryKind, knownSrcs), ) - require.Len(t, adopted, 3) - assert.True(t, hasSplitPackageLibraryLayout(packageLibraryName, adopted)) - - var packageRule *existingPythonSourceRule - for i := range adopted { - if adopted[i].name == packageLibraryName { - packageRule = &adopted[i] - break - } - } - require.NotNil(t, packageRule) - assert.Equal(t, 0, packageRule.srcs.Size()) - assert.Equal(t, 0, packageRule.declaredSrcCount) + require.Len(t, adopted, 2) + assert.False(t, hasSplitPackageLibraryLayout(packageLibraryName, adopted)) } func TestAdoptEmptyAggregatePackageLibraryIgnoredWithoutPerFileLibraries(t *testing.T) { diff --git a/gazelle/python/testdata/file_mode_remove_stale_sourceless_umbrella/BUILD.in b/gazelle/python/testdata/file_mode_remove_stale_sourceless_umbrella/BUILD.in new file mode 100644 index 0000000000..ec9c96ffc6 --- /dev/null +++ b/gazelle/python/testdata/file_mode_remove_stale_sourceless_umbrella/BUILD.in @@ -0,0 +1,16 @@ +# gazelle:python_generation_mode file + +load("@rules_python//python:defs.bzl", "py_library") + +py_library( + name = "widget", + srcs = ["widget.py"], +) + +py_library( + name = "file_mode_remove_stale_sourceless_umbrella", + deps = [ + ":widget", + "//file_mode_remove_stale_sourceless_umbrella/nested", + ], +) diff --git a/gazelle/python/testdata/file_mode_remove_stale_sourceless_umbrella/BUILD.out b/gazelle/python/testdata/file_mode_remove_stale_sourceless_umbrella/BUILD.out new file mode 100644 index 0000000000..094d7aa4d9 --- /dev/null +++ b/gazelle/python/testdata/file_mode_remove_stale_sourceless_umbrella/BUILD.out @@ -0,0 +1,9 @@ +# gazelle:python_generation_mode file + +load("@rules_python//python:defs.bzl", "py_library") + +py_library( + name = "widget", + srcs = ["widget.py"], + visibility = ["//:__subpackages__"], +) diff --git a/gazelle/python/testdata/file_mode_remove_stale_sourceless_umbrella/WORKSPACE b/gazelle/python/testdata/file_mode_remove_stale_sourceless_umbrella/WORKSPACE new file mode 100644 index 0000000000..faff6af87a --- /dev/null +++ b/gazelle/python/testdata/file_mode_remove_stale_sourceless_umbrella/WORKSPACE @@ -0,0 +1 @@ +# This is a Bazel workspace for the Gazelle test data. diff --git a/gazelle/python/testdata/file_mode_remove_stale_sourceless_umbrella/nested/BUILD.in b/gazelle/python/testdata/file_mode_remove_stale_sourceless_umbrella/nested/BUILD.in new file mode 100644 index 0000000000..280765e424 --- /dev/null +++ b/gazelle/python/testdata/file_mode_remove_stale_sourceless_umbrella/nested/BUILD.in @@ -0,0 +1,8 @@ +# gazelle:python_generation_mode file + +load("@rules_python//python:defs.bzl", "py_library") + +py_library( + name = "helper", + srcs = ["helper.py"], +) diff --git a/gazelle/python/testdata/file_mode_remove_stale_sourceless_umbrella/nested/BUILD.out b/gazelle/python/testdata/file_mode_remove_stale_sourceless_umbrella/nested/BUILD.out new file mode 100644 index 0000000000..1d838d1da2 --- /dev/null +++ b/gazelle/python/testdata/file_mode_remove_stale_sourceless_umbrella/nested/BUILD.out @@ -0,0 +1,9 @@ +# gazelle:python_generation_mode file + +load("@rules_python//python:defs.bzl", "py_library") + +py_library( + name = "helper", + srcs = ["helper.py"], + visibility = ["//:__subpackages__"], +) diff --git a/gazelle/python/testdata/file_mode_remove_stale_sourceless_umbrella/nested/helper.py b/gazelle/python/testdata/file_mode_remove_stale_sourceless_umbrella/nested/helper.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/gazelle/python/testdata/file_mode_remove_stale_sourceless_umbrella/test.yaml b/gazelle/python/testdata/file_mode_remove_stale_sourceless_umbrella/test.yaml new file mode 100644 index 0000000000..ed97d539c0 --- /dev/null +++ b/gazelle/python/testdata/file_mode_remove_stale_sourceless_umbrella/test.yaml @@ -0,0 +1 @@ +--- diff --git a/gazelle/python/testdata/file_mode_remove_stale_sourceless_umbrella/widget.py b/gazelle/python/testdata/file_mode_remove_stale_sourceless_umbrella/widget.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/gazelle/python/testdata/file_mode_remove_stale_umbrella_stale_dep/BUILD.in b/gazelle/python/testdata/file_mode_remove_stale_umbrella_stale_dep/BUILD.in new file mode 100644 index 0000000000..3488cfa713 --- /dev/null +++ b/gazelle/python/testdata/file_mode_remove_stale_umbrella_stale_dep/BUILD.in @@ -0,0 +1,19 @@ +# gazelle:python_generation_mode file + +load("@rules_python//python:defs.bzl", "py_library") + +py_library( + name = "widget", + srcs = ["widget.py"], +) + +py_library( + name = "file_mode_remove_stale_umbrella_stale_dep", + deps = [":widget"], +) + +py_library( + name = "consumer", + srcs = ["consumer.py"], + deps = [":file_mode_remove_stale_umbrella_stale_dep"], +) diff --git a/gazelle/python/testdata/file_mode_remove_stale_umbrella_stale_dep/BUILD.out b/gazelle/python/testdata/file_mode_remove_stale_umbrella_stale_dep/BUILD.out new file mode 100644 index 0000000000..84d62b9a19 --- /dev/null +++ b/gazelle/python/testdata/file_mode_remove_stale_umbrella_stale_dep/BUILD.out @@ -0,0 +1,16 @@ +# gazelle:python_generation_mode file + +load("@rules_python//python:defs.bzl", "py_library") + +py_library( + name = "widget", + srcs = ["widget.py"], + visibility = ["//:__subpackages__"], +) + +py_library( + name = "consumer", + srcs = ["consumer.py"], + visibility = ["//:__subpackages__"], + deps = [":widget"], +) diff --git a/gazelle/python/testdata/file_mode_remove_stale_umbrella_stale_dep/WORKSPACE b/gazelle/python/testdata/file_mode_remove_stale_umbrella_stale_dep/WORKSPACE new file mode 100644 index 0000000000..faff6af87a --- /dev/null +++ b/gazelle/python/testdata/file_mode_remove_stale_umbrella_stale_dep/WORKSPACE @@ -0,0 +1 @@ +# This is a Bazel workspace for the Gazelle test data. diff --git a/gazelle/python/testdata/file_mode_remove_stale_umbrella_stale_dep/consumer.py b/gazelle/python/testdata/file_mode_remove_stale_umbrella_stale_dep/consumer.py new file mode 100644 index 0000000000..4e1d7f6f0a --- /dev/null +++ b/gazelle/python/testdata/file_mode_remove_stale_umbrella_stale_dep/consumer.py @@ -0,0 +1,5 @@ +import widget + + +def use_widget() -> None: + widget.do_thing() diff --git a/gazelle/python/testdata/file_mode_remove_stale_umbrella_stale_dep/test.yaml b/gazelle/python/testdata/file_mode_remove_stale_umbrella_stale_dep/test.yaml new file mode 100644 index 0000000000..ed97d539c0 --- /dev/null +++ b/gazelle/python/testdata/file_mode_remove_stale_umbrella_stale_dep/test.yaml @@ -0,0 +1 @@ +--- diff --git a/gazelle/python/testdata/file_mode_remove_stale_umbrella_stale_dep/widget.py b/gazelle/python/testdata/file_mode_remove_stale_umbrella_stale_dep/widget.py new file mode 100644 index 0000000000..93d83bfed4 --- /dev/null +++ b/gazelle/python/testdata/file_mode_remove_stale_umbrella_stale_dep/widget.py @@ -0,0 +1,2 @@ +def do_thing() -> None: + return None diff --git a/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate/BUILD.out b/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate/BUILD.out index b5b2622cd0..9e1e0719fb 100644 --- a/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate/BUILD.out +++ b/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate/BUILD.out @@ -11,11 +11,3 @@ py_library( srcs = ["bar.py"], tags = ["keep_bar"], ) - -# Package library name Gazelle generates in package mode; deps-only aggregate. -py_library( - name = "package_mode_respect_existing_split_package_library_empty_aggregate", - tags = ["keep_pkg"], - visibility = ["//visibility:public"], - deps = [":foo"], -) diff --git a/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate_keep/BUILD.in b/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate_keep/BUILD.in new file mode 100644 index 0000000000..c54a656f9c --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate_keep/BUILD.in @@ -0,0 +1,21 @@ +load("@rules_python//python:defs.bzl", "py_library") + +py_library( + name = "foo", + srcs = ["foo.py"], + tags = ["keep_foo"], +) + +py_library( + name = "bar", + srcs = ["bar.py"], + tags = ["keep_bar"], +) + +# Package library name Gazelle generates in package mode; deps-only aggregate. +py_library( + name = "package_mode_respect_existing_split_package_library_empty_aggregate_keep", + deps = [":foo"], + tags = ["keep_pkg"], + visibility = ["//visibility:public"], +) # keep diff --git a/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate_keep/BUILD.out b/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate_keep/BUILD.out new file mode 100644 index 0000000000..887215c132 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate_keep/BUILD.out @@ -0,0 +1,21 @@ +load("@rules_python//python:defs.bzl", "py_library") + +py_library( + name = "foo", + srcs = ["foo.py"], + tags = ["keep_foo"], +) + +py_library( + name = "bar", + srcs = ["bar.py"], + tags = ["keep_bar"], +) + +# Package library name Gazelle generates in package mode; deps-only aggregate. +py_library( + name = "package_mode_respect_existing_split_package_library_empty_aggregate_keep", + tags = ["keep_pkg"], + visibility = ["//visibility:public"], + deps = [":foo"], +) # keep diff --git a/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate_keep/WORKSPACE b/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate_keep/WORKSPACE new file mode 100644 index 0000000000..faff6af87a --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate_keep/WORKSPACE @@ -0,0 +1 @@ +# This is a Bazel workspace for the Gazelle test data. diff --git a/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate_keep/bar.py b/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate_keep/bar.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate_keep/foo.py b/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate_keep/foo.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate_keep/test.yaml b/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate_keep/test.yaml new file mode 100644 index 0000000000..ed97d539c0 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_split_package_library_empty_aggregate_keep/test.yaml @@ -0,0 +1 @@ +--- From 4f2a3fc3052cf1dfd469ad9f7f2156bbd153cf4c Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 11 Sep 2026 02:09:09 +0000 Subject: [PATCH 17/18] fix(gazelle): reserve only names Gazelle generates Co-authored-by: Alex Martani --- gazelle/python/generate.go | 24 ++++++++++++------- .../BUILD.in | 14 +++++++++++ .../BUILD.out | 14 +++++++++++ .../README.md | 6 +++++ .../WORKSPACE | 1 + .../bar_test.py | 2 ++ .../foo_test.py | 2 ++ .../test.yaml | 1 + 8 files changed, 55 insertions(+), 9 deletions(-) create mode 100644 gazelle/python/testdata/package_mode_respect_existing_package_named_test/BUILD.in create mode 100644 gazelle/python/testdata/package_mode_respect_existing_package_named_test/BUILD.out create mode 100644 gazelle/python/testdata/package_mode_respect_existing_package_named_test/README.md create mode 100644 gazelle/python/testdata/package_mode_respect_existing_package_named_test/WORKSPACE create mode 100644 gazelle/python/testdata/package_mode_respect_existing_package_named_test/bar_test.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_package_named_test/foo_test.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_package_named_test/test.yaml diff --git a/gazelle/python/generate.go b/gazelle/python/generate.go index e90eb92980..ca7c950533 100644 --- a/gazelle/python/generate.go +++ b/gazelle/python/generate.go @@ -631,13 +631,11 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes } } - // generatedTargetNames holds per-file target names Gazelle generates in this - // package. In file mode, an existing rule with one of those names is not a - // hand-written target to adopt: adopting it would let it claim sources that - // belong in other per-file targets, and the generated rule of the same name - // would merge over it and drop them. Package- and project-level library and - // test names are intentionally absent: those targets are regenerated in - // place. Dedicated binary and conftest target names are always excluded. + // generatedTargetNames holds the names Gazelle generates in this package. An + // existing rule with one of those names is not a hand-written target to + // adopt: adopting it would put two rules with the same name into result.Gen, + // where they merge into one and silently orphan the sources of whichever rule + // lost. Names Gazelle does not emit stay available to hand-written targets. packageLibraryName := cfg.RenderLibraryName(packageName) existingPyLibraries := collectExistingPythonSourceRules(args, pyLibraryKind, knownPySrcs) if !cfg.PerFileGeneration() { @@ -673,8 +671,16 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes addTargetNamesForSrcs(pyLibraryFilenames, generatedTargetNames) addTargetNamesForSrcs(pyTestFilenames, generatedTargetNames) } else if !splitPackageLibraryLayout { - generatedTargetNames[packageLibraryName] = struct{}{} - generatedTargetNames[cfg.RenderTestName(packageName)] = struct{}{} + // A name is only reserved when Gazelle emits a target with it. A + // test-only package generates no package library, so a hand-written + // py_test may carry the package library name, and a package without + // tests leaves the generated test name free. + if !pyLibraryFilenames.Empty() { + generatedTargetNames[packageLibraryName] = struct{}{} + } + if !pyTestFilenames.Empty() || hasPyTestEntryPointFile || hasPyTestEntryPointTarget { + generatedTargetNames[cfg.RenderTestName(packageName)] = struct{}{} + } } if hasPyBinaryEntryPointFile { generatedTargetNames[cfg.RenderBinaryName(packageName)] = struct{}{} diff --git a/gazelle/python/testdata/package_mode_respect_existing_package_named_test/BUILD.in b/gazelle/python/testdata/package_mode_respect_existing_package_named_test/BUILD.in new file mode 100644 index 0000000000..81c75fe65b --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_package_named_test/BUILD.in @@ -0,0 +1,14 @@ +# gazelle:python_generation_mode project + +load("@rules_python//python:defs.bzl", "py_test") + +# Test-only package: no package library is generated, so this hand-written +# target may carry the package library name. +py_test( + name = "package_mode_respect_existing_package_named_test", + srcs = [ + "bar_test.py", + "foo_test.py", + ], + tags = ["keep_me"], +) diff --git a/gazelle/python/testdata/package_mode_respect_existing_package_named_test/BUILD.out b/gazelle/python/testdata/package_mode_respect_existing_package_named_test/BUILD.out new file mode 100644 index 0000000000..81c75fe65b --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_package_named_test/BUILD.out @@ -0,0 +1,14 @@ +# gazelle:python_generation_mode project + +load("@rules_python//python:defs.bzl", "py_test") + +# Test-only package: no package library is generated, so this hand-written +# target may carry the package library name. +py_test( + name = "package_mode_respect_existing_package_named_test", + srcs = [ + "bar_test.py", + "foo_test.py", + ], + tags = ["keep_me"], +) diff --git a/gazelle/python/testdata/package_mode_respect_existing_package_named_test/README.md b/gazelle/python/testdata/package_mode_respect_existing_package_named_test/README.md new file mode 100644 index 0000000000..7f1387b40b --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_package_named_test/README.md @@ -0,0 +1,6 @@ +# Package Mode With An Existing Test Named After The Package + +This test verifies that a hand-written `py_test` using the package library name +is preserved in a package that has no library sources. Gazelle generates no +package library there, so the name is free, and the target's sources must not +also appear in a generated `py_test`. diff --git a/gazelle/python/testdata/package_mode_respect_existing_package_named_test/WORKSPACE b/gazelle/python/testdata/package_mode_respect_existing_package_named_test/WORKSPACE new file mode 100644 index 0000000000..faff6af87a --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_package_named_test/WORKSPACE @@ -0,0 +1 @@ +# This is a Bazel workspace for the Gazelle test data. diff --git a/gazelle/python/testdata/package_mode_respect_existing_package_named_test/bar_test.py b/gazelle/python/testdata/package_mode_respect_existing_package_named_test/bar_test.py new file mode 100644 index 0000000000..0debd4f2ee --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_package_named_test/bar_test.py @@ -0,0 +1,2 @@ +def test_bar(): + assert True diff --git a/gazelle/python/testdata/package_mode_respect_existing_package_named_test/foo_test.py b/gazelle/python/testdata/package_mode_respect_existing_package_named_test/foo_test.py new file mode 100644 index 0000000000..30e2c41ab9 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_package_named_test/foo_test.py @@ -0,0 +1,2 @@ +def test_foo(): + assert True diff --git a/gazelle/python/testdata/package_mode_respect_existing_package_named_test/test.yaml b/gazelle/python/testdata/package_mode_respect_existing_package_named_test/test.yaml new file mode 100644 index 0000000000..ed97d539c0 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_package_named_test/test.yaml @@ -0,0 +1 @@ +--- From 271c61a5cd1a644b36615f68211e8405c4e9d359 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 11 Sep 2026 02:48:53 +0000 Subject: [PATCH 18/18] fix(gazelle): preserve excluded package libraries Co-authored-by: Alex Martani --- gazelle/python/generate.go | 7 ++++++ .../README.md | 6 +++++ .../WORKSPACE | 1 + .../models/BUILD.in | 22 +++++++++++++++++++ .../models/BUILD.out | 22 +++++++++++++++++++ .../models/benchling/alphafold.py | 1 + .../models/inductive/logd.py | 1 + .../models/inductive/logd.yaml | 1 + .../models/tunelab/ablab.py | 1 + .../models/tunelab/ablab.yaml | 1 + .../test.yaml | 1 + 11 files changed, 64 insertions(+) create mode 100644 gazelle/python/testdata/package_mode_respect_existing_excluded_subdirs_named_package/README.md create mode 100644 gazelle/python/testdata/package_mode_respect_existing_excluded_subdirs_named_package/WORKSPACE create mode 100644 gazelle/python/testdata/package_mode_respect_existing_excluded_subdirs_named_package/models/BUILD.in create mode 100644 gazelle/python/testdata/package_mode_respect_existing_excluded_subdirs_named_package/models/BUILD.out create mode 100644 gazelle/python/testdata/package_mode_respect_existing_excluded_subdirs_named_package/models/benchling/alphafold.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_excluded_subdirs_named_package/models/inductive/logd.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_excluded_subdirs_named_package/models/inductive/logd.yaml create mode 100644 gazelle/python/testdata/package_mode_respect_existing_excluded_subdirs_named_package/models/tunelab/ablab.py create mode 100644 gazelle/python/testdata/package_mode_respect_existing_excluded_subdirs_named_package/models/tunelab/ablab.yaml create mode 100644 gazelle/python/testdata/package_mode_respect_existing_excluded_subdirs_named_package/test.yaml diff --git a/gazelle/python/generate.go b/gazelle/python/generate.go index ca7c950533..79e41f1dd5 100644 --- a/gazelle/python/generate.go +++ b/gazelle/python/generate.go @@ -832,6 +832,13 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes generateEmptyLibrary = true break } + // A hand-written package library that still lists srcs but owns + // only excluded or otherwise unmanaged files is not adopted and + // leaves nothing for Gazelle to generate. Do not treat it as an + // empty generated library to remove. + if len(r.AttrStrings("srcs")) > 0 { + return + } result.Empty = append(result.Empty, newTargetBuilder( pyLibraryKind, pyLibraryTargetName, diff --git a/gazelle/python/testdata/package_mode_respect_existing_excluded_subdirs_named_package/README.md b/gazelle/python/testdata/package_mode_respect_existing_excluded_subdirs_named_package/README.md new file mode 100644 index 0000000000..41dd140ce0 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_excluded_subdirs_named_package/README.md @@ -0,0 +1,6 @@ +# Package mode with excluded subdirs and a package-named library + +Mirrors monolith layouts where `benchling/`, `inductive/`, and `tunelab/` +subdirectories are excluded from Gazelle but a hand-written `py_library` named +after the package aggregates their sources, `data`, `deps`, and custom attrs. +Gazelle must leave that target unmanaged. diff --git a/gazelle/python/testdata/package_mode_respect_existing_excluded_subdirs_named_package/WORKSPACE b/gazelle/python/testdata/package_mode_respect_existing_excluded_subdirs_named_package/WORKSPACE new file mode 100644 index 0000000000..faff6af87a --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_excluded_subdirs_named_package/WORKSPACE @@ -0,0 +1 @@ +# This is a Bazel workspace for the Gazelle test data. diff --git a/gazelle/python/testdata/package_mode_respect_existing_excluded_subdirs_named_package/models/BUILD.in b/gazelle/python/testdata/package_mode_respect_existing_excluded_subdirs_named_package/models/BUILD.in new file mode 100644 index 0000000000..95719be77a --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_excluded_subdirs_named_package/models/BUILD.in @@ -0,0 +1,22 @@ +# benchling/, inductive/, and tunelab/ are organizational only; keep all srcs in this target. +# gazelle:exclude benchling +# gazelle:exclude inductive +# gazelle:exclude tunelab + +load("@rules_python//python:defs.bzl", "py_library") + +py_library( + name = "models", + srcs = [ + "benchling/alphafold.py", + "inductive/logd.py", + "tunelab/ablab.py", + ], + data = [ + "inductive/logd.yaml", + "tunelab/ablab.yaml", + ], + tags = ["benchling_monolith"], + visibility = ["//visibility:public"], + deps = ["//other:dep"], +) diff --git a/gazelle/python/testdata/package_mode_respect_existing_excluded_subdirs_named_package/models/BUILD.out b/gazelle/python/testdata/package_mode_respect_existing_excluded_subdirs_named_package/models/BUILD.out new file mode 100644 index 0000000000..95719be77a --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_excluded_subdirs_named_package/models/BUILD.out @@ -0,0 +1,22 @@ +# benchling/, inductive/, and tunelab/ are organizational only; keep all srcs in this target. +# gazelle:exclude benchling +# gazelle:exclude inductive +# gazelle:exclude tunelab + +load("@rules_python//python:defs.bzl", "py_library") + +py_library( + name = "models", + srcs = [ + "benchling/alphafold.py", + "inductive/logd.py", + "tunelab/ablab.py", + ], + data = [ + "inductive/logd.yaml", + "tunelab/ablab.yaml", + ], + tags = ["benchling_monolith"], + visibility = ["//visibility:public"], + deps = ["//other:dep"], +) diff --git a/gazelle/python/testdata/package_mode_respect_existing_excluded_subdirs_named_package/models/benchling/alphafold.py b/gazelle/python/testdata/package_mode_respect_existing_excluded_subdirs_named_package/models/benchling/alphafold.py new file mode 100644 index 0000000000..c3746cc944 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_excluded_subdirs_named_package/models/benchling/alphafold.py @@ -0,0 +1 @@ +"""Excluded benchling model stub for Gazelle regression coverage.""" diff --git a/gazelle/python/testdata/package_mode_respect_existing_excluded_subdirs_named_package/models/inductive/logd.py b/gazelle/python/testdata/package_mode_respect_existing_excluded_subdirs_named_package/models/inductive/logd.py new file mode 100644 index 0000000000..d44a17a5f1 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_excluded_subdirs_named_package/models/inductive/logd.py @@ -0,0 +1 @@ +"""Excluded inductive model stub for Gazelle regression coverage.""" diff --git a/gazelle/python/testdata/package_mode_respect_existing_excluded_subdirs_named_package/models/inductive/logd.yaml b/gazelle/python/testdata/package_mode_respect_existing_excluded_subdirs_named_package/models/inductive/logd.yaml new file mode 100644 index 0000000000..34be8ef15b --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_excluded_subdirs_named_package/models/inductive/logd.yaml @@ -0,0 +1 @@ +name: logd diff --git a/gazelle/python/testdata/package_mode_respect_existing_excluded_subdirs_named_package/models/tunelab/ablab.py b/gazelle/python/testdata/package_mode_respect_existing_excluded_subdirs_named_package/models/tunelab/ablab.py new file mode 100644 index 0000000000..8b6ab958bf --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_excluded_subdirs_named_package/models/tunelab/ablab.py @@ -0,0 +1 @@ +"""Excluded tunelab model stub for Gazelle regression coverage.""" diff --git a/gazelle/python/testdata/package_mode_respect_existing_excluded_subdirs_named_package/models/tunelab/ablab.yaml b/gazelle/python/testdata/package_mode_respect_existing_excluded_subdirs_named_package/models/tunelab/ablab.yaml new file mode 100644 index 0000000000..491d84f249 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_excluded_subdirs_named_package/models/tunelab/ablab.yaml @@ -0,0 +1 @@ +name: ablab diff --git a/gazelle/python/testdata/package_mode_respect_existing_excluded_subdirs_named_package/test.yaml b/gazelle/python/testdata/package_mode_respect_existing_excluded_subdirs_named_package/test.yaml new file mode 100644 index 0000000000..ed97d539c0 --- /dev/null +++ b/gazelle/python/testdata/package_mode_respect_existing_excluded_subdirs_named_package/test.yaml @@ -0,0 +1 @@ +---