Use simpler "analyze" logic for chromium_trybot recipe on non-Windows

Use simpler "analyze" logic for chromium_trybot recipe

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

[Paweł Hajdan Jr., 2015-06-15 12:30:00]

phajdan.jr@chromium.org changed reviewers:
+ akuegel@chromium.org, sergiyb@chromium.org

[Adrian Kuegel, 2015-06-15 12:39:47]

From the expectation changes it looks like this means we won't be filtering out
anything for compilation. Is that true, and is this intended?

[Paweł Hajdan Jr., 2015-06-15 13:09:16]

On 2015/06/15 at 12:39:47, akuegel wrote:
> From the expectation changes it looks like this means we won't be filtering
out anything for compilation. Is that true, and is this intended?

It's not intended, and I think it's just that the recipes don't contain the
analyzer logic which would do the actual filtering.

[Adrian Kuegel, 2015-06-15 13:15:20]

On 2015/06/15 13:09:16, Paweł Hajdan Jr. wrote:
> On 2015/06/15 at 12:39:47, akuegel wrote:
> > From the expectation changes it looks like this means we won't be filtering
> out anything for compilation. Is that true, and is this intended?
> 
> It's not intended, and I think it's just that the recipes don't contain the
> analyzer logic which would do the actual filtering.

Hmm, but how are the results of the analyzer used then? The code you get rid of
did the intersection of original compile targets with the compile targets
analyze determined are affected by the patch. I think that is the logic that
does the actual filtering.

[Adrian Kuegel, 2015-06-15 13:21:48]

On 2015/06/15 13:15:20, Adrian Kuegel wrote:
> On 2015/06/15 13:09:16, Paweł Hajdan Jr. wrote:
> > On 2015/06/15 at 12:39:47, akuegel wrote:
> > > From the expectation changes it looks like this means we won't be
filtering
> > out anything for compilation. Is that true, and is this intended?
> > 
> > It's not intended, and I think it's just that the recipes don't contain the
> > analyzer logic which would do the actual filtering.
> 
> Hmm, but how are the results of the analyzer used then? The code you get rid
of
> did the intersection of original compile targets with the compile targets
> analyze determined are affected by the patch. I think that is the logic that
> does the actual filtering.

Btw, I have read crbug/493998. It seems what really needs to be fixed is the
output of self.m.filter.compile_targets
(so the fix needs to be done in the analyze script itself, I presume).

[Paweł Hajdan Jr., 2015-06-15 13:23:03]

On 2015/06/15 at 13:15:20, akuegel wrote:
> Hmm, but how are the results of the analyzer used then?

Please see the call sites, e.g. in the chromium_trybot recipe.

> The code you get rid of did the intersection of original compile targets with
the compile targets analyze determined are affected by the patch.

Correct. I believe this logic is not needed.

> I think that is the logic that does the actual filtering.

I don't think that's accurate. The actual filtering (for gyp) lives in
https://code.google.com/p/chromium/codesearch#chromium/src/tools/gyp/pylib/gy...
. By looking at analyze_details step (e.g.
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
it should be possible to simulate effects of this change. Instead of
compile_targets we'd use self.m.filter.compile_targets .

In fact looking at above example I've noticed
https://code.google.com/p/chromium/issues/detail?id=500534 isn't
Windows-specific. I'll postpone landing this patch then.

[Adrian Kuegel, 2015-06-15 13:38:21]

On 2015/06/15 13:23:03, Paweł Hajdan Jr. wrote:
> On 2015/06/15 at 13:15:20, akuegel wrote:
> > Hmm, but how are the results of the analyzer used then?
> 
> Please see the call sites, e.g. in the chromium_trybot recipe.

From what I can tell, the analyze step is called here:
https://code.google.com/p/chromium/codesearch#chromium/build/scripts/slave/re...
I don't see anything in the recipe that uses self.m.filter.compile_target after
that, so this result would be lost. So I think we depend on
api.chromium_tests.analyze step modifying the compile targets before it returns
them (which is not happening anymore). And the test target specific logic does
apply the same logic no matter if analyze filtered something out or not.

> > The code you get rid of did the intersection of original compile targets
with
> the compile targets analyze determined are affected by the patch.
> 
> Correct. I believe this logic is not needed.

I am still not convinced about that.

> > I think that is the logic that does the actual filtering.
> 
> I don't think that's accurate. The actual filtering (for gyp) lives in
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/gyp/pylib/gy...
> . By looking at analyze_details step (e.g.
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> it should be possible to simulate effects of this change. Instead of
> compile_targets we'd use self.m.filter.compile_targets .
> 
> In fact looking at above example I've noticed
> https://code.google.com/p/chromium/issues/detail?id=500534 isn't
> Windows-specific. I'll postpone landing this patch then.

I think you misunderstood my point: I know that the analyzer script does the
actual filtering, but these lines applied the filtering to the compile targets.
Otherwise you would call analyze, but basically ignore its result.
Maybe I am wrong, but from how I read the code, this seems to be the case.

Sergiy, maybe you can also take a look at this CL?

[Adrian Kuegel, 2015-06-15 13:56:46]

>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/gyp/pylib/gy...
> > . By looking at analyze_details step (e.g.
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> > it should be possible to simulate effects of this change. Instead of
> > compile_targets we'd use self.m.filter.compile_targets .

I agree with that. Probably we should just use self.m.filter.compile_targets
instead of this intersection of compile_targets with
self.m.filter.compile_targets. The gyp analyzer seems to already get the
requested compile targets as input, so it probably already takes them into
account.

[Paweł Hajdan Jr., 2015-06-15 14:13:22]

Ah good catch. Patch updated, and also added a reference to
https://code.google.com/p/chromium/issues/detail?id=493998

[Adrian Kuegel, 2015-06-15 14:19:44]

https://codereview.chromium.org/1187863002/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium_tests/api.py (right):

https://codereview.chromium.org/1187863002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_tests/api.py:529: compile_targets =
list(set(self.m.filter.matching_exes + compile_targets))
I have the feeling that this should be done in any case. Could you please move
this line behind this if legacy_postprocess/else block?

[Paweł Hajdan Jr., 2015-06-15 15:02:44]

https://codereview.chromium.org/1187863002/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium_tests/api.py (right):

https://codereview.chromium.org/1187863002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_tests/api.py:529: compile_targets =
list(set(self.m.filter.matching_exes + compile_targets))
On 2015/06/15 at 14:19:44, Adrian Kuegel wrote:
> I have the feeling that this should be done in any case. Could you please move
this line behind this if legacy_postprocess/else block?

I believe this should be handled by the analyzer. I don't think we should do the
additional post-processing, especially that with
https://codereview.chromium.org/1176553003 we're just using matching_exes
returned by analyzer anyway.

[Adrian Kuegel, 2015-06-15 15:16:19]

On 2015/06/15 15:02:44, Paweł Hajdan Jr. wrote:
>
https://codereview.chromium.org/1187863002/diff/20001/scripts/slave/recipe_mo...
> File scripts/slave/recipe_modules/chromium_tests/api.py (right):
> 
>
https://codereview.chromium.org/1187863002/diff/20001/scripts/slave/recipe_mo...
> scripts/slave/recipe_modules/chromium_tests/api.py:529: compile_targets =
> list(set(self.m.filter.matching_exes + compile_targets))
> On 2015/06/15 at 14:19:44, Adrian Kuegel wrote:
> > I have the feeling that this should be done in any case. Could you please
move
> this line behind this if legacy_postprocess/else block?
> 
> I believe this should be handled by the analyzer. I don't think we should do
the
> additional post-processing, especially that with
> https://codereview.chromium.org/1176553003 we're just using matching_exes
> returned by analyzer anyway.

Ok, makes sense. So LGTM

[Sergiy Byelozyorov, 2015-06-15 18:53:02]

lgtm

[Paweł Hajdan Jr., 2015-06-22 10:46:32]

The CQ bit was checked by phajdan.jr@chromium.org

[commit-bot: I haz the power, 2015-06-22 10:46:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1187863002/20001

[commit-bot: I haz the power, 2015-06-22 10:48:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-22 10:48:40]

Try jobs failed on following builders:
  build_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/build_presubmit...)

[Paweł Hajdan Jr., 2015-06-22 11:50:17]

The CQ bit was checked by phajdan.jr@chromium.org

[Paweł Hajdan Jr., 2015-06-22 11:50:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
sergiyb@chromium.org, akuegel@chromium.org
Link to the patchset: https://codereview.chromium.org/1187863002/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2015-06-22 11:50:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1187863002/40001

[commit-bot: I haz the power, 2015-06-22 11:53:57]

Committed patchset #3 (id:40001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=295776

[scottmg, 2015-06-23 00:04:36]

On 2015/06/22 11:53:57, commit-bot: I haz the power wrote:
> Committed patchset #3 (id:40001) as
> http://src.chromium.org/viewvc/chrome?view=rev&revision=295776

Is it possible this caused
https://code.google.com/p/chromium/issues/detail?id=503103 ?