From c55011e9e86276c1ce3d6e147cf48a837b7cabc9 Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 9 Jan 2024 09:29:06 -0800 Subject: [PATCH] Remove handling for multiple platform mappings in BuildConfigurationKeyProducer. This situation cannot happen. Work towards platform-based flags: #19409. PiperOrigin-RevId: 596958087 Change-Id: I8870973bc0f940375ff9829cb3da30dd5c12c2dc --- .../build/lib/analysis/producers/BUILD | 1 + .../BuildConfigurationKeyProducer.java | 59 +++++++++---------- 2 files changed, 28 insertions(+), 32 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/producers/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/producers/BUILD index 0e6a4aa2cd6e58..ad85f12845bfc7 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/producers/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/producers/BUILD @@ -102,6 +102,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/skyframe/config", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", "//src/main/java/com/google/devtools/build/skyframe", + "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", "//src/main/java/com/google/devtools/common/options", "//third_party:guava", ], 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 4614d6187f9202..189e038095649e 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 @@ -13,20 +13,18 @@ // limitations under the License. package com.google.devtools.build.lib.analysis.producers; -import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.Multimaps; import com.google.devtools.build.lib.analysis.PlatformOptions; import com.google.devtools.build.lib.analysis.config.BuildOptions; import com.google.devtools.build.lib.skyframe.config.BuildConfigurationKey; import com.google.devtools.build.lib.skyframe.config.PlatformMappingValue; import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.devtools.build.skyframe.SkyValue; import com.google.devtools.build.skyframe.state.StateMachine; import com.google.devtools.common.options.OptionsParsingException; -import java.util.Collection; -import java.util.HashMap; import java.util.Map; import java.util.Optional; +import java.util.function.Consumer; /** * Creates the needed {@link BuildConfigurationKey} instances for the given options. @@ -36,7 +34,8 @@ *

The output preserves the iteration order of the input. */ // Logic here must be kept in sync with SkyframeExecutor.createBuildConfigurationKey. -public class BuildConfigurationKeyProducer implements StateMachine { +public class BuildConfigurationKeyProducer implements StateMachine, Consumer { + /** Interface for clients to accept results of this computation. */ public interface ResultSink { @@ -52,7 +51,9 @@ void acceptTransitionedConfigurations( private final Map options; // -------------------- Internal State -------------------- - private final Map platformMappingValues = new HashMap<>(); + // There is only ever a single PlatformMappingValue in use, as the `--platform_mappings` flag + // can not be changed in a transition. + private PlatformMappingValue platformMappingValue; public BuildConfigurationKeyProducer( ResultSink sink, StateMachine runAfter, Map options) { @@ -63,27 +64,28 @@ public BuildConfigurationKeyProducer( @Override public StateMachine step(Tasks tasks) { - // Deduplicates the platform mapping paths and collates the transition keys. - ImmutableListMultimap, String> index = - Multimaps.index( - options.keySet(), transitionKey -> getPlatformMappingsPath(options.get(transitionKey))); - for (Map.Entry, Collection> entry : index.asMap().entrySet()) { - Collection transitionKeys = entry.getValue(); - tasks.lookUp( - PlatformMappingValue.Key.create(entry.getKey().orElse(null)), - // No need to consider error handling: this flag cannot be changed and so errors were - // detected and handled in SkyframeExecutor.createBuildConfigurationKey. - rawValue -> { - var value = (PlatformMappingValue) rawValue; - // Maps the value from all transition keys with the same platform mappings path. - for (String key : transitionKeys) { - platformMappingValues.put(key, value); - } - }); - } + // Use any configuration, since all configurations will have the same platform mapping. + Optional platformMappingsPath = + options.values().stream() + .filter(opts -> opts.contains(PlatformOptions.class)) + .filter(opts -> opts.get(PlatformOptions.class).platformMappings != null) + .map(opts -> opts.get(PlatformOptions.class).platformMappings) + .findAny(); + PlatformMappingValue.Key platformMappingValueKey = + PlatformMappingValue.Key.create(platformMappingsPath.orElse(null)); + // No need to consider error handling: this flag cannot be changed and so errors were + // detected and handled in SkyframeExecutor.createBuildConfigurationKey. + tasks.lookUp(platformMappingValueKey, this); return this::applyMappings; } + @Override + public void accept(SkyValue skyValue) { + if (skyValue instanceof PlatformMappingValue) { + this.platformMappingValue = (PlatformMappingValue) skyValue; + } + } + private StateMachine applyMappings(Tasks tasks) { var result = ImmutableMap.builderWithExpectedSize(options.size()); @@ -91,8 +93,7 @@ private StateMachine applyMappings(Tasks tasks) { String transitionKey = entry.getKey(); BuildConfigurationKey newConfigurationKey; try { - PlatformMappingValue mappingValue = platformMappingValues.get(transitionKey); - BuildOptions mappedOptions = mappingValue.map(entry.getValue()); + BuildOptions mappedOptions = this.platformMappingValue.map(entry.getValue()); newConfigurationKey = BuildConfigurationKey.create(mappedOptions); } catch (OptionsParsingException e) { sink.acceptTransitionError(e); @@ -103,10 +104,4 @@ private StateMachine applyMappings(Tasks tasks) { sink.acceptTransitionedConfigurations(result.buildOrThrow()); return runAfter; } - - private static Optional getPlatformMappingsPath(BuildOptions fromOptions) { - return fromOptions.hasNoConfig() - ? Optional.empty() - : Optional.ofNullable(fromOptions.get(PlatformOptions.class).platformMappings); - } }