Skip to content

Gazelle: Delete stale py_library and py_test targets#3817

Open
taowang487 wants to merge 4 commits into
bazel-contrib:mainfrom
taowang487:taowang/gazelle-stale-target
Open

Gazelle: Delete stale py_library and py_test targets#3817
taowang487 wants to merge 4 commits into
bazel-contrib:mainfrom
taowang487:taowang/gazelle-stale-target

Conversation

@taowang487

@taowang487 taowang487 commented Jun 9, 2026

Copy link
Copy Markdown

Summary

Fix the issue #3375
Right now the py_library and py_test targets with missing srcs wouldn't be cleaned up by Gazelle in file mode.

This can be reproduced by a unit test added in commit ad0e48c

Testing

In commit ad0e48c , without the fix, bazel test //python/... failed in the newly added tests. The stale py_library and py_test cannot be removed.

In latest HEAD d32fb7b, bazel test //python/... can pass

INFO: From Testing //python:python_test_remove_invalid_per_file:
==================== Test output for //python:python_test_remove_invalid_per_file:
--- FAIL: TestGazelleBinary (0.02s)
    --- FAIL: TestGazelleBinary/remove_invalid_per_file (0.08s)
        python_test.go:186: remove_invalid_per_file/BUILD diff (-want,+got):
              (
                """
                ... // 8 identical lines
                )
              
            +   py_library(
            +       name = "deleted_lib",
            +       srcs = ["deleted.py"],
            +       visibility = ["//:__subpackages__"],
            +   )
            + 
            +   py_test(
            +       name = "bar_test",
            +       srcs = ["bar_test.py"],
            +   )
            + 
                py_test(
            -       name = "bar_test",
            -       srcs = ["bar_test.py"],
            +       name = "deleted_test",
            +       srcs = ["deleted_test.py"],
                )
                """
              )

…id_library_package_mode

Clarifies that this test covers package-mode py_library removal (where
__init__.py is missing), distinct from per-file mode stale target removal.

Co-Authored-By: Tao Wang <watao@microsoft.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates Gazelle's Python extension to support identifying and removing invalid py_library and py_test targets by checking their sources against all regular files on disk, whereas previously only py_binary targets were validated. Feedback on the changes points out a critical issue where __main__.py might be incorrectly flagged as an invalid source for py_binary targets, and provides a suggestion to ensure it is added to the validation map if present.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +596 to +605
// allFilesMap extends validFilesMap with all regular files on disk.
// py_binary uses validFilesMap (main modules + generated files), while py_library
// and py_test use allFilesMap since any file is a valid src for them.
allFilesMap := make(map[string]struct{}, len(validFilesMap)+len(args.RegularFiles))
for file := range validFilesMap {
allFilesMap[file] = struct{}{}
}
for _, file := range args.RegularFiles {
allFilesMap[file] = struct{}{}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

If __main__.py is present in the package, it is the standard entry point for py_binary targets. However, since __main__.py is not added to validFilesMap (which only contains main modules detected in appendPyLibrary and generated files), any existing py_binary target (including custom ones) that uses __main__.py as a source will be incorrectly marked as invalid and deleted by Gazelle.

We should ensure __main__.py is added to validFilesMap if it exists in args.RegularFiles.

	// allFilesMap extends validFilesMap with all regular files on disk.
	// py_binary uses validFilesMap (main modules + generated files), while py_library
	// and py_test use allFilesMap since any file is a valid src for them.
	allFilesMap := make(map[string]struct{}, len(validFilesMap)+len(args.RegularFiles))
	for file := range validFilesMap {
		allFilesMap[file] = struct{}{}
	}
	for _, file := range args.RegularFiles {
		allFilesMap[file] = struct{}{}
		if file == pyBinaryEntrypointFilename {
			validFilesMap[pyBinaryEntrypointFilename] = struct{}{}
		}
	}

@taowang487 taowang487 Jun 9, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this comment is correct. Also added a test case for __main__.py in commit ad0e48c and the test can pass.

This target is not deleted by Gazelle

py_binary(
    name = "remove_invalid_per_file_bin",
    srcs = ["__main__.py"],
    main = "__main__.py",
    visibility = ["//:__subpackages__"],
)

@taowang487 taowang487 force-pushed the taowang/gazelle-stale-target branch from e6a29c2 to 805dc5d Compare June 9, 2026 18:15
@taowang487 taowang487 changed the title Taowang/gazelle stale target Gazelle: Delete stale py_library and py_test targets Jun 9, 2026
@taowang487 taowang487 marked this pull request as ready for review June 9, 2026 18:22
taowang487 and others added 2 commits June 9, 2026 11:32
…st removal

Add remove_invalid_library_file_mode: verifies that a py_library target
whose source file was deleted is removed in per-file generation mode.

Add remove_invalid_test: verifies that a py_test target whose source
file was deleted is removed.

Both tests currently fail without the corresponding fix to
getRulesWithInvalidSrcs.

Co-Authored-By: Tao Wang <watao@microsoft.com>
…g srcs

getRulesWithInvalidSrcs only checked py_binary targets for stale source
files. In per-file generation mode, deleting a .py file left orphaned
py_library and py_test targets in BUILD files, causing build failures.

Extend the function to also check py_library and py_test rules. Uses
allFilesMap (all regular files on disk) for these kinds, while py_binary
continues using validFilesMap (main modules only). Rules with no srcs
attr (e.g. deps-only libraries) are left untouched.

Co-Authored-By: Tao Wang <watao@microsoft.com>
@taowang487 taowang487 force-pushed the taowang/gazelle-stale-target branch from 805dc5d to d32fb7b Compare June 9, 2026 18:34

@dougthor42 dougthor42 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Please also update CHANGELOG.md

srcs = ["bar_test.py"],
)

py_test(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some test cases for aliased and mapped rules. For example, someone who has # gazelle:alias_kind my_py_test py_test or # gazelle:map_kind py_binary my_py_binary //bazel_wrappers:python.

It should be OK because you're using kindMatches, but I'd prefer the explicit test.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for calling this out! I added this test but surprisingly it failed.
This seems to be a bug in Gazelle, and I created a draft Gazelle PR for it bazel-contrib/bazel-gazelle#2362
I verified with the Gazelle fix the alias tests can pass (stack rules_python PR taowang487#1)

Let's keep this PR focus on fixing the py_library and py_test and let's fix the alias staleness in follow up PRs.

@taowang487 taowang487 requested a review from rickeylev as a code owner June 10, 2026 17:05
@taowang487

Copy link
Copy Markdown
Author

@dougthor42 Thanks for the review! I updated the changelog and replied to the comments. Can you take another look? Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants