Remove BASH_BIN in favor of referencing bash via /usr/bin/env

[shinypb]

Type of change

Bug fix.

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 official python Docker image all have bash at /bin/bash, but surprisingly, GitHub Actions workers have it at /usr/bin/bash. As a result, containers built there end up with non-functional binaries. This can be worked around by providing entrypoints like ["bash", "<path to my py_binary>"] rather than ["<path to my py_binary>"], but it'd be ideal to not have to do this.

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 question for further discussion on the approach.

Test plan

• Manual testing required. I can put together a sample project if that's useful, but verifying this change just requires running a py_binary and ensuring that it starts up correctly on various host platforms.

[CLAassistant]

All committers have signed the CLA.

[shinypb]

@alexeagle I've got to wait to get corporate approval to sign the CLA, but I'd love a quick sniff-test from Aspect as to whether this is something you'd consider merging if you have some spare cycles. Assuming you're interested, please let me know what other changes and/or information would be required for this to move forward—this is my first attempt to contribute to an Aspect project and I'd like to be a good citizen.

[alexeagle]

Thanks for the explanations @shinypb !

The toolchain we reference here is the exec toolchain, meant for use when bash is running on the machine where Bazel invokes actions.

You're right that this isn't appropriate as a target toolchain - meaning we didn't include our own bash binary in the resulting output artifact, so looking up a runtime path to bash sounds correct to me.

Of course it's less hermetic, especially if you wanted a guarantee about the version of Bash so you could rely on associative arrays.

[shinypb]

Thank you for the clarification around the exec toolchain; I appreciate it. I'm still getting up to speed in this universe.

Is the concern about hermeticity a blocker, or would you like to proceed? If so, let me know what you'd like to see in this PR and I'd happily take a crack at it.

[shinypb]

Hi @alexeagle! Just wanted to do a friendly nudge; is this patch something you think would be useful for the broader community or should I close it out? (I just don't want to clutter your open PRs. 😅 )

[alexeagle]

Yes, sorry I'm just behind getting to this repo. I'd like to understand this one a bit more deeply before merging.

[shinypb]

Totally fine! I'm not in a rush (it's a very easy problem to work around on my end). I'm just sensitive to leaving open source folks with gigantic backlogs of PRs from strangers. 😂

FWIW, I have not encountered the same issue with containers built by rules_js!

[alexeagle]

Okay got it, there's no target toolchain to provide a bash interpreter, so we should rely on the runtime having one that /usr/bin/env can locate.