Intent to implement: //nocheck comment for includes

[Brett Wilson]

BACKGROUND

GN has a checkdeps-like command "gn check <build dir>". Currently, this is run on the GN bots. It runs the check for a whitelisted set of directories listed in the file src/.gn (since the entire tree isn't yet clean).

gn-check differs from checkdeps in that gn-check enforces that a file including a header actually depend on the target with that header, and that the header is marked as publicly accessible to the includer. This is important for several reasons:

- "Analyze" on the bots is based on the dependency tree of the build, not the computed graph of header dependencies. This means that analyze sometimes misses things that depend on a change, and required tests aren't compiled or run. This is a problem in both GN and GYP.

- In GN, public configs (which are how a target can apply include directories and defines to other targets that depend on it) go with the ability to include headers. Enforcing proper dependencies will also enforce that the proper defines are set for using a header. Coming from GYP, any time you have direct_dependent_settings these things can get out of sync, which is why people often use all_dependent_settings as a big hammer to solve the problem.

THE PROBLEM

GN check has a pretty stupid header scanner. It doesn't understand #ifdefs. Consider the source:

  #if defined(ENABLE_DOOM_MELON)

  #include "components/doom_melon/power_controller.h"

  #endif

and the corresponding build logic:

  if (enable_doom_melon) {

    deps += [ "//components/doom_melon" ]

  }

This is consistent and correct, but when the feature is disabled GN will see an include without a corresponding dependency and flag an error.

A mitigating factor is that GN will not flag includes it doesn't know about. Most platform- and feature-specific includes aren't declared in the build when that platform or feature isn't enabled. This means most ifdefed and feature-specific includes still pass the current check step.

A second mitigating factor is that a BUILD file can exclude an entire target from include checking. This can be used for third party code that might do weird things.

PROPOSAL

Add a "// nocheck" comment to the end of includes to exclude them from checking via gn check. This will appear in source code which is why I'm sending this proposal.

This should be quite rare and only on includes in ifdefs (even most ifdefed includes are OK as discussed above).

Example:

  #if defined(ENABLE_DOOM_MELON)

  #include "components/doom_melon/power_controller.h"  // nocheck

  #endif

ALTERNATIVES

A more robust implementation would run gcc (or equivalent) in -MM mode to get the list of includes is depends on in the current build, or extract the computed dependencies from Ninja after a build is complete. This will also catch bad transitive includes which the current checker misses.

But doing this is a bigger effort and there is almost no staffing on the GN project. Any staffing we do have should be used for converting the builders over. This variant also makes the check step either run much slower or more complicated by requiring a build first. I would like to be able to incrementally expand the current checked targets to catch most errors without being blocked on this.

If we implement a better header checker later, it's trivial to remove these comments, and the number of comments we end up needing to add will also inform how important the improved header checker is (currently when you find a bad case you have to give up on the entire directory).

[Dana Jansens]

A probably naive idea: What if the comment instead was something like

#if defined(ENABLE_DOOM_MELON)

#include "components/doom_melon/power_controller.h"  // gncheck(enable_doom_melon)

#endif

Is there something we could look for in GN easily, to determine if it should check that header then?

 

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Brett Wilson]

This is a cool idea I hadn't thought of, but implementing it would be scary.

The thing in parens would need to be a GN expression, and the question is then what context do you use to evaluate that expression? It seems the only possible answer is the context at the bottom of the corresponding target definition (the value of any variable can change at any time, of course). But this then means GN has to save every variable in scope at the end of evaluating every target.

This is a whole lot of stuff for what is probably a minor benefit.

Brett

[Dirk Pranke]

I'm fine w/ Brett's initial suggestion; if doing this allows us to get `gn check` working on more of the codebase, it's worth it.

However, I think we should strive to get a more robust solution place at some point.

I suspect Dana's solution is too much work for too little gain, and we should look to one of the other solutions instead.

-- Dirk

On Wed, Jun 24, 2015 at 11:36 AM, Brett Wilson <bre...@chromium.org> wrote:

--

[Nico Weber]

"nocheck" seems pretty unsearchable and it's not clear that it's related to gn. Maybe you could put "gn" somewhere in there?

In how many files do you think this will be needed?

[Brett Wilson]

I'm fine with:

  // nogncheck

or even:

  // no gn check

My wild guess would be 50-100 files total would have this comment. But this is based on no data, just gut feeling.

Brett

[Thiago Farina]

I would prefer the first alternative, "get the list of includes it depends on", but that is just me.

How hard is to implement this?

--

Thiago Farina

[Brett Wilson]

On Thu, Jun 25, 2015 at 6:49 AM, Thiago Farina <tfa...@chromium.org> wrote:

I would prefer the first alternative, "get the list of includes it depends on", but that is just me.

How hard is to implement this?

This is too hard for me to want to do right now.

Brett

[Carlos Pizano]

looks fine to me, the amended proposal with 'gn' on the name.

[Brett Wilson]

Okay, I will go with:

  // nogncheck

Brett

--

[Dana Jansens]

On Wed, Jul 1, 2015 at 11:31 AM, Brett Wilson <bre...@chromium.org> wrote:

Okay, I will go with:

  // nogncheck

If it's ok, would you mind leaving a link to a crbug.com/xyz on each of these that describes why it's there and what should be done to remove it? Like // nogncheck crbug.com/xyz?

[Brett Wilson]

For the ones that are bugs this sounds good. A bunch will not be bugs, they'll be conditional dependencies that the GN header checker doesn't understand. In this case, it would be good practice to note what's going on in some way, but it wouldn't be via a bug link.

Brett

[Dana Jansens]

On Wed, Jul 1, 2015 at 2:04 PM, Brett Wilson <bre...@chromium.org> wrote:

For the ones that are bugs this sounds good. A bunch will not be bugs, they'll be conditional dependencies that the GN header checker doesn't understand. In this case, it would be good practice to note what's going on in some way, but it wouldn't be via a bug link.

Great, that'd be very helpful. Thanks!

[Brett Wilson]

The // nogncheck annotation has been landed and the GN binary rolled so this should be usable now.

Brett