💈 Remove most GYP_DEFINES logic from tools/clang/scripts/update.py

Remove most GYP_DEFINES logic from tools/clang/scripts/update.py

Now that we use GN, gclient hooks should not depend
on GYP_DEFINES. Removed some of the logic that would disable
downloading of the plugin and instead just always download it.

BUG=615039, 570091

[agrieve, 2016-05-26 15:41:23]

agrieve@chromium.org changed reviewers:
+ dpranke@chromium.org

[agrieve, 2016-05-26 15:41:42]

On 2016/05/26 15:41:23, agrieve wrote:

[Nico, 2016-05-26 15:42:45]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2016-05-26 15:42:45]

as far as i know, we still set GYP_DEFINES during runhooks, even if we don't run
gyp

[agrieve, 2016-05-26 15:46:54]

On 2016/05/26 15:42:45, Nico wrote:
> as far as i know, we still set GYP_DEFINES during runhooks, even if we don't
run
> gyp

Most bots still set it (our official android bots no longer do), but devs don't.
There's another but that points to fix the use-case though:
https://bugs.chromium.org/p/chromium/issues/detail?id=570091. In this case
though, I think it's fine to just err on the download it side for this one.

[agrieve, 2016-05-26 15:47:06]

Description was changed from

==========
Remove most GYP_DEFINES logic from tools/clang/scripts/update.py

Now that we use GN, the logic doesn't make sense. Left in one piece of
logic that might be used by bots still.

BUG=615039
==========

to

==========
Remove most GYP_DEFINES logic from tools/clang/scripts/update.py

Now that we use GN, the logic doesn't make sense. Left in one piece of
logic that might be used by bots still.

BUG=615039,570091
==========

[Nico, 2016-05-26 15:49:37]

On 2016/05/26 15:46:54, agrieve wrote:
> On 2016/05/26 15:42:45, Nico wrote:
> > as far as i know, we still set GYP_DEFINES during runhooks, even if we don't
> run
> > gyp
> 
> Most bots still set it (our official android bots no longer do), but devs
don't.
> There's another but that points to fix the use-case though:
> https://bugs.chromium.org/p/chromium/issues/detail?id=570091. In this case
> though, I think it's fine to just err on the download it side for this one.

That's ok; one of the biggest customers of this script is
https://build.chromium.org/p/chromium.fyi/console?category=clang%20tot , our
waterfall where we build chromium with trunk clang to check that that works.
That waterfall relies on the code you're deleting as far as I know.

[agrieve, 2016-05-26 16:08:37]

On 2016/05/26 15:49:37, Nico wrote:
> On 2016/05/26 15:46:54, agrieve wrote:
> > On 2016/05/26 15:42:45, Nico wrote:
> > > as far as i know, we still set GYP_DEFINES during runhooks, even if we
don't
> > run
> > > gyp
> > 
> > Most bots still set it (our official android bots no longer do), but devs
> don't.
> > There's another but that points to fix the use-case though:
> > https://bugs.chromium.org/p/chromium/issues/detail?id=570091. In this case
> > though, I think it's fine to just err on the download it side for this one.
> 
> That's ok; one of the biggest customers of this script is
> https://build.chromium.org/p/chromium.fyi/console?category=clang%20tot , our
> waterfall where we build chromium with trunk clang to check that that works.
> That waterfall relies on the code you're deleting as far as I know.

Do you think it'll break with this change? 

I think this will download the plugin in all cases where it would before (but
also download in a case where it doesn't).

[agrieve, 2016-05-26 17:07:06]

Description was changed from

==========
Remove most GYP_DEFINES logic from tools/clang/scripts/update.py

Now that we use GN, the logic doesn't make sense. Left in one piece of
logic that might be used by bots still.

BUG=615039,570091
==========

to

==========
Remove most GYP_DEFINES logic from tools/clang/scripts/update.py

Now that we use GN, gclient hooks should not depend
on GYP_DEFINES. Removed some of the logic that would disable
downloading of the plugin and instead just always download it.

BUG=615039,570091
==========

[Nico, 2016-05-26 19:06:57]

it'd build the android asan runtime on the non-android bots for example.

[agrieve, 2016-05-26 19:37:39]

On 2016/05/26 19:06:57, Nico wrote:
> it'd build the android asan runtime on the non-android bots for example.

Ah, okay. 
My change had the default for args.with_android being False when
use_head_revision=True.

So maybe the bug was that it would not build for android when it actually should
(e.g. when OS=android *is* in GYP_DEFINES).

I've reverted that part of it and just left the part that sets need_gold_plugin
based off of GYP_DEFINES. (and I'll add a note about this file in bug 570091).

[Nico, 2016-05-26 19:42:45]

This does not lgtm, it'll still download the gold plugin for all developers,
which is what we're trying to avoid.

What's motivating wanting to change this file?

(See also https://codereview.chromium.org/1908293002/ etc)

[agrieve, 2016-05-26 20:08:23]

On 2016/05/26 19:42:45, Nico wrote:
> This does not lgtm, it'll still download the gold plugin for all developers,
> which is what we're trying to avoid.
> 
> What's motivating wanting to change this file?
> 
> (See also https://codereview.chromium.org/1908293002/ etc)

I'd like to not see GYP_DEFINES anywhere, and I'd *really* not like to have any
cases where it changes the behaviour of "gclient runhooks". 

With the switch to GN, devs no longer set GYP_DEFINES. If a linux dev (where the
gold plugin is required) were to locally do an official build
(is_official_build=true is_chrome_branded=true), their build would fail since
this hook would not have downloaded the plugin.

(as an aside, should this not just keyed off of is_official_build=true? branding
shouldn't affect linker flags...)

[Nico, 2016-05-26 20:22:05]

On 2016/05/26 20:08:23, agrieve wrote:
> On 2016/05/26 19:42:45, Nico wrote:
> > This does not lgtm, it'll still download the gold plugin for all developers,
> > which is what we're trying to avoid.
> > 
> > What's motivating wanting to change this file?
> > 
> > (See also https://codereview.chromium.org/1908293002/ etc)
> 
> I'd like to not see GYP_DEFINES anywhere

Sure, but the way to get there is to come up with a new mechanism that can do
what the old one could, and then migrate to that, and not just delete what's
there, right?

>, and I'd *really* not like to have any
> cases where it changes the behaviour of "gclient runhooks". 
> 
> With the switch to GN, devs no longer set GYP_DEFINES. If a linux dev (where
the
> gold plugin is required)

It is not. Only in official builds, which most people don't use (LTO builds take
like half an hour, so it can't be used for development).

I'd argue that switching the official builds to gn before having a story for
things like this was a bit premature, but hey.

> were to locally do an official build
> (is_official_build=true is_chrome_branded=true), their build would fail since
> this hook would not have downloaded the plugin.

Right, that needs to be done manually.

> 
> (as an aside, should this not just keyed off of is_official_build=true?
branding
> shouldn't affect linker flags...)

[agrieve, 2016-05-27 02:42:29]

On 2016/05/26 20:22:05, Nico (away until Tue May 31) wrote:
> On 2016/05/26 20:08:23, agrieve wrote:
> > On 2016/05/26 19:42:45, Nico wrote:
> > > This does not lgtm, it'll still download the gold plugin for all
developers,
> > > which is what we're trying to avoid.
> > > 
> > > What's motivating wanting to change this file?
> > > 
> > > (See also https://codereview.chromium.org/1908293002/ etc)
> > 
> > I'd like to not see GYP_DEFINES anywhere
> 
> Sure, but the way to get there is to come up with a new mechanism that can do
> what the old one could, and then migrate to that, and not just delete what's
> there, right?
> 
> >, and I'd *really* not like to have any
> > cases where it changes the behaviour of "gclient runhooks". 
> > 
> > With the switch to GN, devs no longer set GYP_DEFINES. If a linux dev (where
> the
> > gold plugin is required)
> 
> It is not. Only in official builds, which most people don't use (LTO builds
take
> like half an hour, so it can't be used for development).
> 
> I'd argue that switching the official builds to gn before having a story for
> things like this was a bit premature, but hey.
> 
> > were to locally do an official build
> > (is_official_build=true is_chrome_branded=true), their build would fail
since
> > this hook would not have downloaded the plugin.
> 
> Right, that needs to be done manually.
> 
> > 
> > (as an aside, should this not just keyed off of is_official_build=true?
> branding
> > shouldn't affect linker flags...)

Alright, I'll abandon this then. Thought it'd be an improvement, but looks like
probably not.