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.

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
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ END_UNRELEASED_TEMPLATE
* (pypi) Wheels with BUILD.bazel (or other special Bazel files) no longer
result in missing files at runtime
([#2782](https://github.com/bazel-contrib/rules_python/issues/2782)).
* (gazelle) Remove py_binary targets with invalid srcs. This includes files
that are not generated or regular files.

{#v0-0-0-added}
### Added
Expand Down
48 changes: 46 additions & 2 deletions gazelle/python/generate.go
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,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 +472,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(cfg, args)
result.Empty = append(result.Empty, emptyRules...)
}
if !collisionErrors.Empty() {
it := collisionErrors.Iterator()
for it.Next() {
Expand All @@ -485,6 +487,48 @@ 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 source files.
// Invalid source files are files that do not exist or not a target.
func (py *Python) getRulesWithInvalidSrcs(cfg *pythonconfig.Config, 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?

if cfg.IgnoresFile(filepath.Base(file)) {
continue
}
filesMap[file] = struct{}{}
}
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
}
hasValidSrcs := true
for _, src := range existingRule.AttrStrings("srcs") {
if isTarget(src) {
continue
}
if _, ok := filesMap[src]; ok {
continue
}
hasValidSrcs = false
break
}
Comment on lines +514 to +524
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
hasValidSrcs := true
for _, src := range existingRule.AttrStrings("srcs") {
if isTarget(src) {
continue
}
if _, ok := filesMap[src]; ok {
continue
}
hasValidSrcs = false
break
}
var hasValidSrcs bool
for _, src := range existingRule.AttrStrings("srcs") {
if isTarget(src) {
hasValidSrcs = true
break
}
if _, ok := filesMap[src]; ok {
hasValidSrcs = true
break
}
}

It should be something like this.

if !hasValidSrcs {
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
11 changes: 11 additions & 0 deletions gazelle/python/testdata/remove_invalid_binary/BUILD.in
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
load("@rules_python//python:defs.bzl", "py_binary")

py_library(
name = "keep_library",
deps = ["//keep_binary:foo"],
)
py_binary(
name = "remove_invalid_binary",
srcs = ["__main__.py"],
visibility = ["//:__subpackages__"],
)
6 changes: 6 additions & 0 deletions gazelle/python/testdata/remove_invalid_binary/BUILD.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
load("@rules_python//python:defs.bzl", "py_library")

py_library(
name = "keep_library",
deps = ["//keep_binary:foo"],
)
3 changes: 3 additions & 0 deletions gazelle/python/testdata/remove_invalid_binary/README.md
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).
Empty file.
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
load("@rules_python//python:defs.bzl", "py_binary", "py_library")

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

py_library(
name = "keep_binary",
srcs = ["foo.py"],
visibility = ["//:__subpackages__"],
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
load("@rules_python//python:defs.bzl", "py_binary", "py_library")

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

py_library(
name = "keep_binary",
srcs = ["foo.py"],
visibility = ["//:__subpackages__"],
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
if __name__ == "__main__":
print("foo")
Empty file.