feat: Support main_module with bootstrap_impl=system_python#3072
feat: Support main_module with bootstrap_impl=system_python#3072brandonchinn178 wants to merge 3 commits intobazel-contrib:mainfrom
Conversation
brandonchinn178
force-pushed
the
bchinn-main-module
branch
from
94428b0 to
c183928
Compare
brandonchinn178
force-pushed
the
bchinn-main-module
branch
2 times, most recently
from
9cc661a to
ffed3fb
Compare
rickeylev
left a comment
rickeylev
left a comment
There was a problem hiding this comment.
Thanks for the PR and test!
aside: if you grant maintainer permssions to edit, i can directly do some of the small things.
| name = "main_module_test", | ||
| srcs = ["main_module.py"], | ||
| bootstrap_impl = "script", | ||
| imports = ["."], |
There was a problem hiding this comment.
This needs to be kept because it ensures the correct entry is added to sys.path. Without it, it can inadvertently rely on the implicit behavior from the flag that adds repos to sys.path and the PYTHONSAFEPATH setting
There was a problem hiding this comment.
I can re-add it, but I don't understand the reasoning. IIUC, imports says "add this directory to PYTHONPATH", but we're specifying main_module with the full absolute path, so imports = ["."] isn't doing anything. Would imports = [] have the behavior you want?
| @@ -0,0 +1,9 @@ | |||
| """ | |||
| A shim to get `main_module` working with `bootstrap_impl=system_python`. | |||
There was a problem hiding this comment.
Instead of creating a shim like this, modify the bootstrap template. All it should really need is to populate a %main_module% variable and pass -m when invoking python. That should simplify things quite a bit.
There was a problem hiding this comment.
hm I could try it again, but it was difficult because then I'd have to update all the locations where %main% is used, since there isn't a main file anymore. It seemed easier to say "we're using the main file logic everywhere still, it just points to a shim"
There was a problem hiding this comment.
yeah, it seems like the notion of a main file is hardcoded in a lot of places in python_bootstrap_template.txt. It's also not immediately clear how the translation to -m <main_module> would work for the coverage path. The shim seems like the easiest solution, especially given this is a deprecated codepath and it should have a negligible performance impact
brandonchinn178
force-pushed
the
bchinn-main-module
branch
from
5791f3d to
6910f2b
Compare
brandonchinn178
force-pushed
the
bchinn-main-module
branch
from
6910f2b to
418a70c
Compare
|
@rickeylev Any updates? |
2 participants
main_modulewas added in #2671, but support was only added forbootstrap_impl=script. Despite only supportingbootstrap_impl=script, both the CHANGELOG and the documentation didn't mention that limitation, making it seem likemain_moduledoesn't work out-of-the-box (given thatbootstrap_impl=system_pythonis the default). See #2765 (comment).This PR adds support for
main_modulewithbootstrap_impl=system_python. This is categorized asfeatbecause it's new functionality, but it can also be conceptualized as afix, since it's bringing the feature in line with docs.