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

Fix shebangs to #!/usr/bin/env bash in all .sh files #535

Merged
merged 2 commits into from
May 13, 2024

Conversation

cheriimoya
Copy link
Contributor

This allows all scripts to be run without hardcoding /bin/bash as an interpreter.
Maybe this is an improvement, maybe not.
Someone should take a look at https://unix.stackexchange.com/a/29620 and document a decision for/against /bin/(ba)sh vs /usr/bin/env bash

Checklist:

  • I added a summary of any user-facing changes (compared to the last release) in the changelog-entries/<PRnumber>.md. -> will probably not affect many users (at least not any users using debian/ubuntu)
  • I will remember to squash-and-merge, providing a useful summary of the changes of this PR.

@MakisH MakisH self-assigned this May 6, 2024
Copy link
Member

@davidscn davidscn left a comment

Choose a reason for hiding this comment

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

Maybe this is an improvement, maybe not.

I would be in favor of using env for this purpose. We had already similar issues in ASTE. Problem: on compute clusters, the hardcoded path might not lead to the version you actually want to use. Here, env takes care of this.

The only change I am not sure about is that you changed the requirement from a POSIX shell to bash. Why not using #!/urs/bin/env sh instead?

breaking-dam-2d/fluid-openfoam/clean.sh Outdated Show resolved Hide resolved
@MakisH
Copy link
Member

MakisH commented May 7, 2024

I thought of this while first creating all these scripts (or at least when making them consistent, in their current version), but I don't remember why I decided to keep the hard-coded path.

If this has already triggered issues, then let's indeed change it. The version argument is a valid one, apparently not only for clusters, but also for macOS.

However, I agree with @davidscn: I chose sh instead of bash as the most portable option, and we have invested some effort to keep scripts compatible with sh. Remember that some people use zsh, some csh, some ksh. Not all shells are compatible with bash. Let's keep it as sh.

@cheriimoya cheriimoya force-pushed the fix/shebangs branch 2 times, most recently from edc68b8 to 96bfbd5 Compare May 10, 2024 06:57
@cheriimoya
Copy link
Contributor Author

Okay, so this time it's using #!/usr/bin/env sh everywhere

@cheriimoya
Copy link
Contributor Author

But interesting: Look at the failing pipeline!

@@ -1,4 +1,4 @@
#!/bin/bash
#!/usr/bin/env sh
Copy link
Member

Choose a reason for hiding this comment

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

To be more concise with my previous comment: the way we call the shell should not alter the shell environment. Not all scripts are POSIX compatible.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with David: it is intentional that most scripts are sh but a few are bash. In any case, the kind of shell is not something we should change here.

@cheriimoya
Copy link
Contributor Author

cheriimoya commented May 13, 2024

Okay, I've split the commit in two parts. Now I got what you meant @davidscn :D
Also grep'ed for /urs;)

Commands I used for reference: sed 's~#!/bin/sh~#!/usr/bin/env sh~' $(rg '#!/bin/sh' -l) -i (rg is ripgrep which is roughly equivalent to grep -r)

@davidscn
Copy link
Member

Thanks!

@davidscn davidscn merged commit f6ae191 into precice:develop May 13, 2024
1 check passed
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