Use correct shebangs on .sh files #16825

simonbyrne · 2016-06-08T13:22:50Z

Some of the shell files were using bash features with /bin/sh, others could have safely used /bin/sh. Now all .sh files pass checkbashisms.

Partially addresses #16819.

yuyichao · 2016-06-08T13:26:32Z

lgtm

simonbyrne · 2016-06-08T13:30:30Z

Here are the remaining warnings in the bash-using files:

├ checkbashisms -f filterArgs.sh 
possible bashism in filterArgs.sh line 7 (alternative test command ([[ foo ]] should be [ foo ])):
    if [[ $i == -L* ]]; then
possible bashism in filterArgs.sh line 7 (should be 'b = a'):
    if [[ $i == -L* ]]; then


├ checkbashisms -f install.sh
possible bashism in install.sh line 19 ($'...' should be "$(printf '...')"):
    DESTFILE=$(LC_ALL=C cp -va $SRC $DEST | sed -e $'s/ -> /\\\n/g' | tail -n 1)


├ checkbashisms -f relative_path.sh
possible bashism in relative_path.sh line 12 (alternative test command ([[ foo ]] should be [ foo ])):
if [[ ! "$source" == /* ]] || [[ ! "$target" == /* ]]; then
possible bashism in relative_path.sh line 12 (should be 'b = a'):
if [[ ! "$source" == /* ]] || [[ ! "$target" == /* ]]; then
possible bashism in relative_path.sh line 20 (alternative test command ([[ foo ]] should be [ foo ])):
while [[ "${target#$common_part/}" == "${target}" ]] && [[ "$common_part" != "/" ]]; do
possible bashism in relative_path.sh line 20 (should be 'b = a'):
while [[ "${target#$common_part/}" == "${target}" ]] && [[ "$common_part" != "/" ]]; do
possible bashism in relative_path.sh line 25 (alternative test command ([[ foo ]] should be [ foo ])):
    if [[ -z $result ]]; then
possible bashism in relative_path.sh line 32 (alternative test command ([[ foo ]] should be [ foo ])):
if [[ "$common_part" == "/" ]]; then
possible bashism in relative_path.sh line 32 (should be 'b = a'):
if [[ "$common_part" == "/" ]]; then
possible bashism in relative_path.sh line 42 (alternative test command ([[ foo ]] should be [ foo ])):
if [[ -n $result ]] && [[ -n $forward_part ]]; then
possible bashism in relative_path.sh line 44 (alternative test command ([[ foo ]] should be [ foo ])):
elif [[ -n $forward_part ]]; then
possible bashism in relative_path.sh line 46 (${foo:3[:1]}):
    result="${forward_part:1}"

tkelman · 2016-06-08T13:32:51Z

contrib/commit-name.sh

+#!/bin/sh
 # This file is a part of Julia. License is MIT: http://julialang.org/license

 last_tag=$(git describe --tags --abbrev=0)


not part of this issue/pr, but the next line really shouldn't say 0.4.0-dev any more. @yuyichao did you not port the improved Compat.jl version back to contrib?

I wasn't even aware of this script .....

You can use find to see all scripts.
$ cd julia
$ find . -name "*.sh"

./contrib/prepare_release.sh ./contrib/fixup-libstdc++.sh ./contrib/commit-name.sh ./contrib/fixup-libgfortran.sh ./contrib/filterArgs.sh ./contrib/travis_fastfail.sh ./contrib/install.sh ./contrib/mac/app/run-install-name-tool-change.sh ./contrib/mac/mac-gtk.sh ./contrib/windows/get_toolchain.sh ./contrib/windows/msys_build.sh ./contrib/windows/winrpm.sh ./contrib/relative_path.sh ./contrib/check-whitespace.sh ./src/flisp/bootstrap.sh ./test/perf/micro/java/setup.sh ./base/version_git.sh

git ls-files '*.sh' is a better bet, as otherwise you'll pick up scripts from dependencies

ghost · 2016-06-08T13:47:02Z

base/version_git.sh is missing a #!/bin/sh on the first line.

yuyichao · 2016-06-08T13:54:20Z

base/version_git.sh is invoked with sh base/version_git.sh directly but adding a shebang won't hurt either.

simonbyrne · 2016-06-08T14:06:45Z

Done. I've also imported the changes to commit-name.sh from Compat.jl

ghost · 2016-06-08T14:24:11Z

Scripts that still don't comply to standards and use bashisms:

contrib/filterArgs.sh
contrib/install.sh
contrib/relative_path.sh

Scripts that are portable:

contrib/prepare_release.sh
contrib/fixup-libstdc++.sh
contrib/commit-name.sh
contrib/fixup-libgfortran.sh
contrib/travis_fastfail.sh
contrib/mac/app/run-install-name-tool-change.sh
contrib/mac/mac-gtk.sh
contrib/windows/get_toolchain.sh
contrib/windows/msys_build.sh
contrib/windows/winrpm.sh
contrib/check-whitespace.sh
src/flisp/bootstrap.sh
test/perf/micro/java/setup.sh
base/version_git.sh

ghost · 2016-06-08T14:28:45Z

Found a caveat on Mac specific code. https://github.com/JuliaLang/julia/blob/master/contrib/mac/app/Makefile#L43

It's not very important, but also won't harm, to use /bin/sh here too. Just so the source is clean from "bash" and people don't get encouraged to use it.

For keeping scripts portable, I propose Travis automatically fails if a pull request has the word "bash" on it ;-)

tkelman · 2016-06-08T14:36:18Z

Or more realistically, set up CI for a system that doesn't have bash installed. We nominally support freebsd but I don't know who (if anyone) is regularly testing it lately, and I think because of these scripts you might need bash installed to build on freebsd anyway.

oh, and a useful github trick - when linking to specific lines of code, hit y and github will refresh the page but at a specific sha, that way the link will always point to what you meant even if code moves around

simonbyrne · 2016-06-08T14:46:54Z

I wouldn't worry about the mac code: bash is already installed on OS X.

I think it's more important to have working scripts (with correct shebangs) than worry too much about working bash-free: e.g. I have no idea how one would write filterArgs.sh in pure sh.

ghost · 2016-06-08T14:59:01Z

Or more realistically, set up CI for a system that doesn't have bash installed

I think a CI for Alpine Linux would be perfect for this, since it has neither glibc or bash installed by default (it uses musl libc and busybox ash). This would solve 2 problems (finding glibc-specific unportable code and bashisms). Alpine is also one damn small distribution.

yuyichao · 2016-06-08T14:59:45Z

I have no idea how one would write filterArgs.sh in pure sh.

It's possible https://wiki.ubuntu.com/DashAsBinSh but not without much longer code, additional dependencies on addition (yet "standard") tools and code that is much harder to understand.

simonbyrne · 2016-06-08T15:14:44Z

I guess we could call perl, since that is an official requirement, but that seems absurd.

ghost · 2016-06-08T15:41:27Z

Problem solved.
#16827

ghost · 2016-06-08T17:07:33Z

With #16829 I think contrib/install.sh becomes posix-compliant.

Last script that doesn't comply to standards and use bashism is contrib/relative_path.sh

ghost · 2016-06-08T18:27:33Z

Looks like this can be merged, all checks passed.

tkelman · 2016-06-08T18:51:24Z

#16827 means filterArgs.sh won't need to change

ghost · 2016-06-08T18:51:47Z

True, good catch.
install.sh too, I think. #16829

ghost · 2016-06-08T19:31:16Z

So the last remaining bashism is contrib/relative_path.sh.
That shell script comes from a stackoverflow answer.
But there are other answer that may be better suited.

One of @simonair solutions is a POSIX shell function. It works with a variety of paths, but does not clean multiple slashes or resolve symlinks. Would that be a problem for Julia's case? If so, we could work from simonair's shell script and try to add that feature.
http://stackoverflow.com/a/14914070

There's also Jens solution, which is pretty cool. But I haven't tested it.
http://stackoverflow.com/a/7023490

Do you think we could use either one of these solutions?

tkelman · 2016-06-08T19:48:31Z

(moved to the right pr)
Looks like contrib/relative_path.sh actually should not have been given the Julia license header since most of it was copied from stack overflow, and prior to this March stack overflow code is CC-BY-SA.

ghost · 2016-06-08T20:34:50Z

@simonbyrne can you please revert changes to filterArgs.sh and install.sh? We already got them covered, #16827 and #16829

And with this PR (after reverting those 2 changes) we get everything else covered, except for contrib/relative_path.sh

ghost · 2016-06-09T13:36:21Z

#16827 and #16829 got merged and contrib/relative_path.sh discussion is taking place at #16838

Now when this PR is merged (after changes to filterArgs.sh and install.sh are reverted by @simonbyrne since those files were already fixed) we will basically bashism-free!

Nice.

simonbyrne · 2016-06-09T13:43:36Z

Updated.

Switches shebangs that can safely use /bin/sh. Partially addresses #16819. Also updates commit-name.sh from Compat.jl

ghost · 2016-06-09T13:50:50Z

Great! I don't think we need to wait for Travis CI again, since this is the same patch minus changes to 2 files. Good to be merged.

tkelman · 2016-06-09T15:11:39Z

Are the shell scripts in contrib/mac even being used any more since 47dc275, or should we delete them? Gtk.jl is handled pretty well by Homebrew I'd say.

simonbyrne mentioned this pull request Jun 8, 2016

Use #!/bin/sh instead of #!/bin/bash for portability #16819

Closed

tkelman reviewed Jun 8, 2016
View reviewed changes

simonbyrne force-pushed the sb/bashisms branch from 31963cc to 47cb15e Compare June 8, 2016 14:02

simonbyrne force-pushed the sb/bashisms branch 2 times, most recently from e8d9804 to f4abb17 Compare June 9, 2016 13:43

Use /bin/sh shebangs on .sh files

c423adf

Switches shebangs that can safely use /bin/sh. Partially addresses #16819. Also updates commit-name.sh from Compat.jl

simonbyrne force-pushed the sb/bashisms branch from f4abb17 to c423adf Compare June 9, 2016 13:49

tkelman merged commit 8479012 into master Jun 9, 2016

tkelman deleted the sb/bashisms branch June 9, 2016 13:53

ghost mentioned this pull request Jun 9, 2016

testing/julia and its dependencies alpinelinux/aports#112

Closed

9 tasks

Uh oh!

Use correct shebangs on .sh files #16825

Use correct shebangs on .sh files #16825

Uh oh!

Conversation

simonbyrne commented Jun 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yuyichao commented Jun 8, 2016

Uh oh!

simonbyrne commented Jun 8, 2016

Uh oh!

tkelman Jun 8, 2016

Choose a reason for hiding this comment

Uh oh!

yuyichao Jun 8, 2016

Choose a reason for hiding this comment

Uh oh!

ghost Jun 8, 2016

Choose a reason for hiding this comment

Uh oh!

simonbyrne Jun 8, 2016

Choose a reason for hiding this comment

Uh oh!

ghost commented Jun 8, 2016

Uh oh!

yuyichao commented Jun 8, 2016

Uh oh!

simonbyrne commented Jun 8, 2016

Uh oh!

ghost commented Jun 8, 2016 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jun 8, 2016

Uh oh!

tkelman commented Jun 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

simonbyrne commented Jun 8, 2016

Uh oh!

ghost commented Jun 8, 2016

Uh oh!

yuyichao commented Jun 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

simonbyrne commented Jun 8, 2016

Uh oh!

ghost commented Jun 8, 2016

Uh oh!

ghost commented Jun 8, 2016

Uh oh!

ghost commented Jun 8, 2016

Uh oh!

tkelman commented Jun 8, 2016

Uh oh!

ghost commented Jun 8, 2016 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jun 8, 2016

Uh oh!

tkelman commented Jun 8, 2016

Uh oh!

ghost commented Jun 8, 2016

Uh oh!

ghost commented Jun 9, 2016

Uh oh!

simonbyrne commented Jun 9, 2016

Uh oh!

ghost commented Jun 9, 2016

Uh oh!

tkelman commented Jun 9, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

simonbyrne commented Jun 8, 2016 •

edited

Loading

ghost commented Jun 8, 2016 •

edited by ghost

Loading

tkelman commented Jun 8, 2016 •

edited

Loading

yuyichao commented Jun 8, 2016 •

edited

Loading

ghost commented Jun 8, 2016 •

edited by ghost

Loading