Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(41)

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
Reviewers:
djsollen, mtklein
CC:
skia-review_googlegroups.com
Base URL:
https://skia.googlesource.com/skia.git@master
Visibility:
Public.

Description

android_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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+10 lines, -11 lines) Patch
M platform_tools/android/bin/android_make View 1 2 2 chunks +10 lines, -11 lines 0 comments Download

Messages

Total messages: 14 (0 generated)
epoger
PTAL https://codereview.chromium.org/223943002/diff/1/platform_tools/android/bin/android_make File platform_tools/android/bin/android_make (left): https://codereview.chromium.org/223943002/diff/1/platform_tools/android/bin/android_make#oldcode3 platform_tools/android/bin/android_make:3: # Fail-fast if anything in the script fails. ...
6 years, 8 months ago (2014-04-03 14:15:37 UTC) #1
mtklein
https://codereview.chromium.org/223943002/diff/1/platform_tools/android/bin/android_make File platform_tools/android/bin/android_make (left): https://codereview.chromium.org/223943002/diff/1/platform_tools/android/bin/android_make#oldcode3 platform_tools/android/bin/android_make:3: # Fail-fast if anything in the script fails. On ...
6 years, 8 months ago (2014-04-03 14:33:51 UTC) #2
djsollen
https://codereview.chromium.org/223943002/diff/1/platform_tools/android/bin/android_make File platform_tools/android/bin/android_make (left): https://codereview.chromium.org/223943002/diff/1/platform_tools/android/bin/android_make#oldcode3 platform_tools/android/bin/android_make:3: # Fail-fast if anything in the script fails. I ...
6 years, 8 months ago (2014-04-03 14:43:51 UTC) #3
mtklein
https://codereview.chromium.org/223943002/diff/1/platform_tools/android/bin/android_make File platform_tools/android/bin/android_make (left): https://codereview.chromium.org/223943002/diff/1/platform_tools/android/bin/android_make#oldcode3 platform_tools/android/bin/android_make:3: # Fail-fast if anything in the script fails. On ...
6 years, 8 months ago (2014-04-03 14:46:28 UTC) #4
epoger
https://codereview.chromium.org/223943002/diff/1/platform_tools/android/bin/android_make File platform_tools/android/bin/android_make (left): https://codereview.chromium.org/223943002/diff/1/platform_tools/android/bin/android_make#oldcode3 platform_tools/android/bin/android_make:3: # Fail-fast if anything in the script fails. On ...
6 years, 8 months ago (2014-04-03 14:50:07 UTC) #5
epoger
https://codereview.chromium.org/223943002/diff/1/platform_tools/android/bin/android_make File platform_tools/android/bin/android_make (right): https://codereview.chromium.org/223943002/diff/1/platform_tools/android/bin/android_make#newcode14 platform_tools/android/bin/android_make:14: ln -sf lib $OUT/lib.target # android_run_skia looks in lib.target; ...
6 years, 8 months ago (2014-04-03 14:52:01 UTC) #6
djsollen
https://codereview.chromium.org/223943002/diff/20001/platform_tools/android/bin/android_make File platform_tools/android/bin/android_make (right): https://codereview.chromium.org/223943002/diff/20001/platform_tools/android/bin/android_make#newcode10 platform_tools/android/bin/android_make:10: I think having set -e after line 9 is ...
6 years, 8 months ago (2014-04-03 15:02:51 UTC) #7
epoger
See patchset 3. https://codereview.chromium.org/223943002/diff/20001/platform_tools/android/bin/android_make File platform_tools/android/bin/android_make (right): https://codereview.chromium.org/223943002/diff/20001/platform_tools/android/bin/android_make#newcode10 platform_tools/android/bin/android_make:10: On 2014/04/03 15:02:52, djsollen wrote: > ...
6 years, 8 months ago (2014-04-03 15:06:01 UTC) #8
djsollen
lgtm
6 years, 8 months ago (2014-04-03 15:07:44 UTC) #9
epoger
The CQ bit was checked by epoger@google.com
6 years, 8 months ago (2014-04-03 15:08:25 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/epoger@google.com/223943002/40001
6 years, 8 months ago (2014-04-03 15:08:38 UTC) #11
commit-bot: I haz the power
Change committed as 14045
6 years, 8 months ago (2014-04-03 15:08:51 UTC) #12
mtklein
lgtm https://codereview.chromium.org/223943002/diff/1/platform_tools/android/bin/android_make File platform_tools/android/bin/android_make (left): https://codereview.chromium.org/223943002/diff/1/platform_tools/android/bin/android_make#oldcode3 platform_tools/android/bin/android_make:3: # Fail-fast if anything in the script fails. ...
6 years, 8 months ago (2014-04-03 15:17:26 UTC) #13
djsollen
6 years, 8 months ago (2014-04-03 15:24:08 UTC) #14
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!

Powered by Google App Engine
This is Rietveld 408576698