fix: python bootstrap uses relative paths - #1444
Open
lf- wants to merge 1 commit into
Open
Conversation
This got us good at Mercury and took down some builds when we enabled
`default_allow_cache_upload = true`.
The primary bug: make_py_package_inplace writes a bootstrap script like
so:
```python
python = str(args.python)
if not os.path.isabs(python) and os.path.sep in python:
python = os.path.abspath(python)
new_data = data.replace("<PYTHON>", f"/usr/bin/env {python}")
```
Absolutizing the interpreter path causes this to be a cache poisoning
bug if ever uploaded. In our case, our python interpreter is an
artifact, so it gets made absolute, thus baking our GitHub actions home
directory into the uploaded actions.
This bug came from 5f5f692 (D79552056) which was endeavouring to make
calling PARs not rely on the working directory anymore which it did
successfully but via means it calls a "terrible idea".
Let's do this in a way that's not terrible.
Why didn't this hit Meta: the RE sandbox is always at `/re_cwd`, and
the Python interpreter is usually(?) at an absolute path.
This also fixes a bug where `allow_cache_upload` (via
`cxx_attrs_get_allow_cache_upload`) got dropped before hitting the PAR
actions, so it's not even possible to turn it off on known non-hermetic
actions.
The methodology behind this PR was that I told some agents to find how
Bazel fixes this Python bootstrapping problem and learn all the footguns
they hit.
One of their impls: https://github.com/bazel-contrib/rules_python/blob/16e167a5b269930f2f5000c4d9dc99b72f930f2b/python/private/pypi/venv_shebang_rewriter.sh
Here's a brief list:
1. `!#/bin/bash` is not compatible with NixOS: `#!/usr/bin/env bash` is
preferable.
2. Apparently Bazel has a bug with their wrappers' $0 where they got
symlink handling wrong: it needs to walk component by component.
3. `exec -a` is not POSIX, so `dash` doesn't implement it. We have to
use a `sh -c` trick instead.
You know the deal, the tests are vibed, you can throw them out or clean
them up as desired, since I can't run them without being at fb.
Fixes: facebook#1135
Fixes: facebook#1268
Contributor
|
This pull request has been imported. If you are a Meta employee, you can view this in D114816218. (Because this pull request was imported automatically, there will not be any future comments.) |
8Keep
approved these changes
Aug 5, 2026
8Keep
left a comment
Contributor
There was a problem hiding this comment.
Review automatically exported from Phabricator review in Meta.
Contributor
|
thanks for the PR @lf- ! I used this to "inspire" a slightly modified fix I'm landing internally now, focused on the relative paths issue. I didn't include the cache upload fix, as it's unrelated to the relative paths fix - we will look into that as a separate fix to follow shortly. |
meta-codesync Bot
pushed a commit
to facebook/buck2-prelude
that referenced
this pull request
Aug 5, 2026
Summary: `make_py_package_inplace.py` absolutizes the interpreter path before putting it in the bootstrapper's `#!` line. When the interpreter is a build artifact, buck2 hands us a project-root-relative path, so the output ends up holding whatever absolute path the *builder* had. The action's command line doesn't contain that path, so its cache key doesn't either: the artifact is not a function of its inputs, and anyone reading it back out of the cache gets a pex pointing at a directory that doesn't exist for them. Absolute and bare-word interpreters are untouched. For the artifact case the shebang becomes a `#!/bin/sh` trampoline that resolves the interpreter against the bootstrapper's own location, which is both relocatable and independent of cwd. It is a shell/Python polyglot so that Windows, which ignores `#!` and runs the bootstrapper as `python <pex>`, still parses the file. Upstream reports: facebook/buck2#1135 and facebook/buck2#1268. Inspired by facebook/buck2#1444 - this is a reworked version of that PR's first half (without the cache upload fix) Reviewed By: 8Keep Differential Revision: D114911001 fbshipit-source-id: fc20faf1729bf69618697a838bb8caffee9e0192
meta-codesync Bot
pushed a commit
that referenced
this pull request
Aug 5, 2026
Summary: `make_py_package_inplace.py` absolutizes the interpreter path before putting it in the bootstrapper's `#!` line. When the interpreter is a build artifact, buck2 hands us a project-root-relative path, so the output ends up holding whatever absolute path the *builder* had. The action's command line doesn't contain that path, so its cache key doesn't either: the artifact is not a function of its inputs, and anyone reading it back out of the cache gets a pex pointing at a directory that doesn't exist for them. Absolute and bare-word interpreters are untouched. For the artifact case the shebang becomes a `#!/bin/sh` trampoline that resolves the interpreter against the bootstrapper's own location, which is both relocatable and independent of cwd. It is a shell/Python polyglot so that Windows, which ignores `#!` and runs the bootstrapper as `python <pex>`, still parses the file. Upstream reports: #1135 and #1268. Inspired by #1444 - this is a reworked version of that PR's first half (without the cache upload fix) Reviewed By: 8Keep Differential Revision: D114911001 fbshipit-source-id: fc20faf1729bf69618697a838bb8caffee9e0192
This file contains hidden or 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
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.
This got us good at Mercury and took down some builds when we enabled
default_allow_cache_upload = true.The primary bug: make_py_package_inplace writes a bootstrap script like so:
Absolutizing the interpreter path causes this to be a cache poisoning bug if ever uploaded. In our case, our python interpreter is an artifact, so it gets made absolute, thus baking our GitHub actions home directory into the uploaded actions.
This bug came from 5f5f692 (D79552056) which was endeavouring to make calling PARs not rely on the working directory anymore which it did successfully but via means it calls a "terrible idea".
Let's do this in a way that's not terrible.
Why didn't this hit Meta: the RE sandbox is always at
/re_cwd, and the Python interpreter is usually(?) at an absolute path.This also fixes a bug where
allow_cache_upload(viacxx_attrs_get_allow_cache_upload) got dropped before hitting the PAR actions, so it's not even possible to turn it off on known non-hermetic actions.The methodology behind this PR was that I told some agents to find how Bazel fixes this Python bootstrapping problem and learn all the footguns they hit.
One of their impls: https://github.com/bazel-contrib/rules_python/blob/16e167a5b269930f2f5000c4d9dc99b72f930f2b/python/private/pypi/venv_shebang_rewriter.sh
Here's a brief list:
!#/bin/bashis not compatible with NixOS:#!/usr/bin/env bashor#!/bin/shis preferable.exec -ais not POSIX, sodashdoesn't implement it. We have to use ash -ctrick instead.You know the deal, the tests are vibed, you can throw them out or clean them up as desired, since I can't run them without being at fb.
Fixes: #1135
Fixes: #1268