https://crrev.com/c/1030270 was the CL which added it this year. Dirk reviewed that.

+rhalavati@ for the question in #0. Though I think the CL description in #c2 answers the question.

tikuta@: does the extra compile cause any trouble for Goma? Or is there any other reason for this bug?

Indeed, the change was intentional at the time, but clearly there's no need to actually build *everything*, and I think maybe I didn't really think through the impact on compile time at the time.

@rhalavati - can we look at this some more and see if we can come up with a better way to scope the targets that might be affected by network annotations? If it's basically "potentially anything in //net or above" then maybe it's not worth trying to do better here.

Feel free to re-add me to the CC: list if a change I made broke something.

I cannot understand the reason of gn_all from  patch description.
But if we want to run 'check_network_annotations' step always, better to specify that in recipe side? Or can we query build files by using gn in check_network_annotations.py to get fine grained affected targets?

"gn_all" basically means "all", and what that patch is trying to say is that we don't actually know which files have network annotations, so if a change affects *any* source code, we should re-run the tests. 

However, we don't want to always run the step; if a change doesn't affect any source code, we don't need to run it.

Then can we extract targets from gn like `gn ls out/Release --type=executable` instead of always having gn_all?
So that we can ignore group target like gn_all, but catch target executables affected by src changes.

Or network_annotation_check has dependency not captured by BUILD.gn?
e.g. I imagine that change for executable A affects annotation check result for executable B if there is some network communication between A and B?

+nicolaso@,

As Dirk noted in 4, the script needs to check all files that include a traffic annotation, or include a call to a certain set of network functions.
Any file that includes either of these items needs to be compiled so that the clang tools would be able to parse it.

If there is a way to exclude test binaries from the target, that's totally safe, and I think we don't need the link step to run the clang tools. But excluding any other target *may* result in neglecting some use cases that need checking.

Changing ownership to Nicolas who is the current owner of network traffic annotations.

We could replace 'gn_all' with the output of this command:

  gn desc out/Default :gn_all deps --testonly=false

It prints out the direct dependencies of 'gn_all', one per line. Should be easy to parse. The output [1] doesn't look like it has test targets in it, so that should solve the case where only test files have been modified.

This looks like it conflicts with bug 854994. Once bug 854994, we will want to run some annotation checks on test files, as well...

[1] https://pastebin.com/Twjas93a

>Once bug 854994, we will want to [...]

Once bug 854994 is fixed*

> so that should solve the case where only test files have been modified.

Not only the case test files are modified, replacing gn_all to each dependent target will reduce the number of compile targets in non-test files modification patch case.

If we use gn_only here, modifying src used only chrome target invokes compile for the target dependent from gn_all. Such unnecessary compiles can be removed if we list up each target in the file.
I think it will reduce time of compile step. So can I ask you to update the targets?

The recipe infrastructure doesn't really have a good mechanism in place to do what #c11 suggests.

However, I think we're probably over-thinking this, as there can't really be more than a few targets that we care about. I'm not sure what else we'd want besides chrome and remoting_host, so maybe just list those two and then add others as we need to?

If it'd help, we could always add a new top-level //:shipped_binaries target that contained those and make :gn_all depend on that instead.

I don't want to have group target for this.
Group target does not prevent unnecessary compile. 

For me, this is high priority because compile step is current slowest step in CQ cycle time.
Let me replace gn_all to targets come from below command.
$ gn refs out/Release/ //net/traffic_annotation/network_traffic_annotation.h --all --type=executable

I'd like to see the difference of CQ performance by this change.

I think now that we have have tests on FYI bots, we can run them with 'all' target, and limit the trybot tests to 'chrome' and 'remoting/host'.

If annotations are added to new targets, the fyi-bots will catch them and the targets for try-bots will be updated accordingly.

#16, thank you for information.
I made patch following your comment.
https://chromium-review.googlesource.com/c/chromium/src/+/1337138

I based two test CLs on it and the result was as expected:

https://crrev.com/c/859279 triggers a full run on all annotations and worked fine on Windows and Linux.
https://crrev.com/c/859338 make a change that requires annotations.xml update and should fail and failed on both platforms.

#18, Thank you for your confirmation!

re #c15:
> I don't want to have group target for this.
> Group target does not prevent unnecessary compile. 

This comment confused me, and I had to dig into the code to make sure I knew (remembered correctly) what was going on. I think I've done that, and now I need to figure out if we have the same understanding or not.

The chromium recipe infrastructure distinguishes between the targets needed to run a test and any additional targets that you might want to compile on a bot (the additional_compile_targets case field in the //testing/buildbot files). There are important differences between these two categories.

In the former, if any of the targets are affected by a change, then *all* of the relevant targets need to be rebuilt, and we run the test. It's important to rebuild all of the targets, because otherwise you might hit a case where the targets were in an inconsistent state due to whatever might've previously been built on the bot in incremental builds (i.e., you don't know what the state of a given target is, or even if it has been built at all).

In the latter case, you don't actually care about the state of targets that aren't affected. 

This has an important side effect on the way we treat group targets. If a test depends on "gn_all", then gn_all (and everything it depends on) needs to be rebuilt. If "gn_all" is listed as an additional_compile_target, then analyze is smart enough to prune the targets that gn_all depends on and only tell the recipe to recompile the targets that matter.

So, it matters whether the targets listed in //testing/scripts/check_network_annotations.py are treated as test targets, or as additional_compile_targets. Because these scripts are actually tests, they should be treated as test targets, and it looks like they are.

And, as a result of all of that, there shouldn't actually be a difference between returning ["//:shipped_binaries"] group target label from the script, and returning ["//chrome:chrome", "//remoting/host:host"], assuming that //:shipped_binaries just has the deps on those two labels.

So, assuming you want to build both chrome and remoting_host if a CL affects *either* binary, then you should be able to use either a group or the two labels. I think this is probably the right thing to do, because as I said, you don't want to have to worry when running the script whether every binary is up to date or only some of them might be.

On the other hand, if you really only want to build the affected binary and not both of them, you can't do that in a single script. So you'd need to split this out into two scripts (or at least two script invocations, except that there's no good way to do pass command line arguments to a ScriptTest currently), one that only looks at chrome and one that only looks at remoting_host.

Hopefully this all makes sense. Let me know if it doesn't? 

Thank you for explanation, Dirk.

> If "gn_all" is listed as an additional_compile_target, then analyze is smart enough to prune the targets that gn_all depends on and only tell the recipe to recompile the targets that matter.

I think this is not true, that's why the compile step has compile target gn_all in addition to base_unittests.
gn cannot distinguish whether analyze user wants to know "gn_all" is affected by changed files or "targets that gn_all depends on" are affected.

In this issue, we want to know whether each of "chrome" and "remoting/host" are affected by changed files, instead of "gn_all".

In the case you cite initially, 'gn_all' is being specified as the *test* target from the script, in which case GN (correctly) won't do the pruning I described. Put differently, the check_annotation script is saying "if the CL affects *anything* that is part of gn_all, please build *all* of gn_all".

If you can point to cases where that's not what's happening -- for example, where gn_all is specified only in additional_compile_targets in the //testing/buildbot/*.json files, and check_annotation isn't being run, and yet we're still compiling gn_all -- I will look into it further, because what I described is what's supposed to happen and if it's not what's happening, that's a fairly severe bug.

+erikchen, since we noticed that the CQ time has gone up recently and maybe what we're talking about here is related to that, too ...

Or, if there's any other builds you want me to look at and try to explain for whatever reason for this, I'm happy to do so :).

Thank you again.

I see, I understood now. Then let me merge the patch. We actually want to test only chrome and remoting/host target for network annotation.

> +erikchen, since we noticed that the CQ time has gone up recently and maybe what we're talking about here is related to that, too ...

Is there any tracking bug related to this?

> We actually want to test only chrome and remoting/host target for network annotation.

Do you agree that creating a group that depended on those two targets would have the same effect? (I don't actually care that much if we create a group or not).

> Is there any tracking bug related to this?

bug 905313, I think.

> Do you agree that creating a group that depended on those two targets would have the same effect? (I don't actually care that much if we create a group or not).

Sorry, I have not yet agreed.
In my understanding, when we have ["//:shipped_binaries"] group target label from the script, I expect ":shipped_binaries" is specified in compile step, that compiles file necessary for both "chrome:chrome" and "remoting/host:host" target.

If we have ["//chrome:chrome", "//remoting/host:host"] in the script instead of group target, I expect one of the target is specified in compile step when CL does not have files that both target depends on. gn analyze drops unnecessary target in this case.

But if we want to test both of "//chrome:chrome" and "//remoting/host:host", even if the change itself affects to one of the binary, we need to have grouped target as you said.


So I want to make sure that what is required by check_network_annotations.py.
Does the script require to build both "chrome:chrome" and "remoting/host:host" if the change affects only one of the targets in terms of compile (not annotation check)?

> In my understanding, when we have ["//:shipped_binaries"] group target label 
> from the script, I expect ":shipped_binaries" is specified in compile step, 
> that compiles file necessary for both "chrome:chrome" and
> "remoting/host:host" target.

Right, that's what I'd expect to happen.

> If we have ["//chrome:chrome", "//remoting/host:host"] in the script instead 
> of group target, I expect one of the target is specified in compile step
> when CL does not have files that both target depends on. gn analyze drops 
> unnecessary target in this case.

I'm saying I would *not* expect this to happen. I'd expect analyze to keep both
targets and for us to compile both targets. 

That's why I say that this should be treated the same as a group target.

If you had a case where things were working the way you'd say, I'd like to look into it. 
I'd consider it to be wrong.

> I'm saying I would *not* expect this to happen. I'd expect analyze to keep both
> targets and for us to compile both targets. 

Seems not? chrome is kept, but remoting/host is not kept by gn analyze in
https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8929658979376204752/+/steps/analyze/0/stdout
for https://chromium-review.googlesource.com/c/chromium/src/+/1337138/7

Or post `gn analyze` step does what you said?

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9d11eb1d67bb9c44e7a2c4785ca3c00c70927f07

commit 9d11eb1d67bb9c44e7a2c4785ca3c00c70927f07
Author: Takuto Ikuta <tikuta@chromium.org>
Date: Tue Nov 20 02:30:00 2018

Update compile target in check_network_annotations.py

This is to prevent unnecessary compiles in CQ compile step.

Bug:  904770 
Change-Id: Ia1080f887725b6e00a27248c60153127db7dbf76
Reviewed-on: https://chromium-review.googlesource.com/c/1337138
Reviewed-by: Nicolas Ouellet-Payeur <nicolaso@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Takuto Ikuta <tikuta@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609568}
[modify] https://crrev.com/9d11eb1d67bb9c44e7a2c4785ca3c00c70927f07/BUILD.gn
[modify] https://crrev.com/9d11eb1d67bb9c44e7a2c4785ca3c00c70927f07/testing/buildbot/gn_isolate_map.pyl
[modify] https://crrev.com/9d11eb1d67bb9c44e7a2c4785ca3c00c70927f07/testing/buildbot/manage.py
[modify] https://crrev.com/9d11eb1d67bb9c44e7a2c4785ca3c00c70927f07/testing/scripts/check_network_annotations.py

re #c28:

It's a little hard to follow what actually happened, but the main reason we compiled both is that we compiled *everything*, because you modified something in //testing/scripts/* which matched the whitelist for ignoring the results of analyze.

If you hadn't changed the script, then I would've expected analyze to do the same thing (and not include remoting_host), but I still would've expected the recipe to compile remoting_host. It's possible there's a bug in that logic, though; I wouldn't be surprised if there was, now that I think about it, and so using the group (as you did) is actually probably safer.

gn_all is not built currently.