Proposal: run "gn check" in Chromium PRESUBMIT

[James Lee]

I would like to get feedback on running "gn check" as part of the Chromium presubmit checks (i.e. in the top-level PRESUBMIT.py).

An annoyance I often come up against is gn dependency checks like this example. I typically don't hit these when building/testing locally, since this check isn't enforced. I only find out about the breakage during the CQ run, which is inefficient (extra context switch, and this delays getting actual test results from the CQ).

This could be avoided by adding "gn check <out_dir>" to pre-upload checks, which would all engineers to catch and fix issues sooner.

• I've run the command locally and it typically takes 5-7s elapsed time (tested on both a MacBook Pro, and a high-end server Linux VM)
• An implementation detail is how to find/pick the output directory
• this could use a simple heuristic like the more recently modified out/* folder, skipping the check if none is found
• alternatively this could look for just "out/Default" (skipping otherwise) which would provide a way for users to opt in/out of this check

I'm happy to send a CL if people (including PRESUBMIT owners) think this is a reasonable idea.

Thanks,

James

[Elly]

Hi James,

At least on my workstation:

```

$ time git cl presubmit --force

Running presubmit commit checks ...

presubmit checks passed.

real 0m01.59s

user 0m01.32s

sys 0m00.41s

ravnica$ time gn check out/rel-cros   

36343 targets out of 36349 checked based on the check_targets or no_check_targets defined in ".gn".

Header dependency check OK

real 0m12.44s

user 5m52.90s

sys 1m50.74s

```

so 'gn check' on my CrOS out directory takes 5-6x longer than 'git cl presubmit' currently does. That said, I know there are presubmit hooks that can be quite slow like check-owners and such, so not all presubmit runs are so speedy.

Maybe this would be a good option to add, though?

-- elly

[Dirk Pranke]

In general, I'm not a huge fan of the idea that we do this by default. In general, I think upload is way too slow (really I'd like it if it was <1 s) and I think this would make upload significantly slower on average.

Second, as you say it's hard to know which directories to check. I'm not sure how well either of your heuristics would work in practice.

Your problem can of course be easily solved by writing a shell function or a wrapper script that simply calls the two commands in sequence (`gn check out/default && git cl upload`), so I would probably first just suggest you do that. If a fair number of people wanted to run this, we could then as Elly suggests maybe add this as an option to upload (`git cl upload --check out/linux --check out/android`). But I'd want a fair amount of demand for this before we add it.

-- Dirk

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.
To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/d5d16915-903c-4b11-b306-18d6065888bbn%40chromium.org.

[Joe Mason]

+1 to uploads taking too long already

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CALyq14-%3DAqogKMXo3qM6ytpkHTSC%2B3jd4wAdw2HLOPYEDYoNTw%40mail.gmail.com.

[James Lee]

Thanks all for the helpful feedback; I hear that the performance tradeoff wouldn't work for many/most folks.

> Your problem can of course be easily solved by writing a shell function or a wrapper script that simply calls the two commands in sequence (`gn check out/default && git cl upload`), so I would probably first just suggest you do that.

Unfortunately I don't think this approach easily works with some internal Google tooling; I'm following up separately with the relevant team so see what's feasible.

James

[Demetrios Papadopoulos]

Perhaps "gn check" could be part of normal builds, instead of the upload presubmit? Would that address the original issue of "I only find out about the breakage during the CQ run ..."  This also has the advantage that an output folder (which is required by gn check) is already known, and no heuristics are needed.

Moreover, this could be an opt-in behavior with a GN flag (something like always_run_gn_check=true...) if we don't want to impose this time penalty on everyone. FWIW, I have compute_inputs_for_analyze=true locally, which while not 100% sure I think it has some overlap with what "gn check" does.

Thanks,

Demetrios

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHXZhwHr8ykH91%3DmRuUeAd7m8iHbA2V-yO5Y%3Dan0X2eaP_eT0A%40mail.gmail.com.

[Dirk Pranke]

It's not obvious to me how you'd actually implement that if you wanted to integrate it into the build itself (i.e., have ninja/siso invoke it), because it's not clear what you'd need the dependency graph to look like: what files would the invocation depend on, and what would it produce to indicate that it was up to date and didn't need to run? E.g., if you were only modifying TypeScript or Python files, presumably you wouldn't actually need (and thus want) GN check to run.

Put differently, neither ninja nor siso (AFAIK) have the concept of "always run this command if you need to build anything". We could maybe modify GN to add a dependency from every C++ file to a 'run gn check' target that just touched a file or something, but that would be something of an ugly hack and I wonder what the performance impact on both running GN and running Nina would be.

You could also, of course, modify autoninja to run it, though.

Apart from that, I'm not so much a fan of this idea because of the performance impact even in the best case, either, so my response is basically the same as it was for modifying upload: you can probably do it yourself in most cases, but if there is sufficient demand, perhaps we could add a flag (if that would work in the cases it needs to).

-- Dirk

[Daniel Cheng]

Could we make `gn gen --check` sticky somehow? When you build with ninja/siso, I think there's a step that re-runs GN if needed. If it were sticky, then people who were OK with the additional build-time hit could just opt into it.

(I thought it used to be sticky, but from some brief testing, that no longer appears to be the case)

Daniel

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CALyq14-tG4mP4vTggKuqgkEwQH7gmN_iLsdA7DiwdDsP_R6koA%40mail.gmail.com.

[Dirk Pranke]

Yes, ninja/siso will rerun GN if it thinks the build is outdated, and `gn check` should be sticky at `gn gen` time. I.e., if you run `gn gen --check`, then the code to automatically rerun `gn gen` when the build changes should include the `--check` flag. If that's not happening, it's probably a bug. (I feel like I actually tested this to be sure a couple months ago and it was working then, but I could be misremembering).

-- Dirk