From 8139503b5c745d6fb9e1ed6d65fd6c6fd6e00ec3 Mon Sep 17 00:00:00 2001 From: jhorvitz Date: Wed, 5 May 2021 08:48:08 -0700 Subject: [PATCH] Remove `defaultBuildOptions` from `PlatformMappingValue`. It really only needs a list of options classes for parsing. Additionally, instead of passing it in, just store this list as a field. There shouldn't be any penalty for doing this, as equality checks will be fast since it's always the same instance in practice, and we don't serialize `PlatformMappingValue`. After this, we can tear out `defaultBuildOptions` entirely. `PlatformMappingValue` was the last one using it. PiperOrigin-RevId: 372134563 --- .../config/ConfigurationResolver.java | 5 +- .../build/lib/skyframe/AspectFunction.java | 26 ++--- .../google/devtools/build/lib/skyframe/BUILD | 3 +- .../lib/skyframe/BuildConfigurationValue.java | 6 +- .../skyframe/ConfiguredTargetFunction.java | 15 +-- .../lib/skyframe/PlatformMappingFunction.java | 49 +++++----- .../lib/skyframe/PlatformMappingValue.java | 96 ++++++++++++------- .../PrepareAnalysisPhaseFunction.java | 13 +-- .../build/lib/skyframe/SkyframeExecutor.java | 21 ++-- .../ConfigurationsForTargetsTest.java | 63 ++++-------- .../skyframe/PlatformMappingFunctionTest.java | 66 +++++-------- .../skyframe/PlatformMappingValueTest.java | 93 ++++++++---------- 12 files changed, 201 insertions(+), 255 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java index 3d4ee05d78d89e..bf3255904fa791 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java @@ -95,19 +95,16 @@ private ValueMissingException() { private final SkyFunction.Environment env; private final TargetAndConfiguration ctgValue; private final BuildConfiguration hostConfiguration; - private final BuildOptions defaultBuildOptions; private final ImmutableMap configConditions; public ConfigurationResolver( SkyFunction.Environment env, TargetAndConfiguration ctgValue, BuildConfiguration hostConfiguration, - BuildOptions defaultBuildOptions, ImmutableMap configConditions) { this.env = env; this.ctgValue = ctgValue; this.hostConfiguration = hostConfiguration; - this.defaultBuildOptions = defaultBuildOptions; this.configConditions = configConditions; } @@ -249,7 +246,7 @@ private ImmutableList resolveGenericTransition( String transitionKey = optionsEntry.getKey(); BuildConfigurationValue.Key buildConfigurationValueKey = BuildConfigurationValue.keyWithPlatformMapping( - platformMappingValue, defaultBuildOptions, depFragments, optionsEntry.getValue()); + platformMappingValue, depFragments, optionsEntry.getValue()); configurationKeys.put(transitionKey, buildConfigurationValueKey); } } catch (OptionsParsingException e) { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java index f34ea5e3b6081a..f7773859c2d01d 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java @@ -37,7 +37,6 @@ import com.google.devtools.build.lib.analysis.TargetAndConfiguration; import com.google.devtools.build.lib.analysis.ToolchainCollection; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; -import com.google.devtools.build.lib.analysis.config.BuildOptions; import com.google.devtools.build.lib.analysis.config.ConfigConditions; import com.google.devtools.build.lib.analysis.config.DependencyEvaluationException; import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException; @@ -83,25 +82,23 @@ /** * The Skyframe function that generates aspects. * - * This class, together with {@link ConfiguredTargetFunction} drives the analysis phase. For more + *

This class, together with {@link ConfiguredTargetFunction} drives the analysis phase. For more * information, see {@link com.google.devtools.build.lib.analysis.RuleConfiguredTargetFactory}. * - * {@link AspectFunction} takes a SkyKey containing an {@link AspectKey} [a tuple of - * (target label, configurations, aspect class and aspect parameters)], - * loads an {@link Aspect} from aspect class and aspect parameters, - * gets a {@link ConfiguredTarget} for label and configurations, and then creates - * a {@link ConfiguredAspect} for a given {@link AspectKey}. + *

{@link AspectFunction} takes a SkyKey containing an {@link AspectKey} [a tuple of (target + * label, configurations, aspect class and aspect parameters)], loads an {@link Aspect} from aspect + * class and aspect parameters, gets a {@link ConfiguredTarget} for label and configurations, and + * then creates a {@link ConfiguredAspect} for a given {@link AspectKey}. * - * See {@link com.google.devtools.build.lib.packages.AspectClass} documentation - * for an overview of aspect-related classes + *

See {@link com.google.devtools.build.lib.packages.AspectClass} documentation for an overview + * of aspect-related classes * * @see com.google.devtools.build.lib.analysis.RuleConfiguredTargetFactory * @see com.google.devtools.build.lib.packages.AspectClass */ -public final class AspectFunction implements SkyFunction { +final class AspectFunction implements SkyFunction { private final BuildViewProvider buildViewProvider; private final RuleClassProvider ruleClassProvider; - private final BuildOptions defaultBuildOptions; /** * Indicates whether the set of packages transitively loaded for a given {@link AspectValue} will * be needed for package root resolution later in the build. If not, they are not collected and @@ -112,13 +109,11 @@ public final class AspectFunction implements SkyFunction { AspectFunction( BuildViewProvider buildViewProvider, RuleClassProvider ruleClassProvider, - boolean storeTransitivePackagesForPackageRootResolution, - BuildOptions defaultBuildOptions) { + boolean storeTransitivePackagesForPackageRootResolution) { this.buildViewProvider = buildViewProvider; this.ruleClassProvider = ruleClassProvider; this.storeTransitivePackagesForPackageRootResolution = storeTransitivePackagesForPackageRootResolution; - this.defaultBuildOptions = defaultBuildOptions; } /** @@ -426,8 +421,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) ruleClassProvider, view.getHostConfiguration(originalTargetAndAspectConfiguration.getConfiguration()), transitivePackagesForPackageRootResolution, - transitiveRootCauses, - defaultBuildOptions); + transitiveRootCauses); } catch (ConfiguredValueCreationException e) { throw new AspectCreationException( e.getMessage(), key.getLabel(), aspectConfiguration, e.getDetailedExitCode()); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD index 14fc81505e9c5c..723ccab8339f99 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD @@ -238,6 +238,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:config/core_options", "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment", "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_class_set", + "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_options", "//src/main/java/com/google/devtools/build/lib/analysis:config/host_transition", "//src/main/java/com/google/devtools/build/lib/analysis:config/invalid_configuration_exception", "//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/configuration_transition", @@ -274,7 +275,6 @@ java_library( "//src/main/java/com/google/devtools/build/lib/collect/nestedset", "//src/main/java/com/google/devtools/build/lib/concurrent", "//src/main/java/com/google/devtools/build/lib/events", - "//src/main/java/com/google/devtools/build/lib/exec:execution_options", "//src/main/java/com/google/devtools/build/lib/io:file_symlink_cycle_uniqueness_function", "//src/main/java/com/google/devtools/build/lib/io:file_symlink_exception", "//src/main/java/com/google/devtools/build/lib/io:file_symlink_infinite_expansion_uniqueness_function", @@ -964,6 +964,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:config/build_configuration", "//src/main/java/com/google/devtools/build/lib/analysis:config/build_options", "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_class_set", + "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_options", "//src/main/java/com/google/devtools/build/lib/analysis:platform_options", "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/concurrent", diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationValue.java index 8d3067a1b8aca5..68542ef297492a 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationValue.java @@ -52,20 +52,16 @@ public BuildConfiguration getConfiguration() { * * @param platformMappingValue sky value that can transform a configuration key based on a * platform mapping - * @param defaultBuildOptions set of native build options without modifications based on parsing - * flags * @param fragments set of options fragments this configuration should cover * @param options the desired configuration * @throws OptionsParsingException if the platform mapping cannot be parsed */ public static Key keyWithPlatformMapping( PlatformMappingValue platformMappingValue, - BuildOptions defaultBuildOptions, FragmentClassSet fragments, BuildOptions options) throws OptionsParsingException { - return platformMappingValue.map( - keyWithoutPlatformMapping(fragments, options), defaultBuildOptions); + return platformMappingValue.map(keyWithoutPlatformMapping(fragments, options)); } /** diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java index 379ac24e82f447..0ac8454d72dbed 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java @@ -136,7 +136,6 @@ public static ConfiguredValueCreationException asConfiguredValueCreationExceptio private final BuildViewProvider buildViewProvider; private final RuleClassProvider ruleClassProvider; private final Semaphore cpuBoundSemaphore; - private final BuildOptions defaultBuildOptions; @Nullable private final ConfiguredTargetProgressReceiver configuredTargetProgress; /** @@ -154,7 +153,6 @@ public static ConfiguredValueCreationException asConfiguredValueCreationExceptio Semaphore cpuBoundSemaphore, boolean storeTransitivePackagesForPackageRootResolution, boolean shouldUnblockCpuWorkWhenFetchingDeps, - BuildOptions defaultBuildOptions, @Nullable ConfiguredTargetProgressReceiver configuredTargetProgress) { this.buildViewProvider = buildViewProvider; this.ruleClassProvider = ruleClassProvider; @@ -162,7 +160,6 @@ public static ConfiguredValueCreationException asConfiguredValueCreationExceptio this.storeTransitivePackagesForPackageRootResolution = storeTransitivePackagesForPackageRootResolution; this.shouldUnblockCpuWorkWhenFetchingDeps = shouldUnblockCpuWorkWhenFetchingDeps; - this.defaultBuildOptions = defaultBuildOptions; this.configuredTargetProgress = configuredTargetProgress; } @@ -314,8 +311,7 @@ public SkyValue compute(SkyKey key, Environment env) throws ConfiguredTargetFunc ruleClassProvider, view.getHostConfiguration(configuration), transitivePackagesForPackageRootResolution, - transitiveRootCauses, - defaultBuildOptions); + transitiveRootCauses); if (!transitiveRootCauses.isEmpty()) { NestedSet causes = transitiveRootCauses.build(); throw new ConfiguredTargetFunctionException( @@ -601,9 +597,6 @@ public static ImmutableSet

Note that this class only parses the mapping-file specific format, parsing (and validation) of - * flags contained therein is left to the invocation of {@link - * PlatformMappingValue#map(BuildConfigurationValue.Key, BuildOptions)}. + * flags contained therein is left to the invocation of {@link PlatformMappingValue#map}. */ -public class PlatformMappingFunction implements SkyFunction { +final class PlatformMappingFunction implements SkyFunction { + + private final ImmutableList> optionsClasses; + + PlatformMappingFunction(ImmutableList> optionsClasses) { + this.optionsClasses = checkNotNull(optionsClasses); + } @Nullable @Override @@ -99,7 +103,7 @@ public PlatformMappingValue compute(SkyKey skyKey, Environment env) throw new PlatformMappingException(e, SkyFunctionException.Transience.TRANSIENT); } - return parse(lines).toPlatformMappingValue(); + return parse(lines).toPlatformMappingValue(optionsClasses); } if (!platformMappingKey.wasExplicitlySetByUser()) { @@ -157,8 +161,8 @@ static Mappings parse(List lines) throws PlatformMappingException { throw parsingException("Expected 'platforms:' or 'flags:' but got " + it.peek()); } - Map> platformsToFlags = ImmutableMap.of(); - Map, Label> flagsToPlatforms = ImmutableMap.of(); + ImmutableMap> platformsToFlags = ImmutableMap.of(); + ImmutableMap, Label> flagsToPlatforms = ImmutableMap.of(); if (it.peek().equalsIgnoreCase("platforms:")) { it.next(); @@ -179,12 +183,12 @@ static Mappings parse(List lines) throws PlatformMappingException { return new Mappings(platformsToFlags, flagsToPlatforms); } - private static ImmutableMap> readPlatformsToFlags( + private static ImmutableMap> readPlatformsToFlags( PeekingIterator it) throws PlatformMappingException { - ImmutableMap.Builder> platformsToFlags = ImmutableMap.builder(); + ImmutableMap.Builder> platformsToFlags = ImmutableMap.builder(); while (it.hasNext() && !it.peek().equalsIgnoreCase("flags:")) { Label platform = readPlatform(it); - Collection flags = readFlags(it); + ImmutableSet flags = readFlags(it); platformsToFlags.put(platform, flags); } @@ -196,11 +200,11 @@ private static ImmutableMap> readPlatformsToFlags( } } - private static ImmutableMap, Label> readFlagsToPlatforms( + private static ImmutableMap, Label> readFlagsToPlatforms( PeekingIterator it) throws PlatformMappingException { - ImmutableMap.Builder, Label> flagsToPlatforms = ImmutableMap.builder(); + ImmutableMap.Builder, Label> flagsToPlatforms = ImmutableMap.builder(); while (it.hasNext() && it.peek().startsWith("--")) { - Collection flags = readFlags(it); + ImmutableSet flags = readFlags(it); Label platform = readPlatform(it); flagsToPlatforms.put(flags, platform); } @@ -259,19 +263,20 @@ private static PlatformMappingException parsingException(String message, Excepti * Simple data holder to make testing easier. Only for use internal to this file/tests thereof. */ @VisibleForTesting - static class Mappings { - final Map> platformsToFlags; - final Map, Label> flagsToPlatforms; + static final class Mappings { + final ImmutableMap> platformsToFlags; + final ImmutableMap, Label> flagsToPlatforms; Mappings( - Map> platformsToFlags, - Map, Label> flagsToPlatforms) { + ImmutableMap> platformsToFlags, + ImmutableMap, Label> flagsToPlatforms) { this.platformsToFlags = platformsToFlags; this.flagsToPlatforms = flagsToPlatforms; } - PlatformMappingValue toPlatformMappingValue() { - return new PlatformMappingValue(platformsToFlags, flagsToPlatforms); + PlatformMappingValue toPlatformMappingValue( + ImmutableList> optionsClasses) { + return new PlatformMappingValue(platformsToFlags, flagsToPlatforms, optionsClasses); } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PlatformMappingValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/PlatformMappingValue.java index 8efb2b870dcbbf..d684d802047a52 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PlatformMappingValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PlatformMappingValue.java @@ -14,18 +14,23 @@ package com.google.devtools.build.lib.skyframe; +import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Preconditions.checkNotNull; + import com.google.common.base.MoreObjects; -import com.google.common.base.Preconditions; import com.google.common.base.Throwables; -import com.google.common.cache.Cache; import com.google.common.cache.CacheBuilder; +import com.google.common.cache.CacheLoader; +import com.google.common.cache.LoadingCache; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.Interner; import com.google.common.collect.Iterables; import com.google.common.util.concurrent.UncheckedExecutionException; import com.google.devtools.build.lib.analysis.PlatformOptions; import com.google.devtools.build.lib.analysis.config.BuildOptions; +import com.google.devtools.build.lib.analysis.config.FragmentOptions; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.concurrent.BlazeInterners; import com.google.devtools.build.lib.concurrent.ThreadSafety; @@ -37,7 +42,6 @@ import com.google.devtools.common.options.OptionsParser; import com.google.devtools.common.options.OptionsParsingException; import com.google.devtools.common.options.OptionsParsingResult; -import java.util.Collection; import java.util.List; import java.util.Map; import java.util.Objects; @@ -56,7 +60,7 @@ public final class PlatformMappingValue implements SkyValue { public static final PlatformMappingValue EMPTY = - new PlatformMappingValue(ImmutableMap.of(), ImmutableMap.of()); + new PlatformMappingValue(ImmutableMap.of(), ImmutableMap.of(), ImmutableList.of()); /** Key for {@link PlatformMappingValue} based on the location of the mapping file. */ @ThreadSafety.Immutable @@ -137,10 +141,11 @@ public String toString() { } } - private final Map> platformsToFlags; - private final Map, Label> flagsToPlatforms; - private final Cache, OptionsParsingResult> parserCache; - private final Cache mappingCache; + private final ImmutableMap> platformsToFlags; + private final ImmutableMap, Label> flagsToPlatforms; + private final ImmutableList> optionsClasses; + private final LoadingCache, OptionsParsingResult> parserCache; + private final LoadingCache mappingCache; /** * Creates a new mapping value which will match on the given platforms (if a target platform is @@ -148,19 +153,39 @@ public String toString() { * * @param platformsToFlags mapping from target platform label to the command line style flags that * should be parsed & modified if that platform is set - * @param flagsToPlatforms mapping from a collection of command line style flags to a target - * platform that should be set if the flags match the mapped options + * @param flagsToPlatforms mapping from a set of command line style flags to a target platform + * that should be set if the flags match the mapped options + * @param optionsClasses default options classes that should be used for options parsing */ PlatformMappingValue( - Map> platformsToFlags, - Map, Label> flagsToPlatforms) { - this.platformsToFlags = Preconditions.checkNotNull(platformsToFlags); - this.flagsToPlatforms = Preconditions.checkNotNull(flagsToPlatforms); + ImmutableMap> platformsToFlags, + ImmutableMap, Label> flagsToPlatforms, + ImmutableList> optionsClasses) { + this.platformsToFlags = checkNotNull(platformsToFlags); + this.flagsToPlatforms = checkNotNull(flagsToPlatforms); + this.optionsClasses = checkNotNull(optionsClasses); this.parserCache = CacheBuilder.newBuilder() .initialCapacity(platformsToFlags.size() + flagsToPlatforms.size()) - .build(); - this.mappingCache = CacheBuilder.newBuilder().weakKeys().build(); + .build( + new CacheLoader, OptionsParsingResult>() { + @Override + public OptionsParsingResult load(ImmutableSet args) + throws OptionsParsingException { + return parse(args); + } + }); + this.mappingCache = + CacheBuilder.newBuilder() + .weakKeys() + .build( + new CacheLoader() { + @Override + public BuildConfigurationValue.Key load(BuildConfigurationValue.Key original) + throws OptionsParsingException { + return computeMapping(original); + } + }); } /** @@ -178,29 +203,26 @@ public String toString() { * * * @param original the key representing the configuration to be mapped - * @param defaultBuildOptions build options as set by default in this server * @return the mapped key if any mapping matched the original or else the original * @throws OptionsParsingException if any of the user configured flags cannot be parsed * @throws IllegalArgumentException if the original does not contain a {@link PlatformOptions} * fragment */ - public BuildConfigurationValue.Key map( - BuildConfigurationValue.Key original, BuildOptions defaultBuildOptions) + public BuildConfigurationValue.Key map(BuildConfigurationValue.Key original) throws OptionsParsingException { try { - return mappingCache.get(original, () -> computeMapping(original, defaultBuildOptions)); + return mappingCache.get(original); } catch (ExecutionException | UncheckedExecutionException e) { Throwables.propagateIfPossible(e.getCause(), OptionsParsingException.class); throw new IllegalStateException(e); } } - private BuildConfigurationValue.Key computeMapping( - BuildConfigurationValue.Key original, BuildOptions defaultBuildOptions) + private BuildConfigurationValue.Key computeMapping(BuildConfigurationValue.Key original) throws OptionsParsingException { BuildOptions originalOptions = original.getOptions(); - Preconditions.checkArgument( + checkArgument( originalOptions.contains(PlatformOptions.class), "When using platform mappings, all configurations must contain platform options"); @@ -218,13 +240,11 @@ private BuildConfigurationValue.Key computeMapping( } modifiedOptions = - originalOptions.applyParsingResult( - parseWithCache(platformsToFlags.get(targetPlatform), defaultBuildOptions)); + originalOptions.applyParsingResult(parseWithCache(platformsToFlags.get(targetPlatform))); } else { boolean mappingFound = false; - for (Map.Entry, Label> flagsToPlatform : flagsToPlatforms.entrySet()) { - if (originalOptions.matches( - parseWithCache(flagsToPlatform.getKey(), defaultBuildOptions))) { + for (Map.Entry, Label> flagsToPlatform : flagsToPlatforms.entrySet()) { + if (originalOptions.matches(parseWithCache(flagsToPlatform.getKey()))) { modifiedOptions = originalOptions.clone(); modifiedOptions.get(PlatformOptions.class).platforms = ImmutableList.of(flagsToPlatform.getValue()); @@ -244,20 +264,20 @@ private BuildConfigurationValue.Key computeMapping( original.getFragments(), modifiedOptions); } - private OptionsParsingResult parseWithCache( - Collection args, BuildOptions defaultBuildOptions) throws OptionsParsingException { + private OptionsParsingResult parseWithCache(ImmutableSet args) + throws OptionsParsingException { try { - return parserCache.get(args, () -> parse(args, defaultBuildOptions)); + return parserCache.get(args); } catch (ExecutionException e) { - throw (OptionsParsingException) e.getCause(); + Throwables.propagateIfPossible(e.getCause(), OptionsParsingException.class); + throw new IllegalStateException(e); } } - private static OptionsParsingResult parse(Iterable args, BuildOptions defaultBuildOptions) - throws OptionsParsingException { + private OptionsParsingResult parse(Iterable args) throws OptionsParsingException { OptionsParser parser = OptionsParser.builder() - .optionsClasses(defaultBuildOptions.getFragmentClasses()) + .optionsClasses(optionsClasses) // We need the ability to re-map internal options in the mappings file. .ignoreInternalOptions(false) .build(); @@ -276,12 +296,13 @@ public boolean equals(Object obj) { } PlatformMappingValue that = (PlatformMappingValue) obj; return this.flagsToPlatforms.equals(that.flagsToPlatforms) - && this.platformsToFlags.equals(that.platformsToFlags); + && this.platformsToFlags.equals(that.platformsToFlags) + && this.optionsClasses.equals(that.optionsClasses); } @Override public int hashCode() { - return 37 * flagsToPlatforms.hashCode() + platformsToFlags.hashCode(); + return Objects.hash(flagsToPlatforms, platformsToFlags, optionsClasses); } @Override @@ -289,6 +310,7 @@ public String toString() { return MoreObjects.toStringHelper(this) .add("flagsToPlatforms", flagsToPlatforms) .add("platformsToFlags", platformsToFlags) + .add("optionsClasses", optionsClasses) .toString(); } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareAnalysisPhaseFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareAnalysisPhaseFunction.java index 1f24cb0f637cfa..eeacb169a24299 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareAnalysisPhaseFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareAnalysisPhaseFunction.java @@ -63,12 +63,9 @@ */ final class PrepareAnalysisPhaseFunction implements SkyFunction { private final ConfiguredRuleClassProvider ruleClassProvider; - private final BuildOptions defaultBuildOptions; - PrepareAnalysisPhaseFunction( - ConfiguredRuleClassProvider ruleClassProvider, BuildOptions defaultBuildOptions) { + PrepareAnalysisPhaseFunction(ConfiguredRuleClassProvider ruleClassProvider) { this.ruleClassProvider = ruleClassProvider; - this.defaultBuildOptions = defaultBuildOptions; } @Override @@ -102,12 +99,12 @@ public PrepareAnalysisPhaseValue compute(SkyKey key, Environment env) try { hostConfigurationKey = BuildConfigurationValue.keyWithPlatformMapping( - platformMappingValue, defaultBuildOptions, allFragments, hostOptions); + platformMappingValue, allFragments, hostOptions); for (BuildOptions buildOptions : getTopLevelBuildOptions(targetOptions, options.getMultiCpu())) { targetConfigurationKeysBuilder.add( BuildConfigurationValue.keyWithPlatformMapping( - platformMappingValue, defaultBuildOptions, allFragments, buildOptions)); + platformMappingValue, allFragments, buildOptions)); } } catch (OptionsParsingException e) { throw new PrepareAnalysisPhaseFunctionException(new InvalidConfigurationException(e)); @@ -280,7 +277,7 @@ private Multimap getConfigurations( for (BuildOptions toOption : toOptions) { configSkyKeys.add( BuildConfigurationValue.keyWithPlatformMapping( - platformMappingValue, defaultBuildOptions, allFragments, toOption)); + platformMappingValue, allFragments, toOption)); } } @@ -306,7 +303,7 @@ private Multimap getConfigurations( for (BuildOptions toOption : toOptions) { SkyKey configKey = BuildConfigurationValue.keyWithPlatformMapping( - platformMappingValue, defaultBuildOptions, allFragments, toOption); + platformMappingValue, allFragments, toOption); BuildConfigurationValue configValue = ((BuildConfigurationValue) configsResult.get(configKey)); // configValue will be null here if there was an exception thrown during configuration diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java index cee511b09c15a5..5e9eff923cd622 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java @@ -478,9 +478,7 @@ protected SkyframeExecutor( this.configuredTargetProgress = configuredTargetProgress; } - private ImmutableMap skyFunctions(PackageFactory pkgFactory) { - ConfiguredRuleClassProvider ruleClassProvider = - (ConfiguredRuleClassProvider) pkgFactory.getRuleClassProvider(); + private ImmutableMap skyFunctions() { BzlLoadFunction bzlLoadFunctionForInliningPackageAndWorkspaceNodes = getBzlLoadFunctionForInliningPackageAndWorkspaceNodes(); // TODO(janakr): use this semaphore to bound memory usage for SkyFunctions besides @@ -539,8 +537,7 @@ private ImmutableMap skyFunctions(PackageFactory p map.put( SkyFunctions.TARGET_PATTERN_PHASE, new TargetPatternPhaseFunction(externalPackageHelper)); map.put( - SkyFunctions.PREPARE_ANALYSIS_PHASE, - new PrepareAnalysisPhaseFunction(ruleClassProvider, defaultBuildOptions)); + SkyFunctions.PREPARE_ANALYSIS_PHASE, new PrepareAnalysisPhaseFunction(ruleClassProvider)); map.put(SkyFunctions.RECURSIVE_PKG, new RecursivePkgFunction(directories)); map.put( SkyFunctions.PACKAGE, @@ -570,15 +567,13 @@ private ImmutableMap skyFunctions(PackageFactory p cpuBoundSemaphore, shouldStoreTransitivePackagesInLoadingAndAnalysis(), shouldUnblockCpuWorkWhenFetchingDeps, - defaultBuildOptions, configuredTargetProgress)); map.put( SkyFunctions.ASPECT, new AspectFunction( new BuildViewProvider(), ruleClassProvider, - shouldStoreTransitivePackagesInLoadingAndAnalysis(), - defaultBuildOptions)); + shouldStoreTransitivePackagesInLoadingAndAnalysis())); map.put(SkyFunctions.LOAD_STARLARK_ASPECT, new ToplevelStarlarkAspectFunction()); map.put(SkyFunctions.ACTION_LOOKUP_CONFLICT_FINDING, new ActionLookupConflictFindingFunction()); map.put( @@ -637,7 +632,9 @@ private ImmutableMap skyFunctions(PackageFactory p map.put(SkyFunctions.REPOSITORY_MAPPING, new RepositoryMappingFunction()); map.put(SkyFunctions.RESOLVED_HASH_VALUES, new ResolvedHashesFunction()); map.put(SkyFunctions.RESOLVED_FILE, new ResolvedFileFunction()); - map.put(SkyFunctions.PLATFORM_MAPPING, new PlatformMappingFunction()); + map.put( + SkyFunctions.PLATFORM_MAPPING, + new PlatformMappingFunction(ruleClassProvider.getConfigurationOptions())); map.put(SkyFunctions.ARTIFACT_NESTED_SET, ArtifactNestedSetFunction.createInstance()); map.putAll(extraSkyFunctions); return ImmutableMap.copyOf(map); @@ -787,10 +784,9 @@ SkyframeBuildView getSkyframeBuildView() { */ protected void init() { progressReceiver = newSkyframeProgressReceiver(); - ImmutableMap skyFunctions = skyFunctions(pkgFactory); memoizingEvaluator = evaluatorSupplier.create( - skyFunctions, + skyFunctions(), evaluatorDiffer(), progressReceiver, graphInconsistencyReceiver, @@ -2116,7 +2112,7 @@ private BuildConfigurationValue.Key toConfigurationKey( throws InvalidConfigurationException { try { return BuildConfigurationValue.keyWithPlatformMapping( - platformMappingValue, defaultBuildOptions, depFragments, toOption); + platformMappingValue, depFragments, toOption); } catch (OptionsParsingException e) { throw new InvalidConfigurationException(Code.INVALID_BUILD_OPTIONS, e); } @@ -2215,7 +2211,6 @@ public BuildConfiguration getConfigurationForTesting( SkyKey key = BuildConfigurationValue.keyWithPlatformMapping( getPlatformMappingValue(eventHandler, options), - defaultBuildOptions, fragments, options); BuildConfigurationValue result = diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsTest.java index 1f96f126c16a17..5f84bf4e368198 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsTest.java @@ -16,7 +16,6 @@ import static com.google.common.truth.Truth.assertThat; -import com.google.common.base.Supplier; import com.google.common.collect.Collections2; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -30,18 +29,12 @@ import com.google.devtools.build.lib.analysis.DependencyResolver; import com.google.devtools.build.lib.analysis.TargetAndConfiguration; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; -import com.google.devtools.build.lib.analysis.config.BuildOptions; import com.google.devtools.build.lib.analysis.config.CompilationMode; -import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider; import com.google.devtools.build.lib.analysis.config.ConfigurationResolver; import com.google.devtools.build.lib.analysis.util.AnalysisMock; import com.google.devtools.build.lib.analysis.util.AnalysisTestCase; -import com.google.devtools.build.lib.causes.Cause; -import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; -import com.google.devtools.build.lib.packages.Aspect; import com.google.devtools.build.lib.packages.Attribute; -import com.google.devtools.build.lib.packages.Package; import com.google.devtools.build.lib.packages.RuleClassProvider; import com.google.devtools.build.lib.packages.Target; import com.google.devtools.build.lib.skyframe.util.SkyframeExecutorTestUtils; @@ -93,13 +86,9 @@ private static class ComputeDependenciesFunction implements SkyFunction { SkyFunctionName.createHermetic("CONFIGURED_TARGET_FUNCTION_COMPUTE_DEPENDENCIES"); private final LateBoundStateProvider stateProvider; - private final Supplier buildOptionsSupplier; - ComputeDependenciesFunction( - LateBoundStateProvider lateBoundStateProvider, - Supplier buildOptionsSupplier) { + ComputeDependenciesFunction(LateBoundStateProvider lateBoundStateProvider) { this.stateProvider = lateBoundStateProvider; - this.buildOptionsSupplier = buildOptionsSupplier; } /** Returns a {@link SkyKey} for a given pair. */ @@ -118,11 +107,8 @@ public SkyFunctionName functionName() { } } - /** - * Returns a {@link OrderedSetMultimap} map representing the - * deps of given target. - */ - static class Value implements SkyValue { + /** Returns an {@link OrderedSetMultimap} representing the deps of given target. */ + static final class Value implements SkyValue { OrderedSetMultimap depMap; Value(OrderedSetMultimap depMap) { @@ -139,15 +125,14 @@ public SkyValue compute(SkyKey skyKey, Environment env) env, new SkyframeDependencyResolver(env), (TargetAndConfiguration) skyKey.argument(), - ImmutableList.of(), - ImmutableMap.of(), - /* toolchainContexts= */ null, - /* useToolchainTransition= */ false, + ImmutableList.of(), + ImmutableMap.of(), + /*toolchainContexts=*/ null, + /*useToolchainTransition=*/ false, stateProvider.lateBoundRuleClassProvider(), stateProvider.lateBoundHostConfig(), - NestedSetBuilder.stableOrder(), - NestedSetBuilder.stableOrder(), - buildOptionsSupplier.get()); + NestedSetBuilder.stableOrder(), + NestedSetBuilder.stableOrder()); return env.valuesMissing() ? null : new Value(depMap); } catch (RuntimeException e) { throw e; @@ -156,8 +141,8 @@ public SkyValue compute(SkyKey skyKey, Environment env) } } - private static class EvalException extends SkyFunctionException { - public EvalException(Exception cause) { + private static final class EvalException extends SkyFunctionException { + EvalException(Exception cause) { super(cause, Transience.PERSISTENT); // We can generalize the transience if/when needed. } } @@ -174,7 +159,7 @@ public String extractTag(SkyKey skyKey) { * the {@link AnalysisMock} instance that instantiates the function gets created before the rest * of the build state. See {@link AnalysisTestCase#createMocks} for details. */ - private class LateBoundStateProvider { + private final class LateBoundStateProvider { RuleClassProvider lateBoundRuleClassProvider() { return ruleClassProvider; } @@ -189,13 +174,10 @@ BuildConfiguration lateBoundHostConfig() { */ private static final class AnalysisMockWithComputeDepsFunction extends AnalysisMock.Delegate { private final LateBoundStateProvider stateProvider; - private final Supplier defaultBuildOptions; - AnalysisMockWithComputeDepsFunction( - LateBoundStateProvider stateProvider, Supplier defaultBuildOptions) { + AnalysisMockWithComputeDepsFunction(LateBoundStateProvider stateProvider) { super(AnalysisMock.get()); this.stateProvider = stateProvider; - this.defaultBuildOptions = defaultBuildOptions; } @Override @@ -205,15 +187,14 @@ public ImmutableMap getSkyFunctions( .putAll(super.getSkyFunctions(directories)) .put( ComputeDependenciesFunction.SKYFUNCTION_NAME, - new ComputeDependenciesFunction(stateProvider, defaultBuildOptions)) + new ComputeDependenciesFunction(stateProvider)) .build(); } - }; + } @Override protected AnalysisMock getAnalysisMock() { - return new AnalysisMockWithComputeDepsFunction( - new LateBoundStateProvider(), () -> skyframeExecutor.getDefaultBuildOptions()); + return new AnalysisMockWithComputeDepsFunction(new LateBoundStateProvider()); } /** Returns the configured deps for a given target. */ @@ -223,12 +204,10 @@ private Multimap getConfiguredDeps( SkyKey key = ComputeDependenciesFunction.key(getTarget(targetLabel), getConfiguration(target)); // Must re-enable analysis for Skyframe functions that create configured targets. skyframeExecutor.getSkyframeBuildView().enableAnalysis(true); - Object evalResult = SkyframeExecutorTestUtils.evaluate( - skyframeExecutor, key, /*keepGoing=*/false, reporter); + EvaluationResult evalResult = + SkyframeExecutorTestUtils.evaluate(skyframeExecutor, key, /*keepGoing=*/ false, reporter); skyframeExecutor.getSkyframeBuildView().enableAnalysis(false); - @SuppressWarnings("unchecked") - SkyValue value = ((EvaluationResult) evalResult).get(key); - return ((ComputeDependenciesFunction.Value) value).depMap; + return evalResult.get(key).depMap; } /** @@ -311,9 +290,7 @@ public void splitDeps() throws Exception { assertThat(deps).hasSize(2); ConfiguredTarget dep1 = deps.get(0); ConfiguredTarget dep2 = deps.get(1); - assertThat( - ImmutableList.of( - getConfiguration(dep1).getCpu(), getConfiguration(dep2).getCpu())) + assertThat(ImmutableList.of(getConfiguration(dep1).getCpu(), getConfiguration(dep2).getCpu())) .containsExactly("armeabi-v7a", "k8"); // We don't care what order split deps are listed, but it must be deterministic. assertThat( diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunctionTest.java index de6fd666a4c0c2..c8adbf0f6cf893 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingFunctionTest.java @@ -25,15 +25,14 @@ import com.google.devtools.build.lib.analysis.config.BuildOptions; import com.google.devtools.build.lib.analysis.config.CoreOptions; import com.google.devtools.build.lib.analysis.config.FragmentClassSet; -import com.google.devtools.build.lib.analysis.config.FragmentOptions; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.rules.repository.RepositoryDelegatorFunction; import com.google.devtools.build.lib.skyframe.util.SkyframeExecutorTestUtils; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.skyframe.EvaluationResult; -import com.google.devtools.common.options.OptionsParsingException; import java.util.Optional; +import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -52,16 +51,20 @@ public final class PlatformMappingFunctionTest extends BuildViewTestCase { private static final FragmentClassSet PLATFORM_FRAGMENT_CLASS = FragmentClassSet.of(ImmutableSet.of(PlatformConfiguration.class)); - private static final ImmutableList> - BUILD_CONFIG_PLATFORM_OPTIONS = ImmutableList.of(CoreOptions.class, PlatformOptions.class); - private static final Label PLATFORM1 = Label.parseAbsoluteUnchecked("//platforms:one"); - private static final BuildOptions DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS = - getDefaultBuildConfigPlatformOptions(); private static final Label DEFAULT_TARGET_PLATFORM = Label.parseAbsoluteUnchecked("@local_config_platform//:host"); + private BuildOptions defaultBuildOptions; + + @Before + public void setDefaultBuildOptions() { + defaultBuildOptions = + BuildOptions.getDefaultBuildOptionsForFragments( + ruleClassProvider.getConfigurationOptions()); + } + @Test public void testMappingFileDoesNotExist() { MissingInputFileException exception = @@ -80,10 +83,9 @@ public void testMappingFileDoesNotExistDefaultLocation() throws Exception { BuildConfigurationValue.Key key = BuildConfigurationValue.keyWithoutPlatformMapping( - PLATFORM_FRAGMENT_CLASS, DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS); + PLATFORM_FRAGMENT_CLASS, defaultBuildOptions); - BuildConfigurationValue.Key mapped = - platformMappingValue.map(key, DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS); + BuildConfigurationValue.Key mapped = platformMappingValue.map(key); assertThat(mapped.getOptions().get(PlatformOptions.class).platforms) .containsExactly(DEFAULT_TARGET_PLATFORM); @@ -111,12 +113,10 @@ public void testMappingFileIsRead() throws Exception { PlatformMappingValue platformMappingValue = executeFunction(PlatformMappingValue.Key.create(PathFragment.create("my_mapping_file"))); - BuildOptions modifiedOptions = DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS.clone(); + BuildOptions modifiedOptions = defaultBuildOptions.clone(); modifiedOptions.get(PlatformOptions.class).platforms = ImmutableList.of(PLATFORM1); - BuildConfigurationValue.Key mapped = - platformMappingValue.map( - keyForOptions(modifiedOptions), DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS); + BuildConfigurationValue.Key mapped = platformMappingValue.map(keyForOptions(modifiedOptions)); assertThat(mapped.getOptions().get(CoreOptions.class).cpu).isEqualTo("one"); } @@ -135,12 +135,10 @@ public void testMappingFileIsRead_fromAlternatePackagePath() throws Exception { PlatformMappingValue platformMappingValue = executeFunction(PlatformMappingValue.Key.create(PathFragment.create("my_mapping_file"))); - BuildOptions modifiedOptions = DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS.clone(); + BuildOptions modifiedOptions = defaultBuildOptions.clone(); modifiedOptions.get(PlatformOptions.class).platforms = ImmutableList.of(PLATFORM1); - BuildConfigurationValue.Key mapped = - platformMappingValue.map( - keyForOptions(modifiedOptions), DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS); + BuildConfigurationValue.Key mapped = platformMappingValue.map(keyForOptions(modifiedOptions)); assertThat(mapped.getOptions().get(CoreOptions.class).cpu).isEqualTo("one"); } @@ -157,12 +155,10 @@ public void handlesNoWorkspaceFile() throws Exception { PlatformMappingValue platformMappingValue = executeFunction(PlatformMappingValue.Key.create(PathFragment.create("my_mapping_file"))); - BuildOptions modifiedOptions = DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS.clone(); + BuildOptions modifiedOptions = defaultBuildOptions.clone(); modifiedOptions.get(PlatformOptions.class).platforms = ImmutableList.of(PLATFORM1); - BuildConfigurationValue.Key mapped = - platformMappingValue.map( - keyForOptions(modifiedOptions), DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS); + BuildConfigurationValue.Key mapped = platformMappingValue.map(keyForOptions(modifiedOptions)); assertThat(mapped.getOptions().get(CoreOptions.class).cpu).isEqualTo("one"); } @@ -180,12 +176,10 @@ public void multiplePackagePaths() throws Exception { PlatformMappingValue platformMappingValue = executeFunction(PlatformMappingValue.Key.create(PathFragment.create("my_mapping_file"))); - BuildOptions modifiedOptions = DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS.clone(); + BuildOptions modifiedOptions = defaultBuildOptions.clone(); modifiedOptions.get(PlatformOptions.class).platforms = ImmutableList.of(PLATFORM1); - BuildConfigurationValue.Key mapped = - platformMappingValue.map( - keyForOptions(modifiedOptions), DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS); + BuildConfigurationValue.Key mapped = platformMappingValue.map(keyForOptions(modifiedOptions)); assertThat(mapped.getOptions().get(CoreOptions.class).cpu).isEqualTo("one"); } @@ -208,12 +202,10 @@ public void multiplePackagePathsFirstWins() throws Exception { PlatformMappingValue platformMappingValue = executeFunction(PlatformMappingValue.Key.create(PathFragment.create("my_mapping_file"))); - BuildOptions modifiedOptions = DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS.clone(); + BuildOptions modifiedOptions = defaultBuildOptions.clone(); modifiedOptions.get(PlatformOptions.class).platforms = ImmutableList.of(PLATFORM1); - BuildConfigurationValue.Key mapped = - platformMappingValue.map( - keyForOptions(modifiedOptions), DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS); + BuildConfigurationValue.Key mapped = platformMappingValue.map(keyForOptions(modifiedOptions)); assertThat(mapped.getOptions().get(CoreOptions.class).cpu).isEqualTo("one"); } @@ -231,12 +223,10 @@ public void ableToChangeInternalOption() throws Exception { PlatformMappingValue platformMappingValue = executeFunction(PlatformMappingValue.Key.create(PathFragment.create("my_mapping_file"))); - BuildOptions modifiedOptions = DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS.clone(); + BuildOptions modifiedOptions = defaultBuildOptions.clone(); modifiedOptions.get(PlatformOptions.class).platforms = ImmutableList.of(PLATFORM1); - BuildConfigurationValue.Key mapped = - platformMappingValue.map( - keyForOptions(modifiedOptions), DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS); + BuildConfigurationValue.Key mapped = platformMappingValue.map(keyForOptions(modifiedOptions)); assertThat(mapped.getOptions().get(CoreOptions.class).outputDirectoryName) .isEqualTo("updated_output_dir"); @@ -256,14 +246,6 @@ private PlatformMappingValue executeFunction(PlatformMappingValue.Key key) throw return result.get(key); } - private static BuildOptions getDefaultBuildConfigPlatformOptions() { - try { - return BuildOptions.of(BUILD_CONFIG_PLATFORM_OPTIONS); - } catch (OptionsParsingException e) { - throw new RuntimeException(e); - } - } - private static BuildConfigurationValue.Key keyForOptions(BuildOptions modifiedOptions) { return BuildConfigurationValue.keyWithoutPlatformMapping( PLATFORM_FRAGMENT_CLASS, modifiedOptions); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingValueTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingValueTest.java index ff9e916bd2350a..d8d79ca97a7cad 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingValueTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/PlatformMappingValueTest.java @@ -29,7 +29,6 @@ import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.common.options.OptionsParsingException; -import java.util.Collection; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -51,21 +50,21 @@ public final class PlatformMappingValueTest { private static final Label PLATFORM2 = Label.parseAbsoluteUnchecked("//platforms:two"); private static final BuildOptions DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS = - getDefaultBuildConfigPlatformOptions(); + BuildOptions.getDefaultBuildOptionsForFragments(BUILD_CONFIG_PLATFORM_OPTIONS); private static final Label DEFAULT_TARGET_PLATFORM = Label.parseAbsoluteUnchecked("@local_config_platform//:host"); @Test public void testMapNoMappings() throws OptionsParsingException { PlatformMappingValue mappingValue = - new PlatformMappingValue(ImmutableMap.of(), ImmutableMap.of()); + new PlatformMappingValue( + ImmutableMap.of(), ImmutableMap.of(), BUILD_CONFIG_PLATFORM_OPTIONS); BuildConfigurationValue.Key key = BuildConfigurationValue.keyWithoutPlatformMapping( PLATFORM_FRAGMENT_CLASS, DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS); - BuildConfigurationValue.Key mapped = - mappingValue.map(key, DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS); + BuildConfigurationValue.Key mapped = mappingValue.map(key); assertThat(mapped.getOptions().get(PlatformOptions.class).platforms) .containsExactly(DEFAULT_TARGET_PLATFORM); @@ -73,17 +72,17 @@ public void testMapNoMappings() throws OptionsParsingException { @Test public void testMapPlatformToFlags() throws Exception { - ImmutableMap> platformsToFlags = - ImmutableMap.of(PLATFORM1, ImmutableList.of("--cpu=one", "--compilation_mode=dbg")); + ImmutableMap> platformsToFlags = + ImmutableMap.of(PLATFORM1, ImmutableSet.of("--cpu=one", "--compilation_mode=dbg")); PlatformMappingValue mappingValue = - new PlatformMappingValue(platformsToFlags, ImmutableMap.of()); + new PlatformMappingValue( + platformsToFlags, ImmutableMap.of(), BUILD_CONFIG_PLATFORM_OPTIONS); BuildOptions modifiedOptions = DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS.clone(); modifiedOptions.get(PlatformOptions.class).platforms = ImmutableList.of(PLATFORM1); - BuildConfigurationValue.Key mapped = - mappingValue.map(keyForOptions(modifiedOptions), DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS); + BuildConfigurationValue.Key mapped = mappingValue.map(keyForOptions(modifiedOptions)); assertThat(mapped.getFragments()).isEqualTo(PLATFORM_FRAGMENT_CLASS); @@ -92,18 +91,18 @@ public void testMapPlatformToFlags() throws Exception { @Test public void testMapFlagsToPlatform() throws Exception { - ImmutableMap, Label> flagsToPlatforms = - ImmutableMap.of(ImmutableList.of("--cpu=one", "--compilation_mode=dbg"), PLATFORM1); + ImmutableMap, Label> flagsToPlatforms = + ImmutableMap.of(ImmutableSet.of("--cpu=one", "--compilation_mode=dbg"), PLATFORM1); PlatformMappingValue mappingValue = - new PlatformMappingValue(ImmutableMap.of(), flagsToPlatforms); + new PlatformMappingValue( + ImmutableMap.of(), flagsToPlatforms, BUILD_CONFIG_PLATFORM_OPTIONS); BuildOptions modifiedOptions = DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS.clone(); modifiedOptions.get(CoreOptions.class).cpu = "one"; modifiedOptions.get(CoreOptions.class).compilationMode = CompilationMode.DBG; - BuildConfigurationValue.Key mapped = - mappingValue.map(keyForOptions(modifiedOptions), DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS); + BuildConfigurationValue.Key mapped = mappingValue.map(keyForOptions(modifiedOptions)); assertThat(mapped.getFragments()).isEqualTo(PLATFORM_FRAGMENT_CLASS); @@ -112,36 +111,36 @@ public void testMapFlagsToPlatform() throws Exception { @Test public void testMapFlagsToPlatformPriority() throws Exception { - ImmutableMap, Label> flagsToPlatforms = + ImmutableMap, Label> flagsToPlatforms = ImmutableMap.of( - ImmutableList.of("--cpu=foo", "--compilation_mode=dbg"), PLATFORM1, - ImmutableList.of("--cpu=foo"), PLATFORM2); + ImmutableSet.of("--cpu=foo", "--compilation_mode=dbg"), PLATFORM1, + ImmutableSet.of("--cpu=foo"), PLATFORM2); PlatformMappingValue mappingValue = - new PlatformMappingValue(ImmutableMap.of(), flagsToPlatforms); + new PlatformMappingValue( + ImmutableMap.of(), flagsToPlatforms, BUILD_CONFIG_PLATFORM_OPTIONS); BuildOptions modifiedOptions = DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS.clone(); modifiedOptions.get(CoreOptions.class).cpu = "foo"; - BuildConfigurationValue.Key mapped = - mappingValue.map(keyForOptions(modifiedOptions), DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS); + BuildConfigurationValue.Key mapped = mappingValue.map(keyForOptions(modifiedOptions)); assertThat(mapped.getOptions().get(PlatformOptions.class).platforms).containsExactly(PLATFORM2); } @Test public void testMapFlagsToPlatformNoneMatching() throws Exception { - ImmutableMap, Label> flagsToPlatforms = - ImmutableMap.of(ImmutableList.of("--cpu=foo", "--compilation_mode=dbg"), PLATFORM1); + ImmutableMap, Label> flagsToPlatforms = + ImmutableMap.of(ImmutableSet.of("--cpu=foo", "--compilation_mode=dbg"), PLATFORM1); PlatformMappingValue mappingValue = - new PlatformMappingValue(ImmutableMap.of(), flagsToPlatforms); + new PlatformMappingValue( + ImmutableMap.of(), flagsToPlatforms, BUILD_CONFIG_PLATFORM_OPTIONS); BuildOptions modifiedOptions = DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS.clone(); modifiedOptions.get(CoreOptions.class).cpu = "bar"; - BuildConfigurationValue.Key mapped = - mappingValue.map(keyForOptions(modifiedOptions), DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS); + BuildConfigurationValue.Key mapped = mappingValue.map(keyForOptions(modifiedOptions)); assertThat(mapped.getOptions().get(PlatformOptions.class).platforms) .containsExactly(DEFAULT_TARGET_PLATFORM); @@ -149,53 +148,51 @@ public void testMapFlagsToPlatformNoneMatching() throws Exception { @Test public void testMapNoPlatformOptions() throws Exception { - ImmutableMap, Label> flagsToPlatforms = - ImmutableMap.of(ImmutableList.of("--cpu=one"), PLATFORM1); + ImmutableMap, Label> flagsToPlatforms = + ImmutableMap.of(ImmutableSet.of("--cpu=one"), PLATFORM1); PlatformMappingValue mappingValue = - new PlatformMappingValue(ImmutableMap.of(), flagsToPlatforms); + new PlatformMappingValue( + ImmutableMap.of(), flagsToPlatforms, BUILD_CONFIG_PLATFORM_OPTIONS); BuildOptions options = BuildOptions.of(ImmutableList.of(CoreOptions.class)); - assertThrows( - IllegalArgumentException.class, - () -> mappingValue.map(keyForOptions(options), DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS)); + assertThrows(IllegalArgumentException.class, () -> mappingValue.map(keyForOptions(options))); } @Test public void testMapNoMappingIfPlatformIsSetButNotMatching() throws Exception { - ImmutableMap> platformsToFlags = - ImmutableMap.of(PLATFORM1, ImmutableList.of("--cpu=one", "--compilation_mode=dbg")); - ImmutableMap, Label> flagsToPlatforms = - ImmutableMap.of(ImmutableList.of("--cpu=one"), PLATFORM1); + ImmutableMap> platformsToFlags = + ImmutableMap.of(PLATFORM1, ImmutableSet.of("--cpu=one", "--compilation_mode=dbg")); + ImmutableMap, Label> flagsToPlatforms = + ImmutableMap.of(ImmutableSet.of("--cpu=one"), PLATFORM1); BuildOptions modifiedOptions = DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS.clone(); modifiedOptions.get(CoreOptions.class).cpu = "one"; modifiedOptions.get(PlatformOptions.class).platforms = ImmutableList.of(PLATFORM2); PlatformMappingValue mappingValue = - new PlatformMappingValue(platformsToFlags, flagsToPlatforms); + new PlatformMappingValue(platformsToFlags, flagsToPlatforms, BUILD_CONFIG_PLATFORM_OPTIONS); - BuildConfigurationValue.Key mapped = - mappingValue.map(keyForOptions(modifiedOptions), DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS); + BuildConfigurationValue.Key mapped = mappingValue.map(keyForOptions(modifiedOptions)); assertThat(keyForOptions(modifiedOptions)).isEqualTo(mapped); } @Test public void testMapNoMappingIfPlatformIsSetAndNoPlatformMapping() throws Exception { - ImmutableMap, Label> flagsToPlatforms = - ImmutableMap.of(ImmutableList.of("--cpu=one"), PLATFORM1); + ImmutableMap, Label> flagsToPlatforms = + ImmutableMap.of(ImmutableSet.of("--cpu=one"), PLATFORM1); BuildOptions modifiedOptions = DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS.clone(); modifiedOptions.get(CoreOptions.class).cpu = "one"; modifiedOptions.get(PlatformOptions.class).platforms = ImmutableList.of(PLATFORM2); PlatformMappingValue mappingValue = - new PlatformMappingValue(ImmutableMap.of(), flagsToPlatforms); + new PlatformMappingValue( + ImmutableMap.of(), flagsToPlatforms, BUILD_CONFIG_PLATFORM_OPTIONS); - BuildConfigurationValue.Key mapped = - mappingValue.map(keyForOptions(modifiedOptions), DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS); + BuildConfigurationValue.Key mapped = mappingValue.map(keyForOptions(modifiedOptions)); assertThat(keyForOptions(modifiedOptions)).isEqualTo(mapped); } @@ -217,14 +214,6 @@ public void testCustomKey() { assertThat(key.wasExplicitlySetByUser()).isTrue(); } - private static BuildOptions getDefaultBuildConfigPlatformOptions() { - try { - return BuildOptions.of(BUILD_CONFIG_PLATFORM_OPTIONS); - } catch (OptionsParsingException e) { - throw new RuntimeException(e); - } - } - private static BuildConfigurationValue.Key keyForOptions(BuildOptions modifiedOptions) { return BuildConfigurationValue.keyWithoutPlatformMapping( PLATFORM_FRAGMENT_CLASS, modifiedOptions);