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.

Sign up for GitHub

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

Jump to bottom

Fix server build script by using more portable and safer bash and rely on PATH when no ANDROID_HOME #1716

Closed
wants to merge 4 commits into from
Closed

Fix server build script by using more portable and safer bash and rely on PATH when no ANDROID_HOME #1716

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2fb0a87
server: build: use a more portable shebang for bash
ljmf00 Aug 27, 2020
1cf5918
server: build: use 'aidl' and 'dx' binaries in PATH as a fallback
ljmf00 Aug 27, 2020
f50d9dc
server: build: make script safer by failing on pipes
ljmf00 Aug 27, 2020
3e0dfb1
server: build: enforce ANDROID_PLATFORMS_PATH when no ANDROID_HOME
ljmf00 Aug 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 15 additions & 6 deletions server/build_without_gradle.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#!/bin/bash
#!/usr/bin/env bash
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ref #426

OK, I don't know which one is good practice (/bin/bash seems less risky to me, but why not).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends of the portability you want to have here. I do think it's a good practice for 2 major reasons:

  1. All Linux distributions I'm aware of are compatible with the approach I described in the patch.

  2. 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.

#
# This script generates the scrcpy binary "manually" (without gradle).
#
Expand All @@ -9,14 +9,24 @@
#
# BUILD_DIR=my_build_dir ./build_without_gradle.sh

set -e
set -eo pipefail
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it too violent?

set -eo pipefail
result=$(echo a | grep b)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends on what behavior you want here. You might want something like this:echo a | grep b || true, instead.


SCRCPY_DEBUG=false
SCRCPY_VERSION_NAME=1.16

PLATFORM=${ANDROID_PLATFORM:-29}
BUILD_TOOLS=${ANDROID_BUILD_TOOLS:-29.0.2}

if [ -z ${ANDROID_HOME+x} ]; then
AIDL="$ANDROID_HOME/build-tools/$BUILD_TOOLS/aidl"
DX="$ANDROID_HOME/build-tools/$BUILD_TOOLS/dx"
else
[ -x "$(which dx)" ] && DX="dx" \
|| (echo "Please provide \$ANDROID_HOME or set \$PATH to include 'dx' executable"; exit 1)
[ -x "$(which aidl)" ] && AIDL="aidl" \
|| (echo "Please provide \$ANDROID_HOME or set \$PATH to include 'aidl' executable"; exit 1)
fi

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

BUILD_DIR="$(realpath ${BUILD_DIR:-build_manual})"
CLASSES_DIR="$BUILD_DIR/classes"
SERVER_DIR=$(dirname "$0")
Expand All @@ -40,9 +50,9 @@ EOF

echo "Generating java from aidl..."
cd "$SERVER_DIR/src/main/aidl"
"$ANDROID_HOME/build-tools/$BUILD_TOOLS/aidl" -o"$CLASSES_DIR" \
"$AIDL" -o"$CLASSES_DIR" \
android/view/IRotationWatcher.aidl
"$ANDROID_HOME/build-tools/$BUILD_TOOLS/aidl" -o"$CLASSES_DIR" \
"$AIDL" -o"$CLASSES_DIR" \
android/content/IOnPrimaryClipChangedListener.aidl

echo "Compiling java sources..."
Expand All @@ -54,8 +64,7 @@ javac -bootclasspath "$ANDROID_HOME/platforms/android-$PLATFORM/android.jar" \

echo "Dexing..."
cd "$CLASSES_DIR"
"$ANDROID_HOME/build-tools/$BUILD_TOOLS/dx" --dex \
--output "$BUILD_DIR/classes.dex" \
"$DX" --dex --output "$BUILD_DIR/classes.dex" \
android/view/*.class \
android/content/*.class \
com/genymobile/scrcpy/*.class \
Expand Down