Pigweed

We should probably use the same approach that we use for facades, i.e. label_flag to specify the path to a target containing the configuration header. For overriding the configuration using defines can be done via cc_library.defines.

Draft implementation: https://pigweed-review.git.corp.google.com/c/pigweed/pigweed/+/137053

This involves some changes to the source files. Unfortunately this is unavoidable, because Bazel doesn't support a "viral compiler flags" mechanism like gn's public_configs, so there's no ergonomic way to provide the -include flag to both a library and all libraries that depend on it.

I'm coming back to this after a long break. Let me summarize where we are:

1. My first draft implementation was simple (https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/137053/4), but was not suitable for upstream because it only worked in Bazel. (It relied on build system sandboxing to ensure the compiler would be handed the correct header.)

2. My second implementation was much more complex, and required changes to the other build systems (https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/137053/12). Those changes are not finished.

3. So, I decided to go back to basics and do something simple: only support setting module configuration in Bazel through compiler defines. In practice, this means that you put the text of your module configuration in the BUILD file rather than a header file. GN and CMake support this, too (in addition to the header-based configuration). I made a new CL for it, https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/181032.

   This is very simple, but also less powerful than providing the configuration through headers. In particular, you can't define the configuration options based on constants coming from other headers.

But maybe (3) is good enough for upstream? Note that if a downstream project that builds with Bazel wants to implement their own configurable modules, they can safely use the pattern of (1).

Approach (3) can be buttressed with some unit tests that verify the two constants (from another header and from the module configuration) are equal. On the whole maybe that's more ergonomic than some very complex implementation that lets you pull in headers.

Discussed this with Wyatt offline. We agreed to start with the simple approach (http://pwrev/181032).

A couple open questions for followup work:

1. How to support module configuration in Soong? Like Bazel, Soong can't virally propagate -include flags. But unlike Bazel, Soong can't propagate defines, either. There's also no clear analog to label_flags or GN vars. This will require a separate effort to figure out.
2. Should we refactor the module configuration mechanism to only support global (not per module) config? The per-module config requires a facade per module, with a different default for each; a global-only config would have just one facade with just one default. Projects could still compose the global config out of multiple #include'd headers if they wish.

Created a spreadsheet tracking the instances of the config pattern: https://docs.google.com/spreadsheets/d/14RMXKFjm3tWIGeFcZsxOwxv3KohRygJiA1hVGC02lVI/edit#gid=0. There are 35 of them in upstream, only one of which I've created a knob for in Bazel so far. I'll try to knock out the rest, and then can close the bug.

Discussed this with Ewout offline a week ago. The simple approach of http://pwrev/181032 may not be sufficient for all downstream users. We'll have to figure out how to support providing the config in a header file in a cross-platform manner. It's been a long while since I looked closely at this, but we did have an idea for how to do this (#comment4), I just never figured out how to get this to work in GN and CMake as well as Bazel (although I believe it was possible).

From in-person discussions with Wyatt: some module configuration options affect the API exposed by the module, in ways that may make the module incompatible with some uses (e.g., https://pigweed.dev/pw_rpc/#c.PW_RPC_METHOD_STORES_TYPE). This suggests that ideally libraries should be able to declare themselves target_compatible_with only some configuration option choices. This suggests setting the options themselves through something like Skylib flags rather than C++ headers, to allow detecting their values via config_settings (similar to https://pigweed.dev/docs/blog/02-bazel-feature-flags.html#more-power-with-no-code-changes-platform-based-skylib-flags).

See also https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/214971/comment/92094781_66b2be93/ for more thoughts on this subject.

Following up on #comment12: see http://pwrev.dev/226551 for a somewhat ad hoc example of such configuration that we rolled out to unblock a particular user.

I think the way we should proceed here is to build on this pattern: https://cs.opensource.google/pigweed/pigweed/+/master:pw_log_string/public/pw_log_string/config.h;l=18-21;drc=f0da6aa28012b4885c53973e7f97ba1a1e86d199