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

Scripts and tools: Use #!/usr/bin/env bash instead of #!/bin/bash. #14831

Merged
merged 1 commit into from Dec 6, 2018

Conversation

Projects
None yet
8 participants
@vim88
Copy link
Contributor

commented Nov 28, 2018

As it was discussed in #13510, it is better to use #!/usr/bin/env bash instead of #!/bin/bash.

@practicalswift

This comment has been minimized.

Copy link
Member

commented Nov 28, 2018

Concept ACK

Take a look at how test/lint/lint-python-shebang.sh enforces the expected shebang for Python scripts.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Nov 28, 2018

I thought we had a linter to lint those, but maybe the linters aren't linting themselves.

@fanquake

This comment has been minimized.

Copy link
Member

commented Nov 29, 2018

utACK 50ec1c8

Checked that this is the only occurrence of #!/bin/bash in the code.

In future, please try and preface your commit message, and write a more descriptive message. i.e scripts: Use #!/usr/bin/env bash over obsoleted #!/bin/bash.

@ch4ot1c

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2018

This note could be added to developer-notes.md.

@practicalswift

This comment has been minimized.

Copy link
Member

commented Nov 30, 2018

@vim88 Could include the dev notes commit from #14846 here and keep these related changes to one PR?

Also, what about adding this:

diff --git a/test/lint/lint-python-shebang.sh b/test/lint/lint-python-shebang.sh
index 4ff87f0bf..3a023fd80 100755
--- a/test/lint/lint-python-shebang.sh
+++ b/test/lint/lint-python-shebang.sh
@@ -1,5 +1,5 @@
 #!/usr/bin/env bash
-# Shebang must use python3 (not python or python2)
+# Assert expected shebang lines

 export LC_ALL=C
 EXIT_CODE=0
@@ -10,4 +10,10 @@ for PYTHON_FILE in $(git ls-files -- "*.py"); do
         EXIT_CODE=1
     fi
 done
+for SHELL_FILE in $(git ls-files -- "*.sh"); do
+    if [[ $(head -n 1 "${SHELL_FILE}") != "#!/usr/bin/env bash" && $(head -n 1 "${SHELL_FILE}") != "#!/bin/sh" ]]; then
+        echo "Missing expected shebang \"#!/usr/bin/env bash\" or \"#!/bin/sh\" in ${SHELL_FILE}"
+        EXIT_CODE=1
+    fi
+done
 exit ${EXIT_CODE}

And then git mv test/lint/lint-python-shebang.sh test/lint/lint-shebang.sh.

That way we'll never have to think about this ever again :-)

@vim88

This comment has been minimized.

Copy link
Contributor Author

commented Nov 30, 2018

I agree that we should keep all these related changes in this PR.

@vim88

This comment has been minimized.

Copy link
Contributor Author

commented Nov 30, 2018

I commited here the DOC additions from #14846 about Scripts shebang to developer-notes.md.

I also added the linting part for Shell Scripts shebang. (Thanks @practicalswift!)

@vim88

This comment has been minimized.

Copy link
Contributor Author

commented Nov 30, 2018

It seems that the Travis CI build failed for "Lint", so the new linting part for Shell Scripts shebang works.

Missing expected shebang "#!/usr/bin/env bash" or "#!/bin/sh" in contrib/qos/tc.sh
Missing expected shebang "#!/usr/bin/env bash" or "#!/bin/sh" in src/qt/res/movies/makespinner.sh
^---- failure generated from test/lint/lint-shebang.sh

Should I add the shebangs to those two files in this PR?
Also, is #!/usr/bin/env bash or #!/bin/sh preffered?

@practicalswift

This comment has been minimized.

Copy link
Member

commented Dec 1, 2018

@vim88 Add shebangs to tc.sh and makespinner.sh too. They should have #!/usr/bin/env bash since they use bash features.

@vim88

This comment has been minimized.

Copy link
Contributor Author

commented Dec 1, 2018

I added the missing #!/usr/bin/env bash shebang to contrib/qos/tc.sh and src/qt/res/movies/makespinner.sh.

@practicalswift

This comment has been minimized.

Copy link
Member

commented Dec 1, 2018

utACK 114ff903b69513fbfb7a3fc7fd19e6ef6ffa5a7e, but please squash commits

Scripts and tools & Docs: Used #!/usr/bin/env bash instead of obsolet…
…e #!/bin/bash, added linting for .sh files shebang and updated the Developer Notes.

@vim88 vim88 force-pushed the vim88:proper_shebang branch to 688f665 Dec 2, 2018

@vim88

This comment has been minimized.

Copy link
Contributor Author

commented Dec 2, 2018

I squashed the commits.

@practicalswift

This comment has been minimized.

Copy link
Member

commented Dec 2, 2018

utACK 688f665

@dongcarl

This comment has been minimized.

Copy link
Contributor

commented Dec 3, 2018

Fantastic! utACK 688f665

@fanquake

This comment has been minimized.

Copy link
Member

commented Dec 3, 2018

re-utACK 688f665

@Empact

This comment has been minimized.

Copy link
Member

commented Dec 4, 2018

utACK 688f665

hebasto pushed a commit to hebasto/bitcoin that referenced this pull request Dec 6, 2018

Merge bitcoin#14831: Scripts and tools: Use #!/usr/bin/env bash inste…
…ad of #!/bin/bash.

688f665 Scripts and tools & Docs: Used #!/usr/bin/env bash instead of obsolete #!/bin/bash, added linting for .sh files shebang and updated the Developer Notes. (vim88)

Pull request description:

  As it was discussed in [bitcoin#13510](bitcoin#13510), it is better to use `#!/usr/bin/env bash` instead of `#!/bin/bash`.

Tree-SHA512: 25f71eb9a6a0cdc91568b5c6863205c5fe095f77a69e633503a2ac7805bd9013af8538e538c0c666ce96a28e3f43ce7a8df5f08d4ff007723bb588d85674f2da

@laanwj laanwj merged commit 688f665 into bitcoin:master Dec 6, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@vim88

This comment has been minimized.

Copy link
Contributor Author

commented Dec 6, 2018

Thank you all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.