Support repeated flags in platform mappings and platform-based flags

[katre]

This was discovered while implementing platform-based flags, #19409. Since this uses the same BuildOptions.applyParsingResult method as platform mappings, the same issue applies to both.

A quick definition: a repeatable flag is a flag where repeated uses accumulate, instead of overwriting. This corresponds to using @Option.allowMultiple in a native flag or defining a Starlark flag using config.string_list(repeatable = True). Commonly used flags that use this are --features and --copt.

Currently, any repeatable flags used in platform mapping or platform based flags will overwrite any previous values. If any platforms set the --features flag, for example, this will remove any values set on the command line.

Ideally we would either intelligently merge these, or at least warn users that it happens.

There are example tests that show the error: https://github.com/bazelbuild/bazel/pull/22386 (see errors: https://buildkite.com/bazel/bazel-bazel-github-presubmit/builds/21626).

This is a tracking issue for dealing with this.

[katre]

Update with the current status and issues:

Background on repeatable flags

Some flags are repeated: these are either native flags with the allowMultiple field set to true, a Starlark flag that uses config.string(allow_multiple = True), or a Starlark flag that uses config.string_list(repeatable = True). When parsing repeated flags, Bazel concatenates values into a list of strings (or other types, for native flags). One important consequence is that it isn't possible to "reset" the list: all values (either from the command line or a bazelrc) are concatenated in the order bazel encounters them (which, in the case of bazelrcs or expanded configs, may be non-obvious).

A consequence of this is that repeatable flags work best in cases where:

1. Order does not matter
2. There is an inbuilt way for whatever processes the flag to handle precedence and possibly remove values.

Platform-based Flags

With platform-based flags, the built-in platform rule has an attribute called flags, which is a list of flags that are added to the current configuration if the platform is the target platform.

In the case of non-repeatable flags, this is fairly easy: the new value replaces the previous value.

For repeatable flags, this is more complicated: the two choices of behavior are to replace the previous value, or to attempt to add the new values to the existing list.

Current behavior

Currently, the merging logic is implemented in ParsedFlagsValue.mergeWith. This completely overrides repeatable flags (see this TODO). This is very simple to implement, but is a problem for users.

Desired behavior

Ideally, users would like for repeated flags to merge in a smarter way. For example, a user who specifies build --features=+enable_lto will be surprised when a platform that specifies --features=+enable_exceptions also removes the lto feature. Similar arguments exist for the commonly-used --define and --copts flags.

Unfortunately, the naive approach is disastrous for bazel's internal state. Platform-based flags are applied on every dependency edge, which leads to pathological behavior:

1. Initial target :foo has a configuration with --features=+enable_lto
2. It has a transition that changes the platform, and depends on :bar, so the new platform adds the new feature and the value is --features=+enable_lto,+enable_exceptions.
3. Except :bar now has a dependency on :baz, and the target platform is unchanged, so platform-based flags again adds the new values, leading to --features=+enable_lto,+enable_exceptions,+enable_exceptions.
   1. This is a new configuration, with a different configuration hash, and so no analysis caching is performed
4. This continues down the dependency tree, with the --features flag getting longer and longer, and every depth in the dependency tree has a different configuration.

The initial fix is to change the logic for ParsedFlagsValue.updateOptionValue to only add the new value if it is not already at the end of the list of values.

However, a pathological set of transitions that change the target platform back and forth could still cause an explosion in the value of the flag, again increasing the number of configurations exponentially.

Next steps

The pathological case is unlikely, so we should probably go ahead with the "check if new values are already at the end of list" approach. However, this requires some degree of checking (and changes to flag merging), and so needs some work.

Open questions:

1. Does the merging behavior need a flag to change how it works?
2. Is this something we can (somehow) ask the flag definition to manage?

[armandomontanez]

The more I think about it, the more I wonder if the incoming configuration (outside of the CLI) should ever affect the final state of a platform transition. https://github.com/bazelbuild/bazel/issues/22457 was an initial step in coming to this conclusion.

For general transitions, it makes sense to augment the current configuration space, but platforms seem like they should be fully specified in a way that resetting all constraints or flags not referenced by the platform results in the same configuration.

For the simple example of "how to I flip a --release flag?", there's maybe a little more discussion to be had with the relationship of --propagate_custom_flag, but I came from a GN world where release and debug were distinct platforms to enable building them in parallel so I'm slightly biased.

[katre]

Hmm, a "total transition" really feels like a separate concept (and then, users could layer a platform change on top of that if they wanted).

Platform-based flags (which is related to but separate from a transition that sets --platforms) is also a separate concept, and really needs to just change the current configuration.

[armandomontanez]

Right, it's only slightly related in that it could theoretically act as a nice "barrier" for this problem of flag accumulation.

Beyond the simple use case of "I want CLI-provided flags to be concatenated with the current target platform's repeated flags," I've yet to come up with a compelling reason to recursively accumulate repeated flags across numerous platform flips. Conceptually, one platform accumulating flags from another platform sounds like the leading description for a bug.

[katre]

Yes, repeated platform changes are conceptually tricky, but I'm not sure how often that happens.

Ideally bazel would track the source of all flags and be able to remove a group from the platform when the platform changes, but that's not currently possible and not really something we're looking to implement.

[gregestren]

Note Starlarkify native Bazel flags would switch all flags from the native Java API to Starlark API. That includes different semantics for allowMultiple, which today are simpler than the native version. And hopefully stay simpler forever.

Not sure if that has any impact on the thought process here.

[trybka]

Just adding that this was (re-?)discovered as part of trying to land https://github.com/bazelbuild/bazel/pull/25750.

The TL;DR was that we're trying to roll this out in a repo where we expect to be able to set --features=system_include_paths globally (at the top-level), and we encountered a platform() with flags=[--features=...] that caused this to get dropped and semantics to change.

Perhaps this is an argument in favor of not using features (or any other allowMultiple) for setting global state, but I think that ship has sailed (see, e.g. legacy_whole_archive).

I don't know that a naive accumulation is correct... there's so many edge cases.

Features are particularly pernicious because of the whole "feature algebra" thing. Features could be modifying rule semantics, enabling toolchain semantics, or something else entirely.

copt is another one to consider, but one could imagine one day that platforms start to routinely bake-in the "platform-specific" copts like target triples, cpu specialization, target_features (e.g. +neon or +fp16) that aren't necessarily intended to "escape" their given platform.

---

The more I consider this, I think @armandomontanez's suggestion in https://github.com/bazelbuild/bazel/issues/22453#issuecomment-2634387000 is worth consideration. IIUC, that translates to the following proposal:

Only manage the top-level flags and a single platform's flags. If you encounter a new platform, throw away the set and start again, combining top-level with just that platform's flags. If you want something more clever, reach for a transition.

A new platform (with or without flags) would also reset back to top-level as a baseline.

At least for features and copt I think that might be an improvement over the current state of surprise.

Thoughts?

[fmeum]

I also like the idea of maintaining the two lists of "top-level" and "per-platform" flag values and keeping the former while switching out the latter.

[gregestren]

Features are particularly pernicious because of the whole "feature algebra" thing. Features could be modifying rule semantics, enabling toolchain semantics, or something else entirely.

Can you show a toy example of this?

Only manage the top-level flags and a single platform's flags. If you encounter a new platform, throw away the set and start again, combining top-level with just that platform's flags. If you want something more clever, reach for a transition.

In terms of feature support I'm 100% behind supporting --features and --copt. They're the highest profile use cases and I think reasoning about their semantics is more manageable than reasoning about all flags' semantics, which can vary arbitrarily.

Technically I'd like to clarify the interplay especially between transitions and platforms. I don't remember order of preference on, say, a configured target that applies both. And how that mixes down its dependencies which will continue to set the platform's flags on every dependency even when they don't have transitions.

A new platform (with or without flags) would also reset back to top-level as a baseline.

So any platform with no flags automatically resets --features, unconditionally?

What happens if the top-level has empty --features, then a transition sets them, then a flag-free platform gets set later? Is that counterintuitive?

At least for features and copt I think that might be an improvement over the current state of surprise.

+1 for this line of thinking. I'm 100% behind "strictly better than the status quo for a small subset of flags we really care about". I think we're more likely to make progress that way vs a more generic solution It's also an opportunity to test some algorithms for possibly wider application.