Fix the gdb path for NDK-r8b.

Fix the gdb path for NDK-r8b.

- Removed android.toolchain property.
- Added android.gdbserver property.
- Changed property-location to check the both file and directory's existence.

TBR=jam
BUG=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=155108

[Yaron, 2012-08-21 19:47:40]

https://chromiumcodereview.appspot.com/10823435/diff/1/content/shell/android/...
File content/shell/android/java/content_shell_apk.xml (right):

https://chromiumcodereview.appspot.com/10823435/diff/1/content/shell/android/...
content/shell/android/java/content_shell_apk.xml:20: <fileset
file="${toolchain.dir}/../../../../../prebuilt/android-arm/gdbserver/gdbserver"/>
I imagine this won't work for x86 apks. What about passing in a
platform-specific variable

[michaelbai, 2012-08-22 21:05:03]

There are too many duplicated gypi code for ant, will file a bug to refactorying
it.

[michaelbai, 2012-08-22 21:05:49]

PTAL

https://chromiumcodereview.appspot.com/10823435/diff/1/content/shell/android/...
File content/shell/android/java/content_shell_apk.xml (right):

https://chromiumcodereview.appspot.com/10823435/diff/1/content/shell/android/...
content/shell/android/java/content_shell_apk.xml:20: <fileset
file="${toolchain.dir}/../../../../../prebuilt/android-arm/gdbserver/gdbserver"/>
On 2012/08/21 19:47:40, Yaron wrote:
> I imagine this won't work for x86 apks. What about passing in a
> platform-specific variable

Done.

[Yaron, 2012-08-22 22:51:34]

+Peter as this can impact webkit build.

http://codereview.chromium.org/10823435/diff/8001/build/android/envsetup_func...
File build/android/envsetup_functions.sh (right):

http://codereview.chromium.org/10823435/diff/8001/build/android/envsetup_func...
build/android/envsetup_functions.sh:110: DEFINES+=" target_arch=ia32
ndk_target_arch=x86 use_libffmpeg=0"
I don't think you need this new variable at all. Look at build/common.gypi. It
looks like the variable is already being re-written from ia32 to x86 and you can
just use target_arch.

It would be good to just simplify that if it's indeed true.l

http://codereview.chromium.org/10823435/diff/8001/content/content_shell.gypi
File content/content_shell.gypi (right):

http://codereview.chromium.org/10823435/diff/8001/content/content_shell.gypi#...
content/content_shell.gypi:667: '-DANDROID_TARGET_ARCH=<(ndk_target_arch)',
It looks like the ANDROID_TOOLCHAIN is only ever used to get the right
gdbserver. You could consolidate this variable with the existing one instead of
adding an extra.

http://codereview.chromium.org/10823435/diff/8001/content/shell/android/java/...
File content/shell/android/java/content_shell_apk.xml (right):

http://codereview.chromium.org/10823435/diff/8001/content/shell/android/java/...
content/shell/android/java/content_shell_apk.xml:20: <fileset
file="${toolchain.dir}/../../../../../prebuilt/android-${ndk.target.arch}/gdbserver/gdbserver"/>
Actually I don't think you can do this in one change because the variable won't
be defined for the webkit apk targets. (e.g.
third_party/WebKit/Tools/DumpRenderTree/DumpRenderTree.gyp/DumpRenderTree.gyp
and there are two others).

I think you need to define the gyp variable in envsetup and in chromium gyp. A
separate change in webkit gyp and then add this target once those are both
landed.

[michaelbai, 2012-08-22 23:11:57]

Actually, there are 3 files need to change in Webkit, I'd like get the right
solution for this, then land the WebKit CL first.


On 2012/08/22 22:51:34, Yaron wrote:
> +Peter as this can impact webkit build.
> 
>
http://codereview.chromium.org/10823435/diff/8001/build/android/envsetup_func...
> File build/android/envsetup_functions.sh (right):
> 
>
http://codereview.chromium.org/10823435/diff/8001/build/android/envsetup_func...
> build/android/envsetup_functions.sh:110: DEFINES+=" target_arch=ia32
> ndk_target_arch=x86 use_libffmpeg=0"
> I don't think you need this new variable at all. Look at build/common.gypi. It
> looks like the variable is already being re-written from ia32 to x86 and you
can
> just use target_arch.
> 
> It would be good to just simplify that if it's indeed true.l
> 
> http://codereview.chromium.org/10823435/diff/8001/content/content_shell.gypi
> File content/content_shell.gypi (right):
> 
>
http://codereview.chromium.org/10823435/diff/8001/content/content_shell.gypi#...
> content/content_shell.gypi:667: '-DANDROID_TARGET_ARCH=<(ndk_target_arch)',
> It looks like the ANDROID_TOOLCHAIN is only ever used to get the right
> gdbserver. You could consolidate this variable with the existing one instead
of
> adding an extra.
> 
>
http://codereview.chromium.org/10823435/diff/8001/content/shell/android/java/...
> File content/shell/android/java/content_shell_apk.xml (right):
> 
>
http://codereview.chromium.org/10823435/diff/8001/content/shell/android/java/...
> content/shell/android/java/content_shell_apk.xml:20: <fileset
>
file="${toolchain.dir}/../../../../../prebuilt/android-${ndk.target.arch}/gdbserver/gdbserver"/>
> Actually I don't think you can do this in one change because the variable
won't
> be defined for the webkit apk targets. (e.g.
> third_party/WebKit/Tools/DumpRenderTree/DumpRenderTree.gyp/DumpRenderTree.gyp
> and there are two others).
> 
> I think you need to define the gyp variable in envsetup and in chromium gyp. A
> separate change in webkit gyp and then add this target once those are both
> landed.

[Peter Beverloo, 2012-08-23 10:45:47]

https://chromiumcodereview.appspot.com/10823435/diff/8001/build/android/envse...
File build/android/envsetup_functions.sh (right):

https://chromiumcodereview.appspot.com/10823435/diff/8001/build/android/envse...
build/android/envsetup_functions.sh:110: DEFINES+=" target_arch=ia32
ndk_target_arch=x86 use_libffmpeg=0"
On 2012/08/22 22:51:35, Yaron wrote:
> I don't think you need this new variable at all. Look at build/common.gypi. It
> looks like the variable is already being re-written from ia32 to x86 and you
can
> just use target_arch.
> 
> It would be good to just simplify that if it's indeed true.l

Agreed. If it turns out you do, please name it "android_ndk_target_arch", as
"ndk" can be ambiguous.

https://chromiumcodereview.appspot.com/10823435/diff/8001/content/shell/andro...
File content/shell/android/java/content_shell_apk.xml (right):

https://chromiumcodereview.appspot.com/10823435/diff/8001/content/shell/andro...
content/shell/android/java/content_shell_apk.xml:20: <fileset
file="${toolchain.dir}/../../../../../prebuilt/android-${ndk.target.arch}/gdbserver/gdbserver"/>
On 2012/08/22 22:51:35, Yaron wrote:
> Actually I don't think you can do this in one change because the variable
won't
> be defined for the webkit apk targets. (e.g.
> third_party/WebKit/Tools/DumpRenderTree/DumpRenderTree.gyp/DumpRenderTree.gyp
> and there are two others).
> 
> I think you need to define the gyp variable in envsetup and in chromium gyp. A
> separate change in webkit gyp and then add this target once those are both
> landed.

We can pass defines/properties to ant without things breaking if they aren't
being used yet, so in this case a WebKit change can land first, after which
(when WebKit rolls into Chromium) this should be fine to land.

I'm a bit concerned about the number of level-ups ("../") this line relies on.
For clarity, also exporting ANDROID_NDK_ROOT may be better, but that would
indeed increase the number of defines even more. The repetition of invocating
and passing all the arguments is too common now, can we consolidate this even
more to a single gypi?

https://chromiumcodereview.appspot.com/10823435/diff/8001/content/shell/conte...
File content/shell/content_shell_ant_helper.sh (right):

https://chromiumcodereview.appspot.com/10823435/diff/8001/content/shell/conte...
content/shell/content_shell_ant_helper.sh:9: # A helper script to help convert
the values of the fifth parameter for
fifth -> last?

https://chromiumcodereview.appspot.com/10823435/diff/8001/content/shell/conte...
content/shell/content_shell_ant_helper.sh:13: buildtype="$(echo "${12}" | tr
'[A-Z]' '[a-z]')"
buildtype="$(echo "${@: -1}" | tr '[A-Z]' '[a-z]')"

would make this much less fragile.

https://chromiumcodereview.appspot.com/10823435/diff/8001/content/shell/conte...
content/shell/content_shell_ant_helper.sh:19: "${11}"
exec "${@:1:$((${#@} - 1))}"

and when we want to add buildtype:
exec "${@:1:$((${#@} - 1))}" "${buildtype}"

The real problem is, however, that this stinks. We should be passing the build
type as the first parameter instead of the last one, as that would allow us to
use "shift" to remove it. The script would end up as something like:

buildtype="$(echo "${1}" | tr '[A-Z]' '[a-z]')"
shift
exec $@ "${buildtype}"

[michaelbai, 2012-08-23 22:52:59]

PTAL

http://codereview.chromium.org/10823435/diff/8001/build/android/envsetup_func...
File build/android/envsetup_functions.sh (right):

http://codereview.chromium.org/10823435/diff/8001/build/android/envsetup_func...
build/android/envsetup_functions.sh:110: DEFINES+=" target_arch=ia32
ndk_target_arch=x86 use_libffmpeg=0"
On 2012/08/23 10:45:48, Peter Beverloo wrote:
> On 2012/08/22 22:51:35, Yaron wrote:
> > I don't think you need this new variable at all. Look at build/common.gypi.
It
> > looks like the variable is already being re-written from ia32 to x86 and you
> can
> > just use target_arch.
> > 
> > It would be good to just simplify that if it's indeed true.l
> 
> Agreed. If it turns out you do, please name it "android_ndk_target_arch", as
> "ndk" can be ambiguous.

Done.

http://codereview.chromium.org/10823435/diff/8001/content/content_shell.gypi
File content/content_shell.gypi (right):

http://codereview.chromium.org/10823435/diff/8001/content/content_shell.gypi#...
content/content_shell.gypi:667: '-DANDROID_TARGET_ARCH=<(ndk_target_arch)',
On 2012/08/22 22:51:35, Yaron wrote:
> It looks like the ANDROID_TOOLCHAIN is only ever used to get the right
> gdbserver. You could consolidate this variable with the existing one instead
of
> adding an extra.

Done.

http://codereview.chromium.org/10823435/diff/8001/content/shell/android/java/...
File content/shell/android/java/content_shell_apk.xml (right):

http://codereview.chromium.org/10823435/diff/8001/content/shell/android/java/...
content/shell/android/java/content_shell_apk.xml:20: <fileset
file="${toolchain.dir}/../../../../../prebuilt/android-${ndk.target.arch}/gdbserver/gdbserver"/>
I agreed and will file a bug for refactoring this part.

On 2012/08/23 10:45:48, Peter Beverloo wrote:
> On 2012/08/22 22:51:35, Yaron wrote:
> > Actually I don't think you can do this in one change because the variable
> won't
> > be defined for the webkit apk targets. (e.g.
> >
third_party/WebKit/Tools/DumpRenderTree/DumpRenderTree.gyp/DumpRenderTree.gyp
> > and there are two others).
> > 
> > I think you need to define the gyp variable in envsetup and in chromium gyp.
A
> > separate change in webkit gyp and then add this target once those are both
> > landed.
> 
> We can pass defines/properties to ant without things breaking if they aren't
> being used yet, so in this case a WebKit change can land first, after which
> (when WebKit rolls into Chromium) this should be fine to land.
> 
> I'm a bit concerned about the number of level-ups ("../") this line relies on.
> For clarity, also exporting ANDROID_NDK_ROOT may be better, but that would
> indeed increase the number of defines even more. The repetition of invocating
> and passing all the arguments is too common now, can we consolidate this even
> more to a single gypi?

http://codereview.chromium.org/10823435/diff/8001/content/shell/content_shell...
File content/shell/content_shell_ant_helper.sh (right):

http://codereview.chromium.org/10823435/diff/8001/content/shell/content_shell...
content/shell/content_shell_ant_helper.sh:13: buildtype="$(echo "${12}" | tr
'[A-Z]' '[a-z]')"
On 2012/08/23 10:45:48, Peter Beverloo wrote:
> buildtype="$(echo "${@: -1}" | tr '[A-Z]' '[a-z]')"
> 
> would make this much less fragile.

Done.

http://codereview.chromium.org/10823435/diff/8001/content/shell/content_shell...
content/shell/content_shell_ant_helper.sh:19: "${11}"
On 2012/08/23 10:45:48, Peter Beverloo wrote:
> exec "${@:1:$((${#@} - 1))}"
> 
> and when we want to add buildtype:
> exec "${@:1:$((${#@} - 1))}" "${buildtype}"
> 
> The real problem is, however, that this stinks. We should be passing the build
> type as the first parameter instead of the last one, as that would allow us to
> use "shift" to remove it. The script would end up as something like:
> 
> buildtype="$(echo "${1}" | tr '[A-Z]' '[a-z]')"
> shift
> exec $@ "${buildtype}" 

Done.

[yongsheng, 2012-08-24 07:14:52]

hi, I think the file 'testing/android/native_test_apk.xml' should also be
changed for gdbserver, like content/shell/android/java/content_shell_apk.xml

[michaelbai, 2012-08-24 16:46:48]

Thanks, changed two more xml files.

[Yaron, 2012-08-25 00:14:14]

lgtm

But please make sure Peter's happy. Also, I'm not entirely sure how you're
planning to deploy this. I think that as Peter mentioned, you can land a webkit
change first that defines both ANDROID_GDBSERVER and ANDROID_TOOLCHAIN in gyp,
then land this change and when it rolls into webkit, remove the redundant
ANDROID_TOOLCHAIN.

http://codereview.chromium.org/10823435/diff/6006/build/android/envsetup_func...
File build/android/envsetup_functions.sh (right):

http://codereview.chromium.org/10823435/diff/6006/build/android/envsetup_func...
build/android/envsetup_functions.sh:95: DEFINES+=" target_arch=arm"
Nit: remove the duplicate "target_arch" assignment from each of these and move
to L125.

http://codereview.chromium.org/10823435/diff/6006/build/java.gypi
File build/java.gypi (right):

http://codereview.chromium.org/10823435/diff/6006/build/java.gypi#newcode63
build/java.gypi:63: '-DANDROID_TOOLCHAIN=<(android_toolchain)',
Why is ANDROID_TOOLCHAIN still specified? Is this for webkit?

[michaelbai, 2012-08-29 19:26:52]

Could you take another look?

I will split this CL, to do the three-way process for WebKit change.

http://codereview.chromium.org/10823435/diff/6006/build/android/envsetup_func...
File build/android/envsetup_functions.sh (right):

http://codereview.chromium.org/10823435/diff/6006/build/android/envsetup_func...
build/android/envsetup_functions.sh:95: DEFINES+=" target_arch=arm"
On 2012/08/25 00:14:14, Yaron wrote:
> Nit: remove the duplicate "target_arch" assignment from each of these and move
> to L125.

Done.

http://codereview.chromium.org/10823435/diff/6006/build/java.gypi
File build/java.gypi (right):

http://codereview.chromium.org/10823435/diff/6006/build/java.gypi#newcode63
build/java.gypi:63: '-DANDROID_TOOLCHAIN=<(android_toolchain)',
Removed

On 2012/08/25 00:14:14, Yaron wrote:
> Why is ANDROID_TOOLCHAIN still specified? Is this for webkit?

[Peter Beverloo, 2012-08-30 11:19:14]

SGTM, but please land 10886046 first and then rebase this one.

[michaelbai, 2012-09-05 20:57:02]

Landing this after the webkit change was rolled in.

[commit-bot: I haz the power, 2012-09-05 20:57:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaelbai@chromium.org/10823435/23003

[commit-bot: I haz the power, 2012-09-05 20:57:59]

Presubmit check for 10823435-23003 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content
    content/shell

[commit-bot: I haz the power, 2012-09-05 21:03:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaelbai@chromium.org/10823435/23003

[commit-bot: I haz the power, 2012-09-06 00:13:47]

Failed to apply patch for content/shell/content_shell_ant_helper.sh:
While running patch -p1 --forward --force;
patching file content/shell/content_shell_ant_helper.sh
Hunk #1 FAILED at 10.
1 out of 1 hunk FAILED -- saving rejects to file
content/shell/content_shell_ant_helper.sh.rej

[commit-bot: I haz the power, 2012-09-06 00:28:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaelbai@chromium.org/10823435/22004

[commit-bot: I haz the power, 2012-09-06 02:53:54]

Change committed as 155108