linux_jumbo_rel doesn't test compile when blink paint code changes due to faulty analyzer step |
|||||
Issue descriptionI just did a change in Blink paint code and linux_jumbo_rel, the CQ builder, decided it didn't have to do anything. This might be the reason code that broke jumbo was checked in. https://ci.chromium.org/p/chromium/builders/luci.chromium.try/linux-jumbo-rel/92376 https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8933246391750079984/+/steps/analyze/0/stdout analyze input: { "files": [ "third_party/blink/renderer/core/paint/image_paint_timing_detector.cc", "third_party/blink/renderer/core/paint/text_paint_timing_detector.cc" ], } analyze output: { "compile_targets": [], "status": "No dependency", "test_targets": [] }
,
Oct 8
% gn --version 1463 (e134e493) brettw, do you have any pointers for where this might be go wrong?
,
Oct 8
Reading the gn source for gn analyze, it seems to depend 100% on files being listed in |sources|, even for headers, inc files, proto files and any other file that might affect the target. This won't work for jumbo because if the second level cc files are included in sources, they will be compiled. It also seems a bit fragile since it seems to be relatively common to forgot to list all dependencies in |sources|
,
Oct 8
So the right fix might be to tell the analyze step to not worry about what gn analyze says. There is already that ability, through build machines reading a trybot_analyze_config.json file which is checked in as //testing/buildbot/trybot_analyze_config.json. No trivial change will help here though since the files that should be excluded have no specific file name or path pattern and it also depends on use_jumbo_build in the gn configuration. Maybe the right thing to do is to have a ".*" pattern for jumbo builds, which would require adding knowledge about jumbo builds to the build server system.
,
Oct 8
Yet another alternative is to dump all the .cc files in the "data" variable, with a number of unknown side effects. It was considered as a way to help IDEs, but rejected for the main gn branch. I can't recall if it was broken or if gn just didn't want to have any jumbo support in the binary.
,
Oct 8
I wonder if something has changed, because I feel like analyze used to work on jumbo builds. I'll take a closer look into it. Maybe the action step that is creating the jumbo files isn't listing the source files as inputs? Or maybe analyze isn't looking at inputs properly?
,
Oct 8
I also thought this used to work. I've seen the linux_jumbo_rel bot be very fast at least a couple of days. At the time I assumed that was a good thing. :) Last change to jumbo.gni was 3 months ago with the new algorithm for creating chunks. It didn't change any dependencies. The action step doesn't list the files as inputs deliberately since it does not depend on the files, only the names of the files and those are already known. I think it was bad some way (performance?) to depend on the files but I can't recall why right now. It's been that way since the start anyway. So if it used to work, it worked for some other reason. Ninja's dependency files maybe, those are a more reliable source than gn if they exist.
,
Oct 9
I'm not yet sure what's going on here. On the one hand, analyze seems to be working some of the time: https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8933188810785328016/+/steps/analyze/0/stdout on the other hand, it seems like it shouldn't ever work, because the sources *should* be listed as inputs to the merge scripts, and we're not doing that. Still looking further, because these three situations can't all be true :).
,
Oct 9
Ok, the reason analyze worked in #c8 is because the change modified a header file, which propagates through to the "sources". The "right fix" I think would be to declare all of the source that are listed in a particular jumbo file as inputs to the merge script. That way GN would know about the sources (which it doesn't currently). However, that may not actually work right in the presence of visibility rules, because visibility will apply to the target_name, not target_name + "__jumbo_merge". Fixing this would I think require us to hack up visibility rules all over the place, which we likely don't want to do. We could just turn off analyze on this builder, but that would have unpleasant side effects like building way too much stuff. We're not actually running tests or creating isolates, so dumping the sources into `data` might be the best alternative; it won't have any impact on the build, and I tested that and it appears to work. This might be another place where the only way to do this correctly would be to bake jumbo support into GN directly.
,
Oct 9
There will be some using jumbo that also build and run tests. How bad would it be for them to have a lot of source files in |data|? Is it just when distributing tests to another server it would bad?
,
Oct 17
Yes, having things listed in |data| is ignored by the build, it's only used by tooling afterwards (to build isolates, run `analyze`, etc.)_
,
Nov 5
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by brat...@opera.com
, Oct 8