Set use_goma=1 and gomadir=path in GYP_DEFINES on master.

Set use_goma=1 and gomadir=path in GYP_DEFINES on master.

That way, the bots will use goma the same way developers do. It's also what
recipes and the android buildbot already use.

Once all masters were restarted with this change,
scripts/slave/compile.py can be simplified.

Eventually, --compiler=goma-clang can go away, since --compiler=goma and
GYP_DEFINES=clang=1 should do the right thing after this change.

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

[Nico, 2014-04-08 20:20:45]

Here's a slightly different take on https://codereview.chromium.org/142373002/ .
It uses the fact that goma is always in /b/build/goma (posix) or c:\build\b\goma
on the bots, so the logic can be kept on the master only. (…and once
everything's on recipes, this will all go away anyways.)

[iannucci, 2014-04-08 20:45:50]

https://codereview.chromium.org/228903003/diff/20001/build/scripts/master/fac...
File build/scripts/master/factory/chromium_factory.py (right):

https://codereview.chromium.org/228903003/diff/20001/build/scripts/master/fac...
build/scripts/master/factory/chromium_factory.py:99: gomadir = '/b/build/goma'
!!!!!! :(

This terrifies me (and won't actually work on half of the slaves)... is there a
way to not do this? Why can't we discover this path in gyp_chromium if use_goma
is set but gomadir is not?

[Nico, 2014-04-08 20:51:09]

https://codereview.chromium.org/228903003/diff/20001/build/scripts/master/fac...
File build/scripts/master/factory/chromium_factory.py (right):

https://codereview.chromium.org/228903003/diff/20001/build/scripts/master/fac...
build/scripts/master/factory/chromium_factory.py:99: gomadir = '/b/build/goma'
On 2014/04/08 20:45:50, iannucci wrote:
> !!!!!! :(
> 
> This terrifies me (and won't actually work on half of the slaves)... is there
a
> way to not do this?

We could add a gyp_chromium wrapper script. It's correct for the slaves I looked
at though, so I figured maybe we could get away without doing that.

Which slaves does this not work for?

> Why can't we discover this path in gyp_chromium if use_goma
> is set but gomadir is not?

[iannucci, 2014-04-08 20:54:04]

On 2014/04/08 20:51:09, Nico wrote:
>
https://codereview.chromium.org/228903003/diff/20001/build/scripts/master/fac...
> File build/scripts/master/factory/chromium_factory.py (right):
> 
>
https://codereview.chromium.org/228903003/diff/20001/build/scripts/master/fac...
> build/scripts/master/factory/chromium_factory.py:99: gomadir = '/b/build/goma'
> On 2014/04/08 20:45:50, iannucci wrote:
> > !!!!!! :(
> > 
> > This terrifies me (and won't actually work on half of the slaves)... is
there
> a
> > way to not do this?
> 
> We could add a gyp_chromium wrapper script. It's correct for the slaves I
looked
> at though, so I figured maybe we could get away without doing that.
> 
> Which slaves does this not work for?

Some windows slaves have E:\b\build for example, but in general it's a timebomb
:)

> 
> > Why can't we discover this path in gyp_chromium if use_goma
> > is set but gomadir is not?

[Nico, 2014-04-08 20:58:09]

On Tue, Apr 8, 2014 at 1:54 PM, <iannucci@chromium.org> wrote:

> On 2014/04/08 20:51:09, Nico wrote:
>
> https://codereview.chromium.org/228903003/diff/20001/
> build/scripts/master/factory/chromium_factory.py
>
>> File build/scripts/master/factory/chromium_factory.py (right):
>>
>
>
> https://codereview.chromium.org/228903003/diff/20001/
> build/scripts/master/factory/chromium_factory.py#newcode99
>
>> build/scripts/master/factory/chromium_factory.py:99: gomadir =
>> '/b/build/goma'
>> On 2014/04/08 20:45:50, iannucci wrote:
>> > !!!!!! :(
>> >
>> > This terrifies me (and won't actually work on half of the slaves)... is
>>
> there
>
>> a
>> > way to not do this?
>>
>
>  We could add a gyp_chromium wrapper script. It's correct for the slaves I
>>
> looked
>
>> at though, so I figured maybe we could get away without doing that.
>>
>
>  Which slaves does this not work for?
>>
>
> Some windows slaves have E:\b\build for example


I think this can be fixed by using a drive-relative path (i.e.
'\b\build\goma'). Are there other issues?


> , but in general it's a timebomb
>

Well, it only needs to be better than the timebomb currently living in
compile.py (which I've already triggered), since new waterfalls won't be
using legacy buildbot.


> :)
>
>
>
>  > Why can't we discover this path in gyp_chromium if use_goma
>> > is set but gomadir is not?
>>
>
>
>
> https://codereview.chromium.org/228903003/
>

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

[iannucci, 2014-04-08 22:25:37]

On 2014/04/08 20:58:09, Nico wrote:
> On Tue, Apr 8, 2014 at 1:54 PM, <mailto:iannucci@chromium.org> wrote:
> 
> > On 2014/04/08 20:51:09, Nico wrote:
> >
> > https://codereview.chromium.org/228903003/diff/20001/
> > build/scripts/master/factory/chromium_factory.py
> >
> >> File build/scripts/master/factory/chromium_factory.py (right):
> >>
> >
> >
> > https://codereview.chromium.org/228903003/diff/20001/
> > build/scripts/master/factory/chromium_factory.py#newcode99
> >
> >> build/scripts/master/factory/chromium_factory.py:99: gomadir =
> >> '/b/build/goma'
> >> On 2014/04/08 20:45:50, iannucci wrote:
> >> > !!!!!! :(
> >> >
> >> > This terrifies me (and won't actually work on half of the slaves)... is
> >>
> > there
> >
> >> a
> >> > way to not do this?
> >>
> >
> >  We could add a gyp_chromium wrapper script. It's correct for the slaves I
> >>
> > looked
> >
> >> at though, so I figured maybe we could get away without doing that.
> >>
> >
> >  Which slaves does this not work for?
> >>
> >
> > Some windows slaves have E:\b\build for example
> 
> 
> I think this can be fixed by using a drive-relative path (i.e.
> '\b\build\goma'). Are there other issues?

so instead of TIMEBOMB:\timebomb we'll just have \timebomb? :/

I guess to really fix this, the entity which is producing this path also needs
to be responsible for downloading goma (currently it's slave.DEPS/DEPS).

> 
> 
> > , but in general it's a timebomb
> >
> 
> Well, it only needs to be better than the timebomb currently living in
> compile.py (which I've already triggered), since new waterfalls won't be
> using legacy buildbot.

yeah, but at least in compile.py we could change it without restarting the
master. That will not be the case after this change.

> 
> 
> > :)
> >
> >
> >
> >  > Why can't we discover this path in gyp_chromium if use_goma
> >> > is set but gomadir is not?
> >>
> >
> >
> >
> > https://codereview.chromium.org/228903003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Nico, 2014-04-15 18:10:28]

Ok, I changed this to use a wrapper script for runhooks, so that the goma dir
can be determined on the slave and can be changed without master restarts. PTAL.

[Sam Clegg, 2014-04-15 18:23:13]

I'd love to see this land so that the ARM cross builder can start using goma.

https://codereview.chromium.org/228903003/diff/50001/build/scripts/slave/runh...
File build/scripts/slave/runhooks_wrapper.py (right):

https://codereview.chromium.org/228903003/diff/50001/build/scripts/slave/runh...
build/scripts/slave/runhooks_wrapper.py:27: parser = optparse.OptionParser()
I find that adding description=__doc__ here makes --help much nicer.

https://codereview.chromium.org/228903003/diff/50001/build/scripts/slave/runh...
build/scripts/slave/runhooks_wrapper.py:48: 
nit: trailing newline

[Nico, 2014-04-15 18:30:40]

https://codereview.chromium.org/228903003/diff/50001/build/scripts/slave/runh...
File build/scripts/slave/runhooks_wrapper.py (right):

https://codereview.chromium.org/228903003/diff/50001/build/scripts/slave/runh...
build/scripts/slave/runhooks_wrapper.py:27: parser = optparse.OptionParser()
On 2014/04/15 18:23:14, Sam Clegg wrote:
> I find that adding description=__doc__ here makes --help much nicer.

Done.

https://codereview.chromium.org/228903003/diff/50001/build/scripts/slave/runh...
build/scripts/slave/runhooks_wrapper.py:48: 
On 2014/04/15 18:23:14, Sam Clegg wrote:
> nit: trailing newline

Done.

[iannucci, 2014-04-15 18:45:29]

Thanks for doing this... sorry for being a pain in the ass about it :)

https://codereview.chromium.org/228903003/diff/70001/build/scripts/slave/runh...
File build/scripts/slave/runhooks_wrapper.py (right):

https://codereview.chromium.org/228903003/diff/70001/build/scripts/slave/runh...
build/scripts/slave/runhooks_wrapper.py:23: BUILD_DIR =
os.path.dirname(os.path.dirname(SLAVE_SCRIPTS_DIR))
can we put an os.path.abspath(...) around this?

https://codereview.chromium.org/228903003/diff/70001/build/scripts/slave/runh...
build/scripts/slave/runhooks_wrapper.py:38: gyp_defines =
os.environ.get('GYP_DEFINES', '')
should we check this to ensure it doesn't already have goma stuff in it?

https://codereview.chromium.org/228903003/diff/70001/build/scripts/slave/runh...
build/scripts/slave/runhooks_wrapper.py:39: gyp_defines += ' use_goma=1
gomadir=' + options.goma_dir
IIRC, gomadir needs stupid quoting on windows. I believe it's being parsed with
shlex. I think you can use the python pipes.quote
(https://docs.python.org/2.7/library/pipes.html#pipes.quote) method (yes, it's
undocumented/deprecated)

[Nico, 2014-04-15 18:56:42]

https://codereview.chromium.org/228903003/diff/70001/build/scripts/slave/runh...
File build/scripts/slave/runhooks_wrapper.py (right):

https://codereview.chromium.org/228903003/diff/70001/build/scripts/slave/runh...
build/scripts/slave/runhooks_wrapper.py:23: BUILD_DIR =
os.path.dirname(os.path.dirname(SLAVE_SCRIPTS_DIR))
On 2014/04/15 18:45:29, iannucci wrote:
> can we put an os.path.abspath(...) around this?

This is copypasta'd from compile.py. The SLAVE_SCRIPTS_DIR has an abspath() call
already.

https://codereview.chromium.org/228903003/diff/70001/build/scripts/slave/runh...
build/scripts/slave/runhooks_wrapper.py:38: gyp_defines =
os.environ.get('GYP_DEFINES', '')
On 2014/04/15 18:45:29, iannucci wrote:
> should we check this to ensure it doesn't already have goma stuff in it?

I think it doesn't matter much. No master currently sets use_goma, and setting
it twice is harmless. Masters don't have enough information to set gomadir
(hence this patch).

https://codereview.chromium.org/228903003/diff/70001/build/scripts/slave/runh...
build/scripts/slave/runhooks_wrapper.py:39: gyp_defines += ' use_goma=1
gomadir=' + options.goma_dir
On 2014/04/15 18:45:29, iannucci wrote:
> IIRC, gomadir needs stupid quoting on windows. I believe it's being parsed
with
> shlex. I think you can use the python pipes.quote
> (https://docs.python.org/2.7/library/pipes.html#pipes.quote) method (yes, it's
> undocumented/deprecated)

Oh right, good catch! Done.

[iannucci, 2014-04-15 19:00:36]

lgtm thanks

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

The CQ bit was checked by thakis@chromium.org

[commit-bot: I haz the power, 2014-04-15 19:02:29]

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

[commit-bot: I haz the power, 2014-04-15 19:04:02]

Change committed as 263959

[Nico, 2014-04-15 19:27:04]

Apparently Windows doesn't like the subprocess thing:

http://build.chromium.org/p/chromium.fyi/builders/Chromium%20Win%20Ninja%20Go...

gclient runhooks
Traceback (most recent call last):
  File "..\..\..\scripts\slave\runhooks_wrapper.py", line 49, in <module>
    sys.exit(main())
  File "..\..\..\scripts\slave\runhooks_wrapper.py", line 45, in main
    return chromium_utils.RunCommand(['gclient', 'runhooks'])
  File "C:\b\build\scripts\common\chromium_utils.py", line 868, in
RunCommand
    bufsize=0, **kwargs)
  File "C:\b\depot_tools\python276_bin\lib\subprocess.py", line 709, in
__init__
    errread, errwrite)
  File "C:\b\depot_tools\python276_bin\lib\subprocess.py", line 957, in
_execute_child
    startupinfo)
WindowsError: [Error 2] The system cannot find the file specified


Have you seen this before? Do I have to run gclient through python?



On Tue, Apr 15, 2014 at 12:04 PM, <commit-bot@chromium.org> wrote:

> Change committed as 263959
>
> https://codereview.chromium.org/228903003/
>

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

[Nico, 2014-04-15 19:46:20]

Hm, maybe https://codereview.chromium.org/239153006 will help.


On Tue, Apr 15, 2014 at 12:27 PM, Nico Weber <thakis@chromium.org> wrote:

> Apparently Windows doesn't like the subprocess thing:
>
>
>
http://build.chromium.org/p/chromium.fyi/builders/Chromium%20Win%20Ninja%20Go...
>
> gclient runhooks
> Traceback (most recent call last):
>   File "..\..\..\scripts\slave\runhooks_wrapper.py", line 49, in <module>
>     sys.exit(main())
>   File "..\..\..\scripts\slave\runhooks_wrapper.py", line 45, in main
>     return chromium_utils.RunCommand(['gclient', 'runhooks'])
>   File "C:\b\build\scripts\common\chromium_utils.py", line 868, in
> RunCommand
>     bufsize=0, **kwargs)
>   File "C:\b\depot_tools\python276_bin\lib\subprocess.py", line 709, in
> __init__
>     errread, errwrite)
>   File "C:\b\depot_tools\python276_bin\lib\subprocess.py", line 957, in
> _execute_child
>     startupinfo)
> WindowsError: [Error 2] The system cannot find the file specified
>
>
> Have you seen this before? Do I have to run gclient through python?
>
>
>
> On Tue, Apr 15, 2014 at 12:04 PM, <commit-bot@chromium.org> wrote:
>
>> Change committed as 263959
>>
>> https://codereview.chromium.org/228903003/
>>
>
>

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

[Nico, 2014-04-15 19:54:42]

Looks like that did it.


On Tue, Apr 15, 2014 at 12:46 PM, Nico Weber <thakis@chromium.org> wrote:

> Hm, maybe https://codereview.chromium.org/239153006 will help.
>
>
> On Tue, Apr 15, 2014 at 12:27 PM, Nico Weber <thakis@chromium.org> wrote:
>
>> Apparently Windows doesn't like the subprocess thing:
>>
>>
>>
http://build.chromium.org/p/chromium.fyi/builders/Chromium%20Win%20Ninja%20Go...
>>
>> gclient runhooks
>> Traceback (most recent call last):
>>   File "..\..\..\scripts\slave\runhooks_wrapper.py", line 49, in <module>
>>     sys.exit(main())
>>   File "..\..\..\scripts\slave\runhooks_wrapper.py", line 45, in main
>>     return chromium_utils.RunCommand(['gclient', 'runhooks'])
>>   File "C:\b\build\scripts\common\chromium_utils.py", line 868, in
>> RunCommand
>>     bufsize=0, **kwargs)
>>   File "C:\b\depot_tools\python276_bin\lib\subprocess.py", line 709, in
>> __init__
>>     errread, errwrite)
>>   File "C:\b\depot_tools\python276_bin\lib\subprocess.py", line 957, in
>> _execute_child
>>     startupinfo)
>> WindowsError: [Error 2] The system cannot find the file specified
>>
>>
>> Have you seen this before? Do I have to run gclient through python?
>>
>>
>>
>> On Tue, Apr 15, 2014 at 12:04 PM, <commit-bot@chromium.org> wrote:
>>
>>> Change committed as 263959
>>>
>>> https://codereview.chromium.org/228903003/
>>>
>>
>>
>

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