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

Replace /bin/bash with /bin/sh #3809

Merged
merged 2 commits into from May 10, 2017
Merged

Replace /bin/bash with /bin/sh #3809

merged 2 commits into from May 10, 2017

Conversation

TheLonelyGhost
Copy link
Contributor

@TheLonelyGhost TheLonelyGhost commented Dec 30, 2016

This also addresses idiosyncrasies with echo versus printf (see StackExchange answer about this).

Since bash is a more fully-featured shell and sh (in Ubuntu, this is aliased to /bin/dash) is lighter weight, it makes more sense to use sh to run the realpath script in ./bin/crystal. I've benchmarked it on my system and there's a very slight increase in performance. It also, in part, reduces dependencies to run crystal.

Changes were made to the escape sequence coloring to support more shells, also per StackExchange.

@drhuffman12
Copy link

Some bash vs sh vs [etc] comments are noted at: http://stackoverflow.com/questions/5725296/difference-between-sh-and-bash

@ysbaddaden
Copy link
Contributor

I'm all for using sh vs bash, which would drop a dependency on some platforms, e.g. Alpine, FreeBSD, OpenBSD.

@TheLonelyGhost
Copy link
Contributor Author

One thing to note is that it is still required to run ./bin/ci. I found it difficult to test since it checked for Travis-CI specific values and I didn't want to mutate the script too much for my purposes.

Since there were assumptions made that it was on MacOS and running an Ubuntu docker container, should we be concerned by the following lines?

  on_linux docker run \
    --rm \
    -u $(id -u) \
    -v $(pwd):/mnt \
    -w /mnt \
    -e LIBRARY_PATH="/opt/crystal/embedded/lib/" \
    -e CRYSTAL_CACHE_DIR="/tmp/crystal" \
    "jhass/crystal-build-$ARCH" \
    "$ARCH_CMD" /bin/bash -c "'$command'"

  on_osx PATH="/usr/local/opt/llvm/bin:\$PATH" \
    CRYSTAL_CACHE_DIR="/tmp/crystal" \
    /bin/bash -c "'$command'"

@ysbaddaden
Copy link
Contributor

bin/ci is, as the name implies, tied to the CI solution we use. You may directly call make spec, make std_spec or bin/crystal spec [path/to/spec] instead when developing locally.

@TheLonelyGhost
Copy link
Contributor Author

Let me clarify. I had concerns with us calling bash on the lines mentioned earlier if we're moving forward assuming bash isn't available on the target platform (linux, darwin, etc.). Should these be changed, or are we making the assumption that bash will always installed in the CI environment?

@ysbaddaden
Copy link
Contributor

We'll rework bin/ci when we leave Travis for another platform, where maybe we'll like to avoid bash... or not. It's one thing to require users to have bash installed to hack on Crystal (if we can avoid it, we should) it's another thing to have bash installed in the VMs running our automated test suite.

@TheLonelyGhost
Copy link
Contributor Author

I thought similarly, but also considered that the build environment containing bash might open us up for testing removal of the dependency entirely. If we say it's not required and some deeply nested script calls it, we won't know in the CI tests but users who don't have it installed will see things explode.

Personally, I view this as an acceptable risk. I just wanted to confirm that others viewed it similarly.

@miketheman
Copy link
Contributor

#codetriage

We appear to have left this conversation in agreement that sh is acceptable a replacement for bash in all modified scripts, leaving the CI-related bashisms in place for now.

Recommend for Merge, per 👍 in comments.

@RX14
Copy link
Contributor

RX14 commented May 10, 2017

Is there anything blocking this merge? I've reviewed this PR and it LGTM.

@mverzilli mverzilli merged commit 08c6e8f into crystal-lang:master May 10, 2017
@mverzilli
Copy link

mverzilli commented May 10, 2017

Nothing blocking it! Thanks everyone, especially @TheLonelyGhost for crafting the PR, @ysbaddaden for providing feedback, @miketheman for taking the time to triage it and @RX14 for reviewing and nudging it ❤️

Sorry @miketheman for the horrible omission before :(.

@mverzilli mverzilli added this to the Next milestone May 10, 2017
matiasgarciaisaia added a commit to matiasgarciaisaia/crystal that referenced this pull request May 12, 2017
There were some bashisms left that became a bug since crystal-lang#3809
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.

None yet

7 participants