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

Allow customizing Python stub script's shebang in the Python toolchain #8685

Closed
brandjon opened this issue Jun 20, 2019 · 13 comments
Closed
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Rules-Python Native rules for Python type: feature request

Comments

@brandjon
Copy link
Member

Currently the stub script requires (via its shebang) that the target system have a python command available on PATH. If the target is PY3, the user might reasonable expect that it work if python is not available but python3 is. We can modify the shebang in this case to be #!/usr/bin/env python3. However doing so might break if the system has a (Python 3) python command but not a python3 command.

See #8665 for a case where the shebang has been a problem in practice.

@brandjon brandjon added type: feature request P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Rules-Python Native rules for Python labels Jun 20, 2019
@brandjon
Copy link
Member Author

More generally, we can support customizing the shebang as part of the Python toolchain. I'm not clear on how useful this feature would be in the short to medium term. Long term we'd like to eliminate the Python stub altogether and replace it with a C++ one.

@brandjon brandjon changed the title Don't require python command for stub script for PY3 targets Allow customizing Python stub script's shebang in the Python toolchain May 1, 2020
@brandjon
Copy link
Member Author

brandjon commented May 1, 2020

Repurposing this bug to track the FR to customize the shebang string in the toolchain. That supersedes requests to make it work with a particular interpreter version / system environment xyz. See also #11269.

@ghost
Copy link

ghost commented May 14, 2020

@brandjon I'm not sure if the Bazel maintainers prefer to resolve this independent of issues like #8446 or #137, but I'm pursuing this internally and would be happy to generate a PR within the next day or two.

  • Any naming suggestions for the new py_runtime parameter?
  • Are PyRuntimeInfoTest test cases sufficient, or do I need to get down and dirty with python_stub_test.sh?

@chrislovecnm
Copy link

chrislovecnm commented Jul 30, 2020

@brandjon / @beasleyr-vmw what is the status on this issue? I just ran smack into it with trying to do a hermetic python build. When was this issue introduced?

@fahhem
Copy link
Contributor

fahhem commented Aug 3, 2020

Potentially fixed by: #6632

@lberki lberki added P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) and removed P3 We're not considering working on this, but happy to review a PR. (No assignee) labels Nov 18, 2020
@meteorcloudy
Copy link
Member

This issue shouldn't be P4 anymore since Python 2 has already been deprecated.

@meteorcloudy meteorcloudy added P2 We'll consider working on this in future. (Assignee optional) and removed P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) labels Apr 6, 2021
@meteorcloudy
Copy link
Member

@brandjon Is there any plan to fix this?

@stdbug
Copy link

stdbug commented May 10, 2021

Seems like the problem holds if --build_python_zip flag is used. At least when running bazel test (still puts #!/usr/bin/env python as shebang for the binary, everything is OK in the zip archive)

@meteorcloudy
Copy link
Member

@codeschlosser Are you testing this on Windows?
Probably the problem is in the python launcher for Windows:

if (GetBinaryPathWithoutExtension(python_binary) != L"python") {

@stdbug
Copy link

stdbug commented May 12, 2021

@meteorcloudy Just the opposite, it works without an issue on Windows, I faced the problem on Debian 10. This change seems to resolve it (checked within a Docker container with hermetic python and without system-provided interpreter)

@meteorcloudy
Copy link
Member

@codeschlosser Thanks! Can you send this change as a PR 😃

@EdSchouten
Copy link
Contributor

One downside of the current approach is that in the case of remote execution, workers need to have a copy of Python present, even if you've got a hermetic Python toolchain declared. This leads to situations where people accidentally invoke the wrong copy of Python. To work around this issue, I have written a small Go tool that can be installed as "python" in $PATH. It parses the Python stub script, relaunching it with the copy of Python stored in the input root.

Source code: https://github.com/buildbarn/bb-remote-execution/blob/master/cmd/fake_python/main.go
Precompiled versions can be obtained by looking at the GitHub Actions page for that project.

katre pushed a commit that referenced this issue Jul 12, 2021
Added a `stub_shebang` to `py_runtime` to allow users to specify a custom
"shebang" expression used by the Python stub script.  Motivation is to
support environments where `#!/usr/bin/env python` isn't valid (ex:
no `/usr/bin/python`, but there's a `/usr/bin/python3`).

Closes #8685.

### Guided tour
- Added field to `PyRuntimeInfoApi` inc. default value.
- Added field to `PyRuntimeInfo` provider inc. tests.
- Added field to `py_runtime` Starlark rule inc tests.
- Replaced static `#!/usr/bin/env python` in the stub w/ `%shebang%`.
- There are a few redundancies w/r/t declaring defaults and documentation.
  This is because there are a few separate public APIs (ex:
  `PyRuntimeInfo(...)`, `py_runtime(...)`), and I want to make sure defaults
  appear in the generated docs.

Testing Done:
- `bazelisk test src/test/java/com/google/devtools/build/lib/bazel/rules/python/...`
  `bazelisk test src/test/java/com/google/devtools/build/lib/rules/python/...`

Closes #11434.

PiperOrigin-RevId: 370622012
katre pushed a commit to katre/bazel that referenced this issue Jul 13, 2021
Added a `stub_shebang` to `py_runtime` to allow users to specify a custom
"shebang" expression used by the Python stub script.  Motivation is to
support environments where `#!/usr/bin/env python` isn't valid (ex:
no `/usr/bin/python`, but there's a `/usr/bin/python3`).

Closes bazelbuild#8685.

### Guided tour
- Added field to `PyRuntimeInfoApi` inc. default value.
- Added field to `PyRuntimeInfo` provider inc. tests.
- Added field to `py_runtime` Starlark rule inc tests.
- Replaced static `#!/usr/bin/env python` in the stub w/ `%shebang%`.
- There are a few redundancies w/r/t declaring defaults and documentation.
  This is because there are a few separate public APIs (ex:
  `PyRuntimeInfo(...)`, `py_runtime(...)`), and I want to make sure defaults
  appear in the generated docs.

Testing Done:
- `bazelisk test src/test/java/com/google/devtools/build/lib/bazel/rules/python/...`
  `bazelisk test src/test/java/com/google/devtools/build/lib/rules/python/...`

Closes bazelbuild#11434.

PiperOrigin-RevId: 370622012
katre pushed a commit to katre/bazel that referenced this issue Jul 13, 2021
Added a `stub_shebang` to `py_runtime` to allow users to specify a custom
"shebang" expression used by the Python stub script.  Motivation is to
support environments where `#!/usr/bin/env python` isn't valid (ex:
no `/usr/bin/python`, but there's a `/usr/bin/python3`).

Closes bazelbuild#8685.

### Guided tour
- Added field to `PyRuntimeInfoApi` inc. default value.
- Added field to `PyRuntimeInfo` provider inc. tests.
- Added field to `py_runtime` Starlark rule inc tests.
- Replaced static `#!/usr/bin/env python` in the stub w/ `%shebang%`.
- There are a few redundancies w/r/t declaring defaults and documentation.
  This is because there are a few separate public APIs (ex:
  `PyRuntimeInfo(...)`, `py_runtime(...)`), and I want to make sure defaults
  appear in the generated docs.

Testing Done:
- `bazelisk test src/test/java/com/google/devtools/build/lib/bazel/rules/python/...`
  `bazelisk test src/test/java/com/google/devtools/build/lib/rules/python/...`

Closes bazelbuild#11434.

PiperOrigin-RevId: 370622012
@jheaff1
Copy link
Contributor

jheaff1 commented May 2, 2022

One downside of the current approach is that in the case of remote execution, workers need to have a copy of Python present, even if you've got a hermetic Python toolchain declared. This leads to situations where people accidentally invoke the wrong copy of Python. To work around this issue, I have written a small Go tool that can be installed as "python" in $PATH. It parses the Python stub script, relaunching it with the copy of Python stored in the input root.

Source code: https://github.com/buildbarn/bb-remote-execution/blob/master/cmd/fake_python/main.go Precompiled versions can be obtained by looking at the GitHub Actions page for that project.

Is there any way for Bazel to invoke your fake python without having to manually add it to the PATH? I’m trying to make my builds as hermetic as I can

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Rules-Python Native rules for Python type: feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants