fix: Fix per-file config interaction with one py_binary per main (#1664)
[This previous PR](https://github.com/bazelbuild/rules_python/pull/1584)
added the ability to make a `py_binary` target per file if `if __name__
== "__main__"` tokens were found in the file. This works great in the
default case, but when `python_generation_mode` is set to `file`, the
plugin now attempts to make both a `py_binary` and a `py_library` target
for each main file, which results in an error.
This PR modifies the behavior to work properly with per-file target
generation, and adds tests for this case.
diff --git a/CHANGELOG.md b/CHANGELOG.md
index af61b44..8bd56f1 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -57,6 +57,10 @@
from the target are not supported yet.
([#1612](https://github.com/bazelbuild/rules_python/issues/1612))
+* (gazelle) When `python_generation_mode` is set to `file`, create one `py_binary`
+ target for each file with `if __name__ == "__main__"` instead of just one
+ `py_binary` for the whole module.
+
### Fixed
* (gazelle) The gazelle plugin helper was not working with Python toolchains 3.11
diff --git a/gazelle/README.md b/gazelle/README.md
index a9a69cc..2e2337a 100644
--- a/gazelle/README.md
+++ b/gazelle/README.md
@@ -250,8 +250,20 @@
if __name == "__main__":
```
-Gazelle will create `py_binary` target will be created for every module with such line, with the target name
-being the same as module name.
+Gazelle will create a `py_binary` target for every module with such a line, with
+the target name the same as the module name.
+
+If `python_generation_mode` is set to `file`, then instead of one `py_binary`
+target per module, Gazelle will create one `py_binary` target for each file with
+such a line, and the name of the target will match the name of the script.
+
+Note that it's possible for another script to depend on a `py_binary` target and
+import from the `py_binary`'s scripts. This can have possible negative effects on
+Bazel analysis time and runfiles size compared to depending on a `py_library`
+target. The simplest way to avoid these negative effects is to extract library
+code into a separate script without a `main` line. Gazelle will then create a
+`py_library` target for that library code, and other scripts can depend on that
+`py_library` target.
## Developer Notes
diff --git a/gazelle/python/generate.go b/gazelle/python/generate.go
index 95f5396..ba273be 100644
--- a/gazelle/python/generate.go
+++ b/gazelle/python/generate.go
@@ -225,23 +225,17 @@
log.Fatalf("ERROR: %v\n", err)
}
- // Check if a target with the same name we are generating already
- // exists, and if it is of a different kind from the one we are
- // generating. If so, we have to throw an error since Gazelle won't
- // generate it correctly.
- if err := ensureNoCollision(args.File, pyLibraryTargetName, actualPyLibraryKind); err != nil {
- fqTarget := label.New("", args.Rel, pyLibraryTargetName)
- err := fmt.Errorf("failed to generate target %q of kind %q: %w. "+
- "Use the '# gazelle:%s' directive to change the naming convention.",
- fqTarget.String(), actualPyLibraryKind, err, pythonconfig.LibraryNamingConvention)
- collisionErrors.Add(err)
- }
-
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 {
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)
+ }
}
sort.Strings(mainFileNames)
for _, filename := range mainFileNames {
@@ -262,6 +256,23 @@
}
}
+ // If we're doing per-file generation, srcs could be empty at this point, meaning we shouldn't make a py_library.
+ if srcs.Empty() {
+ return
+ }
+
+ // Check if a target with the same name we are generating already
+ // exists, and if it is of a different kind from the one we are
+ // generating. If so, we have to throw an error since Gazelle won't
+ // generate it correctly.
+ if err := ensureNoCollision(args.File, pyLibraryTargetName, actualPyLibraryKind); err != nil {
+ fqTarget := label.New("", args.Rel, pyLibraryTargetName)
+ err := fmt.Errorf("failed to generate target %q of kind %q: %w. "+
+ "Use the '# gazelle:%s' directive to change the naming convention.",
+ fqTarget.String(), actualPyLibraryKind, err, pythonconfig.LibraryNamingConvention)
+ collisionErrors.Add(err)
+ }
+
pyLibrary := newTargetBuilder(pyLibraryKind, pyLibraryTargetName, pythonProjectRoot, args.Rel, pyFileNames).
addVisibility(visibility).
addSrcs(srcs).
diff --git a/gazelle/python/testdata/binary_without_entrypoint_per_file_generation/BUILD.in b/gazelle/python/testdata/binary_without_entrypoint_per_file_generation/BUILD.in
new file mode 100644
index 0000000..b24a823
--- /dev/null
+++ b/gazelle/python/testdata/binary_without_entrypoint_per_file_generation/BUILD.in
@@ -0,0 +1,4 @@
+# gazelle:python_generation_mode file
+
+# gazelle:resolve py numpy @pip//:numpy
+# gazelle:resolve py pandas @pip//:pandas
diff --git a/gazelle/python/testdata/binary_without_entrypoint_per_file_generation/BUILD.out b/gazelle/python/testdata/binary_without_entrypoint_per_file_generation/BUILD.out
new file mode 100644
index 0000000..bffedb1
--- /dev/null
+++ b/gazelle/python/testdata/binary_without_entrypoint_per_file_generation/BUILD.out
@@ -0,0 +1,46 @@
+load("@rules_python//python:defs.bzl", "py_binary", "py_library")
+
+# gazelle:python_generation_mode file
+
+# gazelle:resolve py numpy @pip//:numpy
+# gazelle:resolve py pandas @pip//:pandas
+
+py_library(
+ name = "lib",
+ srcs = ["lib.py"],
+ visibility = ["//:__subpackages__"],
+ deps = [
+ "@pip//:numpy",
+ "@pip//:pandas",
+ ],
+)
+
+py_library(
+ name = "lib2",
+ srcs = ["lib2.py"],
+ visibility = ["//:__subpackages__"],
+ deps = [
+ ":lib",
+ ":lib_and_main",
+ ],
+)
+
+py_binary(
+ name = "lib_and_main",
+ srcs = ["lib_and_main.py"],
+ visibility = ["//:__subpackages__"],
+)
+
+py_binary(
+ name = "main",
+ srcs = ["main.py"],
+ visibility = ["//:__subpackages__"],
+ deps = ["@pip//:pandas"],
+)
+
+py_binary(
+ name = "main2",
+ srcs = ["main2.py"],
+ visibility = ["//:__subpackages__"],
+ deps = [":lib2"],
+)
diff --git a/gazelle/python/testdata/binary_without_entrypoint_per_file_generation/README.md b/gazelle/python/testdata/binary_without_entrypoint_per_file_generation/README.md
new file mode 100644
index 0000000..9cbe3e9
--- /dev/null
+++ b/gazelle/python/testdata/binary_without_entrypoint_per_file_generation/README.md
@@ -0,0 +1,4 @@
+# Binary without entrypoint
+
+This test case asserts that when there is no __main__.py, a py_binary is generated per file main module, and that this
+py_binary is instead of (not in addition to) any py_library target.
diff --git a/gazelle/python/testdata/binary_without_entrypoint_per_file_generation/WORKSPACE b/gazelle/python/testdata/binary_without_entrypoint_per_file_generation/WORKSPACE
new file mode 100644
index 0000000..faff6af
--- /dev/null
+++ b/gazelle/python/testdata/binary_without_entrypoint_per_file_generation/WORKSPACE
@@ -0,0 +1 @@
+# This is a Bazel workspace for the Gazelle test data.
diff --git a/gazelle/python/testdata/binary_without_entrypoint_per_file_generation/lib.py b/gazelle/python/testdata/binary_without_entrypoint_per_file_generation/lib.py
new file mode 100644
index 0000000..3e1e6b8
--- /dev/null
+++ b/gazelle/python/testdata/binary_without_entrypoint_per_file_generation/lib.py
@@ -0,0 +1,2 @@
+import numpy
+import pandas
diff --git a/gazelle/python/testdata/binary_without_entrypoint_per_file_generation/lib2.py b/gazelle/python/testdata/binary_without_entrypoint_per_file_generation/lib2.py
new file mode 100644
index 0000000..592a2da
--- /dev/null
+++ b/gazelle/python/testdata/binary_without_entrypoint_per_file_generation/lib2.py
@@ -0,0 +1,2 @@
+import lib
+import lib_and_main
diff --git a/gazelle/python/testdata/binary_without_entrypoint_per_file_generation/lib_and_main.py b/gazelle/python/testdata/binary_without_entrypoint_per_file_generation/lib_and_main.py
new file mode 100644
index 0000000..c6e2d49
--- /dev/null
+++ b/gazelle/python/testdata/binary_without_entrypoint_per_file_generation/lib_and_main.py
@@ -0,0 +1,6 @@
+def library_func():
+ print("library_func")
+
+
+if __name__ == "__main__":
+ library_func()
diff --git a/gazelle/python/testdata/binary_without_entrypoint_per_file_generation/main.py b/gazelle/python/testdata/binary_without_entrypoint_per_file_generation/main.py
new file mode 100644
index 0000000..a068203
--- /dev/null
+++ b/gazelle/python/testdata/binary_without_entrypoint_per_file_generation/main.py
@@ -0,0 +1,4 @@
+import pandas
+
+if __name__ == "__main__":
+ run()
diff --git a/gazelle/python/testdata/binary_without_entrypoint_per_file_generation/main2.py b/gazelle/python/testdata/binary_without_entrypoint_per_file_generation/main2.py
new file mode 100644
index 0000000..6f923b8
--- /dev/null
+++ b/gazelle/python/testdata/binary_without_entrypoint_per_file_generation/main2.py
@@ -0,0 +1,4 @@
+import lib2
+
+if __name__ == "__main__":
+ lib2.lib_and_main.library_func()
diff --git a/gazelle/python/testdata/binary_without_entrypoint_per_file_generation/test.yaml b/gazelle/python/testdata/binary_without_entrypoint_per_file_generation/test.yaml
new file mode 100644
index 0000000..2410223
--- /dev/null
+++ b/gazelle/python/testdata/binary_without_entrypoint_per_file_generation/test.yaml
@@ -0,0 +1,17 @@
+# 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.
+
+---
+expect:
+ exit_code: 0
