feat: Creating one py_binary per main module #1584
Conversation
|
Thanks for the PR, does this replace #1582? If so does it make to add some test cases from that PR in order to have coverage for:
|
|
I think #1582 addresses a different problem, but I added the test cases anyways. |
|
I didn't realize there is already an open PR for this. Yes, this reimplements that, without parsing the Python files again. It also looks at tokens instead of string matches, with would cover cases like |
gazelle/python/generate.go
Outdated
| if args.File != nil { | ||
| for _, t := range args.File.Rules { | ||
| if t.Name() == pyBinaryTargetName && t.Kind() != actualPyBinaryKind { | ||
| fqTarget := label.New("", args.Rel, pyBinaryTargetName) | ||
| log.Printf("failed to generate target %q of kind %q: "+ | ||
| "a target of kind %q with the same name already exists.", | ||
| fqTarget.String(), actualPyBinaryKind, t.Kind()) | ||
| continue mainModulesLoop | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: the readability of this code due to nesting and the code location declarations is getting a little hard to follow. If it was possible to reduce indenting and/or split out to separate functions, it would be great, but not blocking this PR.
There was a problem hiding this comment.
extract it to a new function and reuse it in all places with similar logic
| ) | ||
|
|
||
| py_library( | ||
| name = "binary_without_entrypoint", |
There was a problem hiding this comment.
should this be library_without_entrypoint?
There was a problem hiding this comment.
given it a better name
|
Thanks for doing this. I was thinking of adding this myself if I had not discovered your PR through one of mine. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
[This previous PR](#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.
[This previous PR](bazelbuild/rules_python#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.
[This previous PR](bazelbuild/rules_python#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.
Many existing Python repos don't use
__main__.pyto indicate the the main module. Instead, they put something like below in any Python files:This PR makes the Gazelle extension able to recognize main modules like this, when
__main__.pydoesn't exist. This reduces the need to create__main__.pywhen enabling Gazelle extensions in existing Python repos.The current behavior of creating single
py_binaryfor__main__.pyis preserved and takes precedence. So this is a backward-compatible change.Closes #1566.