Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
223943002:
android_make now exactly the same as android_ninja (Closed)
|
Created:
6 years, 8 months ago by epoger Modified:
6 years, 8 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlesource.com/skia.git@master Visibility:
Public. |
Descriptionandroid_make now exactly the same as android_ninja
BUG=skia:2382
NOTREECHECKS=True
NOTRY=True
Committed: http://code.google.com/p/skia/source/detail?r=14045
Patch Set 1 #
Total comments: 11
Patch Set 2 : add TODO #
Total comments: 2
Patch Set 3 : move set -e #Messages
Total messages: 14 (0 generated)
PTAL https://codereview.chromium.org/223943002/diff/1/platform_tools/android/bin/a... File platform_tools/android/bin/android_make (left): https://codereview.chromium.org/223943002/diff/1/platform_tools/android/bin/a... platform_tools/android/bin/android_make:3: # Fail-fast if anything in the script fails. mtklein- I see you added the "set -e" in https://github.com/google/skia/commit/48f9e1f2c1a164128fa3e1fc5e264aab16b2b884 . It causes me trouble, because it prevents me from seeing error messages from android_setup.sh . Is there something important we will be missing if we remove it?
https://codereview.chromium.org/223943002/diff/1/platform_tools/android/bin/a... File platform_tools/android/bin/android_make (left): https://codereview.chromium.org/223943002/diff/1/platform_tools/android/bin/a... platform_tools/android/bin/android_make:3: # Fail-fast if anything in the script fails. On 2014/04/03 14:15:37, epoger wrote: > mtklein- I see you added the "set -e" in > https://github.com/google/skia/commit/48f9e1f2c1a164128fa3e1fc5e264aab16b2b884 . > It causes me trouble, because it prevents me from seeing error messages from > android_setup.sh . Is there something important we will be missing if we remove > it? Yeah, please leave it in once you're done hacking. Failing fast makes it easier to debug. Tends to be subsequent errors are really just rippling consequences of the first command failure, that is, noise until you fix the first problem. https://codereview.chromium.org/223943002/diff/1/platform_tools/android/bin/a... File platform_tools/android/bin/android_make (right): https://codereview.chromium.org/223943002/diff/1/platform_tools/android/bin/a... platform_tools/android/bin/android_make:9: source $SCRIPT_DIR/android_setup.sh Derek, remind me why android_setup.sh is a separate script? Maybe TODO: merge it in here? It's hard to reason about what android_setup.sh's doing for us without hopping back and forth. https://codereview.chromium.org/223943002/diff/1/platform_tools/android/bin/a... platform_tools/android/bin/android_make:14: ln -sf lib $OUT/lib.target # android_run_skia looks in lib.target; ninja writes to lib. Can you add TODO(mtklein): change android_run_skia ?
https://codereview.chromium.org/223943002/diff/1/platform_tools/android/bin/a... File platform_tools/android/bin/android_make (left): https://codereview.chromium.org/223943002/diff/1/platform_tools/android/bin/a... platform_tools/android/bin/android_make:3: # Fail-fast if anything in the script fails. I disagree that it should be added back here. If you want to set it after we source android_setup.sh, so that we exit immediately after gyp, ninja, or ln fails then I think that is fine. However, doing it here breaks android_setup.sh. https://codereview.chromium.org/223943002/diff/1/platform_tools/android/bin/a... File platform_tools/android/bin/android_make (right): https://codereview.chromium.org/223943002/diff/1/platform_tools/android/bin/a... platform_tools/android/bin/android_make:9: source $SCRIPT_DIR/android_setup.sh On 2014/04/03 14:33:51, mtklein wrote: > Derek, remind me why android_setup.sh is a separate script? Maybe TODO: merge > it in here? It's hard to reason about what android_setup.sh's doing for us > without hopping back and forth. The android_setup.sh script is a common include file that is used by most of our android tools (e.g. android_run_skia, android_install_apk, etc.)
https://codereview.chromium.org/223943002/diff/1/platform_tools/android/bin/a... File platform_tools/android/bin/android_make (left): https://codereview.chromium.org/223943002/diff/1/platform_tools/android/bin/a... platform_tools/android/bin/android_make:3: # Fail-fast if anything in the script fails. On 2014/04/03 14:43:51, djsollen wrote: > I disagree that it should be added back here. If you want to set it after we > source android_setup.sh, so that we exit immediately after gyp, ninja, or ln > fails then I think that is fine. However, doing it here breaks > android_setup.sh. I don't get it. Does android_setup.sh intentionally run things that will fail? What's changed that makes this not safe?
https://codereview.chromium.org/223943002/diff/1/platform_tools/android/bin/a... File platform_tools/android/bin/android_make (left): https://codereview.chromium.org/223943002/diff/1/platform_tools/android/bin/a... platform_tools/android/bin/android_make:3: # Fail-fast if anything in the script fails. On 2014/04/03 14:46:29, mtklein wrote: > On 2014/04/03 14:43:51, djsollen wrote: > > I disagree that it should be added back here. If you want to set it after we > > source android_setup.sh, so that we exit immediately after gyp, ninja, or ln > > fails then I think that is fine. However, doing it here breaks > > android_setup.sh. > > I don't get it. Does android_setup.sh intentionally run things that will fail? > What's changed that makes this not safe? The problem is, with "set -e" in place, there are at least some errors that don't show up at all. This is not something that changed with this CL... Try this with the script as it stands before this CL: $ ANDROID_SDK_ROOT="" platform_tools/android/bin/android_ninja $ (It fails, without any explanation as to why) https://codereview.chromium.org/223943002/diff/1/platform_tools/android/bin/a... File platform_tools/android/bin/android_make (right): https://codereview.chromium.org/223943002/diff/1/platform_tools/android/bin/a... platform_tools/android/bin/android_make:14: ln -sf lib $OUT/lib.target # android_run_skia looks in lib.target; ninja writes to lib. On 2014/04/03 14:33:51, mtklein wrote: > Can you add TODO(mtklein): change android_run_skia ? You mean like this?
https://codereview.chromium.org/223943002/diff/1/platform_tools/android/bin/a... File platform_tools/android/bin/android_make (right): https://codereview.chromium.org/223943002/diff/1/platform_tools/android/bin/a... platform_tools/android/bin/android_make:14: ln -sf lib $OUT/lib.target # android_run_skia looks in lib.target; ninja writes to lib. On 2014/04/03 14:50:07, epoger wrote: > On 2014/04/03 14:33:51, mtklein wrote: > > Can you add TODO(mtklein): change android_run_skia ? > > You mean like this? (You'll have to see patchset 2 to see the TODO I added. It doesn't show up if you click on the https://codereview.chromium.org/223943002/diff/1/platform_tools/android/bin/a... comment link.)
https://codereview.chromium.org/223943002/diff/20001/platform_tools/android/b... File platform_tools/android/bin/android_make (right): https://codereview.chromium.org/223943002/diff/20001/platform_tools/android/b... platform_tools/android/bin/android_make:10: I think having set -e after line 9 is a good idea.
See patchset 3. https://codereview.chromium.org/223943002/diff/20001/platform_tools/android/b... File platform_tools/android/bin/android_make (right): https://codereview.chromium.org/223943002/diff/20001/platform_tools/android/b... platform_tools/android/bin/android_make:10: On 2014/04/03 15:02:52, djsollen wrote: > I think having set -e after line 9 is a good idea. Done.
lgtm
The CQ bit was checked by epoger@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/epoger@google.com/223943002/40001
Message was sent while issue was closed.
Change committed as 14045
Message was sent while issue was closed.
lgtm https://codereview.chromium.org/223943002/diff/1/platform_tools/android/bin/a... File platform_tools/android/bin/android_make (left): https://codereview.chromium.org/223943002/diff/1/platform_tools/android/bin/a... platform_tools/android/bin/android_make:3: # Fail-fast if anything in the script fails. On 2014/04/03 14:50:07, epoger wrote: > On 2014/04/03 14:46:29, mtklein wrote: > > On 2014/04/03 14:43:51, djsollen wrote: > > > I disagree that it should be added back here. If you want to set it after > we > > > source android_setup.sh, so that we exit immediately after gyp, ninja, or ln > > > fails then I think that is fine. However, doing it here breaks > > > android_setup.sh. > > > > I don't get it. Does android_setup.sh intentionally run things that will > fail? > > What's changed that makes this not safe? > > The problem is, with "set -e" in place, there are at least some errors that > don't show up at all. This is not something that changed with this CL... > > Try this with the script as it stands before this CL: > > $ ANDROID_SDK_ROOT="" platform_tools/android/bin/android_ninja > $ > > (It fails, without any explanation as to why) Right, so if nothing's changed, let's not change this here. The problems are in android_setup.sh and can be fixed there. Let's say you want to see the ant error message. Write these lines which ant &> /dev/null if [[ "$?" != "0" ]]; then echo "ERROR: Unable to find ant. Please install it before proceeding." exit 1 fi as if ! which -s ant; then echo "Can't find ant on your PATH." exit 1 fi So, for an unset ANDROID_SDK_ROOT, rewrite if [ -z "$ANDROID_SDK_ROOT" ]; then ANDROID_TOOL=$(which android 2>/dev/null) if [ -z "$ANDROID_TOOL" ]; then echo "ERROR: Please define ANDROID_SDK_ROOT in your environment to point" echo " to a valid Android SDK installation." exit 1 fi ANDROID_SDK_ROOT=$(cd $(dirname "$ANDROID_TOOL")/.. && pwd) exportVar ANDROID_SDK_ROOT "$ANDROID_SDK_ROOT" fi as something like if [-z "$ANDROID_SDK_ROOT" ]; then if ANDROID_SDK_ROOT="$(dirname $(which android))/.."; then export ANDROID_SDK_ROOT else echo "No ANDROID_SDK_ROOT set and can't auto detect it from location of android binary." exit 1 fi fi
Message was sent while issue was closed.
On 2014/04/03 15:17:26, mtklein wrote: > lgtm > > https://codereview.chromium.org/223943002/diff/1/platform_tools/android/bin/a... > File platform_tools/android/bin/android_make (left): > > https://codereview.chromium.org/223943002/diff/1/platform_tools/android/bin/a... > platform_tools/android/bin/android_make:3: # Fail-fast if anything in the script > fails. > On 2014/04/03 14:50:07, epoger wrote: > > On 2014/04/03 14:46:29, mtklein wrote: > > > On 2014/04/03 14:43:51, djsollen wrote: > > > > I disagree that it should be added back here. If you want to set it after > > we > > > > source android_setup.sh, so that we exit immediately after gyp, ninja, or > ln > > > > fails then I think that is fine. However, doing it here breaks > > > > android_setup.sh. > > > > > > I don't get it. Does android_setup.sh intentionally run things that will > > fail? > > > What's changed that makes this not safe? > > > > The problem is, with "set -e" in place, there are at least some errors that > > don't show up at all. This is not something that changed with this CL... > > > > Try this with the script as it stands before this CL: > > > > $ ANDROID_SDK_ROOT="" platform_tools/android/bin/android_ninja > > $ > > > > (It fails, without any explanation as to why) > > Right, so if nothing's changed, let's not change this here. The problems are in > android_setup.sh and can be fixed there. > > Let's say you want to see the ant error message. Write these lines > which ant &> /dev/null > if [[ "$?" != "0" ]]; then > echo "ERROR: Unable to find ant. Please install it before proceeding." > exit 1 > fi > as > if ! which -s ant; then > echo "Can't find ant on your PATH." > exit 1 > fi > > So, for an unset ANDROID_SDK_ROOT, rewrite > if [ -z "$ANDROID_SDK_ROOT" ]; then > ANDROID_TOOL=$(which android 2>/dev/null) > if [ -z "$ANDROID_TOOL" ]; then > echo "ERROR: Please define ANDROID_SDK_ROOT in your environment to point" > echo " to a valid Android SDK installation." > exit 1 > fi > ANDROID_SDK_ROOT=$(cd $(dirname "$ANDROID_TOOL")/.. && pwd) > exportVar ANDROID_SDK_ROOT "$ANDROID_SDK_ROOT" > fi > as something like > if [-z "$ANDROID_SDK_ROOT" ]; then > if ANDROID_SDK_ROOT="$(dirname $(which android))/.."; then > export ANDROID_SDK_ROOT > else > echo "No ANDROID_SDK_ROOT set and can't auto detect it from location of > android binary." > exit 1 > fi > fi if you want to upload that CL I'll be happy to LGTM it! |
