Skip to content

fix: changed shebang to make scripts more platform independent #2893

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

Merged
merged 1 commit into from
Apr 20, 2025

Conversation

Tygo-van-den-Hurk
Copy link
Contributor

See for example this thread on why its better to use #!/usr/bin/env bash instead of #!/bin/bash. Without this change these scripts will not run on some platforms. This is not a breaking change for the platforms it already currently works on.

PR check-list

  • Branch has a clean commit history
  • Additional tests are included, if changing behaviors/core code that is testable.
  • Proper Copyright + License headers added to applicable files (Generally, we stick to "The ZMK Contributors" for copyrights to help avoid churn when files get edited)
  • Pre-commit used to check formatting of files, commit messages, etc.
  • Includes any necessary documentation changes.

See for example this thread:

https://stackoverflow.com/questions/21612980/why-is-usr-bin-env-bash-superior-to-bin-bash

on why its better to use '#!/usr/bin/env bash' instead.
Without this change these scripts will not run on some
platforms. This is not a breaking change for the
platforms it already works on.
@Tygo-van-den-Hurk Tygo-van-den-Hurk requested review from a team as code owners March 21, 2025 21:40
@caksoylar
Copy link
Contributor

Out of curiosity, under what systems does it not work? I am aware of this practice but I'd like to learn when this becomes an issue in practice for bash.

@Tygo-van-den-Hurk
Copy link
Contributor Author

Out of curiosity, under what systems does it not work? I am aware of this practice but I'd like to learn when this becomes an issue in practice for bash.

Good question! On NixOS (my system) direct paths are almost always a problem. For example for me, bash located at:

$ which bash
/run/current-system/sw/bin/bash

Which is a symlink to:

/nix/store/f33kh08pa7pmy4kvsmsibda46sh46s66-bash-interactive-5.2p37/bin/bash

But it seems to also be an issue particularly on non-Linux systems. For example, on OpenBSD systems it's usually in /usr/local/bin, cause they install it as an optional package.

@nmunnich
Copy link
Contributor

Should this change also be applied to e.g. app/run-test.sh?

@Tygo-van-den-Hurk
Copy link
Contributor Author

No but good catch, this is cause that’s using sh instead of bash, and it’s defined in the POSIX standard that sh must be at /bin/sh. So any computer that understands shebangs will also have sh at that location, for any other program other then sh, such as bash this isn’t the case. Though technically #!/usr/bin/env sh would work.

@nmunnich
Copy link
Contributor

I'm now mildly curious about the differences in the script that caused one to end up with sh and the other with bash. The nitpicky part of me wants them all to be consistent. That's not something that should be considered at all for this PR, though.

@Tygo-van-den-Hurk
Copy link
Contributor Author

Haha no I get it. It feels cleaner if they’re all the same...

As for why, I think that’s cause people just use bash, so they put #!/bin/bash down. I don’t think they use any bash specific features though... so changing it to #!/bin/sh most likely wouldn’t break anything since they follow the same POSIX standard.

@caksoylar
Copy link
Contributor

There are a few bashisms used in the bash scripts, usually you need a bit more diligence when writing POSIX sh. In my experience, it isn't too hard to make them POSIX compatible but replacing some bash features like arrays can be quite annoying.

Copy link
Contributor

@petejohanson petejohanson left a comment

Choose a reason for hiding this comment

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

Thanks!

@petejohanson petejohanson merged commit 84772eb into zmkfirmware:main Apr 20, 2025
28 checks 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.

4 participants