Skip to content

fix(gazelle) Delete python targets with invalid srcs #3046

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

fix(gazelle) Delete python targets with invalid srcs #3046

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
03ab5e6
Remove invalid srcs in gazelle
yushan8 Jul 1, 2025
13d5396
Update with invalid sources
yushan8 Jul 1, 2025
c573eb3
Update
yushan8 Jul 1, 2025
31dfb88
Update with py_binary kind only
yushan8 Jul 15, 2025
15d4b62
address comments
yushan8 Jul 15, 2025
0485e5f
update
yushan8 Jul 15, 2025
43a48bd
update test
yushan8 Jul 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 43 additions & 2 deletions gazelle/python/generate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import (
"github.com/emirpasic/gods/sets/treeset"
godsutils "github.com/emirpasic/gods/utils"


"github.com/bazel-contrib/rules_python/gazelle/pythonconfig"
)

Expand Down Expand Up @@ -225,7 +226,6 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes

var result language.GenerateResult
result.Gen = make([]*rule.Rule, 0)

collisionErrors := singlylinkedlist.New()

appendPyLibrary := func(srcs *treeset.Set, pyLibraryTargetName string) {
Expand Down Expand Up @@ -473,7 +473,10 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes
result.Gen = append(result.Gen, pyTest)
result.Imports = append(result.Imports, pyTest.PrivateAttr(config.GazelleImportsKey))
}

if !cfg.CoarseGrainedGeneration() {
emptyRules := py.getRulesWithInvalidSrcs(args)
result.Empty = append(result.Empty, emptyRules...)
}
if !collisionErrors.Empty() {
it := collisionErrors.Iterator()
for it.Next() {
Expand All @@ -485,6 +488,44 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes
return result
}

// getRulesWithInvalidSrcs checks existing Python rules in the BUILD file and return the rules with invalid srcs.
func (py *Python) getRulesWithInvalidSrcs(args language.GenerateArgs) (invalidRules []*rule.Rule) {
if args.File == nil {
return
}
filesMap := make(map[string]struct{})
for _, file := range args.RegularFiles {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about those python files in subdirs? Thoughts on my previous comment about reading mainModules instead of args.RegularFiles?

filesMap[file] = struct{}{}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I should have caught this earlier, but I don't think we should read args.RegularFiles here. There is a lot of filtering logic at the beginning of GenerateRules that should also be applied here:

rules_python/gazelle/python/generate.go

Lines 116 to 132 in cab415d

if cfg.IgnoresFile(filepath.Base(f)) {
continue
}
ext := filepath.Ext(f)
if ext == ".py" {
pyFileNames.Add(f)
if !hasPyBinaryEntryPointFile && f == pyBinaryEntrypointFilename {
hasPyBinaryEntryPointFile = true
} else if !hasPyTestEntryPointFile && f == pyTestEntrypointFilename {
hasPyTestEntryPointFile = true
} else if f == conftestFilename {
hasConftestFile = true
} else if matchesAnyGlob(f, testFileGlobs) {
pyTestFilenames.Add(f)
} else {
pyLibraryFilenames.Add(f)
}

For example, if a py_binary only has excluded srcs, it should be cleaned up too.

There is also logic to collect py files from subdirs that is needed by project mode:

rules_python/gazelle/python/generate.go

Line 148 in cab415d

for _, d := range args.Subdirs {

Since we scope this PR down to py_binary only, the only "regular files" we care about are __main__.py and mainModules here:

rules_python/gazelle/python/generate.go

Line 236 in cab415d

allDeps, mainModules, annotations, err := parser.parse(srcs)

The "GenFiles" and "isTarget` logic can stay, because they are not handled anywhere else

for _, file := range args.GenFiles {
filesMap[file] = struct{}{}
}

isTarget := func(src string) bool {
return strings.HasPrefix(src, "@") || strings.HasPrefix(src, "//") || strings.HasPrefix(src, ":")
}
for _, existingRule := range args.File.Rules {
if existingRule.Kind() != pyBinaryKind {
continue
}
allInvalidSrcs := true
Copy link
Contributor

Choose a reason for hiding this comment

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

I would avoid double negative in the code. So instead of "allInvalidSrcs == false", it's easier to understand if we say "hasValidSrcs == true"

for _, src := range existingRule.AttrStrings("srcs") {
if _, ok := filesMap[src]; ok {
allInvalidSrcs = false
break
}
if isTarget(src) {
allInvalidSrcs = false
break
}
}
if allInvalidSrcs {
invalidRules = append(invalidRules, newTargetBuilder(existingRule.Kind(), existingRule.Name(), args.Config.RepoRoot, args.Rel, nil).build())
}
}
return invalidRules
}

// isBazelPackage determines if the directory is a Bazel package by probing for
// the existence of a known BUILD file name.
func isBazelPackage(dir string) bool {
Expand Down
7 changes: 7 additions & 0 deletions gazelle/python/testdata/remove_invalid_binary/BUILD.in
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
load("@rules_python//python:defs.bzl", "py_binary")

py_binary(
name = "remove_invalid_binary",
srcs = ["__main__.py"],
visibility = ["//:__subpackages__"],
)
Empty file added gazelle/python/testdata/remove_invalid_binary/BUILD.out
View file
Open in desktop
Empty file.
3 changes: 3 additions & 0 deletions gazelle/python/testdata/remove_invalid_binary/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# Remove invalid binary

This test case asserts that `py_binary` should be deleted if invalid (no source files).
1 change: 1 addition & 0 deletions gazelle/python/testdata/remove_invalid_binary/WORKSPACE
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
workspace(name = "remove_invalid_binary")
5 changes: 5 additions & 0 deletions gazelle/python/testdata/remove_invalid_binary/keep_binary/BUILD.in
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
py_binary(
name = "keep_target_binary",
srcs = ["//test/binary:__main__.py"],
visibility = ["//:__subpackages__"],
)
5 changes: 5 additions & 0 deletions gazelle/python/testdata/remove_invalid_binary/keep_binary/BUILD.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
py_binary(
name = "keep_target_binary",
srcs = ["//test/binary:__main__.py"],
visibility = ["//:__subpackages__"],
)
15 changes: 15 additions & 0 deletions gazelle/python/testdata/remove_invalid_binary/test.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.

---