clarifying the behavior of analyze for compile-only bots

[Paweł Hajdan, Jr.]

I just wanted to make sure we reach a conclusion even after https://code.google.com/p/chromium/issues/detail?id=529798 is closed.

Maybe we even have one, and in that case I'd like to make sure it's clear to everyone, including myself.

It seems to me that what happened in terms of code, is we're passing a single list of targets to analyze, from which it picks a subset that is affected by the patch.

This works well for builders that run tests, because to run a test it needs to be compiled.

The tricky case are targets like chromium_builder_tests or chromeos_preflight. Above approach makes the bots pretty much always recompile them, even if only a subset of their dependency tree is affected by the patch.

You might wonder what's an example scenario where it'd make a difference. Consider a target which is stale not because it's affected by the patch, but just because the underlying code has changed because we're using a more recent base revision. That target being stale though doesn't change the fact that it's not affected by the patch.

If everyone is fine with above behavior that's also fine for me (as long as we continue to meet CQ cycle time goals). See https://groups.google.com/a/chromium.org/d/msg/infra-dev/s_z7ni9hnGU/hS7kPN3ifpIJ for a past discussion about this, and https://codereview.chromium.org/485873004 for how we had the logic that is now considered legacy since the beginning of "analyze" in recipes.

I'd also like to make sure GN analyze behavior is consistent with gyp analyze behavior.

Paweł

[John Abd-El-Malek]

On Thu, Oct 1, 2015 at 3:50 AM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

I just wanted to make sure we reach a conclusion even after https://code.google.com/p/chromium/issues/detail?id=529798 is closed.

Maybe we even have one, and in that case I'd like to make sure it's clear to everyone, including myself.

It seems to me that what happened in terms of code, is we're passing a single list of targets to analyze, from which it picks a subset that is affected by the patch.

This works well for builders that run tests, because to run a test it needs to be compiled.

The tricky case are targets like chromium_builder_tests or chromeos_preflight. Above approach makes the bots pretty much always recompile them, even if only a subset of their dependency tree is affected by the patch.

Why is that the case?

It's my understanding that we want to only compile the subset of chromium_builder_tests etc that is affected by the patch.  

You might wonder what's an example scenario where it'd make a difference. Consider a target which is stale not because it's affected by the patch, but just because the underlying code has changed because we're using a more recent base revision. That target being stale though doesn't change the fact that it's not affected by the patch.

If everyone is fine with above behavior that's also fine for me (as long as we continue to meet CQ cycle time goals). See https://groups.google.com/a/chromium.org/d/msg/infra-dev/s_z7ni9hnGU/hS7kPN3ifpIJ for a past discussion about this, and https://codereview.chromium.org/485873004 for how we had the logic that is now considered legacy since the beginning of "analyze" in recipes.

I'd also like to make sure GN analyze behavior is consistent with gyp analyze behavior.

Paweł

--
You received this message because you are subscribed to the Google Groups "infra-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+...@chromium.org.
To post to this group, send email to infr...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/CAATLsPYm%2BuswTswaLQErD6zcwKzfe73-9hdkzSOiih90PKZ5cA%40mail.gmail.com.

[Dirk Pranke]

On Thu, Oct 1, 2015 at 11:09 AM, John Abd-El-Malek <j...@chromium.org> wrote:

On Thu, Oct 1, 2015 at 3:50 AM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

I just wanted to make sure we reach a conclusion even after https://code.google.com/p/chromium/issues/detail?id=529798 is closed.

Maybe we even have one, and in that case I'd like to make sure it's clear to everyone, including myself.

Yes, definitely. I meant to send out something yesterday but got derailed by the codereview outage. 

I have a writeup that I will send out in a couple hours after I can get back to my desk and double check my notes

and the code for accuracy.

 

It seems to me that what happened in terms of code, is we're passing a single list of targets to analyze, from which it picks a subset that is affected by the patch.

This works well for builders that run tests, because to run a test it needs to be compiled.

The tricky case are targets like chromium_builder_tests or chromeos_preflight. Above approach makes the bots pretty much always recompile them, even if only a subset of their dependency tree is affected by the patch.

Why is that the case?

It's my understanding that we want to only compile the subset of chromium_builder_tests etc that is affected by the patch.

Correct, that is what we want, and we're capable of doing this today, mostly. I need to double-check some details.

-- Dirk

[Dirk Pranke]

Here's the example that Scott and I were using to get on the same page about this. This is how the code works today,

as far as I know:

Given the following:

- three source files: net_test.cpp, ipc_test.cpp, and gpu_test.cpp

- three exes: net_tests, ipc_tests, and gpu_tests.

- a chromium_builder_tests target that depends on net_tests and ipc_tests

- a bot configured with a compile_target of chromium_builder_tests and also

  told to run gpu_tests and net_tests in the test steps

If I have a patch that modifies net_test.cpp and gpu_test.cpp

Then we call into the analyze module with 

  compile_targets=[chromium_builder_tests] and 

  matching_exes=[chromium_builder_tests, net_tests, gpu_tests] 

  affected_files=[net_test.cpp, gpu_test.cpp]

(matching_exes is the list of tests to run plus the list of compile_targets), which calls

into the filter module w/ the same arguments.

GYP gets handed {files: affected_files, targets: matching_exes}.

It returns: {build_targets: [net_tests, gpu_tests], targets: [chromium_builder_tests, net_tests, gpu_tests]}. 

The filter module then assigns compile_targets=build_targets and matching_exes=targets.

The chromium_trybot recipe then re-adds matching_exes back to compile_targets and ends up telling ninja

to compile [chromium_builder_tests, net_tests, gpu_tests]

Note 1: GN, for the record, is not as smart as GYP and will return [chromium_builder_tests, net_tests, 

gpu_tests] for both fields.

Note 2: If we trip an exclusion, the filter module will return [chromium_builder_tests] for compile_targets and

[chromium_builder_tests, net_tests, gpu_tests] for matching exes (and not call GYP at all). In other words, 

the exclusions will return a different value than GYP would return, and it will build too much (this is fine and

the best we can do because by definition hitting an exclusion means "skip the dependency checking and just

return all of the requested targets")

Note 3: by passing the union of the list of tests and compile_targets to GYP in a single parameter, and then

taking the union of compile_targets and  matching_exes as the result, we defeat any point of having two lists

(from analyze's point of view, and from compile's point of view; there may be other reasons to still have two

lists).

The value GYP returns in targets is the list of input matching_exes that are actually affected by

the files, and the value in build_targets is the *subset* of that that you actually want to compile. It is 

smart enough to detect that chromium_builder_tests is a group target that will includes other unaffected 

targets, and hence we don't want it.

I think we should just return build_targets, and simplify the code to not even try to split things into compile_targets

and matching_exes; we can't usefully keep them apart (and we should modify GN to match GYP's behavior).

In the case of tripping an exclusion, we should just return matching_exes (i.e., everything).

Does that all make sense?

[ There may be some corner cases in how we deal with 'none' targets (groups) in GYP that have actions

associated with them that are slightly trickier, but I'll skip them for purposes of this description. Exactly how

I will get GN to match GYP in this case is TBD ].

-- Dirk

[Dirk Pranke]

As a follow-up to this (after some discussion w/ Brett about how one might implement this behavior on top of or in GN) ...

Much of this complexity comes from the fact that we have these group targets like 'chromium_builder_tests'.

Both Brett and I suspect that there are cases where you do want the group or none targets to build (base_unittests_run)

if the dependencies change and cases where you don't (chromium_builder_tests), and figuring out which case is which

is hard to understand; we already have comments in the chromium_trybot.py where we include too many things for

exactly this reason. I can imagine cases for things like telemetry where the build targets are modeled as groups in GN

that will be harder to deal with, and for which we often deal with them today through the exclusions file.

Given that we now specify the list of tests to run and additional targets to build via the //testing/buildbot/*.json files,

is there a reason to keep these group targets? Why not switch completely to requiring the list of specific targets

to be passed in and ban groups? I would rather spend my time migrating builders to not use groups than

I would trying to add more and more complicated logic to MB or GN to handle this complexity.

[ This was my point #3 in https://code.google.com/p/chromium/issues/detail?id=529798#c27 ].

-- Dirk

[John Abd-El-Malek]

On Thu, Oct 1, 2015 at 3:30 PM, Dirk Pranke <dpr...@chromium.org> wrote:

As a follow-up to this (after some discussion w/ Brett about how one might implement this behavior on top of or in GN) ...

Much of this complexity comes from the fact that we have these group targets like 'chromium_builder_tests'.

Both Brett and I suspect that there are cases where you do want the group or none targets to build (base_unittests_run)

if the dependencies change and cases where you don't (chromium_builder_tests), and figuring out which case is which

is hard to understand; we already have comments in the chromium_trybot.py where we include too many things for

exactly this reason. I can imagine cases for things like telemetry where the build targets are modeled as groups in GN

that will be harder to deal with, and for which we often deal with them today through the exclusions file.

Given that we now specify the list of tests to run and additional targets to build via the //testing/buildbot/*.json files,

is there a reason to keep these group targets? Why not switch completely to requiring the list of specific targets

to be passed in and ban groups? I would rather spend my time migrating builders to not use groups than

I would trying to add more and more complicated logic to MB or GN to handle this complexity.

The one reason I can think of keeping groups is that it's used in a lot of places. I think we don't want to have to list all these compile targets in n places, because then they'd go out of sync.

[John Abd-El-Malek]

On Fri, Oct 2, 2015 at 7:24 AM, John Abd-El-Malek <j...@chromium.org> wrote:

On Thu, Oct 1, 2015 at 3:30 PM, Dirk Pranke <dpr...@chromium.org> wrote:

As a follow-up to this (after some discussion w/ Brett about how one might implement this behavior on top of or in GN) ...

Much of this complexity comes from the fact that we have these group targets like 'chromium_builder_tests'.

Both Brett and I suspect that there are cases where you do want the group or none targets to build (base_unittests_run)

if the dependencies change and cases where you don't (chromium_builder_tests), and figuring out which case is which

is hard to understand; we already have comments in the chromium_trybot.py where we include too many things for

exactly this reason. I can imagine cases for things like telemetry where the build targets are modeled as groups in GN

that will be harder to deal with, and for which we often deal with them today through the exclusions file.

Given that we now specify the list of tests to run and additional targets to build via the //testing/buildbot/*.json files,

is there a reason to keep these group targets? Why not switch completely to requiring the list of specific targets

to be passed in and ban groups? I would rather spend my time migrating builders to not use groups than

I would trying to add more and more complicated logic to MB or GN to handle this complexity.

The one reason I can think of keeping groups is that it's used in a lot of places. I think we don't want to have to list all these compile targets in n places, because then they'd go out of sync.

Perhaps we can have a text file that lists all these targets? Of course, we'd want that to be in the chrome repo. Then maybe we can use the same solution to refer to tests, since we have the list of tests that we run duplicated many times.
 

[Scott Violet]

On Thu, Oct 1, 2015 at 2:43 PM, Dirk Pranke <dpr...@chromium.org> wrote:

Remember that build_targets is the minimal set of targets to build
assuming you want to build all. And by 'all' I mean any target
reachable root from the root nodes, which is different than the all
target. It's my understanding we have bots that don't want to build
'all', so that build_targets as implemented and used isn't right. I
think we need to change build_targets to only return targets that are
descendants of the supplied targets. Then I agree with using
build_targets.

-Scott

[Dirk Pranke]

On Fri, Oct 2, 2015 at 7:25 AM, John Abd-El-Malek <j...@chromium.org> wrote:

On Fri, Oct 2, 2015 at 7:24 AM, John Abd-El-Malek <j...@chromium.org> wrote:

On Thu, Oct 1, 2015 at 3:30 PM, Dirk Pranke <dpr...@chromium.org> wrote:

As a follow-up to this (after some discussion w/ Brett about how one might implement this behavior on top of or in GN) ...

Much of this complexity comes from the fact that we have these group targets like 'chromium_builder_tests'.

Both Brett and I suspect that there are cases where you do want the group or none targets to build (base_unittests_run)

if the dependencies change and cases where you don't (chromium_builder_tests), and figuring out which case is which

is hard to understand; we already have comments in the chromium_trybot.py where we include too many things for

exactly this reason. I can imagine cases for things like telemetry where the build targets are modeled as groups in GN

that will be harder to deal with, and for which we often deal with them today through the exclusions file.

Given that we now specify the list of tests to run and additional targets to build via the //testing/buildbot/*.json files,

is there a reason to keep these group targets? Why not switch completely to requiring the list of specific targets

to be passed in and ban groups? I would rather spend my time migrating builders to not use groups than

I would trying to add more and more complicated logic to MB or GN to handle this complexity.

The one reason I can think of keeping groups is that it's used in a lot of places. I think we don't want to have to list all these compile targets in n places, because then they'd go out of sync.

We already have the "out of sync" problem, in at least two different ways.

First, the //testing/buildbot files list all of the tests individually, so they can (and probably are) out of sync w/ chromium_builder_tests.

Second, the //testing/build files repeat each list of tests for each bot, and so different bots can get potentially out of sync.

 

Perhaps we can have a text file that lists all these targets? Of course, we'd want that to be in the chrome repo. Then maybe we can use the same solution to refer to tests, since we have the list of tests that we run duplicated many times.

I think the //testing/buildbot files were designed initially as the simplest possible thing that could work (and that wasn't a bad decision). 

I think we probably need to come up with a more complicated solution like what you suggest that can reduce the duplication (and I seem to recall discussing this w/ Paweł at some point and him agreeing, though of course I'd like his feedback now).

I will keep chewing on this (and file a bug for the overall problem).

-- Dirk

[Dirk Pranke]

You kept saying that, and I kept not understanding you :(. I think 

it's finally sunk in, though. Thanks for clarifying :)

-- Dirk

[Paweł Hajdan, Jr.]

On Fri, Oct 2, 2015 at 5:18 PM, Scott Violet <s...@chromium.org> wrote:

Remember that build_targets is the minimal set of targets to build
assuming you want to build all. And by 'all' I mean any target
reachable root from the root nodes, which is different than the all
target.

Thanks for the explanation.

 

It's my understanding we have bots that don't want to build
'all', so that build_targets as implemented and used isn't right.

Yeah, we're going away from building 'all' on most bots.

Also see https://groups.google.com/a/chromium.org/d/msg/infra-dev/vAZjvj8xKgU/8JWdpN9JCAAJ for a related thread about compile targets for bots.

 

I think we need to change build_targets to only return targets that are
descendants of the supplied targets. Then I agree with using
build_targets.

Sounds good to me.

Scott, since you know the gyp analyze code the best, could you make the change? I took a look and it'd just take me some time to fully understand the logic there.

Dirk, can we make GN analyze behave the same way? Please let me know if you still have questions or concerns.

Paweł

[Scott Violet]

On Mon, Oct 5, 2015 at 8:00 AM, Paweł Hajdan, Jr.
<phajd...@chromium.org> wrote:
> On Fri, Oct 2, 2015 at 5:18 PM, Scott Violet <s...@chromium.org> wrote:
>>
>> Remember that build_targets is the minimal set of targets to build
>> assuming you want to build all. And by 'all' I mean any target
>> reachable root from the root nodes, which is different than the all
>> target.
>
>
> Thanks for the explanation.
>
>>
>> It's my understanding we have bots that don't want to build
>> 'all', so that build_targets as implemented and used isn't right.
>
>
> Yeah, we're going away from building 'all' on most bots.
>
> Also see
> https://groups.google.com/a/chromium.org/d/msg/infra-dev/vAZjvj8xKgU/8JWdpN9JCAAJ
> for a related thread about compile targets for bots.
>
>>
>> I think we need to change build_targets to only return targets that are
>> descendants of the supplied targets. Then I agree with using
>> build_targets.
>
>
> Sounds good to me.
>
> Scott, since you know the gyp analyze code the best, could you make the
> change? I took a look and it'd just take me some time to fully understand
> the logic there.

I didn't get to this today. I will find time tomorrow to do it.

-Scott

[Scott Violet]

On Mon, Oct 5, 2015 at 4:56 PM, Scott Violet <s...@chromium.org> wrote:
> On Mon, Oct 5, 2015 at 8:00 AM, Paweł Hajdan, Jr.
> <phajd...@chromium.org> wrote:
>> On Fri, Oct 2, 2015 at 5:18 PM, Scott Violet <s...@chromium.org> wrote:
>>>
>>> Remember that build_targets is the minimal set of targets to build
>>> assuming you want to build all. And by 'all' I mean any target
>>> reachable root from the root nodes, which is different than the all
>>> target.
>>
>>
>> Thanks for the explanation.
>>
>>>
>>> It's my understanding we have bots that don't want to build
>>> 'all', so that build_targets as implemented and used isn't right.
>>
>>
>> Yeah, we're going away from building 'all' on most bots.
>>
>> Also see
>> https://groups.google.com/a/chromium.org/d/msg/infra-dev/vAZjvj8xKgU/8JWdpN9JCAAJ
>> for a related thread about compile targets for bots.
>>
>>>
>>> I think we need to change build_targets to only return targets that are
>>> descendants of the supplied targets. Then I agree with using
>>> build_targets.
>>
>>
>> Sounds good to me.
>>
>> Scott, since you know the gyp analyze code the best, could you make the
>> change? I took a look and it'd just take me some time to fully understand
>> the logic there.
>
> I didn't get to this today. I will find time tomorrow to do it.

I spoke with Dirk about this today. His thinking is we should not
supply pseudo-targets to the builders, which would mean we can use the
'targets' output and not change analyze at all.

-Scott

[Dirk Pranke]

(As Scott just noted in his other reply ...)

It's non-trivial to make GN analyze work the ideal way for pseudo-targets (and I have some lingering concerns over how fragile this solution

will be even if we do try to dothis).

If we can get rid of 'chromium_builder_tests' and the other pseudo-targets (including all) then we don't need to change GYP either. 

Is there some reason not to just get rid of the pseudo-targets? I expect that doing so allows us to solve problems by deleting code

and making things simpler, rather than adding code to an already complex and fragile system.

-- Dirk

[Paweł Hajdan, Jr.]

Looks like we're getting quite a lot of back-and-forth here. :-/

Removing the pseudo-targets sounds like a good direction. Quick check: would we remove all of them? What prevents people from adding a target of type "none" with dependencies on other targets? Do we still have targets like that we rely on in other places?

Generally I consider that to be up to Chromium developers, not infra.

Paweł

[Scott Violet]

We could write scripts to make sure that doesn't happen.

-Scott

On Wed, Oct 7, 2015 at 6:33 AM, Paweł Hajdan, Jr.

[Dirk Pranke]

I don't think there's any harm in having the group targets for developer convenience, 

but I think we shouldn't be using them on the bots if they're overlapping with lists 

determined in other ways; using them on the bots makes the overall system 

harder to maintain and understand.

We do also use the group targets in additional_compile_targets on some bots

(e.g., //mandoline:all); ideally we only do this when there aren't tests that overlap

and get us the same coverage (I'm not actually sure if this is true for mandoline

yet or not), and so I'm not sure if I would want to ban them completely.

As Scott suggests, we could potentially catch such additions via presubmit checks

or perhaps just trust in OWNERS review to catch things; the worst case is that

we end up building too much on the trybots, but that's not the end of the world.

I admit I'm not entirely happy with any solution here yet.

-- Dirk

[Scott Violet]

As a chrome engineer using the build bots I like the ability to
specify meta-targets such as mandoline:all. These meta targets are
super convenience. I can use them in my local builds and have the bots
do exactly the same thing. If we enforced no meta targets for the
builders than we're back to two lists: mandoline:all for my day to
day, and then some separate list maintained in a file the bots consume
that easily gets out of date and I can never remember where it lives.

I think I'm back to wanting to use meta targets, and changing
analyzers for both gn and gyp to make this work properly.


-Scott

[Dirk Pranke]

You have to specify the list of test targets regardless; the bots don't

know how to look at mandoline:all to figure out which things

are test targets that should be run and which are unrelated compile

targets (*).

More importantly, a target like 'chromium_builder_tests' *only* exists

to list the targets that might need to be used on the bots; no one would

use it to build locally in normal circumstances. So it's completely

redundant.

If we treat the group targets like a normal build dependency then we

don't have to change GN and the logic is also easy to understand. 

I think for a case where you might want to specify 'mandoline:all'

as an additional_compile_target this seems sensible.

If we're dead set on supporting the 'only build the affected parts of

a meta-target' logic, then I'd want to spend some more time staring

at this to be sure we understand the exact logic and thinking about

how we'd implement it in GN.

-- Dirk

(*) with GN, we could almost figure this out, but we still need

a separate config file to indicate which things should be run under swarming,

additional command line arguments, etc. To get rid of that list we'd

have to add more test awareness to GN, which Brett has historically

not wanted to do.

[Scott Violet]

You are correct about still needing to lists tests else where, but
meta-targets may also build targets that aren't covered by tests,
which mandoline:all currently does.

-Scott

[Dirk Pranke]

Right, and I think using meta-targets for that in the 'additional_compile_targets'

part of the buildbot files is completely reasonable. However, I'm not sure that

the optimization of 'only build the parts of mandoline:all that my patch affected'

is really worth the complexity at that point.

To me, the optimization made a lot more sense for when we were building 'all'

than it does now.

-- Dirk