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);
- }
}
