New system for feature enable/disable flags

[Brett Wilson]

SUMMARY

There is a new system for doing enable/disable build flags that works a bit differently. Use it for new flags and be prepared to encounter #if BUILDFLAG() in the wild.

MORE INFO

If you're adding a new feature flag, please don't add it globally (common.gypi, build/config/BUILD.gn, etc.). Instead, add it to the lowest-level target that needs to know about it, and use the new generated header system to make headers that allow .cc file to see the define if they want to. This helps keep the build more modular.

If you recall writing a flag that you added to common.gypi, especially one only used in few places, please help convert it.

You key off of these flags like:

  #if BUILDFLAG(ENABLE_FOO)

The BUILDFLAG wrapper macro helps prevent some errors. If you forget and do an old-style #ifdef ENABLE_FOO on a new-style flag, it will always be false.

I added a "chrome common features" target that you can add high-level defines for that only the chrome layer needs to know about. This is pretty hooked into the dependency tree so you hopefully won't encounter the problems below.

You can read about the mechanics in //build/buildflag_header.gni

I did a patch for one such flag which you can use as an example: https://codereview.chromium.org/1487873003/

GENERATED HEADER GOTCHAS:

This generates headers at build-time. The following applies to all generated headers (you probably previously encountered these issues the most with protobufs):

- The generate step must be a dependency of any target that uses it, or clean builds will fail (Ninja will notice the header for incremental compiles and will "fix" the ordering). So if you see a compile failure due to a missing header, you probably need to add a dependency. These can be flaky since it depends on the build ordering, so don't ignore it if it goes away!

- In the GN build, the build_header target must only be "somewhere" in the dependency tree. "gn check" will tell if you if you screwed up, but only for the parts of the tree that are whitelisted for "gn check" to run (see src/.gn for the list). Please help expand this list! (*)

- The GYP build is more picky. Since hard dependencies aren't transitive, the entire path from the target where you include the header to the target that generates it must be marked hard, or there must be no intermediate targets. Hopefully we won't have to maintain this much longer.

Brett

(*) Adding a target to "gn check" is easy. Doing this is a nice way to help the GN build without a big time commitment, and helps prevent certain classes of errors.

Open the ".gn" file in "src". Find the big list labeled "check_targets". Add your target. Then run "gn check out/foobar", fix errors it tells you about (almost always just missing deps).

[Ryan Sleevi]

On Tue, Dec 1, 2015 at 9:39 AM, Brett Wilson <bre...@chromium.org> wrote:

- The GYP build is more picky. Since hard dependencies aren't transitive, the entire path from the target where you include the header to the target that generates it must be marked hard, or there must be no intermediate targets. Hopefully we won't have to maintain this much longer.

The wording on this seems a little weird/wrong. It should _not_ be desirable to mark any target as a 'hard' dependency (as that reduces build parallelism and forces choke points).

The lowest level target - the one with the generator generating the h - should be marked as a hard dependency. Every subsequent target should use export_dependent_settings (of the previous target) to express that dependency.

That is, assume you have a graph of A -> B -> C -> D, you don't want B/C/D marked hard, otherwise the build order will be [[A], [B], [C], [D]]. If you mark A hard, export_dependent_settings A from B, export_dependent_settings B from C, and export_dependent_settings C from D, then your build order will be [[A], [B, C, D]].

export_dependent_settings of a 'hard' dependency forces the dependent targets to block on that compilation (and any of the other 'hard' dependencies transitively exported). It stops once you stop export_dependent_settings'ing, which is why you still have to change C to export B and D to export C.

I just wouldn't want to see a ton of 'hard' dependencies introduced into GYP. That'd be a step back.

[Lei Zhang]

What about build flags that are not isolated to one place? e.g.
enable_print_preview or is_chromecast? Will they stay in/near
build/common.gypi + GN equivalent, but get converted to BUILDFLAG() ?

On Tue, Dec 1, 2015 at 9:39 AM, Brett Wilson <bre...@chromium.org> wrote:

> --
> --
> 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]

On Tue, Jan 12, 2016 at 6:15 PM, Lei Zhang <the...@chromium.org> wrote:

What about build flags that are not isolated to one place? e.g.
enable_print_preview or is_chromecast? Will they stay in/near
build/common.gypi + GN equivalent, but get converted to BUILDFLAG() ?

chromecast represents basically an OS-level switch so it makes sense to have in src/build. But by policy we've decided not to make a chromecast define, so this is a GN/GYP flag only.

Any feature-level flag should have a layer which should know about that feature. In most cases this is either in chrome (which would conditionally depend on any related components) or in a component corresponding to that feature (and chrome would get the build flag header from that component). For print preview, I assume it would go in src/printing since that exists, but I don't know enough about the design to be sure. Whether printing works at all may need to be controlled differently. If hypothetically there was a bunch of Blink printing code that we wanted to switch off, we might decide it's a web platform flag that the rest of the product inherits.

Brett