Chrome on Android: fix cross-compilation setup.

Chrome on Android: fix cross-compilation setup.

This patch removes the need of exporting / setting
CROSS_C* variables when calling make for android.
Instead, set them at Makefile generation time so that gyp will
set the right compilers when calling android_gyp.
This also allows goma builds.

BUG=
TEST=builds for android.


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

[bulach, 2012-03-13 14:32:24]

I tried locally with both regular make and goma and it worked fine for me.
please, try it out and let me know your thoughts on this. 

I'll obviously put through try bots once we're all in agreement.

Thanks!

[Peter Beverloo, 2012-03-13 14:58:59]

I'm getting this to work with WebKit right now.

http://codereview.chromium.org/9693042/diff/1/build/android/envsetup.sh
File build/android/envsetup.sh (right):

http://codereview.chromium.org/9693042/diff/1/build/android/envsetup.sh#newco...
build/android/envsetup.sh:77:
LIBGCC_FILE_NAME=`${ANDROID_TOOLCHAIN}/arm-linux-androideabi-gcc \
This seems to defeat the purpose of the firstword-searches earlier on in this
function, why can we refer to the binary name directly here, but not in the
previous occurrences?

[bulach, 2012-03-13 15:12:09]

http://codereview.chromium.org/9693042/diff/1/build/android/envsetup.sh
File build/android/envsetup.sh (right):

http://codereview.chromium.org/9693042/diff/1/build/android/envsetup.sh#newco...
build/android/envsetup.sh:77:
LIBGCC_FILE_NAME=`${ANDROID_TOOLCHAIN}/arm-linux-androideabi-gcc \
On 2012/03/13 14:58:59, Peter Beverloo wrote:
> This seems to defeat the purpose of the firstword-searches earlier on in this
> function, why can we refer to the binary name directly here, but not in the
> previous occurrences?

good point, let me fix this..

[Peter Beverloo, 2012-03-13 16:23:08]

LGTM, but deferring to jrg/yfriedman for final judgement.

This works well with WebKit when used together with the patch attached to the
following bug:
https://bugs.webkit.org/show_bug.cgi?id=80861

http://codereview.chromium.org/9693042/diff/4/build/android/envsetup.sh
File build/android/envsetup.sh (right):

http://codereview.chromium.org/9693042/diff/4/build/android/envsetup.sh#newco...
build/android/envsetup.sh:92: firstword() {
minor readability nit: maybe define firstword() before it's being used.

[Yaron, 2012-03-13 16:51:37]

Ami,

Any advice on how to do this better? This works, but you have to decide on goma
or not at gyp time and I think ideally you should just be able to do it
per-compile invocation. However, if we try and setup the build such that we
don't need to set environment variables outside of when gyp is run, and then
rely on setting the path for goma/not goma, we hit issues. 

I believe the problem is with using make_global_settings which causes make/goma
to find the absolute path to the binary instead of relying on the path to be
correct at compile time. Can you point me to the CrOs setup?

[Yaron, 2012-03-13 16:52:24]

Adding the right email address..

[John Grabowski, 2012-03-13 18:55:30]

LGTM!

[commit-bot: I haz the power, 2012-03-13 19:04:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/9693042/4

[Ami GONE FROM CHROMIUM, 2012-03-13 19:18:31]

drive-by

Yaron: not sure what you mean by the "CrOS setup."  I build using
http://code.google.com/p/chromium/wiki/LinuxChromiumArm?#Recipe1:_Building_fo...
where goma-or-not is determined by $PATH at make/ninja-time, not gyp-time.

http://codereview.chromium.org/9693042/diff/4/build/android/envsetup.sh
File build/android/envsetup.sh (right):

http://codereview.chromium.org/9693042/diff/4/build/android/envsetup.sh#newco...
build/android/envsetup.sh:71: CROSS_CC="$(firstword
"${ANDROID_TOOLCHAIN}"/*-gcc)" \
What's the deal w/ firstword (here & below)?
l.53 asserts ANDROID_TOOLCHAIN is a dir; are you worried about the glob matching
multiple files?  Why would the first one be valider than the rest?

[commit-bot: I haz the power, 2012-03-14 03:12:58]

Try job failure for 9693042-4 (retry) (previous was lost) (previous was lost) on
android for steps "Compile, build".
It's a second try, previously, steps "Compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[bulach, 2012-03-14 12:59:40]

thanks all! apologies for having leftover comments, all addressed now...
the latest patch also further cleans up android buildbot_functions.sh, removes
the "old_make" and paves the way for goma (and well, will hopefully make the
bots happier)..

another look please?

http://codereview.chromium.org/9693042/diff/4/build/android/envsetup.sh
File build/android/envsetup.sh (right):

http://codereview.chromium.org/9693042/diff/4/build/android/envsetup.sh#newco...
build/android/envsetup.sh:71: CROSS_CC="$(firstword
"${ANDROID_TOOLCHAIN}"/*-gcc)" \
On 2012/03/13 19:18:31, Ami Fischman wrote:
> What's the deal w/ firstword (here & below)?
> l.53 asserts ANDROID_TOOLCHAIN is a dir; are you worried about the glob
matching
> multiple files?  Why would the first one be valider than the rest?

good point, thanks! :)
probably some legacy leftover, we just had that downstream but I couldn't find
any reason. simplified the lot..

http://codereview.chromium.org/9693042/diff/4/build/android/envsetup.sh#newco...
build/android/envsetup.sh:92: firstword() {
On 2012/03/13 16:23:09, Peter Beverloo wrote:
> minor readability nit: maybe define firstword() before it's being used.

removed.

[Ami GONE FROM CHROMIUM, 2012-03-14 17:13:18]

http://codereview.chromium.org/9693042/diff/10006/build/android/buildbot_func...
File build/android/buildbot_functions.sh (right):

http://codereview.chromium.org/9693042/diff/10006/build/android/buildbot_func...
build/android/buildbot_functions.sh:133: # PATH=$GOMA_DIR make -j${JOBS} "$@"
s/R/R:$PATH/

http://codereview.chromium.org/9693042/diff/10006/build/android/envsetup.sh
File build/android/envsetup.sh (right):

http://codereview.chromium.org/9693042/diff/10006/build/android/envsetup.sh#n...
build/android/envsetup.sh:84: CROSS_CC="$GOMA_DIR/gomacc
${ANDROID_TOOLCHAIN}/*-gcc" \
This is the wrong way to do this.
goma/not-goma should not be a gyp-time decision.
If you simply set all these variables to the basename of the tool
(compiler/linker/etc) then you can determine goma/not-goma at ninja/make-time by
having $PATH either have gomadir first or $ANDROID_TOOLCHAIN first.

http://codereview.chromium.org/9693042/diff/10006/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/9693042/diff/10006/build/common.gypi#newcode2888
build/common.gypi:2888: # make generator needs these variables at gyp time
This is incorrect.  The generated Makefile will take these settings from the
environment (at make time) if present.

[bulach, 2012-03-14 17:36:42]

thanks Ami! I clarified a few items below, please let me know your thoughts:

http://codereview.chromium.org/9693042/diff/10006/build/android/buildbot_func...
File build/android/buildbot_functions.sh (right):

http://codereview.chromium.org/9693042/diff/10006/build/android/buildbot_func...
build/android/buildbot_functions.sh:133: # PATH=$GOMA_DIR make -j${JOBS} "$@"
On 2012/03/14 17:13:18, Ami Fischman wrote:
> s/R/R:$PATH/

Done.

http://codereview.chromium.org/9693042/diff/10006/build/android/envsetup.sh
File build/android/envsetup.sh (right):

http://codereview.chromium.org/9693042/diff/10006/build/android/envsetup.sh#n...
build/android/envsetup.sh:84: CROSS_CC="$GOMA_DIR/gomacc
${ANDROID_TOOLCHAIN}/*-gcc" \
On 2012/03/14 17:13:18, Ami Fischman wrote:
> This is the wrong way to do this.
> goma/not-goma should not be a gyp-time decision.
> If you simply set all these variables to the basename of the tool
> (compiler/linker/etc) then you can determine goma/not-goma at ninja/make-time
by
> having $PATH either have gomadir first or $ANDROID_TOOLCHAIN first.

I think there are a few issues in play in here, let me try to explain... ;)

1. The original problem I'm trying to solve is to remove an evil "make()" /
"old_make()" function..

2. We're also trying to remove environment dependency on $CC* environment
variables at build time, and have gyp generate the make files self-contained.

3. finally, on the gyp generate make.py GenerateOutput (I'm not entirely sure
why), there's:

  for key, value in make_global_settings_dict:
    if value[0] != '$':
      value = '$(abspath %s)' % value


which requires us to pass the abspath (otherwise we'd end up with invalid
paths), and hence to generate  makefiles for either goma or non-goma...

I'm not sure how to best solve all of these, any ideas?

http://codereview.chromium.org/9693042/diff/10006/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/9693042/diff/10006/build/common.gypi#newcode2888
build/common.gypi:2888: # make generator needs these variables at gyp time
On 2012/03/14 17:13:18, Ami Fischman wrote:
> This is incorrect.  The generated Makefile will take these settings from the
> environment (at make time) if present.

sorry, the comment wasn't clear.
we're doing this on purpose, so that we don't need any environment variable
being set during make time, only these ones at gyp time...

[Ami GONE FROM CHROMIUM, 2012-03-14 17:57:54]

http://codereview.chromium.org/9693042/diff/10006/build/android/envsetup.sh
File build/android/envsetup.sh (right):

http://codereview.chromium.org/9693042/diff/10006/build/android/envsetup.sh#n...
build/android/envsetup.sh:84: CROSS_CC="$GOMA_DIR/gomacc
${ANDROID_TOOLCHAIN}/*-gcc" \
On 2012/03/14 17:36:43, bulach wrote:
> 3. finally, on the gyp generate make.py GenerateOutput (I'm not entirely sure
> why), there's:
>   for key, value in make_global_settings_dict:
>     if value[0] != '$':
>       value = '$(abspath %s)' % value

I believe this is the core of your problem.  Why are you using
make_global_settings in the first place?

[bulach, 2012-03-14 18:28:31]

thanks Ami! more inline, torne@ can provide more details:

http://codereview.chromium.org/9693042/diff/10006/build/android/envsetup.sh
File build/android/envsetup.sh (right):

http://codereview.chromium.org/9693042/diff/10006/build/android/envsetup.sh#n...
build/android/envsetup.sh:84: CROSS_CC="$GOMA_DIR/gomacc
${ANDROID_TOOLCHAIN}/*-gcc" \
On 2012/03/14 17:57:54, Ami Fischman wrote:
> On 2012/03/14 17:36:43, bulach wrote:
> > 3. finally, on the gyp generate make.py GenerateOutput (I'm not entirely
sure
> > why), there's:
> >   for key, value in make_global_settings_dict:
> >     if value[0] != '$':
> >       value = '$(abspath %s)' % value
> 
> I believe this is the core of your problem.  Why are you using
> make_global_settings in the first place?

torne can explain in more details.. going forward with the android unification
and other build systems, we want makefiles to be self-contained and not depend
on setup scripts / environment variables...
the only way we could do that is by using this make_global_settings, but if
there's any other suggestion, I'd be happy to follow! :)

[Ami GONE FROM CHROMIUM, 2012-03-14 19:43:00]

>
> going forward with the android
> unification and other build systems, we want makefiles to be
> self-contained and not depend on setup scripts / environment
> variables...
>

That sounds fine to me.  Setting the env vars for basenames to
cross/host-tools at gyp-time gets you the right toolchains.
$PATH resolution at make/ninja-time gets you goma/not-goma.

the only way we could do that is by using this make_global_settings, but
> if there's any other suggestion, I'd be happy to follow! :)


Why do you need m_g_s to set the compilers?
As an example,
http://code.google.com/p/chromium/wiki/LinuxChromiumArm?#Recipe1:_Building_fo...
is
how I cross-compile chrome for ARM on my desktop.  The resulting
build.ninja (or Makefile, if I generate that) has the cross-compiler
information baked in, and the only env var that matters at make/ninja-time
in that setup is $PATH (for goma/not-goma resolution).

Maybe a better way to get at this is to ask: what's failing for you if you
set the ${CC,CXX,AR,LINK}{,.host} to the relevant basenames and then run
gyp?

[bulach, 2012-03-14 20:04:06]

On 2012/03/14 19:43:00, Ami Fischman wrote:
> >
> > going forward with the android
> > unification and other build systems, we want makefiles to be
> > self-contained and not depend on setup scripts / environment
> > variables...
> >
> 
> That sounds fine to me.  Setting the env vars for basenames to
> cross/host-tools at gyp-time gets you the right toolchains.
> $PATH resolution at make/ninja-time gets you goma/not-goma.
> 
> the only way we could do that is by using this make_global_settings, but
> > if there's any other suggestion, I'd be happy to follow! :)
> 
> 
> Why do you need m_g_s to set the compilers?
> As an example,
>
http://code.google.com/p/chromium/wiki/LinuxChromiumArm?#Recipe1:_Building_fo...
> is
> how I cross-compile chrome for ARM on my desktop.  

my understanding from that page is that the CXX_target and other *_target
variables are exported, no? that's what we don't want to do... :)

> The resulting
> build.ninja (or Makefile, if I generate that) has the cross-compiler
> information baked in, and the only env var that matters at make/ninja-time
> in that setup is $PATH (for goma/not-goma resolution).

again, I'm not too familiar with gyp+ninja, but mine and yaron's tests seems
that:
1. ninja: will generate self contained files with the right paths already baked
in.
2. Makefile: unless we set via m_g_s, it'd get at build time, from the env
variables, which we don't want to set...

> Maybe a better way to get at this is to ask: what's failing for you if you
> set the ${CC,CXX,AR,LINK}{,.host} to the relevant basenames and then run
> gyp?

as above, I think the Makefile generator won't get this if not set via m_g_s..
I'll test again to double check that, but afaict it'd fallback to using the
tools and build time (not gyp time) from the env, which we don't want..

note as well that we're not yet fully supporting ninja, and we'll not be able to
totally drop Makefile either, but if your set up works with both, I'd be happy
to follow!

[Ami GONE FROM CHROMIUM, 2012-03-14 20:09:34]

>
> my understanding from that page is that the CXX_target and other *_target
> variables are exported, no? that's what we don't want to do... :)


export is unnecessary in that setup.

As above, I think the Makefile generator won't get this if not set via
> m_g_s..

I'll test again to double check that, but afaict it'd fallback to using the
> tools and build time (not gyp time) from the env, which we don't want..
>

Is the problem that you want to allow the user to have $CC/$CXX/etc to be
set to something *else* at make-time and have those be ignored?
I don't think that's possible.
But ISTM trying to set up a Makefile to ignore env vars is at odds with
leaving paths not-absolute (to support goma/not-goma at make-time).
Maybe all you need is to unset $CC/CXX/etc in the env after gyp is run so
that what's baked into the Makefile is what is used at make-time?

[bulach, 2012-03-15 10:35:59]

Hi Ami,

first of all, thanks for your patience, really appreciate!
I think I'm still missing some points, let me clarify my questions below:

On 2012/03/14 20:09:34, Ami Fischman wrote:
> >
> > my understanding from that page is that the CXX_target and other *_target
> > variables are exported, no? that's what we don't want to do... :)
> 
> 
> export is unnecessary in that setup.

afaict, we need to either define CXX*_target in the "make" call, or export that
in the environment (note: make, not ninja...).
I don't see how the Makefile would get the CXX*target variables when executing,
would you mind explaining if there's a mechanism I'm missing?

> 
> As above, I think the Makefile generator won't get this if not set via
> > m_g_s..
> 
> I'll test again to double check that, but afaict it'd fallback to using the
> > tools and build time (not gyp time) from the env, which we don't want..
> >
> 
> Is the problem that you want to allow the user to have $CC/$CXX/etc to be
> set to something *else* at make-time and have those be ignored?
> I don't think that's possible.

torne@ can explain in more details, what we want is to NOT have anything set /
exported at make-time..

> But ISTM trying to set up a Makefile to ignore env vars is at odds with
> leaving paths not-absolute (to support goma/not-goma at make-time).
> Maybe all you need is to unset $CC/CXX/etc in the env after gyp is run so
> that what's baked into the Makefile is what is used at make-time?

that's exactly what I'm missing :)
I can't see a way to bake anything in the Makefile during gyp-time, only via
m_g_s..
ninja does it fine, but not the Makefile.. is it correct?

[torne_google.com, 2012-03-15 10:45:39]

On 15 March 2012 10:35,  <bulach@chromium.org> wrote:
> Hi Ami,
>
> first of all, thanks for your patience, really appreciate!
> I think I'm still missing some points, let me clarify my questions below:
>
>
> On 2012/03/14 20:09:34, Ami Fischman wrote:
>>
>> >
>> > my understanding from that page is that the CXX_target and other
>> > *_target
>> > variables are exported, no? that's what we don't want to do... :)
>
>
>
>> export is unnecessary in that setup.
>
>
> afaict, we need to either define CXX*_target in the "make" call, or export
> that
> in the environment (note: make, not ninja...).
> I don't see how the Makefile would get the CXX*target variables when
> executing,
> would you mind explaining if there's a mechanism I'm missing?
>
>
>
>> As above, I think the Makefile generator won't get this if not set via
>> > m_g_s..
>
>
>> I'll test again to double check that, but afaict it'd fallback to using
>> the
>> > tools and build time (not gyp time) from the env, which we don't want..
>> >
>
>
>> Is the problem that you want to allow the user to have $CC/$CXX/etc to be
>> set to something *else* at make-time and have those be ignored?
>> I don't think that's possible.
>
>
> torne@ can explain in more details, what we want is to NOT have anything set
> /
> exported at make-time..

What we want to be able to do is this:
1) developer starts new shell with no special setup
2) developer runs a script which ends up invoking gyp - this can pass
any variables/etc needed, we control this completely
3) developer throws that shell away and starts a new one with no special setup
4) developer runs make

For this to work, the generated Makefile from the gyp make backend
needs to contain the names of the crosscompilers in some form, which
will ultimately set CC.target and friends at build time. We want to do
it this way because we're building in a number of complicated
environments (inside the android tree, outside the android tree, as a
child of the actual android build system, possibly others), and
setting the variables in the environment at build time is either very
fiddly, or causes problems for other parts of Android built in the
same session.

We're not concerned about the environment overriding the Makefile;
nothing else is setting these variables in the environment. It's fine
that if they are set, they take priority. We just want to change the
behaviour when *none* of the variables are set in the environment at
the time "make" is invoked.

>
>> But ISTM trying to set up a Makefile to ignore env vars is at odds with
>> leaving paths not-absolute (to support goma/not-goma at make-time).
>> Maybe all you need is to unset $CC/CXX/etc in the env after gyp is run so
>> that what's baked into the Makefile is what is used at make-time?
>
>
> that's exactly what I'm missing :)
> I can't see a way to bake anything in the Makefile during gyp-time, only via
> m_g_s..
> ninja does it fine, but not the Makefile.. is it correct?
>
> http://codereview.chromium.org/9693042/



-- 
Torne (Richard Coles)
torne@google.com

[Ami GONE FROM CHROMIUM, 2012-03-15 16:44:38]

I was wrong in thinking that make.py baked CC/CXX/etc into Makefile the way
ninja.py bakes them into build.ninja.
IMO that's how it should be, but it isn't.
(if it were, then I think make_global_settings wouldn't need to exist)

I started a thread on gyp-developer asking about the abspath'ification of
make_global_settings' values.

-a

[bulach, 2012-03-15 16:51:17]

thanks Ami! I think we're in the same page now... :)
it'd be great for the gyp generator for both ninja / make  to be consistent in
their way that CC/CXX gets baked in, thanks for driving this with gyp-developer!

any advise on how to proceed? should I go ahead with this or wait for a gyp
patch?

On 2012/03/15 16:44:38, Ami Fischman wrote:
> I was wrong in thinking that make.py baked CC/CXX/etc into Makefile the way
> ninja.py bakes them into build.ninja.
> IMO that's how it should be, but it isn't.
> (if it were, then I think make_global_settings wouldn't need to exist)
> 
> I started a thread on gyp-developer asking about the abspath'ification of
> make_global_settings' values.
> 
> -a

[Ami GONE FROM CHROMIUM, 2012-03-15 16:59:27]

On 2012/03/15 16:51:17, bulach wrote:
> any advise on how to proceed? should I go ahead with this or wait for a gyp
> patch?

I can give you the pros/cons, but I can't choose for your users.
Before your CL your users have to have the env vars set at make time (bad1), but
can choose goma/not-goma without having to re-gyp (good1).
After your CL your users must select goma/not-goma at gyp time (bad2), but can
avoid having env vars set at make-time (good2).

IMO bad1 is significantly less annoying than bad2 (b/c bad1 is solved with a
.bashrc-equivalent once and never thought about again, whereas bad2 imposes a
20-40s tax on changing your mind).  So if it were me I'd not land this CL.

[Ami GONE FROM CHROMIUM, 2012-03-15 17:00:17]

> I can't choose for your users.

To be clear, I didn't mean to be a reviewer on this CL, only a kibitzer. 
Officially removed myself from the reviewer list so you don't need an LGTM from
me.

[bulach, 2012-03-15 17:33:32]

thanks ami!

I'll defer the final decision to jrg@, yaron@ and torne@ then: please, take a
look at the points above and let me know your thoughts!

[John Grabowski, 2012-03-16 04:23:35]

Were I to make a "final" decision, it is LGTM.

We live in an imperfect world.
This CL is imperfect but a make() alias/function blows.

Note that Make has a dep on the gypfiles themselves and re-gyps when needed. 
The only follow-up I would like is to somehow make sure the re-gyp is the
"correct" re-gyp to avoid surprises.

[bulach, 2012-03-16 16:57:20]

Trying to make two johns happy at once :)
jknotten also complained about the re-gyp issue.
the latest patch fixes it (similarly to downstream), and also removes the dual
"android*gyp" function... now the goma/non-goma is toggled by a GOMA_DIR
environment variable set up..

another look please?

[Peter Beverloo, 2012-03-16 17:31:33]

Still LGTM (but once agian, deferring to jrg). Verified that this works well
with WebKit (after some minor changes), both with and without goma.

http://codereview.chromium.org/9693042/diff/12008/build/android/envsetup.sh
File build/android/envsetup.sh (right):

http://codereview.chromium.org/9693042/diff/12008/build/android/envsetup.sh#n...
build/android/envsetup.sh:87: export STRIP=$(echo ${ANDROID_TOOLCHAIN}/*-strip)
nit: you use "echo" here while using "basename" above, we should probably use
the same way for both.

[John Grabowski, 2012-03-19 21:08:05]

Still LGTM 

Let me know when all this is landed; don't want to try and get goma up while
these changes are in-flight

[commit-bot: I haz the power, 2012-03-20 08:30:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/9693042/12008

[commit-bot: I haz the power, 2012-03-20 10:45:28]

Change committed as 127667