Add update-linux-sandbox.sh to build/

build/update-linux-sandbox.sh

+#!/bin/sh

[cmp, 2012/06/05 22:11:29]

add -e

add standard Chromium copyright header

[jln (very slow on Chromium), 2012/06/05 22:43:08]

I specifically didn't do it because I wanted to give more useful error messages.
For instance see lines 37 to 41. WDYT ?

[Markus (顧孟勤), 2012/06/05 23:17:57]

/bin/sh is OK and it results in slightly faster execution and is slightly more
portable. These are all desirable features when writing code that needs to run
on lots of different systems.

In practice, I recommend you use /bin/bash, though. There are a whole bunch of
bash-specific extensions that make your script much easier to maintain.

Furthermore, you might want to consider enforcing strict error handling. This
requires more care in your script, but it is more likely to stop the script
early, if something went wrong. Rather than letting it stomp all over your file
system:

#/bin/bash -e

If you do this, you might want to use trap handlers to let the user know, if the
script exited unexpectedly.

At the top of your script, add:

trap 'trap "" EXIT ERR INT HUP TERM QUIT;
      { tput bel 2>/dev/null || :;
        printf "\n\nExiting unexpectedly!\n"; } >&2;
      exit 1' EXIT ERR INT HUP TERM QUIT

And at the bottom of you script you do:

trap '' EXIT ERR INT HUP TERM QUIT

If you need to do some kind of clean up, whenever your script fails, that would
also be something you can do from the trap handler.

[Markus (顧孟勤), 2012/06/05 23:21:58]

You can still do explicit error handling, even when using "-e".

"-e" only has any effect, if the final exit code in a sequence of commands is
false. In an "if false; then :; fi" command, the "false" is consumed by "if";
so, the shell won't exit on you.

And the exit status of the "if" statement is the same as the exit status of its
body. So, that would be "true" in this case.

+BUILDTYPE=${BUILDTYPE:-"Debug"}

[cmp, 2012/06/05 22:11:29]

insert a blank line before line 2

[jln (very slow on Chromium), 2012/06/05 22:43:08]

Done.

[Markus (顧孟勤), 2012/06/05 23:17:57]

You don't really need the quotes around the string, although it doesn't do any
harm.

But you absolutely should have quotes around any expansion. So, either one of
the following is correct:

BUILDTYPE="${BUILDTYPE:-"Debug"}"
or
BUILDTYPE="${BUILDTYPE:-Debug}"

The latter seems the more common way to do things. But I believe the shell
treats both expressions exactly the same.

+CHROME_SRC_DIR=${CHROME_SRC_DIR:-$(dirname $(readlink -fn $0))/..}

[Markus (顧孟勤), 2012/06/05 23:17:57]

There are a whole bunch of variable expansions here, that really should be
quoted. Otherwise, you can run into nasty surprises. The most common problem
would be with file and directory names that include white space.

CHROME_SRC_DIR="${CHROME_SRC_DIR:-$(dirname -- "$(readlink -f -- "${0}")")/..)"

Also note that we don't really need "-n". The shell strips trailing newlines for
us.

This is still not quite what we want though, since it is somewhat ugly that
there now are "/.." characters in the path.

You can either do another call to readlink:

CHROME_SRC_DIR="${CHROME_SRC_DIR:-$(readlink -f -- "$(dirname -- "$(readlink -f
-- "${0}")")/..")}"

Or you can admit that this is getting to long and write a version that uses
shell built-ins only. It's a little more verbose, but also more readable and
faster:

[ -z "${CHROME_SRC_DIR}" ] && {
  CHROME_SRC_DIR="$(readlink -f -- "${0}")"
  CHROME_SRC_DIR="${CHROME_SRC_DIR%/*/*}"
}

I would probably pick this solution. But the earlier option is valid, too.

In fact, the earlier solution is possibly more correct, if we expect to have
crazy symbolic links (as "readlink", by definition, expands links). On the other
hand, it depending on how the links are set, it could also do the wrong thing.
Without more knowledge about what the user intended to do, this probably can't
be decided.

+CHROME_OUT_DIR="${CHROME_SRC_DIR}/out/${BUILDTYPE}"

[Lei Zhang, 2012/06/05 22:07:45]

Have you considered putting the dev sandbox at say,
/usr/local/sbin/chrome-devel-sandbox? Developers may clobber out/ from time to
time.

[jln (very slow on Chromium), 2012/06/05 22:43:08]

Done.

I was wondering if having a global one could be annoying if people play with
multiple repositories at the same time, but I think it's likely that you
suggested change is an improvement for the vast majority.

[Lei Zhang, 2012/06/05 23:03:18]

I think most people would want to run this once and forget about it. Even with
multiple repositories, they will likely be on the same sandbox API version.

+chrome_sandbox_build_path="${CHROME_OUT_DIR}/chrome_sandbox"

[Markus (顧孟勤), 2012/06/05 23:17:57]

Any reason for you to use lower-case variables here?

Some people use upper-case for variables that they export to child processes,
and they use lower-case for variables that do not get exported. But this is not
very commonly done.

A more common pattern is to use upper-case for global variables, and lower-case
for local variables. This makes sense since it is not always easy to see which
variables are local.

But since you don't use any functions, I don't think this distinction would
apply here.

+# Make sure the path below is not on NFS!

[Markus (顧孟勤), 2012/06/05 23:17:57]

This comment doesn't seem to reflect what the code is doing.

Do you really want to check for NFS? Or do you want to check for "nosuid"? Or
for both?

You could for instance do:

if grep "^$(df "${xxx}"|sed -n '${s/ .*//;p}') " /proc/mounts | egrep
'nfs|nosuid' >&/dev/null; then
  echo "Error! ..." >&2
  exit 1
fi

+chrome_sandbox_installed_path="${CHROME_OUT_DIR}/installed_chrome_sandbox"
+
+if [ ! -d "$CHROME_OUT_DIR" ]

[Markus (顧孟勤), 2012/06/05 23:17:57]

You don't technically need curly braces around this environment variable. But it
is generally good practice. So, please write:

"${CHROME_OUT_DIR}"

Also, I would usually put the "then" on the same line as the "if". But I don't
think we have hard rules either way:

if [ ! -d "${CHROME_OUT_DIR}" ]; then

+then

[cmp, 2012/06/05 22:11:29]

please use this syntax:

  if [ ... ];
  then

[Lei Zhang, 2012/06/05 22:13:27]

Did you mean "if [ ... ]; then" ?

[cmp, 2012/06/05 22:32:32]

I'm retracting this comment since the scripts in src/ and other places are just
not internally consistent on this point.  At least Julien's script is
consistent, so that's enough for me IMO here.

+  echo -n "$CHROME_OUT_DIR does not exist. Use \"BUILDTYPE=Release ${0}\" "

[Markus (顧孟勤), 2012/06/05 23:17:57]

Again, missing curly braces. And we are missing any command line arguments. It
would be nice if we printed those, wouldn't it. Furthermore, "echo -n" is a
little fragile. Slightly safer to use "printf":

printf '%s' "\"${CHROME_OUT_DIR}\" doesn't exist. Use \"BUILDTYPE=Release
${0}${*:+ ${*}}\". "

+  echo "If you are building in Release mode"

[Markus (顧孟勤), 2012/06/05 23:17:57]

This is likely going to result in a line that is longer than 80 characters. So,
you might want to reconsider omitting the "\n" character.

Of course, if you want to be really neat, you can pipe the output through "fold
-s". Use the following to compute the number of columns:

col="$(stty -a 2>/dev/null|sed 's/.*columns *\([0-9]*\).*/\1/;t1;d;:1;q')"
col="${col:-80}"

If you do decide to go this route, and I am not necessarily saying you should,
then the line-wrapping code should probably go into a helper function.

In fact, you might occasionally encounter machines that don't have a copy of
"fold", so you should check for that too. And you might want to redirect all
error messages to stderr:

wrap() {
  local col
  col="$(stty -a 2>/dev/null|sed 's/.*columns *\([0-9]*\).*/\1/;t1;d;:1;q')"
  col="${col:-80}"
  local msg="$(cat)"
  printf '%s\n' "${msg}" | fold -s -w "${col}" >&2 2>/dev/null ||
    printf '%s\n' "${msg}" >&2
}

+  exit 1
+fi
+
+if [ ! -f "$chrome_sandbox_build_path" ]

[Markus (顧孟勤), 2012/06/05 23:17:57]

See above

+then
+  echo -n "Could not find $chrome_sandbox_build_path, "

[Markus (顧孟勤), 2012/06/05 23:17:57]

See above

+  echo "please make sure you build the chrome_sandbox target"
+  exit 1
+fi
+
+installsandbox() {

[Markus (顧孟勤), 2012/06/05 23:17:57]

How come, this is a function. But nothing else in your code seems to be a
function.

Also, this is a somewhat unusual location to define the function. Casual readers
might think it is executed right here.

+  sudo -- cp $chrome_sandbox_build_path $chrome_sandbox_installed_path &&

[Lei Zhang, 2012/06/05 22:07:45]

nit: put variable in double quotes here and below.

[jln (very slow on Chromium), 2012/06/05 22:43:08]

Done.

[Markus (顧孟勤), 2012/06/05 23:17:57]

We really need quotes. Bad things will otherwise happen.

[Markus (顧孟勤), 2012/06/05 23:21:58]

You might want to warn users that you are about to run "sudo" for them -- and
they might now get prompted for their password.

+  sudo -- chown root:root $chrome_sandbox_installed_path &&
+  sudo -- chmod 4755 $chrome_sandbox_installed_path
+  return $?
+}
+
+if [ ! -f "$chrome_sandbox_installed_path" ]

[Markus (顧孟勤), 2012/06/05 23:17:57]

See above

+then
+  echo -n "Could not find $chrome_sandbox_installed_path, "

[Markus (顧孟勤), 2012/06/05 23:17:57]

See above

+  echo "installing it now."
+  installsandbox
+fi
+
+if [ ! -f "$chrome_sandbox_installed_path" ]

[Markus (顧孟勤), 2012/06/05 23:17:57]

See above

+then
+  echo "Failed to install $chrome_sandbox_installed_path"

[Markus (顧孟勤), 2012/06/05 23:17:57]

See above

+  exit 1
+fi
+
+current_api=$($chrome_sandbox_build_path --get-api)

[Markus (顧孟勤), 2012/06/05 23:17:57]

current_api="$("${chrome_sandbox_build_path}" --get-api 2>/dev/null)"

+installed_api=$($chrome_sandbox_installed_path --get-api)

[Markus (顧孟勤), 2012/06/05 23:17:57]

See above

+
+if [ ! x$current_api = x$installed_api ]

[Lei Zhang, 2012/06/05 22:07:45]

nit: You can do: if [ "$foo" != "$bar" ]
Same below.

[jln (very slow on Chromium), 2012/06/05 22:43:08]

Done.

[Markus (顧孟勤), 2012/06/05 23:17:57]

I think, "bash" is actually smart enough to not need the "x". But you are
correct and it is good practice to do so anyway. Nonetheless, we really want
proper quoting:

if [ "x${current_api}" != "x${installed_api}" ]; then

+then
+  echo -n "Your installed setuid sandbox is too old, "
+  echo "installing it now."

[Lei Zhang, 2012/06/05 22:07:45]

nit: this fits on the previous line.

[jln (very slow on Chromium), 2012/06/05 22:43:08]

Done.

+  installsandbox
+  if [ ! $? = 0 ]

[Markus (顧孟勤), 2012/06/05 23:17:57]

If comparing numbers, use the appropriate operator:

if [ "$?" -ne 0 ]; then

+  then
+    echo "Failed to install $chrome_sandbox_installed_path"

[Markus (顧孟勤), 2012/06/05 23:17:57]

See above

+    exit 1
+  fi
+else
+  echo "Your setuid sandbox is up to date"
+  if [ ! x$CHROME_DEVEL_SANDBOX = x"$chrome_sandbox_installed_path" ]

[Markus (顧孟勤), 2012/06/05 23:17:57]

See above

+  then
+    echo -n "Make sure you have \"export "
+    echo -n "CHROME_DEVEL_SANDBOX=$chrome_sandbox_installed_path\" "

[Markus (顧孟勤), 2012/06/05 23:17:57]

See above

+    echo "somewhere in your .bashrc"
+    echo "This variable is currently: ${CHROME_DEVEL_SANDBOX:-"empty"}"

[Markus (顧孟勤), 2012/06/05 23:17:57]

You don't need quotes around "empty"

+  fi
+fi