google / gitiles

A simple browser for Git repositories.
https://gerrit.googlesource.com/gitiles/
Other
582 stars 174 forks source link

./tools/run_dev.sh script uses of 'sh' results in 'Syntax Error: "(" unexpected' #98

Closed electrofelix closed 6 years ago

electrofelix commented 7 years ago

Following the Documentation/developer-guide.md covers using /path/to/gitiles/tools/run_dev.sh to test gitiles locally with git repos. Trying to use this locally to test a fix would fail with the following message:

Warning: ignoring http_proxy in environment.
INFO: Found 1 target...
Target //gitiles-dev:dev up-to-date:
  bazel-bin/gitiles-dev/dev.jar
  bazel-bin/gitiles-dev/dev
INFO: Elapsed time: 0.147s, Critical Path: 0.00s
/home/<user>/git/gitiles/bazel-bin/gitiles-dev/dev: 51: /home/<user>/git/gitiles/bazel-bin/gitiles-dev/dev: Syntax error: "(" unexpected

Eventually spotted that this was due to the bazel generated file /home//git/gitiles/bazel-bin/gitiles-dev/dev requiring bash due to it's use of an array on line 51:

JVM_FLAGS_CMDLINE=()

While the script /path/to/gitiles/tools/run_dev.sh uses sh to run this script. Unfortunately some Linux distros have sh linked to a different shell that bash that more closely matches the original bourne shell and do not support use of Bash arrays (Ubuntu with dash).

It appears that it would be a good idea to switch ./tools/run_dev.sh to use bash to run the script.

electrofelix commented 7 years ago

As mentioned in another bug, I have some work with my employer legal people to work out around signing the Googl CLA to upload this directly myself. However expecting that as it's not uncommon for my collegues to work on projects containing CLA's such as Kubernetes it should be just a case of confirmation that I can go ahead and sign it.

In the mean time here's the simple fix (pretty ridiculously simple) that worked for me

From a47d50eeba8525f6bfe7698a39239fc427828067 Mon Sep 17 00:00:00 2001
From: Darragh Bailey <daragh.bailey@gmail.com>
Date: Mon, 15 May 2017 18:52:14 +0100
Subject: [PATCH] Use bash to run bazel generated script

Bazel generated script makes use of Bash Arrays, and since some distros
may use use a shell other than bash for 'sh' (Ubuntu), make sure to call
'bash' explicitly instead of 'sh'

Change-Id: I6cc5367dca4e9bf7d8930cc401cdf21a1bc6c5ee
---
 tools/run_dev.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/run_dev.sh b/tools/run_dev.sh
index 57f365e..ae52343 100755
--- a/tools/run_dev.sh
+++ b/tools/run_dev.sh
@@ -29,4 +29,4 @@ PROPERTIES="$PROPERTIES --jvm_flag=-Dcom.google.gitiles.sourcePath=$ROOT"
   bazel build gitiles-dev:dev
 )

-sh $ROOT/bazel-bin/gitiles-dev/dev $PROPERTIES
+bash $ROOT/bazel-bin/gitiles-dev/dev $PROPERTIES
-- 
2.7.4
alonbl commented 7 years ago

there is no need to do this, as the dev is executable... in my case I just removed the sh :)

electrofelix commented 7 years ago

@alonbl I assumed there was a reason to specify the shell, rather than call the file directly, as I was unsure whether it would work on other platforms that can use bash, but I'm happy to reduce the patch to just remove the shell call and rely on #!/bin/bash in script.

Possibly bazel didn't mark the generated file as executable at some point.

Change is up at https://gerrit-review.googlesource.com/107353, I'll poke the source of the original change and see if it was just habbit that added in the call to sh, or if there might have been another reason at the time.

electrofelix commented 6 years ago

Changed landed, closing.