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

dump-ice-to-disk/check.sh: convert needless bashism in a /bin/sh script. #119992

Closed
wants to merge 2 commits into from

Conversation

he32
Copy link
Contributor

@he32 he32 commented Jan 15, 2024

This script is marked with #! /bin/sh, but uses Bash-only constructs ("bashisms").
In this case there is a corresponding portable POSIX shell construct, and this
change converts to use that, instead of insisting that /bin/sh must be Bash.

@rustbot
Copy link
Collaborator

rustbot commented Jan 15, 2024

r? @Mark-Simulacrum

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

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 15, 2024
wip-sync pushed a commit to NetBSD/pkgsrc-wip that referenced this pull request Jan 15, 2024
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@Noratrieb
Copy link
Member

Noratrieb commented Jan 15, 2024

can we change it to a bash shebang instead? the bash looks less bad than the posix shell. I don't think it matters which shebang you choose exactly because I don't expect the test suite to actually use the shebang, but it's nice to have one for tools like shellcheck. I'd recommend /usr/bin/env bash

@Mark-Simulacrum
Copy link
Member

Kicking to get another CI run, last one seems to be a spurious failure.

I'm not sure bash shebang is really a delta here, I'm inclined to land as-is. Long-term, #40713 is the real solution to readability.

@Enselic Enselic added A-testsuite Area: The testsuite used to check the correctness of rustc T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 12, 2024
@Noratrieb
Copy link
Member

@rustbot author
waiting for you to address the feedback and just change the shebang

@rustbot rustbot 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 Mar 14, 2024
@he32
Copy link
Contributor Author

he32 commented Mar 14, 2024

can we change it to a bash shebang instead? the bash looks less bad than the posix shell. I don't think it matters which shebang you choose exactly because I don't expect the test suite to actually use the shebang, but it's nice to have one for tools like shellcheck. I'd recommend /usr/bin/env bash

Well. It's evident that you have different priorities than I do, and if this was my code, I would not do that. I would instead put priority on "standard compliance" and "portability", instead of prioritizing "it looks prettier". But this isn't my code or my call to make.

@Noratrieb
Copy link
Member

better idea: just rewrite the test in rust instead: #121876

@Dylan-DPC
Copy link
Member

Closing this pr as it's inactive and better off being rewritten as a rust test instead of this approach

@Dylan-DPC Dylan-DPC closed this Apr 24, 2024
@Dylan-DPC Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants