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

Use more convenient and UNIX-agnostic shebang #45957

Merged
merged 1 commit into from
Dec 4, 2017

Conversation

dereckson
Copy link
Contributor

When using bash-specific features, scripts using env to call bash
are more convenient, as bash be installed in different places
according the OS.

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 13, 2017
@shepmaster shepmaster added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Nov 18, 2017
@shepmaster shepmaster self-assigned this Nov 18, 2017
@shepmaster
Copy link
Member

I'd be against doing this in such a limited fashion, considering that we use /bin/bash in so many other contexts:

$ git grep -h '/bin/bash' | sort | uniq -c
   1     useradd --shell /bin/bash -u $LOCAL_USER_ID -o -c "" -m user
  42 #!/bin/bash
   1 export SHELL=/bin/bash

We do have some precedent for using env though:

$ git grep -h '/usr/bin/env' | sort | uniq -c
  10 #!/usr/bin/env python
   1 #!/usr/bin/env python2.7
   1 #!/usr/bin/env rustx

I can't really speak to how dangerous changing all of the shebangs would be though.

@shepmaster
Copy link
Member

Can you explain a bit more about why you want to make this change?

@shepmaster shepmaster added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 18, 2017
@dereckson
Copy link
Contributor Author

The goal is to ease the testing suite on OpenBSD and other OS where bash isn't shipped by default.

If we switch progressively everything to pure POSIX, we can use the scripts everywhere on the out-of-the-box OS, without having to install bash.

@shepmaster
Copy link
Member

If we switch progressively everything to pure POSIX

I wondered if that was the underlying goal. Let's wait for a decision on #45958 then.

@shepmaster
Copy link
Member

shepmaster commented Nov 24, 2017

Based on alexcrichton's comment:

Yes updating the shebang is fine, [...]

I think it makes sense to update these, but I'd prefer that you'd update all the instances of /bin/bash.

@shepmaster
Copy link
Member

Ping from triage @dereckson — will you have an opportunity to update the other references soon?

When using bash-specific features, scripts using env to call bash
are more convenient, as bash be installed in different places
according the OS.

Same applies for other languages' interpreters.
@dereckson
Copy link
Contributor Author

Okay, so now this commit updates all the relevant shebang, ie the different bash and one python2.7 left.

The only not-env shebang scripts are now:

  • #!/bin/false, documented as invalid in header
  • #!/bin/sh, apparently universally installed at this path

@shepmaster
Copy link
Member

LGTM!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Dec 2, 2017

📌 Commit a4b4a73 has been approved by shepmaster

@kennytm kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 2, 2017
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Dec 3, 2017
…aster

Use more convenient and UNIX-agnostic shebang

When using bash-specific features, scripts using env to call bash
are more convenient, as bash be installed in different places
according the OS.
bors added a commit that referenced this pull request Dec 3, 2017
Rollup of 8 pull requests

- Successful merges: #45957, #46260, #46432, #46442, #46454, #46462, #46465, #46473
- Failed merges:
@bors
Copy link
Contributor

bors commented Dec 4, 2017

⌛ Testing commit a4b4a73 with merge fdfbcf8...

@bors bors merged commit a4b4a73 into rust-lang:master Dec 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

4 participants