Wrapper script has some bashisms left #4410

matiasgarciaisaia · 2017-05-12T22:11:49Z

In #3809, the bin/crystal wrapper script changed from bash to sh, but there were some bashisms left. I've removed them (and the dependency on tput) so we can better support Alpine and any toaster brand that doesn't have Bash built-in :)

In particular, running docker build . stopped working, because it went through the if branch that used remove_path_item - and that was Bash-dependent.

The Docker image doesn't have tput available

There were some bashisms left that became a bug since crystal-lang#3809

bew · 2017-05-13T13:11:39Z

bin/crystal

 }

 ##############################################################################

 __has_colors() {
  local num_colors=$(tput colors 2>/dev/null)

-  if [ "$num_colors" -gt 2 ]; then
+  if [ "0$num_colors" -gt 2 ]; then


What is this 0 in front of $num_colors for ?

It may be a bit too hacky, yes.

The idea is that, if tput is not available (like when building in Docker), then $num_colors would be empty - and the [ will fail. This way, if $num_colors is empty, the string gets to be "0", while if it's not empty, "256" turns into "0256" - and still works.

I can change that to be a different test - ie, if it's empty, assume no color; else test for $num_colors - but I should get the dust off my shell scripting book (?)

Oh I see the trick! I think it's better to avoid tricks, readability should be king everywhere.

To check for empty string, you can use [ -z "$num_colors" ] 😃

Thanks! Just fixed this 👍

bew · 2017-05-13T13:17:36Z

bin/crystal

@@ -1,4 +1,4 @@
-#!/usr/bin/env sh
+#!/bin/sh


Not sure which is the best, the old one is more portable, as sh is not always in /bin/ on all systems (even if it's unlikely).

@bew this PR is basically a follow up to this one: #3809

I see. My comment is still valid (but could be discussed somewhere else if needed)

I thought /bin/sh was actually required by POSIX or something like that.

Is there any system in which you don't have /bin/sh available, but you have /usr/bin/env and sh in some other path?

I'm totally for rolling back to /usr/bin/env sh if it is true that /usr/bin/env is more standard than /bin/sh - but I wouldn't think that's true.

For example, checkbashisms won't recognize the script as an sh script if you keep the /usr/bin/env sh shebang.

As per this stackoverflow answer this is fine:

There are two programs whose location you can rely on on almost every unix variant: /bin/sh and /usr/bin/env.

bcardiff · 2017-05-13T15:29:34Z

bin/crystal

-  work="${work#:}"
-
-  echo "$work"
+  echo -n $path | awk -v RS=: -v ORS=: '$0 != "'$item'"' | sed 's/:$//'


I think we don't have a dependency to awk yet. Is it safe to assume it will be there on all distros? Is there no other lighter approach?

I came to it through this guide and the answer I put in the comment.

It doesn't seem doable without awk/sed/perl/tr - and I think awk is the less rare of them. But we can totally change it.

I don't have much experience with this minimalist systems - I've just targeted the Docker build and made sure it worked.

sed and awk are the most portable of these (they're in posix) so this should be fine also.

See crystal-lang#4410

RX14 · 2017-05-13T17:38:55Z

bin/crystal

@@ -1,4 +1,4 @@
-#!/usr/bin/env sh
+#!/bin/sh


As per this stackoverflow answer this is fine:

There are two programs whose location you can rely on on almost every unix variant: /bin/sh and /usr/bin/env.

RX14 · 2017-05-13T17:40:11Z

bin/crystal

-  work="${work#:}"
-
-  echo "$work"
+  echo -n $path | awk -v RS=: -v ORS=: '$0 != "'$item'"' | sed 's/:$//'


sed and awk are the most portable of these (they're in posix) so this should be fine also.

RX14 · 2017-05-23T20:19:16Z

Please can we hurry this: on stock debian I get ./bin/crystal: 117: [: Illegal number: on master.

See: https://jenkins.crystal-lang.org/job/crystal/job/feature%252Fjenkinsfile/28/execution/node/17/log/

matiasgarciaisaia · 2017-05-23T20:24:07Z

Thanks everyone for the feedback! 👍

matiasgarciaisaia added 2 commits May 12, 2017 19:03

Don't support colors if tput is not available

96f03e4

The Docker image doesn't have tput available

Wrapper script is POSIX sh compatible

9223caa

There were some bashisms left that became a bug since crystal-lang#3809

bew reviewed May 13, 2017

View reviewed changes

mverzilli added kind:bug topic:infrastructure labels May 13, 2017

bcardiff reviewed May 13, 2017

View reviewed changes

Non-hackish test for number of colors available

6f8f2f2

See crystal-lang#4410

RX14 approved these changes May 13, 2017

View reviewed changes

matiasgarciaisaia merged commit 5aa3b15 into crystal-lang:master May 23, 2017

mverzilli added this to the Next milestone May 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wrapper script has some bashisms left #4410

Wrapper script has some bashisms left #4410

matiasgarciaisaia commented May 12, 2017

bew May 13, 2017 •

edited

matiasgarciaisaia May 13, 2017

bew May 13, 2017

matiasgarciaisaia May 13, 2017

bew May 13, 2017

mverzilli May 13, 2017

bew May 13, 2017

matiasgarciaisaia May 13, 2017

matiasgarciaisaia May 13, 2017

RX14 May 13, 2017

bcardiff May 13, 2017

matiasgarciaisaia May 13, 2017

RX14 May 13, 2017

RX14 May 13, 2017

RX14 May 13, 2017

RX14 commented May 23, 2017 •

edited

matiasgarciaisaia commented May 23, 2017

		@@ -1,4 +1,4 @@
		#!/usr/bin/env sh
		#!/bin/sh

		@@ -1,4 +1,4 @@
		#!/usr/bin/env sh
		#!/bin/sh

Wrapper script has some bashisms left #4410

Wrapper script has some bashisms left #4410

Conversation

matiasgarciaisaia commented May 12, 2017

bew May 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RX14 commented May 23, 2017 • edited

matiasgarciaisaia commented May 23, 2017

bew May 13, 2017 •

edited

RX14 commented May 23, 2017 •

edited