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

[Daniel Cheng]

Sure, but the biggest problem with doing it at upload time is "which set of GN argos do you run GN check for"? Not everyone has their build directory at `out/Release` or `out/Debug`.

Daniel

On Mon, 29 Sept 2025 at 05:14, James Lee <ljj...@google.com> wrote:

I think paying a performance penalty on every build is worse than paying the penalty at upload: builds are a much more common operation, and it would be painful if the build-test-fix cycle got slower. Demetrios: were you hoping that running "gn check" as part of a build might not add significant time due to reusing some of the analysis that's already done in the build and/or running in parallel with other build steps?

Another thought I had for presubmits was to run gn check only on the affected targets, by combining "gn refs <modified_c++_files>" and "gn check <target>". From some quick tests this helps a bit but still leaves 1-2s of elapsed time (partly because "gn refs" takes a while, and partly because Chrome often has very large targets that end up being checked). It sounds like this wouldn't make a large enough difference for folks to want this enabled by default.

Cheers,

James

[Erik Staab]

We had a tracking bug for at least part of what's being discussed in this thread crbug.com/41271544.

To summarize the issues people have brought up:

1. It can be slow. On the bots it often takes around a minute (we were even considering taking it out of the critical path to make the more common passing case faster). We don't want this in the local interactive critical path unless we can be sure it will be fast or the user opts in.

2. Checking locally only covers the configuration(s) the person has locally, e.g. not every iOS, Fuchsia, ChromeOS, etc. configuration. How often do the builders catch check failures in configurations people were developing on locally vs something else?

I haven't looked but I wonder if gn check could be optimized at all. I think that would be generally useful for humans and bots.

Also, I wonder if having a builder that just has 6 out/ directories for different configurations to run gn check to give a quick signal would be helpful. At one point we wanted to have builders that auto-trigger on upload and something like this could be a good candidate, IMO.

Thanks,

Erik

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

[Reilly Grant]

I've been doing a bit of internal Google development recently and noting some of the differences from my accustomed Chromium workflow. Something that happens there is that there are some checks run automatically when the CL is uploaded, before presubmits are triggered. This could be a useful compromise between slowing down the upload process and delaying notification to the developer that something is wrong.

Reilly Grant | Software Engineer | rei...@chromium.org | Google Chrome

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

[James Lee]

I think paying a performance penalty on every build is worse than paying the penalty at upload: builds are a much more common operation, and it would be painful if the build-test-fix cycle got slower. Demetrios: were you hoping that running "gn check" as part of a build might not add significant time due to reusing some of the analysis that's already done in the build and/or running in parallel with other build steps?

Another thought I had for presubmits was to run gn check only on the affected targets, by combining "gn refs <modified_c++_files>" and "gn check <target>". From some quick tests this helps a bit but still leaves 1-2s of elapsed time (partly because "gn refs" takes a while, and partly because Chrome often has very large targets that end up being checked). It sounds like this wouldn't make a large enough difference for folks to want this enabled by default.

Cheers,

James

On Fri, 26 Sept 2025 at 18:39, Dirk Pranke <dpr...@chromium.org> wrote:

[Dirk Pranke]

On Mon, Sep 29, 2025 at 5:14 AM James Lee <ljj...@google.com> wrote:

I think paying a performance penalty on every build is worse than paying the penalty at upload: builds are a much more common operation, and it would be painful if the build-test-fix cycle got slower. Demetrios: were you hoping that running "gn check" as part of a build might not add significant time due to reusing some of the analysis that's already done in the build and/or running in parallel with other build steps?

Another thought I had for presubmits was to run gn check only on the affected targets, by combining "gn refs <modified_c++_files>" and "gn check <target>". From some quick tests this helps a bit but still leaves 1-2s of elapsed time (partly because "gn refs" takes a while, and partly because Chrome often has very large targets that end up being checked). It sounds like this wouldn't make a large enough difference for folks to want this enabled by default.

This would be a bit expensive, since you'd be reading in and computing the build graph twice. It would probably be better (and doable) to add something like `gn check <files>` to combine things into a single command.

-- Dirk

[Dirk Pranke]

On Mon, Sep 29, 2025 at 9:32 AM Erik Staab <est...@chromium.org> wrote:

We had a tracking bug for at least part of what's being discussed in this thread crbug.com/41271544.

To summarize the issues people have brought up:

1. It can be slow. On the bots it often takes around a minute (we were even considering taking it out of the critical path to make the more common passing case faster). We don't want this in the local interactive critical path unless we can be sure it will be fast or the user opts in.

2. Checking locally only covers the configuration(s) the person has locally, e.g. not every iOS, Fuchsia, ChromeOS, etc. configuration. How often do the builders catch check failures in configurations people were developing on locally vs something else?

I haven't looked but I wonder if gn check could be optimized at all. I think that would be generally useful for humans and bots.

It would be useful, but gn has been pretty heavily optimized for what it currently does (and how it currently works). I think to make any significant improvements you'd have to change what it is doing or how it works. For example, GN currently computes the entire build graph no matter what you want to do, and recomputes it every time you run. You could potentially change GN to compute a partial graph (e.g., only what it needs for certain targets) and/or you could serialize the build graph to disk to cache it. 

-- Dirk

[Dirk Pranke]

I'm not sure I follow. Do you mean that the change is uploaded and some checks are triggered to run remotely before (or while) presubmit is running locally?

-- Dirk 

[Erik Staab]

On Mon, Sep 29, 2025 at 2:16 PM Dirk Pranke <dpr...@chromium.org> wrote:

I'm not sure I follow. Do you mean that the change is uploaded and some checks are triggered to run remotely before (or while) presubmit is running locally?

I think this is equivalent to what I was suggesting with this comment:

> I wonder if having a builder that just has 6 out/ directories for different configurations to run gn check to give a quick signal would be helpful. At one point we wanted to have builders that auto-trigger on upload and something like this could be a good candidate, IMO.

Upload-triggered builders were something we wanted with the CQ migration to CV, partly inspired by internal Google development.

Thanks,

Erik

[James Lee]

Upload-triggered checks on the Gerrit side does sound like a good compromise.

Gerrit already triggers some "quick" checks on upload (rather than on full CQ runs), such as "Conflict Markers", "Commit Message", "Code Duplication" etc. (https://screenshot.googleplex.com/BFpJahvUuzFXysy): maybe "gn check" could be added at that stage (for a selected set of builders)? I'm not sure how feasible / easy this would be?

Thanks,

James