Skip to content

Commit

Permalink
Remove BASH_BIN in favor of referencing bash via /usr/bin/env
Browse files Browse the repository at this point in the history
While using rules_py in combination with rules_oci, I discovered an unexpected
behaviour: rules_py generates #! shebang lines using the path to bin that the
sh toolchain uses. This has the unexpected side effect of baking in the bash
path from the machine that built the container, which may or may not be the
same bash path that the container itself will have.

I discovered that macOS, Ubuntu, and the default @python docker image all have
bash at `/bin/bash`, but surprisingly, GitHub Actions workers have it at
`/usr/bin/bash`.

We can side step this problem entirely by using `#!/usr/bin/env bash` as our
shebang line, which I believe to be sufficiently portable and robust to use
for rules_py's scripts. See [this Stack Overflow](https://stackoverflow.com/questions/21612980/why-is-usr-bin-env-bash-superior-to-bin-bash)
question for further discussion on the approach.
  • Loading branch information
Mark Christian committed Dec 7, 2023
1 parent e5c2112 commit 4e0755e
Show file tree
Hide file tree
Showing 4 changed files with 2 additions and 6 deletions.
2 changes: 1 addition & 1 deletion py/private/entry.tmpl.sh
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#!{{BASH_BIN}}
#!/usr/bin/env bash

{{BASH_RLOCATION_FN}}

Expand Down
2 changes: 0 additions & 2 deletions py/private/py_binary.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ load("//py/private:utils.bzl", "PY_TOOLCHAIN", "SH_TOOLCHAIN", "dict_to_exports"
load("//py/private/venv:venv.bzl", _py_venv = "py_venv_utils")

def _py_binary_rule_imp(ctx):
bash_bin = ctx.toolchains[SH_TOOLCHAIN].path
interpreter = resolve_toolchain(ctx)
main = ctx.file.main

Expand All @@ -35,7 +34,6 @@ def _py_binary_rule_imp(ctx):
python_interpreter_path = interpreter.python.path if interpreter.uses_interpreter_path else to_rlocation_path(ctx, interpreter.python)

common_substitutions = {
"{{BASH_BIN}}": bash_bin,
"{{BASH_RLOCATION_FN}}": BASH_RLOCATION_FUNCTION,
"{{BINARY_ENTRY_POINT}}": to_rlocation_path(ctx, main),
"{{INTERPRETER_FLAGS}}": " ".join(interpreter.flags),
Expand Down
2 changes: 0 additions & 2 deletions py/private/venv/venv.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ def _get_attr(ctx, attr, override):
return override

def _make_venv(ctx, name = None, strip_pth_workspace_root = None):
bash_bin = ctx.toolchains[SH_TOOLCHAIN].path
interpreter = resolve_toolchain(ctx)

name = _get_attr(ctx.attr, "name", name)
Expand Down Expand Up @@ -126,7 +125,6 @@ def _make_venv(ctx, name = None, strip_pth_workspace_root = None):
venv_directory = ctx.actions.declare_directory("%s.source" % name)

common_substitutions = {
"{{BASH_BIN}}": bash_bin,
"{{BASH_RLOCATION_FN}}": BASH_RLOCATION_FUNCTION,
"{{BAZEL_WORKSPACE_NAME}}": ctx.workspace_name,
"{{INTERPRETER_FLAGS}}": " ".join(interpreter.flags),
Expand Down
2 changes: 1 addition & 1 deletion py/private/venv/venv.tmpl.sh
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#!{{BASH_BIN}}
#!/usr/bin/env bash

USE_MANIFEST_PATH={{USE_MANIFEST_PATH}}

Expand Down

0 comments on commit 4e0755e

Please sign in to comment.