Do not request `PlatformMappingValue` if `--platforms` refers to a `platform` rule with non-empty flags. When `--platforms` refers to a `platform` rule with non-empty flags, the `PlatformMappingValue` is irrelevant. Avoid requesting it to minimize Skyframe edges. PiperOrigin-RevId: 672531227 Change-Id: I448cc110e588aa8b2d06ee55473b204b54b120c6
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/producers/BuildConfigurationKeyProducer.java b/src/main/java/com/google/devtools/build/lib/analysis/producers/BuildConfigurationKeyProducer.java index 36e40b8..c27f116 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/producers/BuildConfigurationKeyProducer.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/producers/BuildConfigurationKeyProducer.java
@@ -32,14 +32,26 @@ import javax.annotation.Nullable; /** - * Creates the needed {@link BuildConfigurationKey} instances for a single {@link BuildOptions}. + * Creates the needed {@link BuildConfigurationKey} instance for a single {@link BuildOptions}, + * including merging in any platform-based flags or a platform mapping. * - * <p>This includes merging in platform mappings and platform-based flags. + * <p>Platform-based flags and platform mappings are mutually exclusive: only one will be applied if + * they are present. Trying to mix and match would be possible but confusing, especially if they try + * to change the same flag. The logic is: + * + * <ul> + * <li>If {@link PlatformOptions#platforms} specifies a target platform, look up the {@link + * PlatformValue}. If it specifies {@linkplain PlatformValue#parsedFlags flags}, use {@link + * ParsedFlagsValue#mergeWith}. + * <li>If {@link PlatformOptions#platforms} does not specify a target platform, or if the target + * platform does not specify {@linkplain PlatformValue#parsedFlags flags}, look up the {@link + * PlatformMappingValue} and use {@link PlatformMappingValue#map}. + * </ul> * * @param <C> The type of the context variable that the producer will pass via the {@link * ResultSink} so that consumers can identify which options are which. */ -public class BuildConfigurationKeyProducer<C> +public final class BuildConfigurationKeyProducer<C> implements StateMachine, ValueOrExceptionSink<PlatformMappingException>, PlatformProducer.ResultSink { @@ -64,8 +76,8 @@ private final BuildOptions options; // -------------------- Internal State -------------------- + private PlatformValue targetPlatformValue; private PlatformMappingValue platformMappingValue; - private Optional<ParsedFlagsValue> platformFlags = Optional.empty(); BuildConfigurationKeyProducer( ResultSink<C> sink, @@ -82,28 +94,62 @@ @Override public StateMachine step(Tasks tasks) throws InterruptedException { - BuildConfigurationKey result = cache.get(this.options); + BuildConfigurationKey result = cache.get(options); if (result != null) { - this.sink.acceptTransitionedConfiguration(this.context, result); - return this.runAfter; + sink.acceptTransitionedConfiguration(context, result); + return runAfter; } // Short-circuit if there are no platform options. - if (!this.options.contains(PlatformOptions.class)) { - return finishConfigurationKeyProcessing(BuildConfigurationKey.create(this.options)); + if (!options.contains(PlatformOptions.class)) { + return finishConfigurationKeyProcessing(BuildConfigurationKey.create(options)); } - // Find platform mappings and platform-based flags for merging. - findPlatformMapping(tasks); - findTargetPlatformInfo(tasks); - return this::applyFlags; + List<Label> targetPlatforms = options.get(PlatformOptions.class).platforms; + if (targetPlatforms.size() == 1) { + // TODO: https://github.com/bazelbuild/bazel/issues/19807 - We define this flag to only use + // the first value and ignore any subsequent ones. Remove this check as part of cleanup. + tasks.enqueue( + new PlatformProducer(targetPlatforms.getFirst(), this, this::checkTargetPlatformFlags)); + return runAfter; + } else { + return mergeFromPlatformMapping(tasks); + } } - private void findPlatformMapping(Tasks tasks) { + private StateMachine checkTargetPlatformFlags(Tasks tasks) { + if (targetPlatformValue == null) { + return DONE; // Error. + } + Optional<ParsedFlagsValue> parsedFlags = targetPlatformValue.parsedFlags(); + if (parsedFlags.isPresent()) { + BuildOptions updatedOptions = parsedFlags.get().mergeWith(options); + return finishConfigurationKeyProcessing(BuildConfigurationKey.create(updatedOptions)); + } else { + return mergeFromPlatformMapping(tasks); + } + } + + private StateMachine mergeFromPlatformMapping(Tasks tasks) { PathFragment platformMappingsPath = options.get(PlatformOptions.class).platformMappings; - PlatformMappingValue.Key platformMappingValueKey = - PlatformMappingValue.Key.create(platformMappingsPath); - tasks.lookUp(platformMappingValueKey, PlatformMappingException.class, this); + tasks.lookUp( + PlatformMappingValue.Key.create(platformMappingsPath), + PlatformMappingException.class, + this); + return this::applyPlatformMapping; + } + + private StateMachine applyPlatformMapping(Tasks tasks) { + if (platformMappingValue == null) { + return DONE; // Error. + } + try { + BuildOptions updatedOptions = platformMappingValue.map(options); + return finishConfigurationKeyProcessing(BuildConfigurationKey.create(updatedOptions)); + } catch (OptionsParsingException e) { + sink.acceptOptionsParsingError(e); + return runAfter; + } } // Handles results from the PlatformMappingValueKey lookup. @@ -119,23 +165,14 @@ if (exception != null) { sink.acceptPlatformMappingError(exception); - } else if (value instanceof PlatformMappingValue platformMappingValue) { - this.platformMappingValue = platformMappingValue; - } - } - - private void findTargetPlatformInfo(Tasks tasks) { - List<Label> targetPlatforms = options.get(PlatformOptions.class).platforms; - if (targetPlatforms.size() == 1) { - // TODO: https://github.com/bazelbuild/bazel/issues/19807 - We define this flag to only use - // the first value and ignore any subsequent ones. Remove this check as part of cleanup. - tasks.enqueue(new PlatformProducer(targetPlatforms.getFirst(), this, StateMachine.DONE)); + } else { + this.platformMappingValue = (PlatformMappingValue) value; } } @Override public void acceptPlatformValue(PlatformValue value) { - this.platformFlags = value.parsedFlags(); + this.targetPlatformValue = value; } @Override @@ -148,49 +185,9 @@ sink.acceptOptionsParsingError(error); } - private StateMachine applyFlags(Tasks tasks) { - if (this.platformMappingValue == null) { - return DONE; // There was an error. - } - - try { - BuildConfigurationKey newConfigurationKey = applyFlagsForOptions(options); - return finishConfigurationKeyProcessing(newConfigurationKey); - } catch (OptionsParsingException e) { - sink.acceptOptionsParsingError(e); - return runAfter; - } - } - private StateMachine finishConfigurationKeyProcessing(BuildConfigurationKey newConfigurationKey) { cache.put(this.options, newConfigurationKey); sink.acceptTransitionedConfiguration(this.context, newConfigurationKey); return this.runAfter; } - - /** - * Apply discovered flags from platforms or platform mappings to the given options, and return the - * {@link BuildConfigurationKey}. - * - * <p>Platform-based flags and platform mappings are mutually exclusive: only one will be applied - * if they are present. Trying to mix and match would be possible but confusing, especially if - * they try to change the same flag. - */ - private BuildConfigurationKey applyFlagsForOptions(BuildOptions options) - throws OptionsParsingException { - // Does the target platform provide any flags? - if (this.platformFlags.isPresent()) { - BuildOptions updatedOptions = this.platformFlags.get().mergeWith(options); - return BuildConfigurationKey.create(updatedOptions); - } - - // Is there a platform mapping? - if (this.platformMappingValue != null) { - BuildOptions mappedOptions = this.platformMappingValue.map(options); - return BuildConfigurationKey.create(mappedOptions); - } - - // Just use the original options. - return BuildConfigurationKey.create(options); - } }