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.

Once all masters were restarted with this change (and chrome/android's
different config was updated), scripts/slave/compile.py can be simplified.

BUG=332697

[iannucci, 2014-01-18 09:59:57]

On 2014/01/18 09:56:22, Nico wrote:

Rietveld? More like Flakiveld...

[iannucci, 2014-01-18 10:02:36]

[no, really, I need patch context for this one :D]

So if gomadir is set, that enables some extra-special sauce in gyp? I should
update the recipes to do that as well, then...

[Nico, 2014-01-18 10:07:07]

On 2014/01/18 10:02:36, iannucci wrote:
> [no, really, I need patch context for this one :D]
> 
> So if gomadir is set, that enables some extra-special sauce in gyp? I should
> update the recipes to do that as well, then...

use_goma=1 enables the sauce. gomadir just defines where goma is, it has a good
default (~/goma) which works for devs but not for bots, so we need to set that
too.

(I tried uploading this thrice. The "raw" button works, or you can
apply_issue.py -i 142373002 to look at the whole file locally)

[Nico, 2014-01-18 10:10:28]

4th time is the charm apparently, there's now a context diff on rietveld too.

[iannucci, 2014-01-18 10:12:55]

Comments:

When can options be None?

I think we tend towards "foo not in bar" rather than "not foo in bar"

I don't think that path.normpath / path.join will work correctly on the master
side of things (cross-platform, different filesystem layouts and all that). What
do you think about gyp_chromium looking for the goma folder and filling in
gomadir if it's not set?

[Nico, 2014-01-18 18:47:30]

On Sat, Jan 18, 2014 at 2:12 AM, <iannucci@chromium.org> wrote:

> Comments:
>
> When can options be None?
>

That's the default value of the options parameter of `def ChromiumFactory`.
If it can never be None, someone could try making this parameter mandatory.


> I think we tend towards "foo not in bar" rather than "not foo in bar"
>

Done.


> I don't think that path.normpath / path.join will work correctly on the
> master
> side of things (cross-platform, different filesystem layouts and all that).


It's what ChromiumCommands's __init__ does to set _chromium_script_dir, so
I would think it works.


> What do you think about gyp_chromium looking for the goma folder and
> filling in
> gomadir if it's not set?


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

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

[iannucci, 2014-01-18 20:24:41]

On 2014/01/18 18:47:30, Nico wrote:
> On Sat, Jan 18, 2014 at 2:12 AM, <mailto:iannucci@chromium.org> wrote:
> 
> > Comments:
> >
> > When can options be None?
> >
> 
> That's the default value of the options parameter of `def ChromiumFactory`.
> If it can never be None, someone could try making this parameter mandatory.
> 
> 
> > I think we tend towards "foo not in bar" rather than "not foo in bar"
> >
> 
> Done.
> 
> 
> > I don't think that path.normpath / path.join will work correctly on the
> > master
> > side of things (cross-platform, different filesystem layouts and all that).
> 
> 
> It's what ChromiumCommands's __init__ does to set _chromium_script_dir, so
> I would think it works.
> 
> 

I think that uses a custom PathJoin method for exactly this reason?

> > What do you think about gyp_chromium looking for the goma folder and
> > filling in
> > gomadir if it's not set?
> 
> 
> > https://codereview.chromium.org/142373002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Nico, 2014-01-18 21:00:52]

On Sat, Jan 18, 2014 at 12:24 PM, <iannucci@chromium.org> wrote:

> On 2014/01/18 18:47:30, Nico wrote:
>
>  On Sat, Jan 18, 2014 at 2:12 AM, <mailto:iannucci@chromium.org> wrote:
>>
>
>  > Comments:
>> >
>> > When can options be None?
>> >
>>
>
>  That's the default value of the options parameter of `def
>> ChromiumFactory`.
>> If it can never be None, someone could try making this parameter
>> mandatory.
>>
>
>
>  > I think we tend towards "foo not in bar" rather than "not foo in bar"
>> >
>>
>
>  Done.
>>
>
>
>  > I don't think that path.normpath / path.join will work correctly on the
>> > master
>> > side of things (cross-platform, different filesystem layouts and all
>> that).
>>
>
>
>  It's what ChromiumCommands's __init__ does to set _chromium_script_dir, so
>> I would think it works.
>>
>
>
>
> I think that uses a custom PathJoin method for exactly this reason?


PathJoin more or less does normpath(join(*args)), no? (It uses posixpath
and ntpath, but I don't understand why)


>
>
>  > What do you think about gyp_chromium looking for the goma folder and
>> > filling in
>> > gomadir if it's not set?
>>
>
>
>  > https://codereview.chromium.org/142373002/
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
>
> https://codereview.chromium.org/142373002/
>

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

[iannucci, 2014-01-18 21:07:29]

On 2014/01/18 21:00:52, Nico wrote:
> On Sat, Jan 18, 2014 at 12:24 PM, <mailto:iannucci@chromium.org> wrote:
> 
> > On 2014/01/18 18:47:30, Nico wrote:
> >
> >  On Sat, Jan 18, 2014 at 2:12 AM, <mailto:iannucci@chromium.org> wrote:
> >>
> >
> >  > Comments:
> >> >
> >> > When can options be None?
> >> >
> >>
> >
> >  That's the default value of the options parameter of `def
> >> ChromiumFactory`.
> >> If it can never be None, someone could try making this parameter
> >> mandatory.
> >>
> >
> >
> >  > I think we tend towards "foo not in bar" rather than "not foo in bar"
> >> >
> >>
> >
> >  Done.
> >>
> >
> >
> >  > I don't think that path.normpath / path.join will work correctly on the
> >> > master
> >> > side of things (cross-platform, different filesystem layouts and all
> >> that).
> >>
> >
> >
> >  It's what ChromiumCommands's __init__ does to set _chromium_script_dir, so
> >> I would think it works.
> >>
> >
> >
> >
> > I think that uses a custom PathJoin method for exactly this reason?
> 
> 
> PathJoin more or less does normpath(join(*args)), no? (It uses posixpath
> and ntpath, but I don't understand why)
> 

It tries to figure out what the /slave/ OS is, and thus generate a path which is
compatible with what the slave expects. The master is always running on a posix
system, so if you generate a path with os.path, it'll be posix-ish, but when it
gets to a windows slave, the scripts there will undoubtedly interact poorly with
it.

> 
> >
> >
> >  > What do you think about gyp_chromium looking for the goma folder and
> >> > filling in
> >> > gomadir if it's not set?
> >>
> >
> >
> >  > https://codereview.chromium.org/142373002/
> >> >
> >>
> >
> >  To unsubscribe from this group and stop receiving emails from it, send an
> >>
> > email
> >
> >> to mailto:chromium-reviews+unsubscribe@chromium.org.
> >>
> >
> >
> >
> > https://codereview.chromium.org/142373002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[iannucci, 2014-01-18 21:09:41]

But the larger point is: generating paths on the master is awkward and strange,
IMO (since you're generating paths for a different filesystem layout on a
different machine running a different OS)... as much as possible, it's more
sensical to generate those sorts of paths on the system which is going to
consume them. That said, I'd accept the same PathJoin style logic here (maybe
there's a way to actually use the real PathJoin method?)

[iannucci, 2014-01-18 21:11:27]

On 2014/01/18 21:09:41, iannucci wrote:
> But the larger point is: generating paths on the master is awkward and
strange,
> IMO (since you're generating paths for a different filesystem layout on a
> different machine running a different OS)... as much as possible, it's more
> sensical to generate those sorts of paths on the system which is going to
> consume them. That said, I'd accept the same PathJoin style logic here (maybe
> there's a way to actually use the real PathJoin method?)

Actually, to this point, the cwd of this code is the master directory, not the
slave/<name>/build directory, which means normpath is going to do the wrong
thing anyhow.

[iannucci, 2014-01-18 21:12:22]

On 2014/01/18 21:11:27, iannucci wrote:
> On 2014/01/18 21:09:41, iannucci wrote:
> > But the larger point is: generating paths on the master is awkward and
> strange,
> > IMO (since you're generating paths for a different filesystem layout on a
> > different machine running a different OS)... as much as possible, it's more
> > sensical to generate those sorts of paths on the system which is going to
> > consume them. That said, I'd accept the same PathJoin style logic here
(maybe
> > there's a way to actually use the real PathJoin method?)
> 
> Actually, to this point, the cwd of this code is the master directory, not the
> slave/<name>/build directory, which means normpath is going to do the wrong
> thing anyhow.

[[unless there's an evil chdir somewhere in the master code to try to occupy the
same directory as the slave.... I'm not sure... I try to generate these paths on
the slave for exactly this reason :D]]

[Nico, 2014-01-18 21:14:51]

I agree it's icky, but it's how the system currently works.

I'll change this to do the PathJoin thing (and add a comment there that
explains what's happening there, since I didn't understand that)


On Sat, Jan 18, 2014 at 1:12 PM, <iannucci@chromium.org> wrote:

> On 2014/01/18 21:11:27, iannucci wrote:
>
>> On 2014/01/18 21:09:41, iannucci wrote:
>> > But the larger point is: generating paths on the master is awkward and
>> strange,
>> > IMO (since you're generating paths for a different filesystem layout on
>> a
>> > different machine running a different OS)... as much as possible, it's
>> more
>> > sensical to generate those sorts of paths on the system which is going
>> to
>> > consume them. That said, I'd accept the same PathJoin style logic here
>>
> (maybe
>
>> > there's a way to actually use the real PathJoin method?)
>>
>
>  Actually, to this point, the cwd of this code is the master directory,
>> not the
>> slave/<name>/build directory, which means normpath is going to do the
>> wrong
>> thing anyhow.
>>
>
> [[unless there's an evil chdir somewhere in the master code to try to
> occupy the
> same directory as the slave.... I'm not sure... I try to generate these
> paths on
> the slave for exactly this reason :D]]
>
> https://codereview.chromium.org/142373002/
>

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

[Nico, 2014-01-18 21:23:29]

On Sat, Jan 18, 2014 at 1:11 PM, <iannucci@chromium.org> wrote:

> On 2014/01/18 21:09:41, iannucci wrote:
>
>> But the larger point is: generating paths on the master is awkward and
>>
> strange,
>
>> IMO (since you're generating paths for a different filesystem layout on a
>> different machine running a different OS)... as much as possible, it's
>> more
>> sensical to generate those sorts of paths on the system which is going to
>> consume them. That said, I'd accept the same PathJoin style logic here
>> (maybe
>> there's a way to actually use the real PathJoin method?)
>>
>
> Actually, to this point, the cwd of this code is the master directory, not
> the
> slave/<name>/build directory, which means normpath is going to do the wrong
> thing anyhow.
>

Ah, this is correct, hmm. abspath to the rescue?


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

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

[iannucci, 2014-01-19 02:54:00]

On 2014/01/18 21:23:29, Nico wrote:
> On Sat, Jan 18, 2014 at 1:11 PM, <mailto:iannucci@chromium.org> wrote:
> 
> > On 2014/01/18 21:09:41, iannucci wrote:
> >
> >> But the larger point is: generating paths on the master is awkward and
> >>
> > strange,
> >
> >> IMO (since you're generating paths for a different filesystem layout on a
> >> different machine running a different OS)... as much as possible, it's
> >> more
> >> sensical to generate those sorts of paths on the system which is going to
> >> consume them. That said, I'd accept the same PathJoin style logic here
> >> (maybe
> >> there's a way to actually use the real PathJoin method?)
> >>
> >
> > Actually, to this point, the cwd of this code is the master directory, not
> > the
> > slave/<name>/build directory, which means normpath is going to do the wrong
> > thing anyhow.
> >
> 
> Ah, this is correct, hmm. abspath to the rescue?

Well... abspath definitely won't work for a windows slave (how would the linux
master know which drive letter to use?).

I would really recommend having gyp_chromium find goma, if use_goma is defined,
but gomadir is not... 

This could also do the right thing for devs who put goma in the normal spot.
Then the configuration is just 'use_goma=1'. The downside, of course, is if you
directly use gyp instead of gyp_chromium. But I'm pretty sure you're asking for
it if you do that anyway...

> 
> 
> >
> > https://codereview.chromium.org/142373002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Nico, 2014-01-23 17:10:04]

On 2014/01/19 02:54:00, iannucci wrote:
> On 2014/01/18 21:23:29, Nico wrote:
> > On Sat, Jan 18, 2014 at 1:11 PM, <mailto:iannucci@chromium.org> wrote:
> > 
> > > On 2014/01/18 21:09:41, iannucci wrote:
> > >
> > >> But the larger point is: generating paths on the master is awkward and
> > >>
> > > strange,
> > >
> > >> IMO (since you're generating paths for a different filesystem layout on a
> > >> different machine running a different OS)... as much as possible, it's
> > >> more
> > >> sensical to generate those sorts of paths on the system which is going to
> > >> consume them. That said, I'd accept the same PathJoin style logic here
> > >> (maybe
> > >> there's a way to actually use the real PathJoin method?)
> > >>
> > >
> > > Actually, to this point, the cwd of this code is the master directory, not
> > > the
> > > slave/<name>/build directory, which means normpath is going to do the
wrong
> > > thing anyhow.
> > >
> > 
> > Ah, this is correct, hmm. abspath to the rescue?
> 
> Well... abspath definitely won't work for a windows slave (how would the linux
> master know which drive letter to use?).

Oh, good point.

> I would really recommend having gyp_chromium find goma, if use_goma is
defined,
> but gomadir is not... 
> 
> This could also do the right thing for devs who put goma in the normal spot.
> Then the configuration is just 'use_goma=1'. The downside, of course, is if
you
> directly use gyp instead of gyp_chromium. But I'm pretty sure you're asking
for
> it if you do that anyway...

Just "use_goma=1" is already doing the right thing for devs: build/common.gypi
defaults gomadir to ~/goma (on posix). If gyp_chromium sets it,
build/common.gypi won't have a chance to do this. And setting GYP_DEFINES isn't
gyp_chromium's business (much less setting some to values outside of the
checkout, for stuff that only works on our bots.)

I can see two possible approaches:

1.) Add a slave script that wraps runhooks and do what you propose there. That
would allow doing other gyp-related stuff there too, so that these things can be
changed without a master restart.

2.) Do some WithProperties() thing for gomadir and use that from the master
config.

Does either of these sound ok? (I kinda like 1.)

> > 
> > 
> > >
> > > https://codereview.chromium.org/142373002/
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

[Nico, 2014-01-27 19:24:33]

On Thu, Jan 23, 2014 at 9:10 AM, <thakis@chromium.org> wrote:

> On 2014/01/19 02:54:00, iannucci wrote:
>
>> On 2014/01/18 21:23:29, Nico wrote:
>> > On Sat, Jan 18, 2014 at 1:11 PM, <mailto:iannucci@chromium.org> wrote:
>> >
>> > > On 2014/01/18 21:09:41, iannucci wrote:
>> > >
>> > >> But the larger point is: generating paths on the master is awkward
>> and
>> > >>
>> > > strange,
>> > >
>> > >> IMO (since you're generating paths for a different filesystem layout
>> on a
>> > >> different machine running a different OS)... as much as possible,
>> it's
>> > >> more
>> > >> sensical to generate those sorts of paths on the system which is
>> going to
>> > >> consume them. That said, I'd accept the same PathJoin style logic
>> here
>> > >> (maybe
>> > >> there's a way to actually use the real PathJoin method?)
>> > >>
>> > >
>> > > Actually, to this point, the cwd of this code is the master
>> directory, not
>> > > the
>> > > slave/<name>/build directory, which means normpath is going to do the
>>
> wrong
>
>> > > thing anyhow.
>> > >
>> >
>> > Ah, this is correct, hmm. abspath to the rescue?
>>
>
>  Well... abspath definitely won't work for a windows slave (how would the
>> linux
>> master know which drive letter to use?).
>>
>
> Oh, good point.
>
>
>  I would really recommend having gyp_chromium find goma, if use_goma is
>>
> defined,
>
>> but gomadir is not...
>>
>
>  This could also do the right thing for devs who put goma in the normal
>> spot.
>> Then the configuration is just 'use_goma=1'. The downside, of course, is
>> if
>>
> you
>
>> directly use gyp instead of gyp_chromium. But I'm pretty sure you're
>> asking
>>
> for
>
>> it if you do that anyway...
>>
>
> Just "use_goma=1" is already doing the right thing for devs:
> build/common.gypi
> defaults gomadir to ~/goma (on posix). If gyp_chromium sets it,
> build/common.gypi won't have a chance to do this. And setting GYP_DEFINES
> isn't
> gyp_chromium's business (much less setting some to values outside of the
> checkout, for stuff that only works on our bots.)
>
> I can see two possible approaches:
>
> 1.) Add a slave script that wraps runhooks and do what you propose there.
> That
> would allow doing other gyp-related stuff there too, so that these things
> can be
> changed without a master restart.
>
> 2.) Do some WithProperties() thing for gomadir and use that from the master
> config.
>
> Does either of these sound ok? (I kinda like 1.)


^ iannucci: Thoughts on this?


>
>
>  >
>> >
>> > >
>> > > https://codereview.chromium.org/142373002/
>> > >
>> >
>> > To unsubscribe from this group and stop receiving emails from it, send
>> an
>> email
>> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
>
> https://codereview.chromium.org/142373002/
>

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

[iannucci, 2014-01-28 14:15:27]

On 2014/01/27 19:24:33, Nico wrote:
> On Thu, Jan 23, 2014 at 9:10 AM, <mailto:thakis@chromium.org> wrote:
> 
> > On 2014/01/19 02:54:00, iannucci wrote:
> >
> >> On 2014/01/18 21:23:29, Nico wrote:
> >> > On Sat, Jan 18, 2014 at 1:11 PM, <mailto:iannucci@chromium.org> wrote:
> >> >
> >> > > On 2014/01/18 21:09:41, iannucci wrote:
> >> > >
> >> > >> But the larger point is: generating paths on the master is awkward
> >> and
> >> > >>
> >> > > strange,
> >> > >
> >> > >> IMO (since you're generating paths for a different filesystem layout
> >> on a
> >> > >> different machine running a different OS)... as much as possible,
> >> it's
> >> > >> more
> >> > >> sensical to generate those sorts of paths on the system which is
> >> going to
> >> > >> consume them. That said, I'd accept the same PathJoin style logic
> >> here
> >> > >> (maybe
> >> > >> there's a way to actually use the real PathJoin method?)
> >> > >>
> >> > >
> >> > > Actually, to this point, the cwd of this code is the master
> >> directory, not
> >> > > the
> >> > > slave/<name>/build directory, which means normpath is going to do the
> >>
> > wrong
> >
> >> > > thing anyhow.
> >> > >
> >> >
> >> > Ah, this is correct, hmm. abspath to the rescue?
> >>
> >
> >  Well... abspath definitely won't work for a windows slave (how would the
> >> linux
> >> master know which drive letter to use?).
> >>
> >
> > Oh, good point.
> >
> >
> >  I would really recommend having gyp_chromium find goma, if use_goma is
> >>
> > defined,
> >
> >> but gomadir is not...
> >>
> >
> >  This could also do the right thing for devs who put goma in the normal
> >> spot.
> >> Then the configuration is just 'use_goma=1'. The downside, of course, is
> >> if
> >>
> > you
> >
> >> directly use gyp instead of gyp_chromium. But I'm pretty sure you're
> >> asking
> >>
> > for
> >
> >> it if you do that anyway...
> >>
> >
> > Just "use_goma=1" is already doing the right thing for devs:
> > build/common.gypi
> > defaults gomadir to ~/goma (on posix). If gyp_chromium sets it,
> > build/common.gypi won't have a chance to do this. And setting GYP_DEFINES
> > isn't
> > gyp_chromium's business (much less setting some to values outside of the
> > checkout, for stuff that only works on our bots.)
> >
> > I can see two possible approaches:
> >
> > 1.) Add a slave script that wraps runhooks and do what you propose there.
> > That
> > would allow doing other gyp-related stuff there too, so that these things
> > can be
> > changed without a master restart.
> >
> > 2.) Do some WithProperties() thing for gomadir and use that from the master
> > config.
> >
> > Does either of these sound ok? (I kinda like 1.)
> 
> 
> ^ iannucci: Thoughts on this?
> 

Sorry, lost track of this...

Hm... #1 is interesting. So all slaves which are running gclient runhooks right
now would run bot_runhooks.py or something like that (which would find+set this
path)? We could also pass bot_runhooks.py the master/buildername, and move GYP
configurations from the master into bot_runhooks.py too, though I'm not sure how
much value there is in that over just transitioning builders to recipes.

I'd be +1 on changing the runhooks step to run a bot-script which just injects
this value before running `gclient runhooks`... could be tricky to get right
though, and is potentially very disruptive.

At the risk of future-me coming and stabbing me in the face later, what about
having the master set the path to a relative directory (would be relative to
src/build, I guess)? I'm not sure about slash-normalizations though...

> 
> >
> >
> >  >
> >> >
> >> > >
> >> > > https://codereview.chromium.org/142373002/
> >> > >
> >> >
> >> > To unsubscribe from this group and stop receiving emails from it, send
> >> an
> >> email
> >> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> >>
> >
> >
> >
> > https://codereview.chromium.org/142373002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

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

moving this to https://codereview.chromium.org/228903003/ …