understanding "analyze" behavior - unit tests in returned compile targets even with only "chrome" requested

[Paweł Hajdan, Jr.]

Hi Scott, could you help me understand what's going on here?

I'm wondering - I could be just misusing "analyze", or it could be a bug, or maybe something else - maybe a combination of both.

Please see http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_chromium_compile_only_ng/builds/4081 .

http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_chromium_compile_only_ng/builds/4081/steps/analyze/logs/analyze_details shows only "chrome" and "chrome_initial" in "original_compile_targets", but "analyze" seems to return a lot of unit tests as targets to compile.

Shouldn't there be a check to only return ones covered by originally passed compile targets?

I'm considering reverting https://codereview.chromium.org/1187863002 which change the way recipes work with "analyze". I'm just not sure what the correct behavior would be here - could you recommend the best course of action?

Thanks

Paweł

[Scott Violet]

Currently analyzer returns, in build_targets, the minimal set of
targets necessary to build that are reachable from the roots.

Imagine you have two complete repos at tip of tree. You build all in
both. Then you apply the patch to both. In the first repo you again
build all, but in the second repo you instead built the set of targets
output as build_targets from analyzer. The output of the two repos
should now be exactly the same. In other words building the
build_targets get you to all.

Based on your question it seems as though you're expecting
build_targets to only include things that are children of the supplied
targets. That isn't the case. I could certainly make it that way, but
for the time being it isn't. The rationale for this behavior is at the
time I did analyzer we always built All. If that isn't the case, and
we want to restrict to a subset, then we'll need to add support for
that to analyzer.

-Scott

[Dirk Pranke]

Scott,

My understanding of the file format is documented here:

https://chromium.googlesource.com/chromium/src/+/master/tools/mb/docs/user_guide.md#mb_analyze

I believe that the GN analyze logic (as implemented in MB) conforms to the spec; in GN's analyze, however,

I always set build_targets=targets (i.e., there is no subsetting).

It sounds like you're saying that the output 'build_targets' can include things not in in the input 'targets'; I assume

that's true for 'targets' also (since the output 'build_targets' is supposed to be a subset of 'targets')?

It seems to me that we should implement what I have specced in the gyp analyze, to match behavior and

to avoid building things we don't actually need.

Would it be hard to add that to the GYP analyzer? It would be simple to add it to MB if need be (and we

might even be able to share the logic) but not everything is running MB yet (though everything will eventually

need to go through MB). It's not clear to me if it's better for us to share more logic between gyp and gn

via MB or if it's better to keep all of the gyp logic in one place.

-- Dirk

--
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/CAKARY_%3DBRe41pW0cp%2Bfs2NYiSO2F%2BC6Q3AVSU%3DLktxJfn1h7Gw%40mail.gmail.com.

[Scott Violet]

On Tue, Jun 23, 2015 at 6:00 PM, Dirk Pranke <dpr...@chromium.org> wrote:
> Scott,
>
> My understanding of the file format is documented here:
>
> https://chromium.googlesource.com/chromium/src/+/master/tools/mb/docs/user_guide.md#mb_analyze
>
> I believe that the GN analyze logic (as implemented in MB) conforms to the
> spec; in GN's analyze, however,
> I always set build_targets=targets (i.e., there is no subsetting).
>
> It sounds like you're saying that the output 'build_targets' can include
> things not in in the input 'targets';

That is correct.

> I assume
> that's true for 'targets' also (since the output 'build_targets' is supposed
> to be a subset of 'targets')?

No, targets is only ever a subset of targets.

> It seems to me that we should implement what I have specced in the gyp
> analyze, to match behavior and
> to avoid building things we don't actually need.
>
> Would it be hard to add that to the GYP analyzer? It would be simple to add
> it to MB if need be (and we
> might even be able to share the logic) but not everything is running MB yet
> (though everything will eventually
> need to go through MB). It's not clear to me if it's better for us to share
> more logic between gyp and gn
> via MB or if it's better to keep all of the gyp logic in one place.

It is certainly possible to add this logic to analyze and it shouldn't
be that hard. The one caveat is that this only makes sense if we never
supply All as a target, and only supply real exes as targets. The
reason you don't want to supply All as a target is because targets are
returned if any of their deps (all they way down) contain one of the
changed files. So that if you did supply All as a target nearly every
change would return All, which defeats the purpose.

At the time I did analyze we generally built the all target. This is
why we have the logic we have now. If that is no longer the case, then
we can certainly change around analyze.

-Scott

[Dirk Pranke]

On Wed, Jun 24, 2015 at 8:55 AM, Scott Violet <s...@chromium.org> wrote:

On Tue, Jun 23, 2015 at 6:00 PM, Dirk Pranke <dpr...@chromium.org> wrote:
> Scott,
>
> My understanding of the file format is documented here:
>
> https://chromium.googlesource.com/chromium/src/+/master/tools/mb/docs/user_guide.md#mb_analyze
>
> I believe that the GN analyze logic (as implemented in MB) conforms to the
> spec; in GN's analyze, however,
> I always set build_targets=targets (i.e., there is no subsetting).
>
> It sounds like you're saying that the output 'build_targets' can include
> things not in in the input 'targets';

That is correct.

> I assume
> that's true for 'targets' also (since the output 'build_targets' is supposed
> to be a subset of 'targets')?

No, targets is only ever a subset of targets.

I'm a bit confused; if build_targets is not a subset of (output) targets, and build_targets

is the minimal subset of things needed to build all of targets, why isn't build_targets

just 'All' (which would, of course, be useless). I guess I don't understand the logic of how you compute build_targets if it can include things that weren't listed in the input targets?

I can swing by later today to discuss.

-- Dirk

[Dirk Pranke]

Okay, Scott and I talked this through ...

In short, we should change the filter/api.py script to ignore the 'build_targets' param, and use just the 'targets' param 

in the output; once we've done that, we should remove 'build_targets' altogether.

build_targets exists for two reasons: 

1) in the belief that we might be able to shorten the list of targets passed to the compile when some targets are dependencies of others

2) to avoid potential weirdness that might arise if you pass in 'all' as part of the input 'targets' spec, in case you don't really want to build all.

Based on my calculations, we will probably never need to actually worry about #1; in the worst case, it's hard to picture the command line

exceeding 5000 chars, and up to 8000 should be safe. However, as we've discussed before, we can add response files to ninja if need be.

Scott added the code for 2) to work around cases where we were passing in 'all' to analyze but didn't really mean want all. We think at 

this point such things should just be bugs that get fixed, and it's not clear if anyone will ever pass in 'all' now (it's possible to do so via

additional_compile_targets in the //buildbot/testing/*.json files, but no one seems to do so).

It seems clear that at this point having both 'targets' and 'build_targets' is just kinda confusing, and we think we don't need it (in fact,

in the GN implementation they always have the same values).

Scott, did I miss anything? Paweł, does that make sense?

-- Dirk

[Scott Violet]

You got it.

Just for clarity, when I wrote analyzer we were building 'All' on the
bots and wanted to make sure bots continued to effectively build all
even if they only ran a set of tests. Hence the need for build_targets
in addition to targets. As that is no longer the case we can ignore
build_targets entirely.

-Scott

[Paweł Hajdan, Jr.]

Thanks!

I uploaded https://codereview.chromium.org/1212413002 as the first step.

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/CAKARY_%3DAca7fhwWw0UeoadhYxSztdq3Av21r%3DDLfFja5TKoH5A%40mail.gmail.com.