Add gomadir for GYP_DEFINES

Add use_goma for GYP_DEFINES

If use_goma is specified in GYP_DEFINES, gomacc is automatically set as
CC wrapper and CXX wrapper.  It uses the default goma directory if the gomadir
option is not given.  No need to fix PATH when running ninja (or make).
This is the revival of https://codereview.chromium.org/12419003/,
and expected to work with https://codereview.chromium.org/66303010/.
Note: Goma team suggest not to put gomadir in PATH if you use use_goma option.

Currently, this works well with ninja (default build system of chromium).
It does not work well with make in some case (e.g. build on linux with make).

According to the review comment, ANDROID_GOMA_WRAPPER has already been
obsoleted.  ANDROID_GOMA_WRAPPER will be just ignored after this change.

Example 1
$ GYP_DEFINES="clang=1 use_goma=1" build/gyp_chromium
(no need to set CC/CXX at this stage)

build.ninja will have
cc = ${HOME}/gomacc ../../third_party/llvm-build/Release+Asserts/bin/clang
cxx = ${HOME}/gomacc ../../third_party/llvm-build/Release+Asserts/bin/clang++

The user can run ninja without setting PATH or so.
$ ninja -C out/Release -j100

Example 2

$ GYP_DEFINES="use_goma=1 gomadir=/path/to/goma" build/gyp_chromium

build.ninja will have
cc = /path/to/goma/gomacc gcc
cxx = /path/to/goma/gomacc g++

user can run ninja without setting PATH or so.
$ ninja -C out/Release -j100

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

[Yoshisato Yanagisawa, 2013-11-12 01:52:46]

Will you check this before I will ask reviewers you have listed in
https://codereview.chromium.org/12419003/?

Thank you in advance,

[Yoshisato Yanagisawa, 2013-11-13 22:08:35]

Hi,

This is the revival of https://codereview.chromium.org/12419003/.
I hope we can provide unified way to start using goma on every platform.

Will you review this?

Thank you in advance,

[Nico, 2013-11-13 22:12:14]

I'll repeat what I said on the old CL: """I too am not sure if starting goma at
gyp time will work well.

Most confusion seems to be caused by people not setting their PATH correctly,
which is solved by the _wrapper parts of this CL. Maybe we should require people
start up goma manually for now (i.e. omit that part from this CL) and see if
that's good enough? If people continue to get that wrong, we can add that part
later."""

I'd also just support goma=1, which would assume that goma is in $HOME/goma.
That makes setting it easier.

[Yoshisato Yanagisawa, 2013-11-13 22:41:08]

On 2013/11/13 22:12:14, Nico wrote:
> I'll repeat what I said on the old CL: """I too am not sure if starting goma
at
> gyp time will work well.
> 
> Most confusion seems to be caused by people not setting their PATH correctly,
> which is solved by the _wrapper parts of this CL. Maybe we should require
people
> start up goma manually for now (i.e. omit that part from this CL) and see if
> that's good enough? If people continue to get that wrong, we can add that part
> later."""

Let me ask expected behavior if goma is not available.
Is it fine to fail at build time?

> I'd also just support goma=1, which would assume that goma is in $HOME/goma.
> That makes setting it easier.

Since we need to keep supporting ANDROID_GOMA_DIR env, how about doing like
this?
- goma=1 -> use gomacc in the "default (commonly used in manual)" goma
directory.
- having ANDROID_GOMA_DIR env. + OS==android -> automatically set goma=1 and set
gomadir=$ANDROID_GOMA_DIR.
- goma=1 and gomadir=xxx -> add gomacc in the gomadir directory.
- otherwise -> do not use goma.

[michaelbai, 2013-11-13 22:56:24]

Why do you need to support ANDROID_GOMA_DIR?

FYI, I use  'PATH=/path/to/goma/:$PATH ninja -C out/Debug -j 200 all' to
build chrome android, it works well.


On Wed, Nov 13, 2013 at 2:41 PM, <yyanagisawa@chromium.org> wrote:

> On 2013/11/13 22:12:14, Nico wrote:
>
>> I'll repeat what I said on the old CL: """I too am not sure if starting
>> goma
>>
> at
>
>> gyp time will work well.
>>
>
>  Most confusion seems to be caused by people not setting their PATH
>> correctly,
>> which is solved by the _wrapper parts of this CL. Maybe we should require
>>
> people
>
>> start up goma manually for now (i.e. omit that part from this CL) and see
>> if
>> that's good enough? If people continue to get that wrong, we can add that
>> part
>> later."""
>>
>
> Let me ask expected behavior if goma is not available.
> Is it fine to fail at build time?
>
>
>  I'd also just support goma=1, which would assume that goma is in
>> $HOME/goma.
>> That makes setting it easier.
>>
>
> Since we need to keep supporting ANDROID_GOMA_DIR env, how about doing like
> this?
> - goma=1 -> use gomacc in the "default (commonly used in manual)" goma
> directory.
> - having ANDROID_GOMA_DIR env. + OS==android -> automatically set goma=1
> and set
> gomadir=$ANDROID_GOMA_DIR.
> - goma=1 and gomadir=xxx -> add gomacc in the gomadir directory.
> - otherwise -> do not use goma.
>
>
> https://codereview.chromium.org/69293004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2013-11-13 23:00:22]

On Wed, Nov 13, 2013 at 2:41 PM, <yyanagisawa@chromium.org> wrote:

> On 2013/11/13 22:12:14, Nico wrote:
>
>> I'll repeat what I said on the old CL: """I too am not sure if starting
>> goma
>>
> at
>
>> gyp time will work well.
>>
>
>  Most confusion seems to be caused by people not setting their PATH
>> correctly,
>> which is solved by the _wrapper parts of this CL. Maybe we should require
>>
> people
>
>> start up goma manually for now (i.e. omit that part from this CL) and see
>> if
>> that's good enough? If people continue to get that wrong, we can add that
>> part
>> later."""
>>
>
> Let me ask expected behavior if goma is not available.
> Is it fine to fail at build time?


I think so. That's the current behavior too. If people run into this a lot,
we can consider automatically starting goma at gyp time, but that's easy to
add on later too. Most people seem to have issues with setting up PATH and
whatnot, so I'd only address that first.


>
>
>  I'd also just support goma=1, which would assume that goma is in
>> $HOME/goma.
>> That makes setting it easier.
>>
>
> Since we need to keep supporting ANDROID_GOMA_DIR env, how about doing like
> this?
> - goma=1 -> use gomacc in the "default (commonly used in manual)" goma
> directory.
> - having ANDROID_GOMA_DIR env. + OS==android -> automatically set goma=1
> and set
> gomadir=$ANDROID_GOMA_DIR.
> - goma=1 and gomadir=xxx -> add gomacc in the gomadir directory.
> - otherwise -> do not use goma.
>

Sounds like maybe ANDROID_GOMA_DIR maybe doesn't have to be supported.


>
>
> https://codereview.chromium.org/69293004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Yoshisato Yanagisawa, 2013-11-14 00:10:18]

On 2013/11/13 23:00:22, Nico wrote:
> On Wed, Nov 13, 2013 at 2:41 PM, <mailto:yyanagisawa@chromium.org> wrote:

> > Let me ask expected behavior if goma is not available.
> > Is it fine to fail at build time?
> 
> 
> I think so. That's the current behavior too. If people run into this a lot,
> we can consider automatically starting goma at gyp time, but that's easy to
> add on later too. Most people seem to have issues with setting up PATH and
> whatnot, so I'd only address that first.

OK, I will change the gypi like that.
i.e. neigher status check nor compiler_proxy start at gyp. It just set gomacc in
the default directory or given |gomadir| directory as wrapper.

> > Since we need to keep supporting ANDROID_GOMA_DIR env, how about doing like
> > this?
> > - goma=1 -> use gomacc in the "default (commonly used in manual)" goma
> > directory.
> > - having ANDROID_GOMA_DIR env. + OS==android -> automatically set goma=1
> > and set
> > gomadir=$ANDROID_GOMA_DIR.
> > - goma=1 and gomadir=xxx -> add gomacc in the gomadir directory.
> > - otherwise -> do not use goma.
> >
> 
> Sounds like maybe ANDROID_GOMA_DIR maybe doesn't have to be supported.

Can we also think ${CHROME_SRC} used with ANDROID_GOMA_DIR obsolete?
I saw this:
> ['CC', '<!(/bin/echo -n ${ANDROID_GOMA_WRAPPER}
${CHROME_SRC}/<(make_clang_dir)/bin/clang)'],

Is it OK just like this?
['CC', '<(make_clang_dir)/bin/clang'],
or should I keep ${CHROME_SRC}?
['CC', '${CHROME_SRC}/<(make_clang_dir)/bin/clang'],

[Nico, 2013-11-17 10:03:12]

On Thu, Nov 14, 2013 at 12:10 AM, <yyanagisawa@chromium.org> wrote:

> On 2013/11/13 23:00:22, Nico wrote:
>
>  On Wed, Nov 13, 2013 at 2:41 PM, <mailto:yyanagisawa@chromium.org> wrote:
>>
>
>  > Let me ask expected behavior if goma is not available.
>> > Is it fine to fail at build time?
>>
>
>
>  I think so. That's the current behavior too. If people run into this a
>> lot,
>> we can consider automatically starting goma at gyp time, but that's easy
>> to
>> add on later too. Most people seem to have issues with setting up PATH and
>> whatnot, so I'd only address that first.
>>
>
> OK, I will change the gypi like that.
> i.e. neigher status check nor compiler_proxy start at gyp. It just set
> gomacc in
> the default directory or given |gomadir| directory as wrapper.
>
>
>  > Since we need to keep supporting ANDROID_GOMA_DIR env, how about doing
>> like
>> > this?
>> > - goma=1 -> use gomacc in the "default (commonly used in manual)" goma
>> > directory.
>> > - having ANDROID_GOMA_DIR env. + OS==android -> automatically set goma=1
>> > and set
>> > gomadir=$ANDROID_GOMA_DIR.
>> > - goma=1 and gomadir=xxx -> add gomacc in the gomadir directory.
>> > - otherwise -> do not use goma.
>> >
>>
>
>  Sounds like maybe ANDROID_GOMA_DIR maybe doesn't have to be supported.
>>
>
> Can we also think ${CHROME_SRC} used with ANDROID_GOMA_DIR obsolete?
> I saw this:
>
>> ['CC', '<!(/bin/echo -n ${ANDROID_GOMA_WRAPPER}
>>
> ${CHROME_SRC}/<(make_clang_dir)/bin/clang)'],
>
> Is it OK just like this?
> ['CC', '<(make_clang_dir)/bin/clang'],
> or should I keep ${CHROME_SRC}?
> ['CC', '${CHROME_SRC}/<(make_clang_dir)/bin/clang'],
>

I'm not sure. Let's try the simpler one first, and see what happens? This
can run through the trybots. If those are happy, we should check in the
simpler version and hope that nothing breaks.


>
>
>
> https://codereview.chromium.org/69293004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Yoshisato Yanagisawa, 2013-11-21 06:04:05]

On 2013/11/17 10:03:12, Nico (in Tokyo until Nov 25) wrote:
> > Is it OK just like this?
> > ['CC', '<(make_clang_dir)/bin/clang'],
> > or should I keep ${CHROME_SRC}?
> > ['CC', '${CHROME_SRC}/<(make_clang_dir)/bin/clang'],
> >
> 
> I'm not sure. Let's try the simpler one first, and see what happens? This
> can run through the trybots. If those are happy, we should check in the
> simpler version and hope that nothing breaks.

Current change went simple way.

Will you take a look at the change again?

Thanks.

[Yoshisato Yanagisawa, 2013-11-26 11:05:38]

Fixed some issues on Windows.

Will you take another look at the change?
If nothing looks bad, will you give me LGTM?

Thank you in advance,

[Nico, 2013-11-26 21:58:22]

lgtm with comments addressed.

https://codereview.chromium.org/69293004/diff/1240001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/69293004/diff/1240001/build/common.gypi#newco...
build/common.gypi:812: # TODO(yyanagisawa): set "ok" only if goma is available
(if needed).
I'd remove this for now and just check "use_goma==1" below, since this is set
unconditionally at the moment.

https://codereview.chromium.org/69293004/diff/1240001/build/common.gypi#newco...
build/common.gypi:4711: ['goma_status=="ok" and ("<(GENERATOR)"=="ninja" or
clang==1 or OS=="android")', {
I think OS==android implies <(GENERATOR) = ninja, so you can remove the check
for android.

[Yoshisato Yanagisawa, 2013-11-27 02:01:39]

Thanks for the review.

https://codereview.chromium.org/69293004/diff/1240001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/69293004/diff/1240001/build/common.gypi#newco...
build/common.gypi:812: # TODO(yyanagisawa): set "ok" only if goma is available
(if needed).
On 2013/11/26 21:58:23, Nico wrote:
> I'd remove this for now and just check "use_goma==1" below, since this is set
> unconditionally at the moment.

Done.

https://codereview.chromium.org/69293004/diff/1240001/build/common.gypi#newco...
build/common.gypi:4711: ['goma_status=="ok" and ("<(GENERATOR)"=="ninja" or
clang==1 or OS=="android")', {
On 2013/11/26 21:58:23, Nico wrote:
> I think OS==android implies <(GENERATOR) = ninja, so you can remove the check
> for android.

Done.

[Yoshisato Yanagisawa, 2013-11-27 02:32:38]

Since the patch set 8 did not get ${HOME}, I have fixed it in patch set 9.

Will you take another look at this?
Thank you in advance,

[Nico, 2013-11-27 02:35:43]

still lgtm

Sorry, should have caught this :-(

[Yoshisato Yanagisawa, 2013-11-27 02:56:11]

Thanks for the review.
That's what I should have done before sending the code review.

I will hit "Commit" button when trybot results looks not failed by this change.

[commit-bot: I haz the power, 2013-11-29 01:44:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yyanagisawa@chromium.org/69293004/1300001

[commit-bot: I haz the power, 2013-11-29 08:20:43]

Change committed as 237900