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

Cleaning of shell scripts #5778

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

ElectreAAS
Copy link
Collaborator

@ElectreAAS ElectreAAS commented Jan 11, 2024

Shellcheck!

This PR includes several rounds of applying shellcheck to all the scripts in the repo.

Commit details

The first commit (faa02b2) was done by hand, and only adds shebangs to scripts that missed them.

The second & third commits (8273b4f & f3bfaaa) are the result of letting shellcheck auto-fix the problems it found. I still reviewed all changes (and removed some of them) by hand before committing.

The fourth commit (395322d) was done by hand again, applying changes that are potentially breaking. By this I mean that the previous behaviour was ambiguous / using a bespoke feature, and I clarified it, but that can mean misanderstanding.

Special notes

Most changes present here have to do with preventing unexpected globbing and word-splitting, which are classic problems in scripts ("oh this thing breaks when there are spaces in filenames, how odd!"). Some have to do with POSIX sh not defining certain things well or at all.

One change that is notably absent is eval $(opam env) which triggers a warning from shellcheck, but I'm pretty sure the 'weird' behaviour is indented.

Additional notes & context for reviewers

I came across some weird-looking scripts while trying to pinpoint the root cause of #5761. I figured it'd be easier to write correct fish scripts if I knew what the bash equivalents were, but my editor kept saying that everything was ambiguous.

I'll also note that I did not change src/state/shellscripts/prompt.sh at all because I just don't understand it. A comment there says "this script should be portable across all shells in common use.", but fish doesn't use PS1, and the script itself uses local variables that are undefined in POSIX sh.
If that's of interest to maintainers, I could write a prompt.fish to palliate this.

@ElectreAAS
Copy link
Collaborator Author

Note: the only "completely wrong" thing I encountered is this line in sandbox_exec.sh, where the argument to -z is always false due to literal strings. If this was intended to check if the directory existed (my best guess), I can add a fix for it in this PR or make a separate one, as it's a bugfix and not a refactoring.

Copy link
Member

@kit-ty-kate kit-ty-kate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me overall on quick first glance aside from adding the shebang to the files in src/state/shellscripts/* which i feel a bit icky about are those files are sourced directly anyway and in some cases it's not clear which shell is to be used or even where their locations is (it could be /usr/local/bin/bash on BSDs for example)

src/state/shellscripts/env_hook.sh Outdated Show resolved Hide resolved
src/state/shellscripts/complete.sh Outdated Show resolved Hide resolved
shell/re-patch.sh Outdated Show resolved Hide resolved
@kit-ty-kate
Copy link
Member

Note: the only "completely wrong" thing I encountered is this line in sandbox_exec.sh, where the argument to -z is always false due to literal strings. If this was intended to check if the directory existed (my best guess), I can add a fix for it in this PR or make a separate one, as it's a bugfix and not a refactoring.

If you can open a separate PR for that, that would be perfect indeed. Thanks a lot!

@ElectreAAS
Copy link
Collaborator Author

If you can open a separate PR for that, that would be perfect indeed. Thanks a lot!

-> #5780

@ElectreAAS
Copy link
Collaborator Author

ElectreAAS commented Jan 12, 2024

in some cases it's not clear which shell is to be used or even where their locations is (it could be /usr/local/bin/bash on BSDs for example)

I agree and could change offenders from /bin/bash to /usr/bin/env bash. This still isn't 100% portable (at least according to SO user CodeClown42), but it's probably better for BSDs as you mention.

In the end I agree with you, and reverted these changes in 735cdc1

Note: I also changed 2 scripts from sh to bash as they use bash-only features
Signed-off-by: Ambre Austen Suhamy <[email protected]>
Note: Special reviewing attention should go to line 35 in .github/scripts/main/solvers.sh as I am not sure if word splitting was intended here.
Signed-off-by: Ambre Austen Suhamy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants