android envsetup: Stop honoring --target-arch parameter.

android envsetup: Stop honoring --target-arch parameter.

Most people don't use this parameter and get arm binaries. This is
still the default, so this change shouldn't affect most people.

Folks should instead pass -Dtarget_arch to gyp. (Or, soon, envsetup will stop
clobbering GYP_DEFINES, then you can just add target_arch to your GYP_DEFINES.)
Note that in gyp land, 'mips' is called 'mipsel' and 'x86' is called 'ia32'.
'arm' stays 'arm'.

So for example, instead of running

  . build/android/envsetup.sh --target-arch=mips
  android_gyp

you'd run

  . build/android/envsetup.sh
  android_gyp -Dtarget_arch=mipsel

I updated the bots I was able to find to pass the -D flag in addition to
--target-arch. After this CL here is in, I'll update the bots to stop passing
--target-arch, and then I'll make --target-arch a hard error in this script for
a while, to make sure nobody still uses it.

BUG=330631, 34476
R=torne@chromium.org
TBR=yfriedman

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252034

[Nico, 2014-02-19 03:13:22]

miguelg: No idea if the cr part is correct, please look carefully at that.

(Yaron: I think mips-builder uses the bb_run_bot.py stuff, so this CL should
take care of that bot.)

[Miguel Garcia, 2014-02-19 09:11:24]

On 2014/02/19 03:13:22, Nico wrote:
> miguelg: No idea if the cr part is correct, please look carefully at that.
> 
> (Yaron: I think mips-builder uses the bb_run_bot.py stuff, so this CL should
> take care of that bot.)

Adding Ian for the cr piece. I think the changes in android.py work but not so
sure about the ones in arch.py

Could you open a clean shell, go the src directory and type

source tools/cr/cr-bash-helpers.sh
cr init --o out_android/Debug --p android 
cr run

to verify that things keep working?
if you don't have/want the android setup let's at least validate it for linux

source tools/cr/cr-bash-helpers.sh
cr init --o out_linux/Debug --p linux
cr run

[iancottrell, 2014-02-19 09:58:20]

https://codereview.chromium.org/171903002/diff/350001/tools/cr/cr/base/androi...
File tools/cr/cr/base/android.py (right):

https://codereview.chromium.org/171903002/diff/350001/tools/cr/cr/base/androi...
tools/cr/cr/base/android.py:63: 'GYP_DEFINES', '') + '
target_arch={CR_ENVSETUP_ARCH}'
This feels like the wrong place to do this.
Is there any reason this needs to be android specific?
It might be better to have arch directly modify GYP_DEFINES for all platforms.
GYP_DEFINES in general has some problems, mostly because ramming a map of
settings into a single string is a vile and horrible practice to start with :) 
I have considered adding more magic to help with it, for instance auto merging
all variables that start with a GYP_SET_ prefix or something.
This would allow arch to have GYP_SET_target_arch set on it, and it would just
work. It would also make it easy to override and view all the other gyp options
on a per output basis.
If it is going to stay in this file, it should probably move down to the
GYP_DEFINES specific modification part below this.

https://codereview.chromium.org/171903002/diff/350001/tools/cr/cr/base/arch.py
File tools/cr/cr/base/arch.py (right):

https://codereview.chromium.org/171903002/diff/350001/tools/cr/cr/base/arch.p...
tools/cr/cr/base/arch.py:32: CR_ENVSETUP_ARCH='ia32',
I think rather than repurpose this variable, we should just add another one with
a better name, maybe CR_GYP_ARCH.
We can always strip this variable out later on.

[benm (inactive), 2014-02-19 11:45:22]

+Ross as MIPS build instructions will change.

[Nico, 2014-02-19 14:04:04]

kenm: Where are the mips build instructions?

https://codereview.chromium.org/171903002/diff/350001/tools/cr/cr/base/androi...
File tools/cr/cr/base/android.py (right):

https://codereview.chromium.org/171903002/diff/350001/tools/cr/cr/base/androi...
tools/cr/cr/base/android.py:63: 'GYP_DEFINES', '') + '
target_arch={CR_ENVSETUP_ARCH}'
On 2014/02/19 09:58:21, iancottrell wrote:
> This feels like the wrong place to do this.
> Is there any reason this needs to be android specific?

Yes: envsetup is android-specific, and I'm changing how envsetup works.

> It might be better to have arch directly modify GYP_DEFINES for all platforms.
> GYP_DEFINES in general has some problems, mostly because ramming a map of
> settings into a single string is a vile and horrible practice to start with :)


You can pass -D for every single define to gyp instead if you think that's
nicer.

> I have considered adding more magic to help with it, for instance auto merging
> all variables that start with a GYP_SET_ prefix or something.
> This would allow arch to have GYP_SET_target_arch set on it, and it would just
> work. It would also make it easy to override and view all the other gyp
options
> on a per output basis.
> If it is going to stay in this file, it should probably move down to the
> GYP_DEFINES specific modification part below this.

Which part? The `if key == 'GYP_DEFINES'` bit? That only runs if envsetup
changes GYP_DEFINES, right? It'll stop doing that soon, so that line will be
dead soon.

(I have no opinion how the cr bit should work. Tell me what to do, and I'll
gladly do it.)

[benm (inactive), 2014-02-19 14:08:59]

On 2014/02/19 14:04:04, Nico wrote:
> kenm: Where are the mips build instructions?

Currently maintained downstream; I can send you the link. Having said that
though, we could probably move them upstream. WDYT Ross?

> 
>
https://codereview.chromium.org/171903002/diff/350001/tools/cr/cr/base/androi...
> File tools/cr/cr/base/android.py (right):
> 
>
https://codereview.chromium.org/171903002/diff/350001/tools/cr/cr/base/androi...
> tools/cr/cr/base/android.py:63: 'GYP_DEFINES', '') + '
> target_arch={CR_ENVSETUP_ARCH}'
> On 2014/02/19 09:58:21, iancottrell wrote:
> > This feels like the wrong place to do this.
> > Is there any reason this needs to be android specific?
> 
> Yes: envsetup is android-specific, and I'm changing how envsetup works.
> 
> > It might be better to have arch directly modify GYP_DEFINES for all
platforms.
> > GYP_DEFINES in general has some problems, mostly because ramming a map of
> > settings into a single string is a vile and horrible practice to start with
:)
> 
> 
> You can pass -D for every single define to gyp instead if you think that's
> nicer.
> 
> > I have considered adding more magic to help with it, for instance auto
merging
> > all variables that start with a GYP_SET_ prefix or something.
> > This would allow arch to have GYP_SET_target_arch set on it, and it would
just
> > work. It would also make it easy to override and view all the other gyp
> options
> > on a per output basis.
> > If it is going to stay in this file, it should probably move down to the
> > GYP_DEFINES specific modification part below this.
> 
> Which part? The `if key == 'GYP_DEFINES'` bit? That only runs if envsetup
> changes GYP_DEFINES, right? It'll stop doing that soon, so that line will be
> dead soon.
> 
> (I have no opinion how the cr bit should work. Tell me what to do, and I'll
> gladly do it.)

[iancottrell, 2014-02-19 14:18:24]

https://codereview.chromium.org/171903002/diff/350001/tools/cr/cr/base/androi...
File tools/cr/cr/base/android.py (right):

https://codereview.chromium.org/171903002/diff/350001/tools/cr/cr/base/androi...
tools/cr/cr/base/android.py:63: 'GYP_DEFINES', '') + '
target_arch={CR_ENVSETUP_ARCH}'
On 2014/02/19 14:04:06, Nico wrote:
> On 2014/02/19 09:58:21, iancottrell wrote:
> > This feels like the wrong place to do this.
> > Is there any reason this needs to be android specific?
> 
> Yes: envsetup is android-specific, and I'm changing how envsetup works.

I mean that setting the arch for gyp does not need to be specific to android, it
would be cleaner for it not to be in this file at all.
I am happy for you to commit it here and file a bug against me to make it more
general though.

> 
> > It might be better to have arch directly modify GYP_DEFINES for all
platforms.
> > GYP_DEFINES in general has some problems, mostly because ramming a map of
> > settings into a single string is a vile and horrible practice to start with
:)
> 
> 
> You can pass -D for every single define to gyp instead if you think that's
> nicer.
> 
> > I have considered adding more magic to help with it, for instance auto
merging
> > all variables that start with a GYP_SET_ prefix or something.
> > This would allow arch to have GYP_SET_target_arch set on it, and it would
just
> > work. It would also make it easy to override and view all the other gyp
> options
> > on a per output basis.
> > If it is going to stay in this file, it should probably move down to the
> > GYP_DEFINES specific modification part below this.
> 
> Which part? The `if key == 'GYP_DEFINES'` bit? That only runs if envsetup
> changes GYP_DEFINES, right? It'll stop doing that soon, so that line will be
> dead soon.
Except the code you are adding is faking envsetup modifying GYP_DEFINES :)
The reason it would be better down there is that up here it is modifying
context.exported which is not safe, and will also get dropped if the line below
is removed
Down there, it would get added to the new override added to context.

> 
> (I have no opinion how the cr bit should work. Tell me what to do, and I'll
> gladly do it.)

[rmcilroy, 2014-02-19 14:18:52]

On 2014/02/19 14:08:59, benm wrote:
> On 2014/02/19 14:04:04, Nico wrote:
> > kenm: Where are the mips build instructions?
> 
> Currently maintained downstream; I can send you the link. Having said that
> though, we could probably move them upstream. WDYT Ross?

Yes, I can update the relevant points and add them to the upstream documentation
once this CL lands.

[Nico, 2014-02-19 16:38:39]

I tried cr, it seems to work with this patch. I ran:

  git checkout master
  time python tools/cr/main.py init -o out_cr_mips/Debug --p android
--arch=mips32
  git cl patch -b arch 171903002
  time python tools/cr/main.py init -o out_cr_mips2/Debug --p android
--arch=mips32
  gvim -d out_cr_mips/Debug/obj/base/base.ninja
out_cr_mips2/Debug/obj/base/base.ninja

The generated build files are identical. I did the same without the --arch
parameter (for the arm default).

[Torne, 2014-02-19 16:46:25]

general approcah and webview bits LGTM - can't comment on cr but it sounds like
that's working too ;)

[Nico, 2014-02-19 16:49:14]

On 2014/02/19 16:46:25, Torne wrote:
> general approcah and webview bits LGTM - can't comment on cr but it sounds
like
> that's working too ;)

Thanks!

iancottrell, mgarcia: Since cr seems to be functional, I'll go ahead and check
this in and you can move things around in cr to make it look like you want in a
follow-up, is that ok? (Also, should iancotrell be in tools/cr/OWNERS?)

[Nico, 2014-02-19 18:10:31]

The CQ bit was checked by thakis@chromium.org

[commit-bot: I haz the power, 2014-02-19 18:16:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thakis@chromium.org/171903002/350001

[Nico, 2014-02-19 18:20:33]

Committed patchset #10 manually as r252034 (tree was closed).

[rmcilroy, 2014-02-20 10:13:04]

On 2014/02/19 18:20:33, Nico wrote:
> Committed patchset #10 manually as r252034 (tree was closed).

I updated both the internal and external documentation Android build
documentation to reflect this change.