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: make sys.executable work with script bootstrap #2409

Merged
merged 10 commits into from
Nov 21, 2024

Conversation

rickeylev
Copy link
Collaborator

@rickeylev rickeylev commented Nov 14, 2024

When --bootstrap_impl=script is used, PYTHONPATH is no longer used to set the import
paths, which means subprocesses no longer inherit the Bazel paths. This is generally a
good thing, but breaks when sys.executable is used to directly invoke the interpreter.
Such an invocation assumes the interpreter will have the same packages available and works
with the system_python bootstrap.

To fix, have the script bootstrap use a basic virtual env. This allows it to intercept
interpreter startup even when the Bazel executable isn't invoked. Under the hood, there's
two pieces to make this work. The first is a binary uses declare_symlink() to write a
relative-path based symlink that points to the underlying Python interpreter. The second
piece is a site init hook (triggered by a .pth file using an import line) performs
sys.path setup as part of site (import site) initialization.

Fixes #2169

@rickeylev rickeylev force-pushed the fix.script.boot.sys.exe branch 9 times, most recently from bd85a4c to d68262c Compare November 17, 2024 04:07
@aignas
Copy link
Collaborator

aignas commented Nov 19, 2024

FYI, I am curious if the next version of this and #2359 combined could make it easy to have a venv for any target in the monorepo. This would mimic what aspect has built for their users - a venv for any target so that IDE integration works smoothly.

@rickeylev rickeylev force-pushed the fix.script.boot.sys.exe branch 2 times, most recently from 3ea4bcc to 287b7bf Compare November 19, 2024 01:24
@rickeylev
Copy link
Collaborator Author

FYI, I am curious if the next version of this and #2359 combined could make it easy to have a venv for any target in the monorepo. This would mimic what aspect has built for their users - a venv for any target so that IDE integration works smoothly.

I'm not sure I follow. With this PR, each binary will have its own venv, so an editor could be pointed to that directly. Do you mean something like, you do a one time setup to point your editor to some "well known path", and then run bazel run @rules_python//tools/whatever --binary=//my:bin to "populate" that well known path? And thus, you can "switch" between the venvs your editor sees by running the bazel command?

@aignas
Copy link
Collaborator

aignas commented Nov 19, 2024

I think you understood it.

I was thinking that you could do either:

bazel run @rules_python//python:bin/activate --binary=//my:bin
vim

or tell VSCode or similar that the Python interpreter for //my:bin is in $(bazel run --run_under=echo @rules_python//python:bin/python --binary=//my:bin).

We could also symlink it to a well known location using https://github.com/buildbuddy-io/bazel_env.bzl

@rickeylev rickeylev marked this pull request as ready for review November 19, 2024 01:45
@rickeylev
Copy link
Collaborator Author

Ok, ready for review

python/private/py_executable_bazel.bzl Outdated Show resolved Hide resolved
python/private/py_executable_bazel.bzl Outdated Show resolved Hide resolved
python/private/py_executable_bazel.bzl Show resolved Hide resolved
python/private/py_executable_bazel.bzl Show resolved Hide resolved
python/private/site_init_template.py Show resolved Hide resolved
python/private/site_init_template.py Outdated Show resolved Hide resolved
python/private/site_init_template.py Outdated Show resolved Hide resolved
python/private/stage1_bootstrap_template.sh Outdated Show resolved Hide resolved
python/private/stage2_bootstrap_template.py Show resolved Hide resolved
python/private/stage2_bootstrap_template.py Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@rickeylev rickeylev left a comment

Choose a reason for hiding this comment

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

Addressed most, still have 1 or 2 left. Thanks!

python/private/py_executable_bazel.bzl Outdated Show resolved Hide resolved
python/private/py_executable_bazel.bzl Show resolved Hide resolved
python/private/py_executable_bazel.bzl Show resolved Hide resolved
python/private/site_init_template.py Show resolved Hide resolved
python/private/stage1_bootstrap_template.sh Outdated Show resolved Hide resolved
Copy link
Collaborator

@aignas aignas left a comment

Choose a reason for hiding this comment

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

OK, LGTM in general.

python/private/py_executable_bazel.bzl Show resolved Hide resolved
@rickeylev rickeylev added this pull request to the merge queue Nov 21, 2024
Merged via the queue into bazelbuild:main with commit 8ff4386 Nov 21, 2024
4 checks passed
@rickeylev rickeylev deleted the fix.script.boot.sys.exe branch November 21, 2024 04:51
@ewianda
Copy link
Contributor

ewianda commented Nov 21, 2024

❤️

@ewianda
Copy link
Contributor

ewianda commented Nov 22, 2024

hi @rickeylev this doesn't seem to be working, Not sure if it is my system or what.

~/projects/rules_python/examples/bzlmod (main =)
$ bazelisk test //tests:version_default_test  --@rules_python//python/config_settings:bootstrap_impl=script --cache_test_results=no
INFO: Invocation ID: ac4e384e-27c6-47c2-b5a6-6f6b35eb16e8
WARNING: Build option --enable_runfiles has changed, discarding analysis cache (this can be expensive, see https://bazel.build/advanced/performance/iteration-speed).
INFO: Analyzed target //tests:version_default_test (0 packages loaded, 3996 targets configured).
FAIL: //tests:version_default_test (see /home/ewianda/.cache/bazel/_bazel_ewianda/229007d1af36718f54c73697ae3aff83/execroot/_main/bazel-out/k8-fastbuild/testlogs/tests/version_default_test/test.log)
INFO: From Testing //tests:version_default_test:
==================== Test output for //tests:version_default_test:
ERROR: Python interpreter not found: /home/ewianda/.cache/bazel/_bazel_ewianda/229007d1af36718f54c73697ae3aff83/sandbox/linux-sandbox/47/execroot/_main/bazel-out/k8-fastbuild/bin/tests/version_default_test.runfiles/_main/tests/_version_default_test.venv/bin/python3
================================================================================
INFO: Found 1 test target...
Target //tests:version_default_test up-to-date:
  bazel-bin/tests/version_default_test
INFO: Elapsed time: 0.343s, Critical Path: 0.10s
INFO: 2 processes: 2 linux-sandbox.
INFO: Build completed, 1 test FAILED, 2 total actions
//tests:version_default_test                                             FAILED in 0.0s

@ewianda
Copy link
Contributor

ewianda commented Nov 22, 2024

Hmm actually it fails on WSL Ubuntu and is fine on a regular Linux Machine. Says a broken symbolic link

file  /home/ewianda/.cache/bazel/_bazel_ewianda/6091a940c516b5ac88940b423928af22/sandbox/linux-sandbox/7/execroot/_main/bazel-out/k8-fastbuild/bin/benchsci/test_pg13.runfiles/_main/benchsci/_test_pg13.venv/bin/python3
/home/ewianda/.cache/bazel/_bazel_ewianda/6091a940c516b5ac88940b423928af22/sandbox/linux-sandbox/7/execroot/_main/bazel-out/k8-fastbuild/bin/benchsci/test_pg13.runfiles/_main/benchsci/_test_pg13.venv/bin/python3: broken symbolic link to ../../../../../rules_python~~python~python_3_11_6_x86_64-unknown-linux-gnu/bin/python3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

plain invocation of python subprocess doesn't inherit sys.path for bootstrap_impl=script
3 participants