Let Python stubs respect RUNFILES_* while finding the module space #14740
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
EdSchouten
force-pushed
the
eschouten/20220205-python-runfiles-dir
branch
4 times, most recently
from
0aa9bb0 to
538e2e0
Compare
EdSchouten
commented
Feb 16, 2022
src/main/java/com/google/devtools/build/lib/bazel/rules/python/python_stub_template.txt
Outdated
Show resolved
Hide resolved
sgowroji
added
the
awaiting-review
|
Friendly ping! |
|
Hi @brandjon; happen to have a couple of spare cycles to look into this? Thanks! |
EdSchouten
force-pushed
the
eschouten/20220205-python-runfiles-dir
branch
from
538e2e0 to
868279b
Compare
|
Hey @comius, would you be interested in reviewing this change for me? Thanks! |
|
Hmmm... The CI failures for "buildkite/bazel-bazel-github-presubmit/darwin-openjdk-11-xcode-shard-*" seem to be caused by infrastructure failure. The underlying system ran out of disk space? |
EdSchouten
force-pushed
the
eschouten/20220205-python-runfiles-dir
branch
from
868279b to
af4d5e5
Compare
EdSchouten
force-pushed
the
eschouten/20220205-python-runfiles-dir
branch
from
af4d5e5 to
e59eb0d
Compare
rickeylev
reviewed
Sep 20, 2022
src/main/java/com/google/devtools/build/lib/bazel/rules/python/python_stub_template.txt
Outdated
Show resolved
Hide resolved
src/main/java/com/google/devtools/build/lib/bazel/rules/python/python_stub_template.txt
Outdated
Show resolved
Hide resolved
src/main/java/com/google/devtools/build/lib/bazel/rules/python/python_stub_template.txt
Outdated
Show resolved
Hide resolved
src/main/java/com/google/devtools/build/lib/bazel/rules/python/python_stub_template.txt
Outdated
Show resolved
Hide resolved
EdSchouten
force-pushed
the
eschouten/20220205-python-runfiles-dir
branch
from
e59eb0d to
9b00616
Compare
|
Hi @rickeylev, I think I've addressed all of your comments. PTAL! |
EdSchouten
force-pushed
the
eschouten/20220205-python-runfiles-dir
branch
from
9b00616 to
73c2f4c
Compare
When invoking a py_binary() through an sh_binary() using the Bash runfiles library, the location of the py_binary() will be resolved from the runfiles manifest file. This means that the argv[0] of the Python stub script will not point to a location under the runfiles directory of the shell script, but to a location in Bazel's execroot. Normally this does not lead to any issues, as argv[0] + ".runfiles" happens to point to be a valid runfiles directory as well. It does become problematic when --nobuild_runfile_links is provided, as in that case only the outer shell script is guaranteed to have a runfiles directory; not the inner Python script. This change extends the Python stub template to also consider RUNFILES_DIR when no runfiles directory can be found. Even though it's not technically correct, we also attempt to derive the runfiles directory from RUNFILES_MANIFEST_FILE. I suspect that this is a necessity as long as py_binary()s cannot operate purely using a manifest file. They currently depend on a concrete instantiation of the runfiles directory. Fixes: bazelbuild#11997
EdSchouten
force-pushed
the
eschouten/20220205-python-runfiles-dir
branch
from
73c2f4c to
86521ab
Compare
rickeylev
approved these changes
Sep 26, 2022
rickeylev
added
awaiting-PR-merge
|
Ok, I think I set the labels up correctly here for the "import it into google" people to process it. If you don't hear back in a day that the import process has begun, tag me and I'll check it didn't fall into some void |
|
Hey @rickeylev, just tagging you because two days have passed. :D |
|
Hello @EdSchouten, Thank you for the patience. I will update once it gets merged. |
|
Just to give an update: I've been fighting with our internal windows test environment to make the test added to python_stub_test.sh pass for the last day or so. For whatever reason, it seems to be missing a lot of the supporting code/config that the github environment has. If I don't have it figured out by Monday, I'll just disable this test in our internal builds for now. |
|
Woo, got everything happy! Code is out for review internally; should land early next week. |
sgowroji
removed
the
awaiting-PR-merge
aiuto
pushed a commit
to aiuto/bazel
that referenced
this pull request
Oct 12, 2022
When invoking a py_binary() through an sh_binary() using the Bash runfiles library, the location of the py_binary() will be resolved from the runfiles manifest file. This means that the argv[0] of the Python stub script will not point to a location under the runfiles directory of the shell script, but to a location in Bazel's execroot. Normally this does not lead to any issues, as argv[0] + ".runfiles" happens to point to be a valid runfiles directory as well. It does become problematic when --nobuild_runfile_links is provided, as in that case only the outer shell script is guaranteed to have a runfiles directory; not the inner Python script. This change extends the Python stub template to also consider RUNFILES_DIR when no runfiles directory can be found. Even though it's not technically correct, we also attempt to derive the runfiles directory from RUNFILES_MANIFEST_FILE. I suspect that this is a necessity as long as py_binary()s cannot operate purely using a manifest file. They currently depend on a concrete instantiation of the runfiles directory. Fixes: bazelbuild#11997 Closes bazelbuild#14740. PiperOrigin-RevId: 478857199 Change-Id: I8cc6ea014bfd4b9ea2f1672e8e814ba38a5bf471
github-merge-queue bot
pushed a commit
to bazelbuild/rules_python
that referenced
this pull request
Sep 26, 2023
Pass the environment generated by the `runfiles` helper so that the Python launcher can find the runfiles correctly as implemented in bazelbuild/bazel#14740. Fixes #1426
github-merge-queue bot
pushed a commit
to bazelbuild/rules_python
that referenced
this pull request
Sep 26, 2023
Pass the environment generated by the `runfiles` helper so that the Python launcher can find the runfiles correctly as implemented in bazelbuild/bazel#14740. Fixes #1426
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When invoking a py_binary() through an sh_binary() using the Bash
runfiles library, the location of the py_binary() will be resolved from
the runfiles manifest file. This means that the argv[0] of the Python
stub script will not point to a location under the runfiles directory of
the shell script, but to a location in Bazel's execroot.
Normally this does not lead to any issues, as argv[0] + ".runfiles"
happens to point to be a valid runfiles directory as well. It does
become problematic when --nobuild_runfile_links is provided, as in that
case only the outer shell script is guaranteed to have a runfiles
directory; not the inner Python script.
This change extends the Python stub template to also consider
RUNFILES_DIR when no runfiles directory can be found. Even though it's
not technically correct, we also attempt to derive the runfiles
directory from RUNFILES_MANIFEST_FILE. I suspect that this is a
necessity as long as py_binary()s cannot operate purely using a manifest
file. They currently depend on a concrete instantiation of the runfiles
directory.
Fixes: #11997