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

Convert posix scripts to bash #71932

Closed
wants to merge 1 commit into from

Conversation

Daniel-Worrall
Copy link
Contributor

This addresses #31036

Additional PRs for this issue are in their respective repos:

Files not converted are:

  • src/tools/clippy/util/dev (This is deprecated)
  • src/tools/rust-installer/test/image1/bin/program
  • src/tools/rust-installer/test/image1/bin/program2
  • src/tools/rust-installer/test/image1/bin/oldprogram
  • src/tools/rust-installer/test/image1/bin/cargo

All except the first are single-lined files that hold just #!/bin/sh

@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 5, 2020
@Dylan-DPC-zz
Copy link

r? @Dylan-DPC

@rust-highfive rust-highfive assigned Dylan-DPC-zz and unassigned kennytm May 5, 2020
@Mark-Simulacrum
Copy link
Member

The internal scripts are probably fine to convert here but I'm not sure about the external-facing scripts (e.g. rust-gdb and configure) -- those will need some analysis of the platforms we support and whether bash is available on them out of the box.

@RalfJung
Copy link
Member

RalfJung commented May 5, 2020

The last activity in that issue was almost 3 years ago, and I see no clear team consensus for replacing #!/bin/sh by #!/bin/sh -- the last message is a plan/request for gathering more data.

So before changing anything I'd suggest we determine if there is consensus for the change.

@Daniel-Worrall
Copy link
Contributor Author

As for internal/external scripts, stdarch & rust-installer PRs hold internal script changes, and miri holds an external script change.

@raphaelcohn
Copy link

Please, for the love of everything, keep scripts as POSIX and not bash. bash is a pig, it's not as commonly installed as one might think and build systems should make as few assumptions of build environment as possible. Complex build environments are responsible for many, many defects over the years.

bash is not installed by default even on modern Linux setups, eg Alpine. It's got a nasty history of security defects, and, if building from scratch to make a reproducible system, makes far too many assumptions and requires far too many other dependencies.

@Mark-Simulacrum
Copy link
Member

I'm going to nominate this for the infra team, but I myself am inclined to close this -- I just don't think we're really in need of this change right now (to my knowledge the scripts have not faced any significant maintenance burden) and I don't think we have the time/ability to properly evaluate the breakage (or lack thereof) this could cause.

@Mark-Simulacrum Mark-Simulacrum added I-nominated T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels May 15, 2020
@Dylan-DPC-zz
Copy link

r? @Mark-Simulacrum

@Mark-Simulacrum
Copy link
Member

We discussed this at the last infrastructure team meeting and felt that there was not currently bandwidth in the team to properly evaluate this change. Specifically, we want to do a survey of various platforms to determine the best shell (if there is one) to use. It was also brought up that we would generally accept fixes to our shell code if there are minor changes that would make it compatible with a wider variety of shells, but that changing the shebang is unlikely to be helpful -- in particular, it seems like finding a shebang that will actually exist and be consistent across all platforms isn't likely.

With that in mind, I'm going to go ahead and close this PR. I personally would prefer to close the issue that spawned it as well, since I don't expect us to do anything here unless there's active interest in changing things (and then having an issue open seems like it brings no benefit). But I don't know that we have consensus on that so not going to close it.

Thanks @Daniel-Worrall!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants