Skip to content

Conversation

oliverlee
Copy link
Contributor

This allows systems which don't have this location (e.g. NixOS) to run the scripts.

This allows systems which don't have this location (e.g. NixOS) to run
the scripts.
@oliverlee
Copy link
Contributor Author

oliverlee commented Sep 1, 2025

I suppose the failures are related to this, but I am not familiar with rules_rust:

# calling this script, which would make /usr/bin/ld unreachable. For example,
# rules_rust does not set PATH (unless the user explicitly sets PATH in env
# through attributes) [1] when calling rustc, and rustc does not replace an

@RyanTorok
Copy link

RyanTorok commented Sep 9, 2025

As you mostly pointed out already, using /usr/bin/env is a double-edged sword. On one hand, it's guaranteed to exist on all POSIX-compliant systems, so it will be present on, e.g. NixOS. On the other hand, it's entirely reliant on what the calling ruleset puts in $PATH.

I'm not too familiar with the internals of rules_rust either, but based solely on the comment you tagged above, it appears that rules_rust doesn't add /usr/bin to $PATH, which means /usr/bin/env won't find /usr/bin/bash, even though the latter exists.

I'm not sure if this is the cleanest solution possible, but if you really need bash for these scripts, you might consider having a small wrapper script that uses #!/bin/sh (which is required to exist on POSIX-compliant systems) and searches manually for the right shell. You could default to /bin/bash and then fall back to /usr/bin/env bash if the former doesn't exist. Then just invoke the existing script using the shell you find.

Hope this helps!

@fmeum
Copy link
Member

fmeum commented Sep 9, 2025

Would it be possible to rewrite these scripts so that they only require /bin/sh? Some of them are only relevant for development and could just use /usr/bin/env bash.

@RyanTorok
Copy link

RyanTorok commented Sep 9, 2025

It's really only the two cc_wrapper scripts that matter. Keeping the test scripts as #!/bin/bash is probably fine, unless you're planning on running CI on a non-FHS platform. The only other script besides those is llvm_checksums.sh, which should almost always be run manually, so it's also not a major issue.

I think rewriting the two cc_wrapper scripts to support #!/bin/sh seems like a reasonable choice. Note that different platforms use a different shell in the role of /bin/sh, e.g. many Linux distros including Ubuntu now use dash instead of bash as /bin/sh. Limiting the script to POSIX-standard features should hopefully avoid future headaches.

@oliverlee
Copy link
Contributor Author

Thanks for your comments.

It's really only the two cc_wrapper scripts that matter.

Probably only toolchain/cc_wrapper.sh.tpl matters. If toolchain/osx_cc_wrapper.sh.tpl is only used on macOS, we can probably assume /bin/bash exists.

I'm not sure I'll have time to look into this in the near term, so feel free to take over if you're interested.

RyanTorok pushed a commit to RyanTorok/toolchains_llvm that referenced this pull request Sep 10, 2025
Some Linux distributions, such as NixOS, do not provide /bin/bash. This
commit aims to make the LLVM toolchain compatible with these platforms
by only using POSIX-compatible features in the shell script and
replacing the shebang with #!/bin/sh .

This commit does not update the corresponding MacOS script, as all MacOS
builds should have /bin/bash.

Supercedes bazel-contrib#543.
RyanTorok pushed a commit to RyanTorok/toolchains_llvm that referenced this pull request Sep 10, 2025
Some Linux distributions, such as NixOS, do not provide /bin/bash. This
commit aims to make the LLVM toolchain compatible with these platforms
by only using POSIX-compatible features in the shell script and
replacing the shebang with #!/bin/sh .

This commit does not update the corresponding MacOS script, as all MacOS
builds should have /bin/bash.

Supercedes bazel-contrib#543.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants