Skip to content
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

fix: Fix per-file config interaction with one py_binary per main #1664

Merged
merged 3 commits into from
Jan 9, 2024
Merged
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
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ A brief description of the categories of changes:
* (toolchains) `py_runtime` can now take an executable target. Note: runfiles
from the target are not supported yet.

* (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
Expand Down
16 changes: 14 additions & 2 deletions gazelle/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -250,8 +250,20 @@ When no such entry point exists, Gazelle will look for a line like this in the t
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

Expand Down
35 changes: 23 additions & 12 deletions gazelle/python/generate.go
Original file line number Diff line number Diff line change
Expand Up @@ -225,23 +225,17 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes
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 {
Expand All @@ -262,6 +256,23 @@ 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 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).
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# gazelle:python_generation_mode file

# gazelle:resolve py numpy @pip//:numpy
# gazelle:resolve py pandas @pip//:pandas
Original file line number Diff line number Diff line change
@@ -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"],
)
Original file line number Diff line number Diff line change
@@ -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.
aignas marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# This is a Bazel workspace for the Gazelle test data.
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
import numpy
import pandas
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
import lib
import lib_and_main
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
def library_func():
print("library_func")


if __name__ == "__main__":
library_func()
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
import pandas

if __name__ == "__main__":
run()
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
import lib2

if __name__ == "__main__":
lib2.lib_and_main.library_func()
Original file line number Diff line number Diff line change
@@ -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
Loading