Fix server build script by using more portable and safer bash and rely on PATH when no ANDROID_HOME #1716
Conversation
This patch should increase the portability of bash scripts across various *nix systems such as BSD-like distributions. Signed-off-by: Luís Ferreira <contact@lsferreira.net>
When $ANDROID_HOME variable is missing, you should rely on $PATH. This make things easier to build from scratch on distros like Arch Linux or Gentoo. Signed-off-by: Luís Ferreira <contact@lsferreira.net>
The bash shell normally only looks at the exit code of the last command of a pipeline. This behavior is not ideal as it causes the -e option to only be able to act on the exit code of a pipeline’s last command. This is where -o pipefail comes in. This particular option sets the exit code of a pipeline to that of the rightmost command to exit with a non-zero status, or to zero if all commands of the pipeline exit successfully. See more about safer bash scripts: https://vaneyckt.io/posts/safer_bash_scripts_with_set_euxo_pipefail/ Signed-off-by: Luís Ferreira <contact@lsferreira.net>
| @@ -9,14 +9,24 @@ | |||
| # | |||
| # BUILD_DIR=my_build_dir ./build_without_gradle.sh | |||
|
|
|||
| set -e | |||
| set -eo pipefail | |||
There was a problem hiding this comment.
Isn't it too violent?
set -eo pipefail
result=$(echo a | grep b)There was a problem hiding this comment.
It depends on what behavior you want here. You might want something like this:echo a | grep b || true, instead.
| @@ -1,4 +1,4 @@ | |||
| #!/bin/bash | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
Ref #426
OK, I don't know which one is good practice (/bin/bash seems less risky to me, but why not).
There was a problem hiding this comment.
It depends of the portability you want to have here. I do think it's a good practice for 2 major reasons:
-
All Linux distributions I'm aware of are compatible with the approach I described in the patch.
-
Some BSD-based distributions like FreeBSD doesn't support your approach. See here: https://forums.freebsd.org/threads/bash-not-found.31849/ .
The env binary simply search for the executable in the provided PATH environment variable, so, here, you rely on it instead of providing the full path.
I didn't noticed the previous patch you referred, so if you're good with this, I can rebase and drop it.
| [ -x "$(which aidl)" ] && AIDL="aidl" \ | ||
| || (echo "Please provide \$ANDROID_HOME or set \$PATH to include 'aidl' executable"; exit 1) | ||
| fi | ||
|
|
There was a problem hiding this comment.
For info, why do you need this? Where do your dx and aidl come from?
(That way, the dx and aidl version are not necessarily from the expected platform.)
There was a problem hiding this comment.
For info, why do you need this? Where do your dx and aidl come from?
The problem I'm trying to solve here is building scrcpy completely from scratch with android-tools compiled from source. On Arch Linux scrcpy is built using a prebuilt scrcpy-server.
Because AOSP source has a huge and monolithic build system and because there's no proper tagging, Arch Linux, to build everything from source, need to do this: https://github.com/archlinux/svntogit-community/blob/packages/android-tools/trunk/generate_build.rb#L3 .
(That way, the dx and aidl version are not necessarily from the expected platform.)
You're right. In fact, I didn't covered this, if you depend on a specific $PLATFORM and $BUILD_TOOLS versions, then, this is a problem.
Signed-off-by: Luís Ferreira <contact@lsferreira.net>
See the commit messages.