Delete code associated with `--experimental_dynamic_configs=retroactive`. This flag value is unused and untested outside of a couple unit tests for individual components. Removing it simplifies work on b/185778053, specifically unblocking the removal of `BuildOptions.DiffForReconstruction`. Also took this opportunity to apply fixes to warnings in the affected files. PiperOrigin-RevId: 369970226
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD index 367f5da..472f6a8 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD
@@ -1581,10 +1581,8 @@ ":config/fragment_options", "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec", - "//src/main/java/com/google/devtools/build/lib/skyframe/trimming:trimmed_configuration_cache", "//src/main/java/com/google/devtools/build/lib/util", "//src/main/java/com/google/devtools/common/options", - "//third_party:flogger", "//third_party:guava", "//third_party:jsr305", "//third_party/protobuf:protobuf_java",
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java index 077e652..0801c60 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
@@ -100,49 +100,44 @@ import javax.annotation.Nullable; /** - * The BuildView presents a semantically-consistent and transitively-closed - * dependency graph for some set of packages. + * The BuildView presents a semantically-consistent and transitively-closed dependency graph for + * some set of packages. * * <h2>Package design</h2> * - * <p>This package contains the Blaze dependency analysis framework (aka - * "analysis phase"). The goal of this code is to perform semantic analysis of - * all of the build targets required for a given build, to report - * errors/warnings for any problems in the input, and to construct an "action - * graph" (see {@code lib.actions} package) correctly representing the work to - * be done during the execution phase of the build. + * <p>This package contains the Blaze dependency analysis framework (aka "analysis phase"). The goal + * of this code is to perform semantic analysis of all of the build targets required for a given + * build, to report errors/warnings for any problems in the input, and to construct an "action + * graph" (see {@code lib.actions} package) correctly representing the work to be done during the + * execution phase of the build. * - * <p><b>Configurations</b> the inputs to a build come from two sources: the - * intrinsic inputs, specified in the BUILD file, are called <em>targets</em>. - * The environmental inputs, coming from the build tool, the command-line, or - * configuration files, are called the <em>configuration</em>. Only when a - * target and a configuration are combined is there sufficient information to - * perform a build. </p> + * <p><b>Configurations</b> the inputs to a build come from two sources: the intrinsic inputs, + * specified in the BUILD file, are called <em>targets</em>. The environmental inputs, coming from + * the build tool, the command-line, or configuration files, are called the <em>configuration</em>. + * Only when a target and a configuration are combined is there sufficient information to perform a + * build. * - * <p>Targets are implemented by the {@link Target} hierarchy in the {@code - * lib.packages} code. Configurations are implemented by {@link - * BuildConfiguration}. The pair of these together is represented by an - * instance of class {@link ConfiguredTarget}; this is the root of a hierarchy - * with different implementations for each kind of target: source file, derived - * file, rules, etc. + * <p>Targets are implemented by the {@link Target} hierarchy in the {@code lib.packages} code. + * Configurations are implemented by {@link BuildConfiguration}. The pair of these together is + * represented by an instance of class {@link ConfiguredTarget}; this is the root of a hierarchy + * with different implementations for each kind of target: source file, derived file, rules, etc. * - * <p>The framework code in this package (as opposed to its subpackages) is - * responsible for constructing the {@code ConfiguredTarget} graph for a given - * target and configuration, taking care of such issues as: + * <p>The framework code in this package (as opposed to its subpackages) is responsible for + * constructing the {@code ConfiguredTarget} graph for a given target and configuration, taking care + * of such issues as: + * * <ul> * <li>caching common subgraphs. * <li>detecting and reporting cycles. * <li>correct propagation of errors through the graph. - * <li>reporting universal errors, such as dependencies from production code - * to tests, or to experimental branches. + * <li>reporting universal errors, such as dependencies from production code to tests, or to + * experimental branches. * <li>capturing and replaying errors. - * <li>maintaining the graph from one build to the next to - * avoid unnecessary recomputation. + * <li>maintaining the graph from one build to the next to avoid unnecessary recomputation. * <li>checking software licenses. * </ul> * - * <p>See also {@link ConfiguredTarget} which documents some important - * invariants. + * <p>See also {@link ConfiguredTarget} which documents some important invariants. */ public class BuildView { private static final GoogleLogger logger = GoogleLogger.forEnclosingClass(); @@ -154,12 +149,11 @@ private final ConfiguredRuleClassProvider ruleClassProvider; - /** - * A factory class to create the coverage report action. May be null. - */ + /** A factory class to create the coverage report action. May be null. */ @Nullable private final CoverageReportActionFactory coverageReportActionFactory; - public BuildView(BlazeDirectories directories, + public BuildView( + BlazeDirectories directories, ConfiguredRuleClassProvider ruleClassProvider, SkyframeExecutor skyframeExecutor, CoverageReportActionFactory coverageReportActionFactory) { @@ -191,8 +185,7 @@ @VisibleForTesting static LinkedHashSet<ConfiguredTarget> filterTestsByTargets( Collection<ConfiguredTarget> targets, Set<Label> allowedTargetLabels) { - return targets - .stream() + return targets.stream() .filter(ct -> allowedTargetLabels.contains(ct.getLabel())) .collect(Collectors.toCollection(LinkedHashSet::new)); } @@ -228,8 +221,9 @@ if (viewOptions.skyframePrepareAnalysis) { PrepareAnalysisPhaseValue prepareAnalysisPhaseValue; try (SilentCloseable c = Profiler.instance().profile("Prepare analysis phase")) { - prepareAnalysisPhaseValue = skyframeExecutor.prepareAnalysisPhase( - eventHandler, targetOptions, multiCpu, loadingResult.getTargetLabels()); + prepareAnalysisPhaseValue = + skyframeExecutor.prepareAnalysisPhase( + eventHandler, targetOptions, multiCpu, loadingResult.getTargetLabels()); // Determine the configurations configurations = @@ -243,12 +237,7 @@ // needed. This requires cleaning up the invalidation in SkyframeBuildView.setConfigurations. try (SilentCloseable c = Profiler.instance().profile("createConfigurations")) { configurations = - skyframeExecutor - .createConfigurations( - eventHandler, - targetOptions, - multiCpu, - keepGoing); + skyframeExecutor.createConfigurations(eventHandler, targetOptions, multiCpu, keepGoing); } try (SilentCloseable c = Profiler.instance().profile("AnalysisUtils.getTargetsWithConfigs")) { topLevelTargetsWithConfigsResult = @@ -261,10 +250,9 @@ eventHandler, configurations, viewOptions.maxConfigChangesToShow); if (configurations.getTargetConfigurations().size() == 1) { - eventBus - .post( - new MakeEnvironmentEvent( - configurations.getTargetConfigurations().get(0).getMakeEnvironment())); + eventBus.post( + new MakeEnvironmentEvent( + configurations.getTargetConfigurations().get(0).getMakeEnvironment())); } for (BuildConfiguration targetConfig : configurations.getTargetConfigurations()) { eventBus.post(targetConfig.toBuildEvent()); @@ -274,8 +262,7 @@ topLevelTargetsWithConfigsResult.getTargetsAndConfigs(); // Report the generated association of targets to configurations - Multimap<Label, BuildConfiguration> byLabel = - ArrayListMultimap.<Label, BuildConfiguration>create(); + Multimap<Label, BuildConfiguration> byLabel = ArrayListMultimap.create(); for (TargetAndConfiguration pair : topLevelTargetsWithConfigs) { byLabel.put(pair.getLabel(), pair.getConfiguration()); } @@ -334,13 +321,6 @@ String starlarkFunctionName = aspect.substring(delimiterPosition + 1); for (TargetAndConfiguration targetSpec : topLevelTargetsWithConfigs) { - if (targetSpec.getConfiguration() != null - && targetSpec.getConfiguration().trimConfigurationsRetroactively()) { - String errorMessage = - "Aspects were requested, but are not supported in retroactive trimming mode."; - throw new ViewCreationFailedException( - errorMessage, createFailureDetail(errorMessage, Analysis.Code.ASPECT_PREREQ_UNMET)); - } aspectConfigurations.put( Pair.of(targetSpec.getLabel(), aspect), targetSpec.getConfiguration()); aspectKeys.add( @@ -359,14 +339,6 @@ if (aspectFactoryClass != null) { for (TargetAndConfiguration targetSpec : topLevelTargetsWithConfigs) { - if (targetSpec.getConfiguration() != null - && targetSpec.getConfiguration().trimConfigurationsRetroactively()) { - String errorMessage = - "Aspects were requested, but are not supported in retroactive trimming mode."; - throw new ViewCreationFailedException( - errorMessage, - createFailureDetail(errorMessage, Analysis.Code.ASPECT_PREREQ_UNMET)); - } // For invoking top-level aspects, use the top-level configuration for both the // aspect and the base target while the top-level configuration is untrimmed. BuildConfiguration configuration = targetSpec.getConfiguration(); @@ -445,8 +417,10 @@ int numTargetsToAnalyze = topLevelTargetsWithConfigs.size(); int numSuccessful = skyframeAnalysisResult.getConfiguredTargets().size(); if (0 < numSuccessful && numSuccessful < numTargetsToAnalyze) { - String msg = String.format("Analysis succeeded for only %d of %d top-level targets", - numSuccessful, numTargetsToAnalyze); + String msg = + String.format( + "Analysis succeeded for only %d of %d top-level targets", + numSuccessful, numTargetsToAnalyze); eventHandler.handle(Event.info(msg)); logger.atInfo().log(msg); } @@ -765,7 +739,7 @@ ImmutableSet.Builder<ConfiguredTarget> targetsToTest = ImmutableSet.builder(); ImmutableSet.Builder<ConfiguredTarget> targetsToTestExclusive = ImmutableSet.builder(); for (ConfiguredTarget configuredTarget : allTestTargets) { - Target target = null; + Target target; try { target = packageManager.getTarget(eventHandler, configuredTarget.getLabel()); } catch (NoSuchTargetException | NoSuchPackageException e) { @@ -795,10 +769,10 @@ } /** - * Tests and clears the current thread's pending "interrupted" status, and - * throws InterruptedException iff it was set. + * Tests and clears the current thread's pending "interrupted" status, and throws + * InterruptedException iff it was set. */ - private final void pollInterruptedStatus() throws InterruptedException { + private static void pollInterruptedStatus() throws InterruptedException { if (Thread.interrupted()) { throw new InterruptedException(); }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java b/src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java index 35b3fc0..aa53a09 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java
@@ -55,22 +55,15 @@ /** * The implementation of AnalysisEnvironment used for analysis. It tracks metadata for each - * configured target, such as the errors and warnings emitted by that target. It is intended that - * a separate instance is used for each configured target, so that these don't mix up. + * configured target, such as the errors and warnings emitted by that target. It is intended that a + * separate instance is used for each configured target, so that these don't mix up. */ -public class CachingAnalysisEnvironment implements AnalysisEnvironment { - private final ArtifactFactory artifactFactory; +public final class CachingAnalysisEnvironment implements AnalysisEnvironment { + private final ArtifactFactory artifactFactory; private final ActionLookupKey owner; - /** - * If this is the system analysis environment, then errors and warnings are directly reported - * to the global reporter, rather than stored, i.e., we don't track here whether there are any - * errors. - */ - private final boolean isSystemEnv; private final boolean extendedSanityChecks; private final boolean allowAnalysisFailures; - private final ActionKeyContext actionKeyContext; private boolean enabled = true; @@ -88,19 +81,18 @@ * * <p>The artifact is stored so that we can deduplicate artifacts created multiple times. */ - private Map<Artifact, Object> artifacts; + private Map<Artifact, Object> artifacts = new HashMap<>(); /** * The list of actions registered by the configured target this analysis environment is * responsible for. May get cleared out at the end of the analysis of said target. */ - final List<ActionAnalysisMetadata> actions = new ArrayList<>(); + private final List<ActionAnalysisMetadata> actions = new ArrayList<>(); public CachingAnalysisEnvironment( ArtifactFactory artifactFactory, ActionKeyContext actionKeyContext, ActionLookupKey owner, - boolean isSystemEnv, boolean extendedSanityChecks, boolean allowAnalysisFailures, ExtendedEventHandler errorEventListener, @@ -109,14 +101,12 @@ this.artifactFactory = artifactFactory; this.actionKeyContext = actionKeyContext; this.owner = Preconditions.checkNotNull(owner); - this.isSystemEnv = isSystemEnv; this.extendedSanityChecks = extendedSanityChecks; this.allowAnalysisFailures = allowAnalysisFailures; this.errorEventListener = errorEventListener; this.skyframeEnv = env; this.starlarkBuiltinsValue = starlarkBuiltinsValue; middlemanFactory = new MiddlemanFactory(artifactFactory, this); - artifacts = new HashMap<>(); } public void disable(Target target) { @@ -146,7 +136,7 @@ */ public void verifyGeneratedArtifactHaveActions(Target target) { Collection<String> orphanArtifacts = getOrphanArtifactMap().values(); - List<String> checkedActions = null; + List<String> checkedActions; if (!orphanArtifacts.isEmpty()) { checkedActions = Lists.newArrayListWithCapacity(actions.size()); for (ActionAnalysisMetadata action : actions) { @@ -244,10 +234,6 @@ @Override public boolean hasErrors() { - // The system analysis environment never has errors. - if (isSystemEnv) { - return false; - } Preconditions.checkState(enabled); return ((StoredEventHandler) errorEventListener).hasErrors(); } @@ -295,7 +281,7 @@ PathFragment rootRelativePath, ArtifactRoot root, boolean contentBasedPath) { Preconditions.checkState(enabled); return dedupAndTrackArtifactAndOrigin( - artifactFactory.getDerivedArtifact(rootRelativePath, root, getOwner(), contentBasedPath), + artifactFactory.getDerivedArtifact(rootRelativePath, root, owner, contentBasedPath), extendedSanityChecks ? new Throwable() : null); } @@ -304,7 +290,7 @@ Preconditions.checkState(enabled); return (SpecialArtifact) dedupAndTrackArtifactAndOrigin( - artifactFactory.getTreeArtifact(rootRelativePath, root, getOwner()), + artifactFactory.getTreeArtifact(rootRelativePath, root, owner), extendedSanityChecks ? new Throwable() : null); } @@ -313,7 +299,7 @@ Preconditions.checkState(enabled); return (SpecialArtifact) dedupAndTrackArtifactAndOrigin( - artifactFactory.getSymlinkArtifact(rootRelativePath, root, getOwner()), + artifactFactory.getSymlinkArtifact(rootRelativePath, root, owner), extendedSanityChecks ? new Throwable() : null); } @@ -327,13 +313,13 @@ PathFragment rootRelativePath, ArtifactRoot root) { Preconditions.checkState(enabled); return dedupAndTrackArtifactAndOrigin( - artifactFactory.getFilesetArtifact(rootRelativePath, root, getOwner()), + artifactFactory.getFilesetArtifact(rootRelativePath, root, owner), extendedSanityChecks ? new Throwable() : null); } @Override public Artifact getConstantMetadataArtifact(PathFragment rootRelativePath, ArtifactRoot root) { - return artifactFactory.getConstantMetadataArtifact(rootRelativePath, root, getOwner()); + return artifactFactory.getConstantMetadataArtifact(rootRelativePath, root, owner); } @Override @@ -363,12 +349,12 @@ } @Override - public StarlarkSemantics getStarlarkSemantics() throws InterruptedException { + public StarlarkSemantics getStarlarkSemantics() { return starlarkBuiltinsValue.starlarkSemantics; } @Override - public ImmutableMap<String, Object> getStarlarkDefinedBuiltins() throws InterruptedException { + public ImmutableMap<String, Object> getStarlarkDefinedBuiltins() { return starlarkBuiltinsValue.exportedToJava; }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java index ad63bb9..fbd8e11 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
@@ -769,14 +769,6 @@ } /** - * Returns whether we should trim configurations to only include the fragments needed to correctly - * analyze a rule. - */ - public boolean trimConfigurationsRetroactively() { - return options.configsMode == CoreOptions.ConfigsMode.RETROACTIVE; - } - - /** * <b>>Experimental feature:</b> if true, qualifying outputs use path prefixes based on their * content instead of the traditional <code>blaze-out/$CPU-$COMPILATION_MODE</code>. *
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildOptions.java index 92ef550..bb123e8 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildOptions.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildOptions.java
@@ -14,7 +14,6 @@ package com.google.devtools.build.lib.analysis.config; -import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; import com.google.common.annotations.VisibleForTesting; @@ -38,7 +37,6 @@ import com.google.devtools.build.lib.skyframe.serialization.ObjectCodec; import com.google.devtools.build.lib.skyframe.serialization.SerializationContext; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; -import com.google.devtools.build.lib.skyframe.trimming.ConfigurationComparer; import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.build.lib.util.OrderedSetMultimap; import com.google.devtools.common.options.OptionDefinition; @@ -87,12 +85,10 @@ public static BuildOptions getDefaultBuildOptionsForFragments( List<Class<? extends FragmentOptions>> fragmentClasses) { - ArrayList<String> collector = new ArrayList<>(); try { - String[] stringCollector = new String[collector.size()]; - return BuildOptions.of(fragmentClasses, collector.toArray(stringCollector)); + return BuildOptions.of(fragmentClasses); } catch (OptionsParsingException e) { - throw new IllegalArgumentException("Failed to parse default options", e); + throw new IllegalArgumentException("Failed to parse empty options", e); } } @@ -531,11 +527,6 @@ return this; } - /** Returns whether the builder contains a particular Starlark option. */ - boolean contains(Label key) { - return starlarkOptions.containsKey(key); - } - /** Removes the value for the {@link FragmentOptions} with the given FragmentOptions class. */ public Builder removeFragmentOptions(Class<? extends FragmentOptions> key) { fragmentOptions.remove(key); @@ -975,82 +966,6 @@ } /** - * Compares the fragment sets in the options described by two diffs with the same base. - * - * @see ConfigurationComparer - */ - public static ConfigurationComparer.Result compareFragments( - OptionsDiffForReconstruction left, OptionsDiffForReconstruction right) { - // TODO: Add support for marking Starlark options as known default when trimming - // (sentinel object?) - checkArgument( - Arrays.equals(left.baseFingerprint, right.baseFingerprint), - "Can't compare diffs with different bases: %s and %s", - left, - right); - // This code effectively looks up each piece of data (native fragment or Starlark option) in - // this table (numbers reference comments in the code below): - // ▼left right▶ (none) extraSecond extraFirst differing - // (none) equal right only (#4) left only (#4) different (#1) - // extraSecond left only (#4) compare (#3) (impossible) (impossible) - // extraFirst right only (#4) (impossible) equal right only (#4) - // differing different (#1) (impossible) left only (#4) compare (#2) - - // Any difference in shared data is grounds to return DIFFERENT, which happens if: - // 1a. any starlark option was changed by one diff, but is neither changed nor removed by - // the other - if (left.hasChangeToStarlarkOptionUnchangedIn(right) - || right.hasChangeToStarlarkOptionUnchangedIn(left)) { - return ConfigurationComparer.Result.DIFFERENT; - } - // 1b. any native fragment was changed by one diff, but is neither changed nor removed by - // the other - if (left.hasChangeToNativeFragmentUnchangedIn(right) - || right.hasChangeToNativeFragmentUnchangedIn(left)) { - return ConfigurationComparer.Result.DIFFERENT; - } - // 2a. any starlark option was changed by both diffs, but to different values - if (!commonKeysHaveEqualValues( - left.differingStarlarkOptions, right.differingStarlarkOptions)) { - return ConfigurationComparer.Result.DIFFERENT; - } - // 2b. any native fragment was changed by both diffs, but to different values - if (!commonKeysHaveEqualValues(left.differingOptions, right.differingOptions)) { - return ConfigurationComparer.Result.DIFFERENT; - } - // 3a. any starlark option was added by both diffs, but with different values - if (!commonKeysHaveEqualValues( - left.extraSecondStarlarkOptions, right.extraSecondStarlarkOptions)) { - return ConfigurationComparer.Result.DIFFERENT; - } - // 3b. any native fragment was added by both diffs, but with different values - if (!commonKeysHaveEqualValues( - left.getExtraSecondFragmentsByClass(), right.getExtraSecondFragmentsByClass())) { - return ConfigurationComparer.Result.DIFFERENT; - } - - // At this point DIFFERENT is definitely not the result, so depending on which side(s) have - // extra data, we can decide which of the remaining choices to return. (#4) - boolean leftHasExtraData = left.hasExtraNativeFragmentsOrStarlarkOptionsNotIn(right); - boolean rightHasExtraData = right.hasExtraNativeFragmentsOrStarlarkOptionsNotIn(left); - - if (leftHasExtraData && rightHasExtraData) { - // If both have data that the other does not, all-shared-fragments-are-equal is all - // that can be said. - return ConfigurationComparer.Result.ALL_SHARED_FRAGMENTS_EQUAL; - } else if (leftHasExtraData) { - // If only the left instance has extra data, left is a superset of right. - return ConfigurationComparer.Result.SUPERSET; - } else if (rightHasExtraData) { - // If only the right instance has extra data, left is a subset of right. - return ConfigurationComparer.Result.SUBSET; - } else { - // If there is no extra data, the two options described by these diffs are equal. - return ConfigurationComparer.Result.EQUAL; - } - } - - /** * Clears {@link #cachedReconstructed} so that tests can cover the core logic of {@link * #applyDiff}. */ @@ -1059,67 +974,6 @@ cachedReconstructed = new SoftReference<>(null); } - private boolean hasChangeToStarlarkOptionUnchangedIn(OptionsDiffForReconstruction that) { - Set<Label> starlarkOptionsChangedOrRemovedInThat = - Sets.union( - that.differingStarlarkOptions.keySet(), - ImmutableSet.copyOf(that.extraFirstStarlarkOptions)); - return !starlarkOptionsChangedOrRemovedInThat.containsAll( - this.differingStarlarkOptions.keySet()); - } - - private boolean hasChangeToNativeFragmentUnchangedIn(OptionsDiffForReconstruction that) { - Set<Class<? extends FragmentOptions>> nativeFragmentsChangedOrRemovedInThat = - Sets.union(that.differingOptions.keySet(), that.extraFirstFragmentClasses); - return !nativeFragmentsChangedOrRemovedInThat.containsAll(this.differingOptions.keySet()); - } - - private Map<Class<? extends FragmentOptions>, FragmentOptions> - getExtraSecondFragmentsByClass() { - ImmutableMap.Builder<Class<? extends FragmentOptions>, FragmentOptions> builder = - new ImmutableMap.Builder<>(); - for (FragmentOptions options : extraSecondFragments) { - builder.put(options.getClass(), options); - } - return builder.build(); - } - - private static <K> boolean commonKeysHaveEqualValues(Map<K, ?> left, Map<K, ?> right) { - Set<K> commonKeys = Sets.intersection(left.keySet(), right.keySet()); - for (K commonKey : commonKeys) { - if (!Objects.equals(left.get(commonKey), right.get(commonKey))) { - return false; - } - } - return true; - } - - private boolean hasExtraNativeFragmentsOrStarlarkOptionsNotIn( - OptionsDiffForReconstruction that) { - // extra fragments/options can be... - // starlark options added by this diff, but not that one - if (!that.extraSecondStarlarkOptions - .keySet() - .containsAll(this.extraSecondStarlarkOptions.keySet())) { - return true; - } - // native fragments added by this diff, but not that one - if (!that.getExtraSecondFragmentsByClass() - .keySet() - .containsAll(this.getExtraSecondFragmentsByClass().keySet())) { - return true; - } - // starlark options removed by that diff, but not this one - if (!this.extraFirstStarlarkOptions.containsAll(that.extraFirstStarlarkOptions)) { - return true; - } - // native fragments removed by that diff, but not this one - if (!this.extraFirstFragmentClasses.containsAll(that.extraFirstFragmentClasses)) { - return true; - } - return false; - } - @Override public boolean equals(Object o) { if (this == o) {
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 c0a2d63..5f67920 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
@@ -44,7 +44,6 @@ import com.google.devtools.build.lib.packages.AttributeTransitionData; import com.google.devtools.build.lib.packages.ConfiguredAttributeMapper; import com.google.devtools.build.lib.packages.Target; -import com.google.devtools.build.lib.packages.TargetUtils; import com.google.devtools.build.lib.skyframe.BuildConfigurationValue; import com.google.devtools.build.lib.skyframe.PackageValue; import com.google.devtools.build.lib.skyframe.PlatformMappingValue; @@ -58,7 +57,6 @@ import com.google.devtools.common.options.OptionsParsingException; import java.util.ArrayList; import java.util.Collection; -import java.util.Collections; import java.util.Comparator; import java.util.HashMap; import java.util.HashSet; @@ -216,24 +214,7 @@ } private Dependency resolveHostTransition( - Dependency.Builder dependencyBuilder, DependencyKey dependencyKey) - throws DependencyEvaluationException { - // The current rule's host configuration can also be used for the dep. We short-circuit - // the standard transition logic for host transitions because these transitions are - // uniquely frequent. It's possible, e.g., for every node in the configured target graph - // to incur multiple host transitions. So we aggressively optimize to avoid hurting - // analysis time. - if (hostConfiguration.trimConfigurationsRetroactively() - && !dependencyKey.getAspects().isEmpty()) { - String message = - ctgValue.getLabel() - + " has aspects attached, but these are not supported in retroactive" - + " trimming mode."; - env.getListener() - .handle(Event.error(TargetUtils.getLocationMaybe(ctgValue.getTarget()), message)); - throw new DependencyEvaluationException(new InvalidConfigurationException(message)); - } - + Dependency.Builder dependencyBuilder, DependencyKey dependencyKey) { return dependencyBuilder .setConfiguration(hostConfiguration) .setAspects(dependencyKey.getAspects()) @@ -332,8 +313,7 @@ throw new DependencyEvaluationException(e); } - Collections.sort(dependencies, SPLIT_DEP_ORDERING); - return ImmutableList.copyOf(dependencies); + return ImmutableList.sortedCopyOf(SPLIT_DEP_ORDERING, dependencies); } private ImmutableList<String> collectTransitionKeys(Attribute attribute) @@ -566,13 +546,8 @@ LinkedHashSet<TargetAndConfiguration> result = new LinkedHashSet<>(); for (TargetAndConfiguration originalInput : defaultContext) { - if (successfullyEvaluatedTargets.containsKey(originalInput)) { - // The configuration was successfully evaluated. - result.add(successfullyEvaluatedTargets.get(originalInput)); - } else { - // Either the configuration couldn't be determined (e.g. loading phase error) or it's null. - result.add(originalInput); - } + // If the configuration couldn't be determined (e.g. loading phase error), use the original. + result.add(successfullyEvaluatedTargets.getOrDefault(originalInput, originalInput)); } return new TopLevelTargetsAndConfigsResult(result, hasError); }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java index 0097e18..6d501e3 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java
@@ -622,13 +622,7 @@ */ ON, /** Default mode: Each configured target is evaluated with all fragments known to Blaze. */ - NOTRIM, - /** - * Experimental mode: Each configured target is evaluated with only the configuration fragments - * it needs by visiting them with a full configuration to begin with and collapsing the - * configuration down to the fragments which were actually used. - */ - RETROACTIVE; + NOTRIM } /** Converter for --experimental_dynamic_configs. */
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 971e2e1..91b121a 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
@@ -128,7 +128,7 @@ * @throws AspectCreationException if the value loaded is not a {@link StarlarkDefinedAspect}. */ @Nullable - static StarlarkDefinedAspect loadStarlarkDefinedAspect( + private static StarlarkDefinedAspect loadStarlarkDefinedAspect( Environment env, StarlarkAspectClass starlarkAspectClass) throws AspectCreationException, InterruptedException { Label extensionLabel = starlarkAspectClass.getExtensionLabel(); @@ -269,9 +269,6 @@ throw new IllegalStateException("Unexpected exception from BuildConfigurationFunction when " + "computing " + key.getAspectConfigurationKey(), e); } - if (aspectConfiguration.trimConfigurationsRetroactively()) { - throw new AssertionError("Aspects should NEVER be evaluated in retroactive trimming mode."); - } } ConfiguredTarget associatedTarget = baseConfiguredTargetValue.getConfiguredTarget(); @@ -301,9 +298,6 @@ configuration = ((BuildConfigurationValue) result.get(associatedTarget.getConfigurationKey())) .getConfiguration(); - if (configuration.trimConfigurationsRetroactively()) { - throw new AssertionError("Aspects should NEVER be evaluated in retroactive trimming mode."); - } } try { associatedConfiguredTargetAndData = @@ -389,8 +383,6 @@ ToolchainContextKey.key() .configurationKey(BuildConfigurationValue.key(configuration)) .requiredToolchainTypeLabels(requiredToolchains) - .shouldSanityCheckConfiguration( - configuration.trimConfigurationsRetroactively()) .build(), ToolchainException.class); } catch (ToolchainException e) { @@ -553,14 +545,13 @@ } /** - * Collect all SkyKeys that are needed for a given list of AspectKeys, - * including transitive dependencies. + * Collect all SkyKeys that are needed for a given list of AspectKeys, including transitive + * dependencies. * - * Also collects all propagating aspects in correct order. + * <p>Also collects all propagating aspects in correct order. */ - private ImmutableMap<AspectDescriptor, SkyKey> getSkyKeysForAspectsAndCollectAspectPath( - ImmutableList<AspectKey> keys, - ImmutableList.Builder<SkyKey> aspectPathBuilder) { + private static ImmutableMap<AspectDescriptor, SkyKey> getSkyKeysForAspectsAndCollectAspectPath( + ImmutableList<AspectKey> keys, ImmutableList.Builder<SkyKey> aspectPathBuilder) { HashMap<AspectDescriptor, SkyKey> result = new HashMap<>(); for (AspectKey key : keys) { buildSkyKeys(key, result, aspectPathBuilder); @@ -568,7 +559,9 @@ return ImmutableMap.copyOf(result); } - private void buildSkyKeys(AspectKey key, HashMap<AspectDescriptor, SkyKey> result, + private static void buildSkyKeys( + AspectKey key, + HashMap<AspectDescriptor, SkyKey> result, ImmutableList.Builder<SkyKey> aspectPathBuilder) { if (result.containsKey(key.getAspectDescriptor())) { return; @@ -657,7 +650,7 @@ StoredEventHandler events = new StoredEventHandler(); CachingAnalysisEnvironment analysisEnvironment = view.createAnalysisEnvironment( - key, false, events, env, aspectConfiguration, starlarkBuiltinsValue); + key, events, env, aspectConfiguration, starlarkBuiltinsValue); ConfiguredAspect configuredAspect; if (aspect.getDefinition().applyToGeneratingRules()
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 165cf98..0ad6577 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -8,7 +8,6 @@ "//src/main/java/com/google/devtools/build/lib/skyframe/packages:srcs", "//src/main/java/com/google/devtools/build/lib/skyframe/proto:srcs", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization:srcs", - "//src/main/java/com/google/devtools/build/lib/skyframe/trimming:srcs", ], visibility = ["//src:__subpackages__"], ) @@ -78,7 +77,6 @@ "TopLevelActionLookupConflictFindingFunction.java", "ToplevelStarlarkAspectFunction.java", "TransitiveTargetFunction.java", - "TrimmedConfigurationProgressReceiver.java", "WorkspaceFileFunction.java", "actiongraph/ActionGraphDump.java", "actiongraph/v2/ActionGraphDump.java", @@ -299,7 +297,6 @@ "//src/main/java/com/google/devtools/build/lib/rules/cpp:cpp_interface", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec:serialization-constant", - "//src/main/java/com/google/devtools/build/lib/skyframe/trimming:trimmed_configuration_cache", "//src/main/java/com/google/devtools/build/lib/util", "//src/main/java/com/google/devtools/build/lib/util:TestType", "//src/main/java/com/google/devtools/build/lib/util:abrupt_exit_exception",
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 950693a..5f03dc0 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
@@ -317,7 +317,7 @@ env, resolver, ctgValue, - ImmutableList.<Aspect>of(), + ImmutableList.of(), configConditions.asProviders(), unloadedToolchainContexts == null ? null @@ -521,8 +521,7 @@ ToolchainContextKey.key() .configurationKey(toolchainConfig) .requiredToolchainTypeLabels(requiredDefaultToolchains) - .execConstraintLabels(defaultExecConstraintLabels) - .shouldSanityCheckConfiguration(configuration.trimConfigurationsRetroactively()); + .execConstraintLabels(defaultExecConstraintLabels); if (parentToolchainContextKey != null) { // Find out what execution platform the parent used, and force that. @@ -550,7 +549,6 @@ .configurationKey(toolchainConfig) .requiredToolchainTypeLabels(execGroup.requiredToolchains()) .execConstraintLabels(execGroup.execCompatibleWith()) - .shouldSanityCheckConfiguration(configuration.trimConfigurationsRetroactively()) .build()); } @@ -610,7 +608,6 @@ * @param env the Skyframe environment * @param resolver the dependency resolver * @param ctgValue the label and the configuration of the node - * @param aspects * @param configConditions the configuration conditions for evaluating the attributes of the node * @param toolchainContexts the toolchain context for this target * @param ruleClassProvider rule class provider for determining the right configuration fragments @@ -739,15 +736,6 @@ .collect(Collectors.toList()); if (configLabels.isEmpty()) { return ConfigConditions.EMPTY; - } else if (ctgValue.getConfiguration().trimConfigurationsRetroactively()) { - String message = - target.getLabel() - + " has configurable attributes, but these are not supported in retroactive trimming " - + "mode."; - env.getListener().handle(Event.error(TargetUtils.getLocationMaybe(target), message)); - throw new DependencyEvaluationException( - new ConfiguredValueCreationException( - message, ctgValue.getLabel(), ctgValue.getConfiguration())); } // Collect the actual deps without a configuration transition (since by definition config @@ -905,20 +893,7 @@ BuildConfiguration depConfiguration = dep.getConfiguration(); BuildConfigurationValue.Key depKey = depValue.getConfiguredTarget().getConfigurationKey(); - // Retroactive trimming may change the configuration associated with the dependency. - // If it does, we need to get that instance. - // TODO(b/140632978): doing these individually instead of doing them all at once may - // end up being wasteful use of Skyframe. Although these configurations are guaranteed - // to be in the Skyframe cache (because the dependency would have had to retrieve them - // to be created in the first place), looking them up repeatedly may be slower than - // just keeping a local cache and assigning the same configuration to all the CTs - // which need it. Profile this and see if there's a better way. if (depKey != null && !depKey.equals(BuildConfigurationValue.key(depConfiguration))) { - if (!depConfiguration.trimConfigurationsRetroactively()) { - throw new AssertionError( - "Loading configurations mid-dependency resolution should ONLY happen when " - + "retroactive trimming is enabled."); - } depConfiguration = ((BuildConfigurationValue) env.getValue(depKey)).getConfiguration(); } @@ -973,7 +948,7 @@ } @Nullable - private ConfiguredTargetValue createConfiguredTarget( + private static ConfiguredTargetValue createConfiguredTarget( SkyframeBuildView view, Environment env, Target target, @@ -994,7 +969,7 @@ StoredEventHandler events = new StoredEventHandler(); CachingAnalysisEnvironment analysisEnvironment = view.createAnalysisEnvironment( - configuredTargetKey, false, events, env, configuration, starlarkBuiltinsValue); + configuredTargetKey, events, env, configuration, starlarkBuiltinsValue); Preconditions.checkNotNull(depValueMap); ConfiguredTarget configuredTarget; @@ -1032,8 +1007,7 @@ configuration == null ? null : configuration.getEventId().getConfiguration(), - createDetailedExitCode( - event.getMessage(), Code.CONFIGURED_VALUE_CREATION_FAILED))) + createDetailedExitCode(event.getMessage()))) .collect(Collectors.toList())); throw new ConfiguredTargetFunctionException( new ConfiguredValueCreationException( @@ -1070,11 +1044,11 @@ } } - private static DetailedExitCode createDetailedExitCode(String message, Code code) { + private static DetailedExitCode createDetailedExitCode(String message) { return DetailedExitCode.of( FailureDetail.newBuilder() .setMessage(message) - .setAnalysis(Analysis.newBuilder().setCode(code)) + .setAnalysis(Analysis.newBuilder().setCode(Code.CONFIGURED_VALUE_CREATION_FAILED)) .build()); } @@ -1093,16 +1067,16 @@ * Used to declare all the exception types that can be wrapped in the exception thrown by {@link * ConfiguredTargetFunction#compute}. */ - static final class ConfiguredTargetFunctionException extends SkyFunctionException { - private ConfiguredTargetFunctionException(ConfiguredValueCreationException e) { + private static final class ConfiguredTargetFunctionException extends SkyFunctionException { + ConfiguredTargetFunctionException(ConfiguredValueCreationException e) { super(e, Transience.PERSISTENT); } - private ConfiguredTargetFunctionException(ActionConflictException e) { + ConfiguredTargetFunctionException(ActionConflictException e) { super(e, Transience.PERSISTENT); } - private ConfiguredTargetFunctionException(InvalidExecGroupException e) { + ConfiguredTargetFunctionException(InvalidExecGroupException e) { super(e, Transience.PERSISTENT); } }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtil.java b/src/main/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtil.java index e49675f..f0317b0 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtil.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtil.java
@@ -14,17 +14,13 @@ package com.google.devtools.build.lib.skyframe; -import static com.google.common.base.Predicates.equalTo; -import static com.google.common.base.Predicates.not; import static com.google.common.collect.ImmutableSet.toImmutableSet; -import static java.util.stream.Collectors.joining; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.ConfiguredTargetValue; -import com.google.devtools.build.lib.analysis.PlatformConfiguration; import com.google.devtools.build.lib.analysis.platform.PlatformInfo; import com.google.devtools.build.lib.analysis.platform.PlatformProviderUtils; import com.google.devtools.build.lib.cmdline.Label; @@ -49,11 +45,8 @@ @Nullable public static Map<ConfiguredTargetKey, PlatformInfo> getPlatformInfo( - ImmutableList<ConfiguredTargetKey> platformKeys, - Environment env, - boolean sanityCheckConfiguration) + ImmutableList<ConfiguredTargetKey> platformKeys, Environment env) throws InterruptedException, InvalidPlatformException { - validatePlatformKeys(platformKeys, env); if (env.valuesMissing()) { return null; @@ -72,7 +65,7 @@ boolean valuesMissing = env.valuesMissing(); Map<ConfiguredTargetKey, PlatformInfo> platforms = valuesMissing ? null : new HashMap<>(); for (ConfiguredTargetKey key : platformKeys) { - PlatformInfo platformInfo = findPlatformInfo(key, values.get(key), sanityCheckConfiguration); + PlatformInfo platformInfo = findPlatformInfo(key, values.get(key)); if (!valuesMissing && platformInfo != null) { platforms.put(key, platformInfo); } @@ -142,8 +135,7 @@ ConfiguredTargetKey key, ValueOrException3< ConfiguredValueCreationException, NoSuchThingException, ActionConflictException> - valueOrException, - boolean sanityCheckConfiguration) + valueOrException) throws InvalidPlatformException { try { @@ -153,29 +145,6 @@ } ConfiguredTarget configuredTarget = ctv.getConfiguredTarget(); - BuildConfigurationValue.Key configurationKey = configuredTarget.getConfigurationKey(); - // This check is necessary because trimming for other rules assumes that platform resolution - // uses the platform fragment and _only_ the platform fragment. Without this check, it's - // possible another fragment could slip in without us realizing, and thus break this - // assumption. - if (sanityCheckConfiguration - && configurationKey.getFragments().stream() - .anyMatch(not(equalTo(PlatformConfiguration.class)))) { - // Only the PlatformConfiguration fragment may be present on a platform rule in retroactive - // trimming mode. - String extraFragmentDescription = - configurationKey.getFragments().stream() - .filter(not(equalTo(PlatformConfiguration.class))) - .map(Class::getSimpleName) - .collect(joining(",")); - throw new InvalidPlatformException( - configuredTarget.getLabel(), - "has fragments other than PlatformConfiguration, " - + "which is forbidden in retroactive trimming mode: " - + "extra fragments are [" - + extraFragmentDescription - + "]"); - } PlatformInfo platformInfo = PlatformProviderUtils.platform(configuredTarget); if (platformInfo == null) { throw new InvalidPlatformException(configuredTarget.getLabel());
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredExecutionPlatformsFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredExecutionPlatformsFunction.java index b4ca46d..65d7cb1 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredExecutionPlatformsFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredExecutionPlatformsFunction.java
@@ -14,8 +14,6 @@ package com.google.devtools.build.lib.skyframe; -import static com.google.common.base.Predicates.equalTo; -import static com.google.common.base.Predicates.not; import static com.google.common.collect.ImmutableList.toImmutableList; import com.google.auto.value.AutoValue; @@ -97,8 +95,7 @@ // Load the configured target for each, and get the declared execution platforms providers. ImmutableList<ConfiguredTargetKey> registeredExecutionPlatformKeys = - configureRegisteredExecutionPlatforms( - env, configuration, configuration.trimConfigurationsRetroactively(), platformLabels); + configureRegisteredExecutionPlatforms(env, configuration, platformLabels); if (env.valuesMissing()) { return null; } @@ -125,13 +122,9 @@ return externalPackage.getRegisteredExecutionPlatforms(); } - private ImmutableList<ConfiguredTargetKey> configureRegisteredExecutionPlatforms( - Environment env, - BuildConfiguration configuration, - boolean sanityCheckConfiguration, - List<Label> labels) + private static ImmutableList<ConfiguredTargetKey> configureRegisteredExecutionPlatforms( + Environment env, BuildConfiguration configuration, List<Label> labels) throws InterruptedException, RegisteredExecutionPlatformsFunctionException { - ImmutableList<ConfiguredTargetKey> keys = labels.stream() .map( @@ -157,22 +150,6 @@ } ConfiguredTarget target = ((ConfiguredTargetValue) valueOrException.get()).getConfiguredTarget(); - // This check is necessary because trimming for other rules assumes that platform resolution - // uses the platform fragment and _only_ the platform fragment. Without this check, it's - // possible another fragment could slip in without us realizing, and thus break this - // assumption. - if (sanityCheckConfiguration - && target.getConfigurationKey().getFragments().stream() - .anyMatch(not(equalTo(PlatformConfiguration.class)))) { - // Only the PlatformConfiguration fragment may be present on a platform rule in - // retroactive trimming mode. - throw new RegisteredExecutionPlatformsFunctionException( - new InvalidPlatformException( - target.getLabel(), - "has fragments other than PlatformConfiguration, " - + "which is forbidden in retroactive trimming mode"), - Transience.PERSISTENT); - } PlatformInfo platformInfo = PlatformProviderUtils.platform(target); if (platformInfo == null) { throw new RegisteredExecutionPlatformsFunctionException(
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredToolchainsFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredToolchainsFunction.java index 024601d..e90f6c8 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredToolchainsFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredToolchainsFunction.java
@@ -14,8 +14,8 @@ package com.google.devtools.build.lib.skyframe; +import static com.google.common.base.MoreObjects.firstNonNull; import static com.google.common.collect.ImmutableList.toImmutableList; -import static java.util.stream.Collectors.joining; import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; @@ -97,13 +97,9 @@ return RegisteredToolchainsValue.create(registeredToolchains); } - private Iterable<? extends String> getWorkspaceToolchains(Environment env) + private static ImmutableList<String> getWorkspaceToolchains(Environment env) throws InterruptedException { - List<String> patterns = getRegisteredToolchains(env); - if (patterns == null) { - return ImmutableList.of(); - } - return patterns; + return firstNonNull(getRegisteredToolchains(env), ImmutableList.of()); } /** @@ -113,7 +109,8 @@ */ @Nullable @VisibleForTesting - public static List<String> getRegisteredToolchains(Environment env) throws InterruptedException { + public static ImmutableList<String> getRegisteredToolchains(Environment env) + throws InterruptedException { PackageValue externalPackageValue = (PackageValue) env.getValue(PackageValue.key(LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER)); if (externalPackageValue == null) { @@ -124,10 +121,8 @@ return externalPackage.getRegisteredToolchains(); } - private ImmutableList<DeclaredToolchainInfo> configureRegisteredToolchains( - Environment env, - BuildConfiguration configuration, - List<Label> labels) + private static ImmutableList<DeclaredToolchainInfo> configureRegisteredToolchains( + Environment env, BuildConfiguration configuration, List<Label> labels) throws InterruptedException, RegisteredToolchainsFunctionException { ImmutableList<SkyKey> keys = labels.stream() @@ -155,28 +150,6 @@ ConfiguredTarget target = ((ConfiguredTargetValue) valueOrException.get()).getConfiguredTarget(); - if (configuration.trimConfigurationsRetroactively() - && !target.getConfigurationKey().getFragments().isEmpty()) { - // No fragment may be present on a toolchain rule in retroactive trimming mode. - // This is because trimming expects that platform and toolchain resolution uses only - // the platform configuration. In theory, this means toolchains could use platforms, but - // the current expectation is that toolchains should not use anything at all, so better - // to go with the stricter expectation for now. - String extraFragmentDescription = - target.getConfigurationKey().getFragments().stream() - .map(cl -> cl.getSimpleName()) - .collect(joining(",")); - - throw new RegisteredToolchainsFunctionException( - new InvalidToolchainLabelException( - toolchainLabel, - "this toolchain uses configuration, which is forbidden in retroactive trimming " - + "mode: " - + "extra fragments are [" - + extraFragmentDescription - + "]"), - Transience.PERSISTENT); - } DeclaredToolchainInfo toolchainInfo = PlatformProviderUtils.declaredToolchainInfo(target); if (toolchainInfo == null) { throw new RegisteredToolchainsFunctionException(
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SingleToolchainResolutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/SingleToolchainResolutionFunction.java index 8d92213..ee67d95 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SingleToolchainResolutionFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SingleToolchainResolutionFunction.java
@@ -88,7 +88,6 @@ key.toolchainTypeLabel(), key.availableExecutionPlatformKeys(), key.targetPlatformKey(), - configuration.trimConfigurationsRetroactively(), toolchains.registeredToolchains(), env, debug ? env.getListener() : null); @@ -104,7 +103,6 @@ Label toolchainTypeLabel, List<ConfiguredTargetKey> availableExecutionPlatformKeys, ConfiguredTargetKey targetPlatformKey, - boolean sanityCheckConfigurations, ImmutableList<DeclaredToolchainInfo> toolchains, Environment env, @Nullable EventHandler eventHandler) @@ -113,15 +111,14 @@ // Load the PlatformInfo needed to check constraints. Map<ConfiguredTargetKey, PlatformInfo> platforms; try { - platforms = PlatformLookupUtil.getPlatformInfo( - new ImmutableList.Builder<ConfiguredTargetKey>() + ImmutableList.<ConfiguredTargetKey>builderWithExpectedSize( + availableExecutionPlatformKeys.size() + 1) .add(targetPlatformKey) .addAll(availableExecutionPlatformKeys) .build(), - env, - sanityCheckConfigurations); + env); if (env.valuesMissing()) { return null; } @@ -318,15 +315,15 @@ * Used to indicate errors during the computation of an {@link SingleToolchainResolutionValue}. */ private static final class ToolchainResolutionFunctionException extends SkyFunctionException { - public ToolchainResolutionFunctionException(NoToolchainFoundException e) { + ToolchainResolutionFunctionException(NoToolchainFoundException e) { super(e, Transience.PERSISTENT); } - public ToolchainResolutionFunctionException(InvalidToolchainLabelException e) { + ToolchainResolutionFunctionException(InvalidToolchainLabelException e) { super(e, Transience.PERSISTENT); } - public ToolchainResolutionFunctionException(InvalidPlatformException e) { + ToolchainResolutionFunctionException(InvalidPlatformException e) { super(e, Transience.PERSISTENT); } }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java index 1554ccf..713d543 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
@@ -332,12 +332,7 @@ skyframeExecutor.handleAnalysisInvalidatingChange(); } } - if (configurations.getTargetConfigurations().stream() - .anyMatch(BuildConfiguration::trimConfigurationsRetroactively)) { - skyframeExecutor.activateRetroactiveTrimming(); - } else { - skyframeExecutor.deactivateRetroactiveTrimming(); - } + skyframeAnalysisWasDiscarded = false; this.configurations = configurations; setTopLevelHostConfiguration(configurations.getHostConfiguration()); @@ -981,7 +976,6 @@ CachingAnalysisEnvironment createAnalysisEnvironment( ActionLookupKey owner, - boolean isSystemEnv, ExtendedEventHandler eventHandler, Environment env, BuildConfiguration config, @@ -992,7 +986,6 @@ artifactFactory, skyframeExecutor.getActionKeyContext(), owner, - isSystemEnv, extendedSanityChecks, allowAnalysisFailures, eventHandler,
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 86fe88d..56715ae 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
@@ -87,7 +87,6 @@ import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.analysis.config.BuildConfigurationCollection; import com.google.devtools.build.lib.analysis.config.BuildOptions; -import com.google.devtools.build.lib.analysis.config.BuildOptions.OptionsDiffForReconstruction; import com.google.devtools.build.lib.analysis.config.BuildOptionsView; import com.google.devtools.build.lib.analysis.config.ConfigurationResolver; import com.google.devtools.build.lib.analysis.config.CoreOptions; @@ -171,7 +170,6 @@ import com.google.devtools.build.lib.skyframe.PackageLookupFunction.CrossRepositoryLabelViolationStrategy; import com.google.devtools.build.lib.skyframe.SkyframeActionExecutor.ActionCompletedReceiver; import com.google.devtools.build.lib.skyframe.SkyframeActionExecutor.ProgressSupplier; -import com.google.devtools.build.lib.skyframe.trimming.TrimmedConfigurationCache; import com.google.devtools.build.lib.util.AbruptExitException; import com.google.devtools.build.lib.util.DetailedExitCode; import com.google.devtools.build.lib.util.ResourceUsage; @@ -375,11 +373,6 @@ private final PathResolverFactory pathResolverFactory = new PathResolverFactoryImpl(); @Nullable private final NonexistentFileReceiver nonexistentFileReceiver; - private final TrimmedConfigurationCache<SkyKey, Label, OptionsDiffForReconstruction> - trimmingCache = TrimmedConfigurationProgressReceiver.buildCache(); - private final TrimmedConfigurationProgressReceiver trimmingListener = - new TrimmedConfigurationProgressReceiver(trimmingCache); - private boolean siblingRepositoryLayout = false; private Map<String, String> lastRemoteDefaultExecProperties; @@ -855,7 +848,6 @@ public void resetEvaluator() { init(); emittedEventState.clear(); - clearTrimmingCache(); skyframeBuildView.reset(); } @@ -885,26 +877,11 @@ public void handleAnalysisInvalidatingChange() { logger.atInfo().log("Dropping configured target data"); analysisCacheDiscarded = true; - clearTrimmingCache(); skyframeBuildView.clearInvalidatedActionLookupKeys(); skyframeBuildView.clearLegacyData(); ArtifactNestedSetFunction.getInstance().resetArtifactNestedSetFunctionMaps(); } - /** Activates retroactive trimming (idempotently, so has no effect if already active). */ - void activateRetroactiveTrimming() { - trimmingListener.activate(); - } - - /** Deactivates retroactive trimming (idempotently, so has no effect if already inactive). */ - void deactivateRetroactiveTrimming() { - trimmingListener.deactivate(); - } - - protected void clearTrimmingCache() { - trimmingCache.clear(); - } - /** Used with dump --rules. */ public static class RuleStat { private final String key; @@ -1901,19 +1878,6 @@ // when depConfig is non-null, so we need to explicitly override it in that case. resolvedConfig = null; } else if (!configKey.equals(BuildConfigurationValue.key(depConfig))) { - // Retroactive trimming may change the configuration associated with the dependency. - // If it does, we need to get that instance. - // TODO(b/140632978): doing these individually instead of doing them all at once may - // end up being wasteful use of Skyframe. Although these configurations are guaranteed - // to be in the Skyframe cache (because the dependency would have had to retrieve - // them to be created in the first place), looking them up repeatedly may be slower - // than just keeping a local cache and assigning the same configuration to all the - // CTs which need it. Profile this and see if there's a better way. - if (!depConfig.trimConfigurationsRetroactively()) { - throw new AssertionError( - "Loading configurations mid-dependency resolution should ONLY happen when " - + "retroactive trimming is enabled."); - } resolvedConfig = getConfiguration(eventHandler, mergedTarget.getConfigurationKey()); } cts.put( @@ -1923,7 +1887,6 @@ packageValue.getPackage().getTarget(configuredTarget.getLabel().getName()), resolvedConfig, null)); - } catch (DuplicateException | NoSuchTargetException e) { throw new IllegalStateException( String.format("Error creating %s", configuredTarget.getLabel()), e); @@ -3104,7 +3067,6 @@ /** This receiver is only needed for loading, so it is null otherwise. */ @Override public void invalidated(SkyKey skyKey, InvalidationState state) { - trimmingListener.invalidated(skyKey, state); if (ignoreInvalidations) { return; } @@ -3113,7 +3075,6 @@ @Override public void enqueueing(SkyKey skyKey) { - trimmingListener.enqueueing(skyKey); if (ignoreInvalidations) { return; } @@ -3130,7 +3091,6 @@ @Nullable ErrorInfo newError, Supplier<EvaluationSuccessState> evaluationSuccessState, EvaluationState state) { - trimmingListener.evaluated(skyKey, newValue, newError, evaluationSuccessState, state); if (ignoreInvalidations) { return; }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ToolchainContextKey.java b/src/main/java/com/google/devtools/build/lib/skyframe/ToolchainContextKey.java index f82b27a..1062675 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ToolchainContextKey.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ToolchainContextKey.java
@@ -33,7 +33,6 @@ return new AutoValue_ToolchainContextKey.Builder() .requiredToolchainTypeLabels(ImmutableSet.of()) .execConstraintLabels(ImmutableSet.of()) - .shouldSanityCheckConfiguration(false) .forceExecutionPlatform(Optional.empty()); } @@ -48,8 +47,6 @@ abstract ImmutableSet<Label> execConstraintLabels(); - abstract boolean shouldSanityCheckConfiguration(); - abstract Optional<Label> forceExecutionPlatform(); /** Builder for {@link ToolchainContextKey}. */ @@ -65,8 +62,6 @@ Builder execConstraintLabels(Label... execConstraintLabels); - Builder shouldSanityCheckConfiguration(boolean shouldSanityCheckConfiguration); - ToolchainContextKey build(); Builder forceExecutionPlatform(Optional<Label> execPlatform);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ToolchainResolutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ToolchainResolutionFunction.java index 470f9fd..714cfb4 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ToolchainResolutionFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ToolchainResolutionFunction.java
@@ -87,11 +87,7 @@ // Load the configured target for the toolchain types to ensure that they are valid and // resolve aliases. ImmutableMap<Label, ToolchainTypeInfo> resolvedToolchainTypes = - loadToolchainTypes( - env, - configuration, - key.requiredToolchainTypeLabels(), - key.shouldSanityCheckConfiguration()); + loadToolchainTypes(env, configuration, key.requiredToolchainTypeLabels()); builder.setRequestedLabelToToolchainType(resolvedToolchainTypes); ImmutableSet<Label> resolvedToolchainTypeLabels = resolvedToolchainTypes.values().stream() @@ -106,8 +102,7 @@ key.configurationKey(), configuration, platformConfiguration, - key.execConstraintLabels(), - key.shouldSanityCheckConfiguration()); + key.execConstraintLabels()); if (env.valuesMissing()) { return null; } @@ -119,8 +114,7 @@ resolvedToolchainTypeLabels, key.forceExecutionPlatform().map(platformKeys::find), builder, - platformKeys, - key.shouldSanityCheckConfiguration()); + platformKeys); UnloadedToolchainContext unloadedToolchainContext = builder.build(); if (debug) { @@ -150,11 +144,10 @@ } /** Returns a map from the requested toolchain type to the {@link ToolchainTypeInfo} provider. */ - private ImmutableMap<Label, ToolchainTypeInfo> loadToolchainTypes( + private static ImmutableMap<Label, ToolchainTypeInfo> loadToolchainTypes( Environment environment, BuildConfiguration configuration, - ImmutableSet<Label> requestedToolchainTypeLabels, - boolean shouldSanityCheckConfiguration) + ImmutableSet<Label> requestedToolchainTypeLabels) throws InvalidToolchainTypeException, InterruptedException, ValueMissingException { ImmutableSet<ConfiguredTargetKey> toolchainTypeKeys = requestedToolchainTypeLabels.stream() @@ -167,8 +160,7 @@ .collect(toImmutableSet()); ImmutableMap<Label, ToolchainTypeInfo> resolvedToolchainTypes = - ToolchainTypeLookupUtil.resolveToolchainTypes( - environment, toolchainTypeKeys, shouldSanityCheckConfiguration); + ToolchainTypeLookupUtil.resolveToolchainTypes(environment, toolchainTypeKeys); if (environment.valuesMissing()) { throw new ValueMissingException(); } @@ -210,14 +202,13 @@ } } - private PlatformKeys loadPlatformKeys( + private static PlatformKeys loadPlatformKeys( SkyFunction.Environment environment, boolean debug, BuildConfigurationValue.Key configurationKey, BuildConfiguration configuration, PlatformConfiguration platformConfiguration, - ImmutableSet<Label> execConstraintLabels, - boolean shouldSanityCheckConfiguration) + ImmutableSet<Label> execConstraintLabels) throws InterruptedException, ValueMissingException, InvalidConstraintValueException, InvalidPlatformException { // Determine the target and host platform keys. @@ -237,9 +228,7 @@ // Load the host and target platforms early, to check for errors. PlatformLookupUtil.getPlatformInfo( - ImmutableList.of(hostPlatformKey, targetPlatformKey), - environment, - shouldSanityCheckConfiguration); + ImmutableList.of(hostPlatformKey, targetPlatformKey), environment); if (environment.valuesMissing()) { throw new ValueMissingException(); } @@ -251,20 +240,18 @@ configurationKey, configuration, hostPlatformKey, - execConstraintLabels, - shouldSanityCheckConfiguration); + execConstraintLabels); return PlatformKeys.create(hostPlatformKey, targetPlatformKey, executionPlatformKeys); } - private ImmutableList<ConfiguredTargetKey> loadExecutionPlatformKeys( + private static ImmutableList<ConfiguredTargetKey> loadExecutionPlatformKeys( SkyFunction.Environment environment, boolean debug, BuildConfigurationValue.Key configurationKey, BuildConfiguration configuration, ConfiguredTargetKey defaultPlatformKey, - ImmutableSet<Label> execConstraintLabels, - boolean shouldSanityCheckConfiguration) + ImmutableSet<Label> execConstraintLabels) throws InterruptedException, ValueMissingException, InvalidConstraintValueException, InvalidPlatformException { RegisteredExecutionPlatformsValue registeredExecutionPlatforms = @@ -294,20 +281,15 @@ .collect(toImmutableList()); return filterAvailablePlatforms( - environment, - debug, - availableExecutionPlatformKeys, - execConstraintKeys, - shouldSanityCheckConfiguration); + environment, debug, availableExecutionPlatformKeys, execConstraintKeys); } /** Returns only the platform keys that match the given constraints. */ - private ImmutableList<ConfiguredTargetKey> filterAvailablePlatforms( + private static ImmutableList<ConfiguredTargetKey> filterAvailablePlatforms( SkyFunction.Environment environment, boolean debug, ImmutableList<ConfiguredTargetKey> platformKeys, - ImmutableList<ConfiguredTargetKey> constraintKeys, - boolean shouldSanityCheckConfiguration) + ImmutableList<ConfiguredTargetKey> constraintKeys) throws InterruptedException, ValueMissingException, InvalidConstraintValueException, InvalidPlatformException { @@ -323,8 +305,7 @@ // platform is the host platform), Skyframe will return the correct results immediately without // need of a restart. Map<ConfiguredTargetKey, PlatformInfo> platformInfoMap = - PlatformLookupUtil.getPlatformInfo( - platformKeys, environment, shouldSanityCheckConfiguration); + PlatformLookupUtil.getPlatformInfo(platformKeys, environment); if (platformInfoMap == null) { throw new ValueMissingException(); } @@ -340,7 +321,7 @@ } /** Returns {@code true} if the given platform has all of the constraints. */ - private boolean filterPlatform( + private static boolean filterPlatform( SkyFunction.Environment environment, boolean debug, PlatformInfo platformInfo, @@ -365,14 +346,13 @@ return missingConstraints.isEmpty(); } - private void determineToolchainImplementations( + private static void determineToolchainImplementations( Environment environment, BuildConfigurationValue.Key configurationKey, ImmutableSet<Label> requiredToolchainTypeLabels, Optional<ConfiguredTargetKey> forcedExecutionPlatform, UnloadedToolchainContextImpl.Builder builder, - PlatformKeys platformKeys, - boolean shouldSanityCheckConfiguration) + PlatformKeys platformKeys) throws InterruptedException, ValueMissingException, InvalidPlatformException, NoMatchingPlatformException, UnresolvedToolchainsException, InvalidToolchainLabelException { @@ -452,8 +432,7 @@ Map<ConfiguredTargetKey, PlatformInfo> platforms = PlatformLookupUtil.getPlatformInfo( ImmutableList.of(selectedExecutionPlatformKey.get(), platformKeys.targetPlatformKey()), - environment, - shouldSanityCheckConfiguration); + environment); if (platforms == null) { throw new ValueMissingException(); } @@ -521,13 +500,11 @@ return false; } - Map<ToolchainTypeInfo, Label> toolchains = resolvedToolchains.row(executionPlatformKey); - if (!toolchains.keySet().containsAll(requiredToolchainTypes)) { - // Not all toolchains are present, ignore this execution platform. - return false; - } - - return true; + // Unless all toolchains are present, ignore this execution platform. + return resolvedToolchains + .row(executionPlatformKey) + .keySet() + .containsAll(requiredToolchainTypes); } @@ -614,7 +591,7 @@ /** Used to indicate errors during the computation of an {@link UnloadedToolchainContextImpl}. */ private static final class ToolchainResolutionFunctionException extends SkyFunctionException { - public ToolchainResolutionFunctionException(ToolchainException e) { + ToolchainResolutionFunctionException(ToolchainException e) { super(e, Transience.PERSISTENT); } }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ToolchainTypeLookupUtil.java b/src/main/java/com/google/devtools/build/lib/skyframe/ToolchainTypeLookupUtil.java index c77da68..df229c1 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ToolchainTypeLookupUtil.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ToolchainTypeLookupUtil.java
@@ -14,7 +14,6 @@ package com.google.devtools.build.lib.skyframe; -import static java.util.stream.Collectors.joining; import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException; @@ -37,9 +36,7 @@ @Nullable public static ImmutableMap<Label, ToolchainTypeInfo> resolveToolchainTypes( - Environment env, - Iterable<ConfiguredTargetKey> toolchainTypeKeys, - boolean sanityCheckConfiguration) + Environment env, Iterable<ConfiguredTargetKey> toolchainTypeKeys) throws InterruptedException, InvalidToolchainTypeException { Map< SkyKey, @@ -55,8 +52,7 @@ Map<Label, ToolchainTypeInfo> results = valuesMissing ? null : new HashMap<>(); for (ConfiguredTargetKey key : toolchainTypeKeys) { Label originalLabel = key.getLabel(); - ToolchainTypeInfo toolchainTypeInfo = - findToolchainTypeInfo(key, values.get(key), sanityCheckConfiguration); + ToolchainTypeInfo toolchainTypeInfo = findToolchainTypeInfo(key, values.get(key)); if (!valuesMissing && toolchainTypeInfo != null) { // These are only different if the toolchain type was aliased. results.put(originalLabel, toolchainTypeInfo); @@ -75,8 +71,7 @@ ConfiguredTargetKey key, ValueOrException3< ConfiguredValueCreationException, NoSuchThingException, ActionConflictException> - valueOrException, - boolean sanityCheckConfiguration) + valueOrException) throws InvalidToolchainTypeException { try { @@ -86,26 +81,6 @@ } ConfiguredTarget configuredTarget = ctv.getConfiguredTarget(); - BuildConfigurationValue.Key configurationKey = configuredTarget.getConfigurationKey(); - // This check is necessary because trimming for other rules assumes that platform resolution - // uses the platform fragment and _only_ the platform fragment. Without this check, it's - // possible another fragment could slip in without us realizing, and thus break this - // assumption. - if (sanityCheckConfiguration && !configurationKey.getFragments().isEmpty()) { - // No fragments may be present on a toolchain_type rule in retroactive - // trimming mode. - String extraFragmentDescription = - configurationKey.getFragments().stream() - .map(cl -> cl.getSimpleName()) - .collect(joining(",")); - throw new InvalidToolchainTypeException( - configuredTarget.getLabel(), - "has configuration fragments, " - + "which is forbidden in retroactive trimming mode: " - + "extra fragments are [" - + extraFragmentDescription - + "]"); - } ToolchainTypeInfo toolchainTypeInfo = PlatformProviderUtils.toolchainType(configuredTarget); if (toolchainTypeInfo == null) { if (PlatformProviderUtils.declaredToolchainInfo(configuredTarget) != null) { @@ -120,9 +95,9 @@ return toolchainTypeInfo; } catch (ConfiguredValueCreationException e) { - throw new InvalidToolchainTypeException(key.getLabel(), e); + throw new InvalidToolchainTypeException(e); } catch (NoSuchThingException e) { - throw new InvalidToolchainTypeException(key.getLabel(), e); + throw new InvalidToolchainTypeException(e); } catch (ActionConflictException e) { throw new InvalidToolchainTypeException(key.getLabel(), e); } @@ -136,12 +111,12 @@ super(formatError(label, DEFAULT_ERROR)); } - InvalidToolchainTypeException(Label label, ConfiguredValueCreationException e) { + InvalidToolchainTypeException(ConfiguredValueCreationException e) { // Just propagate the inner exception, because it's directly actionable. super(e); } - public InvalidToolchainTypeException(Label label, NoSuchThingException e) { + public InvalidToolchainTypeException(NoSuchThingException e) { // Just propagate the inner exception, because it's directly actionable. super(e); }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TrimmedConfigurationProgressReceiver.java b/src/main/java/com/google/devtools/build/lib/skyframe/TrimmedConfigurationProgressReceiver.java deleted file mode 100644 index 0480cf7..0000000 --- a/src/main/java/com/google/devtools/build/lib/skyframe/TrimmedConfigurationProgressReceiver.java +++ /dev/null
@@ -1,133 +0,0 @@ -// Copyright 2019 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.build.lib.skyframe; - -import com.google.common.base.Preconditions; -import com.google.devtools.build.lib.analysis.config.BuildOptions; -import com.google.devtools.build.lib.cmdline.Label; -import com.google.devtools.build.lib.skyframe.trimming.TrimmedConfigurationCache; -import com.google.devtools.build.skyframe.ErrorInfo; -import com.google.devtools.build.skyframe.EvaluationProgressReceiver; -import com.google.devtools.build.skyframe.SkyKey; -import com.google.devtools.build.skyframe.SkyValue; -import java.util.function.Supplier; -import javax.annotation.Nullable; - -/** - * Skyframe progress receiver which keeps a {@link TrimmedConfigurationCache} in sync with Skyframe - * invalidations and revalidations. - */ -public final class TrimmedConfigurationProgressReceiver implements EvaluationProgressReceiver { - - private final TrimmedConfigurationCache<SkyKey, Label, BuildOptions.OptionsDiffForReconstruction> - cache; - - private boolean enabled = true; - - public TrimmedConfigurationProgressReceiver( - TrimmedConfigurationCache<SkyKey, Label, BuildOptions.OptionsDiffForReconstruction> cache) { - this.cache = cache; - } - - public static TrimmedConfigurationCache<SkyKey, Label, BuildOptions.OptionsDiffForReconstruction> - buildCache() { - return new TrimmedConfigurationCache<>( - TrimmedConfigurationProgressReceiver::extractLabel, - TrimmedConfigurationProgressReceiver::extractOptionsDiff, - BuildOptions.OptionsDiffForReconstruction::compareFragments); - } - - public TrimmedConfigurationCache<SkyKey, Label, BuildOptions.OptionsDiffForReconstruction> - getCache() { - return this.cache; - } - - public static Label extractLabel(SkyKey key) { - Preconditions.checkArgument(isKeyCacheable(key)); - ConfiguredTargetKey configuredTargetKey = (ConfiguredTargetKey) key; - return configuredTargetKey.getLabel(); - } - - public static BuildOptions.OptionsDiffForReconstruction extractOptionsDiff(SkyKey key) { - Preconditions.checkArgument(isKeyCacheable(key)); - ConfiguredTargetKey configuredTargetKey = (ConfiguredTargetKey) key; - BuildConfigurationValue.Key configurationKey = configuredTargetKey.getConfigurationKey(); - return configurationKey.getOptionsDiff(); - } - - public static boolean isKeyCacheable(SkyKey key) { - if (!(key instanceof ConfiguredTargetKey)) { - // Only configured targets go in the cache. - // TODO(b/129286648): add aspect support - return false; - } - ConfiguredTargetKey configuredTargetKey = (ConfiguredTargetKey) key; - if (configuredTargetKey.getConfigurationKey() == null) { - // Null-configured targets do not go in the cache. - return false; - } - return true; - } - - public void activate() { - if (this.enabled) { - return; - } - this.enabled = true; - } - - public void deactivate() { - if (!this.enabled) { - return; - } - this.enabled = false; - this.cache.clear(); - } - - @Override - public void invalidated(SkyKey key, InvalidationState state) { - if (!enabled || !isKeyCacheable(key)) { - return; - } - switch (state) { - case DIRTY: - cache.invalidate(key); - break; - case DELETED: - cache.remove(key); - break; - } - } - - @Override - public void evaluated( - SkyKey key, - @Nullable SkyValue newValue, - @Nullable ErrorInfo newError, - Supplier<EvaluationSuccessState> evaluationSuccessState, - EvaluationState state) { - if (!enabled || !isKeyCacheable(key)) { - return; - } - switch (state) { - case BUILT: - // Do nothing; the evaluation would have handled putting itself (back) in the cache. - break; - case CLEAN: - cache.revalidate(key); - break; - } - } -}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/trimming/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/trimming/BUILD deleted file mode 100644 index fb13104..0000000 --- a/src/main/java/com/google/devtools/build/lib/skyframe/trimming/BUILD +++ /dev/null
@@ -1,26 +0,0 @@ -# Classes which provide support for automatically trimming configuration to avoid wasted work during a build. - -load("@rules_java//java:defs.bzl", "java_library") - -package( - default_visibility = ["//src:__subpackages__"], -) - -filegroup( - name = "srcs", - srcs = glob(["**"]), - visibility = ["//src:__subpackages__"], -) - -java_library( - name = "trimmed_configuration_cache", - srcs = [ - "ConfigurationComparer.java", - "KeyAndState.java", - "TrimmedConfigurationCache.java", - ], - deps = [ - "//third_party:auto_value", - "//third_party:guava", - ], -)
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/trimming/ConfigurationComparer.java b/src/main/java/com/google/devtools/build/lib/skyframe/trimming/ConfigurationComparer.java deleted file mode 100644 index 91b63b4..0000000 --- a/src/main/java/com/google/devtools/build/lib/skyframe/trimming/ConfigurationComparer.java +++ /dev/null
@@ -1,72 +0,0 @@ -// Copyright 2018 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.build.lib.skyframe.trimming; - -import java.util.function.BiFunction; - -/** - * Interface describing a function which compares two configurations and determines their - * relationship. - */ -@FunctionalInterface -public interface ConfigurationComparer<ConfigurationT> - extends BiFunction<ConfigurationT, ConfigurationT, ConfigurationComparer.Result> { - /** The outcome of comparing two configurations. */ - public enum Result { - /** - * All fragments in the first configuration are present and equal in the second, and vice versa. - */ - EQUAL(true, true, true), - /** - * All shared fragments are equal, but the second configuration has additional fragments the - * first does not. - */ - SUBSET(true, false, true), - /** - * All shared fragments are equal, but the first configuration has additional fragments the - * second does not. - */ - SUPERSET(false, true, true), - /** - * The two configurations each have fragments the other does not, but the shared fragments are - * equal. - */ - ALL_SHARED_FRAGMENTS_EQUAL(false, false, true), - /** At least one fragment shared between the two configurations is unequal. */ - DIFFERENT(false, false, false); - - private final boolean isSubsetOrEqual; - private final boolean isSupersetOrEqual; - private final boolean hasEqualSharedFragments; - - Result(boolean isSubsetOrEqual, boolean isSupersetOrEqual, boolean hasEqualSharedFragments) { - this.isSubsetOrEqual = isSubsetOrEqual; - this.isSupersetOrEqual = isSupersetOrEqual; - this.hasEqualSharedFragments = hasEqualSharedFragments; - } - - public boolean isSubsetOrEqual() { - return isSubsetOrEqual; - } - - public boolean isSupersetOrEqual() { - return isSupersetOrEqual; - } - - public boolean hasEqualSharedFragments() { - return hasEqualSharedFragments; - } - } -}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/trimming/KeyAndState.java b/src/main/java/com/google/devtools/build/lib/skyframe/trimming/KeyAndState.java deleted file mode 100644 index b30f42c..0000000 --- a/src/main/java/com/google/devtools/build/lib/skyframe/trimming/KeyAndState.java +++ /dev/null
@@ -1,64 +0,0 @@ -// Copyright 2018 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.build.lib.skyframe.trimming; - -import com.google.auto.value.AutoValue; - -/** - * A pair of some key type and a valid/invalid state, for use in {@link TrimmedConfigurationCache}. - */ -@AutoValue -abstract class KeyAndState<KeyT> { - enum State { - VALID(true), - POSSIBLY_INVALID(false); - - private final boolean isKnownValid; - - State(boolean isKnownValid) { - this.isKnownValid = isKnownValid; - } - - boolean isKnownValid() { - return isKnownValid; - } - } - - abstract KeyT getKey(); - - abstract State getState(); - - static <KeyT> KeyAndState<KeyT> create(KeyT key) { - return create(key, State.VALID); - } - - private static <KeyT> KeyAndState<KeyT> create(KeyT key, State state) { - return new AutoValue_KeyAndState<>(key, state); - } - - KeyAndState<KeyT> asValidated() { - if (State.VALID.equals(getState())) { - return this; - } - return create(getKey(), State.VALID); - } - - KeyAndState<KeyT> asInvalidated() { - if (State.POSSIBLY_INVALID.equals(getState())) { - return this; - } - return create(getKey(), State.POSSIBLY_INVALID); - } -}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/trimming/TrimmedConfigurationCache.java b/src/main/java/com/google/devtools/build/lib/skyframe/trimming/TrimmedConfigurationCache.java deleted file mode 100644 index ea82f13..0000000 --- a/src/main/java/com/google/devtools/build/lib/skyframe/trimming/TrimmedConfigurationCache.java +++ /dev/null
@@ -1,392 +0,0 @@ -// Copyright 2018 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.build.lib.skyframe.trimming; - -import com.google.common.base.Preconditions; -import java.util.Map.Entry; -import java.util.Optional; -import java.util.concurrent.ConcurrentHashMap; -import java.util.function.Function; -import java.util.function.UnaryOperator; - -/** - * Cache which tracks canonical invocations and matches keys to equivalent keys (after trimming). - * - * <p>This cache can be built independently of the massive build dependency that is build-base - * (SkyFunctions and BuildConfiguration and so on), and so it is - thus, it uses type parameters to - * speak more abstractly about what it cares about. - * - * <p>Consider a {@code <KeyT>} as a pair of {@code <DescriptorT>} and {@code <ConfigurationT>}. The - * descriptor describes what the key builds, while the configuration describes how to build it. - * - * <p>For example, a ConfiguredTargetKey is made up of a Label, which is its descriptor, and a - * BuildConfiguration, which is its configuration. An AspectKey is made up of a Label and a set of - * AspectDescriptors describing the aspect and the aspects it depends on, all of which are part of - * the AspectKey's descriptor, and also has a BuildConfiguration, which is its configuration. - * - * <p>A key always uses all of its descriptor, but it may only use part of its configuration. A Java - * configured target may have no use for Python configuration, for example. Thus, it would produce - * the same result to evaluate that target with a configuration which doesn't include Python data. - * Reducing the configuration to the subset configuration which only includes the bits the target - * actually needs is called trimming the configuration. - * - * <p>If this trimmed configuration is a subset of another configuration, then building whatever the - * descriptor refers to with that other configuration will produce the same result as the trimmed - * configuration, which is the same result as the configuration that the trimmed configuration was - * trimmed from. - * - * <p>This cache provides methods for matching keys which would evaluate to the same result because - * they have the same descriptor and trim to the same configuration, allowing callers to avoid doing - * work that has already been done. It also permits invalidating, revalidating, and removing these - * keys, as might happen during their lifecycle (if something they depend on has changed, etc.). - * - * <p>Internally, this cache is essentially a very sparse table. Each row, headed by a descriptor, - * describes the possible configurations of that descriptor. Columns, headed by a trimmed - * configuration, represent minimal configurations that descriptors can be invoked with. And a cell - * contains the key which corresponds to the canonical invocation of that descriptor with that - * configuration. - * - * <p>This class expects to be used in ways which are consistent with trimming. That is to say: - * - * <ul> - * <li>If the same key is put in the cache twice with different trimmed configurations, it must be - * invalidated between the two puts. Afterward, the original trimmed configuration is no - * longer valid for the rest of this build. - * <li>No trimmed configuration must be put in the cache which has equal values for every fragment - * it shares with another trimmed configuration already in the cache, unless the key - * associated with the other configuration has been invalidated. Afterward, the configuration - * which had previously been invalidated is no longer valid for the rest of this build. - * <li>Methods which read and add to the cache - {@link #get(KeyT)}, {@link #revalidate(KeyT)}, - * and {@link #putIfAbsent(KeyT, ConfigurationT)} - may be used together in one phase of the - * build. Methods which remove from the cache - {@link #invalidate(KeyT)}, {@link - * #remove(KeyT)}, and {@link #clear()} - may be used together in another phase of the build. - * Calls to these groups of methods must never be interleaved. - * </ul> - * - * <p>If used as described above, this class is thread-safe. - */ -public final class TrimmedConfigurationCache<KeyT, DescriptorT, ConfigurationT> { - - // ======== Tuning parameters ========== - /** The initial capacity of the cache of descriptors. */ - private static final int CACHE_INITIAL_SIZE = 100; - /** The table density for the cache of descriptors. */ - private static final float CACHE_LOAD_FACTOR = 0.9f; - /** The number of threads expected to be writing to the descriptor cache at a time. */ - private static final int CACHE_CONCURRENCY_LEVEL = 16; - /** - * The number of configurations to expect in a single descriptor - that is, the initial capacity - * of descriptors' maps. - */ - private static final int EXPECTED_CONFIGURATIONS_PER_DESCRIPTOR = 4; - /** - * The table density for the {@link ConcurrentHashMap ConcurrentHashMaps} created for tracking - * configurations of each descriptor. - */ - private static final float DESCRIPTOR_LOAD_FACTOR = 0.9f; - /** The number of threads expected to be writing to a single descriptor at a time. */ - private static final int DESCRIPTOR_CONCURRENCY_LEVEL = 1; - - private final Function<KeyT, DescriptorT> descriptorExtractor; - private final Function<KeyT, ConfigurationT> configurationExtractor; - - private final ConfigurationComparer<ConfigurationT> configurationComparer; - - private volatile ConcurrentHashMap< - DescriptorT, ConcurrentHashMap<ConfigurationT, KeyAndState<KeyT>>> - descriptors; - - /** - * Constructs a new TrimmedConfigurationCache with the given methods of extracting descriptors and - * configurations from keys, and uses the given predicate to determine the relationship between - * two configurations. - * - * <p>{@code configurationComparer} should be consistent with equals - that is, - * {@code a.equals(b) == b.equals(a) == configurationComparer.compare(a, b).equals(Result.EQUAL)} - */ - public TrimmedConfigurationCache( - Function<KeyT, DescriptorT> descriptorExtractor, - Function<KeyT, ConfigurationT> configurationExtractor, - ConfigurationComparer<ConfigurationT> configurationComparer) { - this.descriptorExtractor = descriptorExtractor; - this.configurationExtractor = configurationExtractor; - this.configurationComparer = configurationComparer; - this.descriptors = newCacheMap(); - } - - /** - * Looks for a key with the same descriptor as the input key, which has a configuration that - * trimmed to a subset of the input key's. - * - * <p>Note that this is not referring to a <em>proper</em> subset; it's quite possible for a key - * to "trim" to a configuration equal to its configuration. That is, without anything being - * removed. - * - * <p>If such a key has been added to this cache, it is returned in a present {@link Optional}. - * Invoking this key will produce the same result as invoking the input key. - * - * <p>If no such key has been added to this cache, or if a key has been added to the cache and - * subsequently been the subject of an {@link #invalidate(KeyT)}, an absent Optional will be - * returned instead. No currently-valid key has trimmed to an equivalent configuration, and so the - * input key should be executed. - */ - public Optional<KeyT> get(KeyT input) { - DescriptorT descriptor = getDescriptorFor(input); - ConcurrentHashMap<ConfigurationT, KeyAndState<KeyT>> trimmingsOfDescriptor = - descriptors.get(descriptor); - if (trimmingsOfDescriptor == null) { - // There are no entries at all for this descriptor. - return Optional.empty(); - } - ConfigurationT candidateConfiguration = getConfigurationFor(input); - for (Entry<ConfigurationT, KeyAndState<KeyT>> entry : trimmingsOfDescriptor.entrySet()) { - ConfigurationT trimmedConfig = entry.getKey(); - KeyAndState<KeyT> canonicalKeyAndState = entry.getValue(); - if (canSubstituteFor(candidateConfiguration, trimmedConfig, canonicalKeyAndState)) { - return Optional.of(canonicalKeyAndState.getKey()); - } - } - return Optional.empty(); - } - - /** - * Returns whether the given trimmed configuration and key are a suitable substitute for the - * candidate configuration. - */ - private boolean canSubstituteFor( - ConfigurationT candidateConfiguration, - ConfigurationT trimmedConfiguration, - KeyAndState<KeyT> canonicalKeyAndState) { - return canonicalKeyAndState.getState().isKnownValid() - && compareConfigurations(trimmedConfiguration, candidateConfiguration).isSubsetOrEqual(); - } - - /** - * Attempts to record the given key as the canonical invocation for its descriptor and the - * passed-in trimmed configuration. - * - * <p>The trimmed configuration must be a subset of the input key's configuration. Otherwise, - * {@link IllegalArgumentException} will be thrown. - * - * <p>If another key matching this configuration is found, that key will be returned. That key - * represents the canonical invocation, which should produce the same result as the input key. It - * may have been previously invalidated, but will be considered revalidated at this point. - * - * <p>Otherwise, if the input key is the first to trim to this configuration, the input key is - * returned. - */ - public KeyT putIfAbsent(KeyT canonicalKey, ConfigurationT trimmedConfiguration) { - ConfigurationT fullConfiguration = getConfigurationFor(canonicalKey); - Preconditions.checkArgument( - compareConfigurations(trimmedConfiguration, fullConfiguration).isSubsetOrEqual()); - ConcurrentHashMap<ConfigurationT, KeyAndState<KeyT>> trimmingsOfDescriptor = - descriptors.computeIfAbsent(getDescriptorFor(canonicalKey), unused -> newDescriptorMap()); - KeyAndState<KeyT> currentMapping = - trimmingsOfDescriptor.compute( - trimmedConfiguration, - (configuration, currentValue) -> { - if (currentValue == null) { - return KeyAndState.create(canonicalKey); - } else { - return currentValue.asValidated(); - } - }); - boolean newlyAdded = currentMapping.getKey().equals(canonicalKey); - int failedRemoves; - do { - failedRemoves = 0; - for (Entry<ConfigurationT, KeyAndState<KeyT>> entry : trimmingsOfDescriptor.entrySet()) { - if (entry.getValue().getState().equals(KeyAndState.State.POSSIBLY_INVALID)) { - // Remove invalidated keys where: - // * the same key evaluated to a different configuration than it does now - // * (for trimmed configurations not yet seen) the new trimmed configuration has equal - // values for every fragment it shares with the old configuration (including subsets - // or supersets). - // These are keys we know will not be revalidated as part of the current build. - // Although it also ensures that we don't remove the entry we just added, the check for - // invalidation is mainly to avoid wasting time checking entries that are still valid for - // the current build and therefore will not match either of these properties. - if (entry.getValue().getKey().equals(canonicalKey) - || (newlyAdded - && compareConfigurations(trimmedConfiguration, entry.getKey()) - .hasEqualSharedFragments())) { - if (!trimmingsOfDescriptor.remove(entry.getKey(), entry.getValue())) { - // It's possible that this entry was removed by another thread in the meantime. - failedRemoves += 1; - } - } - } - } - } while (failedRemoves > 0); - return currentMapping.getKey(); - } - - /** - * Marks the given key as invalidated. - * - * <p>An invalidated key will not be returned from {@link #get(KeyT)}, as it cannot be proven that - * the key will still trim to the same configuration. - * - * <p>This invalidation is undone if the input key is passed to {@link #revalidate(KeyT)}, or if - * the configuration it originally trimmed to is passed to a call of {@link putIfAbsent(KeyT, - * ConfigurationT)}. This is true regardless of whether the key passed to putIfAbsent is the same - * as the input to this method. - * - * <p>If the key is not currently canonical for any descriptor/configuration pair, or if the key - * had previously been invalidated and not revalidated, this method has no effect. - */ - public void invalidate(KeyT key) { - updateEntryWithRetries(key, KeyAndState::asInvalidated); - } - - /** - * Unmarks the given key as invalidated. - * - * <p>This undoes the effects of {@link #invalidate(KeyT)}, allowing the key to be returned from - * {@link #get(KeyT)} again. - * - * <p>If the key is not currently canonical for any descriptor/configuration pair, or if the key - * had not previously been invalidated or had since been revalidated, this method has no effect. - */ - public void revalidate(KeyT key) { - updateEntryWithRetries(key, KeyAndState::asValidated); - } - - /** - * Completely removes the given key from the cache. - * - * <p>After this call, {@link #get(KeyT)} and {@link #putIfAbsent(KeyT, ConfigurationT)} will no - * longer return this key unless it is put back in the cache with putIfAbsent. - * - * <p>If the key is not currently canonical for any descriptor/configuration pair, this method has - * no effect. - */ - public void remove(KeyT key) { - // Return null from the transformer to remove the key from the map. - updateEntryWithRetries(key, unused -> null); - - DescriptorT descriptor = getDescriptorFor(key); - ConcurrentHashMap<ConfigurationT, KeyAndState<KeyT>> trimmingsOfDescriptor = - descriptors.get(descriptor); - if (trimmingsOfDescriptor != null && trimmingsOfDescriptor.isEmpty()) { - descriptors.remove(descriptor, trimmingsOfDescriptor); - } - } - - /** - * Finds the entry in the cache where the given key is canonical and updates or removes it. - * - * <p>The transformation is applied transactionally; that is, if another change has happened since - * the value was first looked up, the new value is retrieved and the transformation is applied - * again. This repeats until there are no conflicts. - * - * <p>This method has no effect if this key is currently not canonical. - * - * @param transformation The transformation to apply to the given entry. The entry will be - * replaced with the value returned from invoking this on the original value. If it returns - * null, the entry will be removed instead. If it returns the same instance, nothing will be - * done to the entry. - */ - private void updateEntryWithRetries(KeyT key, UnaryOperator<KeyAndState<KeyT>> transformation) { - while (!updateEntryIfNoConflicts(key, transformation)) {} - } - - /** - * Finds the entry in the cache where the given key is canonical and updates or removes it. - * - * <p>Only one attempt is made, and if there's a collision with another change, false is returned - * and the map is not changed. - * - * <p>This method succeeds (returns {@code true}) without doing anything if this key is currently - * not canonical. - * - * @param transformation The transformation to apply to the given entry. The entry will be - * replaced with the value returned from invoking this on the original value. If it returns - * null, the entry will be removed instead. If it returns the same instance, nothing will be - * done to the entry. - */ - private boolean updateEntryIfNoConflicts( - KeyT key, UnaryOperator<KeyAndState<KeyT>> transformation) { - DescriptorT descriptor = getDescriptorFor(key); - ConcurrentHashMap<ConfigurationT, KeyAndState<KeyT>> trimmingsOfDescriptor = - descriptors.get(descriptor); - if (trimmingsOfDescriptor == null) { - // There are no entries at all for this descriptor. - return true; - } - - for (Entry<ConfigurationT, KeyAndState<KeyT>> entry : trimmingsOfDescriptor.entrySet()) { - KeyAndState<KeyT> currentValue = entry.getValue(); - if (currentValue.getKey().equals(key)) { - KeyAndState<KeyT> newValue = transformation.apply(currentValue); - if (newValue == null) { - return trimmingsOfDescriptor.remove(entry.getKey(), currentValue); - } else if (newValue != currentValue) { - return trimmingsOfDescriptor.replace(entry.getKey(), currentValue, newValue); - } else { - // newValue == currentValue, there's nothing to do - return true; - } - } - } - // The key requested wasn't in the map, so there's nothing to do - return true; - } - - /** - * Removes all keys from this cache, resetting it to its empty state. - * - * <p>This is equivalent to calling {@link #remove(KeyT)} on every key which had ever been passed - * to {@link #putIfAbsent(KeyT, ConfigurationT)}. - */ - public void clear() { - // Getting a brand new instance lets the old map be garbage collected, reducing its memory - // footprint from its previous expansions. - this.descriptors = newCacheMap(); - } - - /** Retrieves the descriptor by calling the descriptorExtractor. */ - private DescriptorT getDescriptorFor(KeyT key) { - return descriptorExtractor.apply(key); - } - - /** Retrieves the configuration by calling the configurationExtractor. */ - private ConfigurationT getConfigurationFor(KeyT key) { - return configurationExtractor.apply(key); - } - - /** - * Checks whether the first configuration is equal to or a subset of the second by calling the - * configurationComparer. - */ - private ConfigurationComparer.Result compareConfigurations( - ConfigurationT left, ConfigurationT right) { - return configurationComparer.apply(left, right); - } - - /** Generates a new map suitable for storing the cache as a whole. */ - private ConcurrentHashMap<DescriptorT, ConcurrentHashMap<ConfigurationT, KeyAndState<KeyT>>> - newCacheMap() { - return new ConcurrentHashMap<>(CACHE_INITIAL_SIZE, CACHE_LOAD_FACTOR, CACHE_CONCURRENCY_LEVEL); - } - - /** Generates a new map suitable for storing the cache of configurations for a descriptor. */ - private ConcurrentHashMap<ConfigurationT, KeyAndState<KeyT>> newDescriptorMap() { - return new ConcurrentHashMap<>( - EXPECTED_CONFIGURATIONS_PER_DESCRIPTOR, - DESCRIPTOR_LOAD_FACTOR, - DESCRIPTOR_CONCURRENCY_LEVEL); - } -}
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/BUILD b/src/test/java/com/google/devtools/build/lib/analysis/BUILD index a7c1bef..f7c236a 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/test/java/com/google/devtools/build/lib/analysis/BUILD
@@ -137,7 +137,6 @@ "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils:depsutils", - "//src/main/java/com/google/devtools/build/lib/skyframe/trimming:trimmed_configuration_cache", "//src/main/java/com/google/devtools/build/lib/util", "//src/main/java/com/google/devtools/build/lib/util:detailed_exit_code", "//src/main/java/com/google/devtools/build/lib/util:filetype", @@ -160,12 +159,10 @@ "//src/test/java/com/google/devtools/build/lib/packages:testutil", "//src/test/java/com/google/devtools/build/lib/rules/platform:testutil", "//src/test/java/com/google/devtools/build/lib/skyframe:testutil", - "//src/test/java/com/google/devtools/build/lib/skyframe/trimming:trimmable_test_fragments", "//src/test/java/com/google/devtools/build/lib/testutil", "//src/test/java/com/google/devtools/build/lib/testutil:JunitUtils", "//src/test/java/com/google/devtools/build/lib/testutil:TestConstants", "//src/test/java/com/google/devtools/build/skyframe:testutil", - "//third_party:auto_value", "//third_party:guava", "//third_party:guava-testlib", "//third_party:jsr305",
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/config/BuildOptionsCompareFragmentsTest.java b/src/test/java/com/google/devtools/build/lib/analysis/config/BuildOptionsCompareFragmentsTest.java deleted file mode 100644 index 3464e5f..0000000 --- a/src/test/java/com/google/devtools/build/lib/analysis/config/BuildOptionsCompareFragmentsTest.java +++ /dev/null
@@ -1,474 +0,0 @@ -// Copyright 2018 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -package com.google.devtools.build.lib.analysis.config; - -import static com.google.common.truth.Truth.assertThat; -import static org.junit.Assert.assertThrows; - -import com.google.auto.value.AutoValue; -import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; -import com.google.devtools.build.lib.analysis.config.BuildOptions.OptionsDiffForReconstruction; -import com.google.devtools.build.lib.cmdline.Label; -import com.google.devtools.build.lib.skyframe.trimming.ConfigurationComparer; -import com.google.devtools.build.lib.skyframe.trimming.TrimmableTestConfigurationFragments.AOptions; -import com.google.devtools.build.lib.skyframe.trimming.TrimmableTestConfigurationFragments.BOptions; -import java.util.Objects; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; -import org.junit.runners.Parameterized; -import org.junit.runners.Parameterized.Parameters; - -/** Tests of compareFragments in BuildOptions.OptionsDiffForReconstruction. */ -public final class BuildOptionsCompareFragmentsTest { - - /** Test cases for BuildOptionsCompareFragmentsTest. */ - @AutoValue - public abstract static class Case { - public abstract String getName(); - - public abstract BuildOptions getBase(); - - public abstract BuildOptions getLeft(); - - public abstract BuildOptions getRight(); - - public abstract ConfigurationComparer.Result getLeftToRightResult(); - - public abstract ConfigurationComparer.Result getRightToLeftResult(); - - public static Builder named(String name) { - return new AutoValue_BuildOptionsCompareFragmentsTest_Case.Builder().setName(name); - } - - /** Quick builder for test cases. */ - @AutoValue.Builder - public abstract static class Builder { - public abstract Builder setName(String name); - - public abstract Builder setBase(BuildOptions base); - - public Builder setBase(OptionsBuilder base) throws Exception { - return this.setBase(base.build()); - } - - public abstract Builder setLeft(BuildOptions left); - - public Builder setLeft(OptionsBuilder left) throws Exception { - return this.setLeft(left.build()); - } - - public abstract Builder setRight(BuildOptions right); - - public Builder setRight(OptionsBuilder right) throws Exception { - return this.setRight(right.build()); - } - - public Builder setResult(ConfigurationComparer.Result result) { - return this.setLeftToRightResult(result).setRightToLeftResult(result); - } - - public abstract Builder setLeftToRightResult(ConfigurationComparer.Result result); - - public abstract Builder setRightToLeftResult(ConfigurationComparer.Result result); - - public abstract Case build(); - } - - @Override - public final String toString() { - if (Objects.equals(this.getLeftToRightResult(), this.getRightToLeftResult())) { - return String.format("%s [result = %s]", this.getName(), this.getLeftToRightResult()); - } else { - return String.format( - "%s [compare(left, right) = %s; compare(right, left) = %s]", - this.getName(), this.getLeftToRightResult(), this.getRightToLeftResult()); - } - } - } - - /** Quick builder for BuildOptions instances. */ - public static final class OptionsBuilder { - private final ImmutableMap.Builder<Label, Object> starlarkOptions = - new ImmutableMap.Builder<>(); - private final ImmutableList.Builder<Class<? extends FragmentOptions>> fragments = - new ImmutableList.Builder<>(); - private final ImmutableList.Builder<String> nativeOptions = new ImmutableList.Builder<>(); - - public OptionsBuilder withNativeFragment( - Class<? extends FragmentOptions> fragment, String... flags) { - this.fragments.add(fragment); - this.nativeOptions.add(flags); - return this; - } - - public OptionsBuilder withStarlarkOption(String setting, Object value) { - this.starlarkOptions.put(Label.parseAbsoluteUnchecked(setting), value); - return this; - } - - public OptionsBuilder withStarlarkOption(String setting) { - return this.withStarlarkOption(setting, setting); - } - - public BuildOptions build() throws Exception { - return BuildOptions.builder() - .addStarlarkOptions(this.starlarkOptions.build()) - .merge( - BuildOptions.of( - this.fragments.build(), this.nativeOptions.build().toArray(new String[0]))) - .build(); - } - } - - /** Test cases for compareFragments which produce an ordinary result. */ - @RunWith(Parameterized.class) - public static final class NormalCases { - - @Parameters(name = "{index}: {0}") - public static Iterable<Case> cases() throws Exception { - return ImmutableList.of( - Case.named("both options equal to the base") - .setResult(ConfigurationComparer.Result.EQUAL) - .setBase( - new OptionsBuilder() - .withNativeFragment(AOptions.class) - .withStarlarkOption("//alpha")) - .setLeft( - new OptionsBuilder() - .withNativeFragment(AOptions.class) - .withStarlarkOption("//alpha")) - .setRight( - new OptionsBuilder() - .withNativeFragment(AOptions.class) - .withStarlarkOption("//alpha")) - .build(), - Case.named("both sides change native fragment to same value") - .setResult(ConfigurationComparer.Result.EQUAL) - .setBase(new OptionsBuilder().withNativeFragment(AOptions.class, "--alpha=base")) - .setLeft(new OptionsBuilder().withNativeFragment(AOptions.class, "--alpha=new")) - .setRight(new OptionsBuilder().withNativeFragment(AOptions.class, "--alpha=new")) - .build(), - Case.named("both sides add native fragment with same value") - .setResult(ConfigurationComparer.Result.EQUAL) - .setBase(new OptionsBuilder()) - .setLeft(new OptionsBuilder().withNativeFragment(AOptions.class, "--alpha=new")) - .setRight(new OptionsBuilder().withNativeFragment(AOptions.class, "--alpha=new")) - .build(), - Case.named("both sides remove same native fragment") - .setResult(ConfigurationComparer.Result.EQUAL) - .setBase(new OptionsBuilder().withNativeFragment(AOptions.class)) - .setLeft(new OptionsBuilder()) - .setRight(new OptionsBuilder()) - .build(), - Case.named("both sides change Starlark option to same value") - .setResult(ConfigurationComparer.Result.EQUAL) - .setBase(new OptionsBuilder().withStarlarkOption("//alpha", "base")) - .setLeft(new OptionsBuilder().withStarlarkOption("//alpha", "new")) - .setRight(new OptionsBuilder().withStarlarkOption("//alpha", "new")) - .build(), - Case.named("both sides add Starlark option with same value") - .setResult(ConfigurationComparer.Result.EQUAL) - .setBase(new OptionsBuilder()) - .setLeft(new OptionsBuilder().withStarlarkOption("//alpha", "new")) - .setRight(new OptionsBuilder().withStarlarkOption("//alpha", "new")) - .build(), - Case.named("both sides remove same Starlark option") - .setResult(ConfigurationComparer.Result.EQUAL) - .setBase(new OptionsBuilder().withStarlarkOption("//alpha")) - .setLeft(new OptionsBuilder()) - .setRight(new OptionsBuilder()) - .build(), - Case.named("native fragment removed from right") - .setLeftToRightResult(ConfigurationComparer.Result.SUPERSET) - .setRightToLeftResult(ConfigurationComparer.Result.SUBSET) - .setBase(new OptionsBuilder().withNativeFragment(AOptions.class)) - .setLeft(new OptionsBuilder().withNativeFragment(AOptions.class)) - .setRight(new OptionsBuilder()) - .build(), - Case.named("native fragment added to right") - .setLeftToRightResult(ConfigurationComparer.Result.SUBSET) - .setRightToLeftResult(ConfigurationComparer.Result.SUPERSET) - .setBase(new OptionsBuilder()) - .setLeft(new OptionsBuilder()) - .setRight(new OptionsBuilder().withNativeFragment(AOptions.class)) - .build(), - Case.named("native fragment changed in left and removed from right") - .setLeftToRightResult(ConfigurationComparer.Result.SUPERSET) - .setRightToLeftResult(ConfigurationComparer.Result.SUBSET) - .setBase(new OptionsBuilder().withNativeFragment(AOptions.class, "--alpha=base")) - .setLeft(new OptionsBuilder().withNativeFragment(AOptions.class, "--alpha=left")) - .setRight(new OptionsBuilder()) - .build(), - Case.named("native fragment added to left and another fragment removed from right") - .setLeftToRightResult(ConfigurationComparer.Result.SUPERSET) - .setRightToLeftResult(ConfigurationComparer.Result.SUBSET) - .setBase(new OptionsBuilder().withNativeFragment(AOptions.class)) - .setLeft( - new OptionsBuilder() - .withNativeFragment(AOptions.class) - .withNativeFragment(BOptions.class)) - .setRight(new OptionsBuilder()) - .build(), - Case.named("Starlark option removed from right") - .setLeftToRightResult(ConfigurationComparer.Result.SUPERSET) - .setRightToLeftResult(ConfigurationComparer.Result.SUBSET) - .setBase(new OptionsBuilder().withStarlarkOption("//alpha")) - .setLeft(new OptionsBuilder().withStarlarkOption("//alpha")) - .setRight(new OptionsBuilder()) - .build(), - Case.named("Starlark option added to right") - .setLeftToRightResult(ConfigurationComparer.Result.SUBSET) - .setRightToLeftResult(ConfigurationComparer.Result.SUPERSET) - .setBase(new OptionsBuilder()) - .setLeft(new OptionsBuilder()) - .setRight(new OptionsBuilder().withStarlarkOption("//alpha")) - .build(), - Case.named("Starlark option changed in left and removed from right") - .setLeftToRightResult(ConfigurationComparer.Result.SUPERSET) - .setRightToLeftResult(ConfigurationComparer.Result.SUBSET) - .setBase(new OptionsBuilder().withStarlarkOption("//alpha", "base")) - .setLeft(new OptionsBuilder().withStarlarkOption("//alpha", "left")) - .setRight(new OptionsBuilder()) - .build(), - Case.named("Starlark option added to left and another option removed from right") - .setLeftToRightResult(ConfigurationComparer.Result.SUPERSET) - .setRightToLeftResult(ConfigurationComparer.Result.SUBSET) - .setBase(new OptionsBuilder().withStarlarkOption("//alpha")) - .setLeft( - new OptionsBuilder().withStarlarkOption("//alpha").withStarlarkOption("//bravo")) - .setRight(new OptionsBuilder()) - .build(), - Case.named("different native fragment added to each side") - .setResult(ConfigurationComparer.Result.ALL_SHARED_FRAGMENTS_EQUAL) - .setBase(new OptionsBuilder()) - .setLeft(new OptionsBuilder().withNativeFragment(AOptions.class)) - .setRight(new OptionsBuilder().withNativeFragment(BOptions.class)) - .build(), - Case.named("different native fragment removed from each side") - .setResult(ConfigurationComparer.Result.ALL_SHARED_FRAGMENTS_EQUAL) - .setBase( - new OptionsBuilder() - .withNativeFragment(AOptions.class) - .withNativeFragment(BOptions.class)) - .setLeft(new OptionsBuilder().withNativeFragment(AOptions.class)) - .setRight(new OptionsBuilder().withNativeFragment(BOptions.class)) - .build(), - Case.named("native fragment added and different fragment removed on left") - .setResult(ConfigurationComparer.Result.ALL_SHARED_FRAGMENTS_EQUAL) - .setBase(new OptionsBuilder().withNativeFragment(BOptions.class)) - .setLeft(new OptionsBuilder().withNativeFragment(AOptions.class)) - .setRight(new OptionsBuilder().withNativeFragment(BOptions.class)) - .build(), - Case.named( - "native fragment added to right; " - + "other fragment changed on left and removed from right") - .setResult(ConfigurationComparer.Result.ALL_SHARED_FRAGMENTS_EQUAL) - .setBase(new OptionsBuilder().withNativeFragment(AOptions.class, "--alpha=base")) - .setLeft(new OptionsBuilder().withNativeFragment(AOptions.class, "--alpha=left")) - .setRight(new OptionsBuilder().withNativeFragment(BOptions.class)) - .build(), - Case.named("native fragment changed on each side, removed from the other") - .setResult(ConfigurationComparer.Result.ALL_SHARED_FRAGMENTS_EQUAL) - .setBase( - new OptionsBuilder() - .withNativeFragment(AOptions.class, "--alpha=base") - .withNativeFragment(BOptions.class, "--bravo=base")) - .setLeft(new OptionsBuilder().withNativeFragment(AOptions.class, "--alpha=left")) - .setRight(new OptionsBuilder().withNativeFragment(BOptions.class, "--bravo=right")) - .build(), - Case.named( - "native fragment changed on left, removed from right; " - + "other fragment removed from left") - .setResult(ConfigurationComparer.Result.ALL_SHARED_FRAGMENTS_EQUAL) - .setBase( - new OptionsBuilder() - .withNativeFragment(AOptions.class, "--alpha=base") - .withNativeFragment(BOptions.class)) - .setLeft(new OptionsBuilder().withNativeFragment(AOptions.class, "--alpha=left")) - .setRight(new OptionsBuilder().withNativeFragment(BOptions.class)) - .build(), - Case.named("different Starlark option added to each side") - .setResult(ConfigurationComparer.Result.ALL_SHARED_FRAGMENTS_EQUAL) - .setBase(new OptionsBuilder()) - .setLeft(new OptionsBuilder().withStarlarkOption("//alpha")) - .setRight(new OptionsBuilder().withStarlarkOption("//bravo")) - .build(), - Case.named("different Starlark option removed from each side") - .setResult(ConfigurationComparer.Result.ALL_SHARED_FRAGMENTS_EQUAL) - .setBase( - new OptionsBuilder().withStarlarkOption("//alpha").withStarlarkOption("//bravo")) - .setLeft(new OptionsBuilder().withStarlarkOption("//alpha")) - .setRight(new OptionsBuilder().withStarlarkOption("//bravo")) - .build(), - Case.named("Starlark option added and different option removed on left") - .setResult(ConfigurationComparer.Result.ALL_SHARED_FRAGMENTS_EQUAL) - .setBase(new OptionsBuilder().withStarlarkOption("//bravo")) - .setLeft(new OptionsBuilder().withStarlarkOption("//alpha")) - .setRight(new OptionsBuilder().withStarlarkOption("//bravo")) - .build(), - Case.named( - "Starlark option added to right; " - + "other option changed on left and removed from right") - .setResult(ConfigurationComparer.Result.ALL_SHARED_FRAGMENTS_EQUAL) - .setBase(new OptionsBuilder().withStarlarkOption("//alpha", "base")) - .setLeft(new OptionsBuilder().withStarlarkOption("//alpha", "left")) - .setRight(new OptionsBuilder().withStarlarkOption("//bravo")) - .build(), - Case.named("Starlark option changed on each side, removed from the other") - .setResult(ConfigurationComparer.Result.ALL_SHARED_FRAGMENTS_EQUAL) - .setBase( - new OptionsBuilder() - .withStarlarkOption("//alpha", "base") - .withStarlarkOption("//bravo", "base")) - .setLeft(new OptionsBuilder().withStarlarkOption("//alpha", "left")) - .setRight(new OptionsBuilder().withStarlarkOption("//bravo", "right")) - .build(), - Case.named( - "Starlark option changed on left, removed from right; " - + "other option removed from left") - .setResult(ConfigurationComparer.Result.ALL_SHARED_FRAGMENTS_EQUAL) - .setBase( - new OptionsBuilder() - .withStarlarkOption("//alpha", "base") - .withStarlarkOption("//bravo")) - .setLeft(new OptionsBuilder().withStarlarkOption("//alpha", "left")) - .setRight(new OptionsBuilder().withStarlarkOption("//bravo")) - .build(), - Case.named("Starlark option removed from left, native option removed from right") - .setResult(ConfigurationComparer.Result.ALL_SHARED_FRAGMENTS_EQUAL) - .setBase( - new OptionsBuilder() - .withNativeFragment(AOptions.class) - .withStarlarkOption("//bravo")) - .setLeft(new OptionsBuilder().withNativeFragment(AOptions.class)) - .setRight(new OptionsBuilder().withStarlarkOption("//bravo")) - .build(), - Case.named("Starlark option added to left, native option added to right") - .setResult(ConfigurationComparer.Result.ALL_SHARED_FRAGMENTS_EQUAL) - .setBase(new OptionsBuilder()) - .setLeft(new OptionsBuilder().withStarlarkOption("//alpha")) - .setRight(new OptionsBuilder().withNativeFragment(BOptions.class)) - .build(), - Case.named("native fragment is unchanged in left, changes in right") - .setResult(ConfigurationComparer.Result.DIFFERENT) - .setBase(new OptionsBuilder().withNativeFragment(AOptions.class, "--alpha=base")) - .setLeft(new OptionsBuilder().withNativeFragment(AOptions.class, "--alpha=base")) - .setRight(new OptionsBuilder().withNativeFragment(AOptions.class, "--alpha=right")) - .build(), - Case.named("native fragment is changed to different values") - .setResult(ConfigurationComparer.Result.DIFFERENT) - .setBase(new OptionsBuilder().withNativeFragment(AOptions.class, "--alpha=base")) - .setLeft(new OptionsBuilder().withNativeFragment(AOptions.class, "--alpha=left")) - .setRight(new OptionsBuilder().withNativeFragment(AOptions.class, "--alpha=right")) - .build(), - Case.named("native fragment is added with different values") - .setResult(ConfigurationComparer.Result.DIFFERENT) - .setBase(new OptionsBuilder()) - .setLeft(new OptionsBuilder().withNativeFragment(AOptions.class, "--alpha=left")) - .setRight(new OptionsBuilder().withNativeFragment(AOptions.class, "--alpha=right")) - .build(), - Case.named("Starlark option is unchanged in left, changes in right") - .setResult(ConfigurationComparer.Result.DIFFERENT) - .setBase(new OptionsBuilder().withStarlarkOption("//alpha", "base")) - .setLeft(new OptionsBuilder().withStarlarkOption("//alpha", "base")) - .setRight(new OptionsBuilder().withStarlarkOption("//alpha", "right")) - .build(), - Case.named("Starlark option is changed to different values") - .setResult(ConfigurationComparer.Result.DIFFERENT) - .setBase(new OptionsBuilder().withStarlarkOption("//alpha", "base")) - .setLeft(new OptionsBuilder().withStarlarkOption("//alpha", "left")) - .setRight(new OptionsBuilder().withStarlarkOption("//alpha", "right")) - .build(), - Case.named("Starlark option is added with different values") - .setResult(ConfigurationComparer.Result.DIFFERENT) - .setBase(new OptionsBuilder()) - .setLeft(new OptionsBuilder().withStarlarkOption("//alpha", "left")) - .setRight(new OptionsBuilder().withStarlarkOption("//alpha", "right")) - .build()); - } - - private final Case testCase; - - public NormalCases(Case testCase) { - this.testCase = testCase; - } - - @Test - public void compareLeftToRight() throws Exception { - OptionsDiffForReconstruction diffLeft = - BuildOptions.diffForReconstruction(testCase.getBase(), testCase.getLeft()); - OptionsDiffForReconstruction diffRight = - BuildOptions.diffForReconstruction(testCase.getBase(), testCase.getRight()); - - assertThat(OptionsDiffForReconstruction.compareFragments(diffLeft, diffRight)) - .isEqualTo(testCase.getLeftToRightResult()); - } - - @Test - public void compareRightToLeft() throws Exception { - OptionsDiffForReconstruction diffLeft = - BuildOptions.diffForReconstruction(testCase.getBase(), testCase.getLeft()); - OptionsDiffForReconstruction diffRight = - BuildOptions.diffForReconstruction(testCase.getBase(), testCase.getRight()); - - assertThat(OptionsDiffForReconstruction.compareFragments(diffRight, diffLeft)) - .isEqualTo(testCase.getRightToLeftResult()); - } - } - - /** Test cases for compareFragments which produce errors. */ - @RunWith(JUnit4.class) - public static final class ExceptionalCases { - @Test - public void withDifferentBases_throwsError() throws Exception { - BuildOptions baseA = - new OptionsBuilder() - .withNativeFragment(AOptions.class, "--alpha=A") - .withNativeFragment(BOptions.class, "--bravo=base") - .build(); - BuildOptions newA = - new OptionsBuilder() - .withNativeFragment(AOptions.class, "--alpha=A") - .withNativeFragment(BOptions.class, "--bravo=new") - .build(); - BuildOptions baseB = - new OptionsBuilder() - .withNativeFragment(AOptions.class, "--alpha=B") - .withNativeFragment(BOptions.class, "--bravo=base") - .build(); - BuildOptions newB = - new OptionsBuilder() - .withNativeFragment(AOptions.class, "--alpha=B") - .withNativeFragment(BOptions.class, "--bravo=old") - .build(); - - OptionsDiffForReconstruction diffA = BuildOptions.diffForReconstruction(baseA, newA); - OptionsDiffForReconstruction diffB = BuildOptions.diffForReconstruction(baseB, newB); - - IllegalArgumentException forwardException = - assertThrows( - IllegalArgumentException.class, - () -> OptionsDiffForReconstruction.compareFragments(diffA, diffB)); - assertThat(forwardException).hasMessageThat().contains("diffs with different bases"); - - IllegalArgumentException reverseException = - assertThrows( - IllegalArgumentException.class, - () -> OptionsDiffForReconstruction.compareFragments(diffB, diffA)); - assertThat(reverseException).hasMessageThat().contains("diffs with different bases"); - } - } -}
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java index 3f8b306..293a0b7 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java
@@ -18,7 +18,6 @@ import static com.google.common.collect.ImmutableMap.toImmutableMap; import static com.google.common.collect.Streams.stream; -import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Functions; import com.google.common.base.Preconditions; import com.google.common.collect.Collections2; @@ -32,7 +31,6 @@ import com.google.common.collect.Maps; import com.google.common.collect.Multimap; import com.google.common.collect.Sets; -import com.google.common.collect.Streams; import com.google.common.eventbus.EventBus; import com.google.devtools.build.lib.actions.ActionLookupKey; import com.google.devtools.build.lib.actions.ArtifactFactory; @@ -160,7 +158,6 @@ this.skyframeBuildView = skyframeExecutor.getSkyframeBuildView(); } - @VisibleForTesting public Set<ActionLookupKey> getSkyframeEvaluatedActionLookupKeyCountForTesting() { Set<ActionLookupKey> actionLookupKeys = populateActionLookupKeyMapAndGetDiff(); Preconditions.checkState( @@ -188,7 +185,6 @@ /** * Returns whether the given configured target has errors. */ - @VisibleForTesting public boolean hasErrors(ConfiguredTarget configuredTarget) { return configuredTarget == null; } @@ -223,8 +219,7 @@ eventBus); } - /** Sets the configurations. Not thread-safe. DO NOT CALL except from tests! */ - @VisibleForTesting + /** Sets the configurations. Not thread-safe. */ public void setConfigurationsForTesting( EventHandler eventHandler, BuildConfigurationCollection configurations) { skyframeBuildView.setConfigurations( @@ -242,7 +237,6 @@ * the fragments needed by the fragment and its transitive closure. Else unconditionally includes * all fragments. */ - @VisibleForTesting public BuildConfiguration getConfigurationForTesting( Target target, BuildConfiguration config, ExtendedEventHandler eventHandler) throws InvalidConfigurationException, InterruptedException { @@ -262,22 +256,19 @@ * Sets the possible artifact roots in the artifact factory. This allows the factory to resolve * paths with unknown roots to artifacts. */ - @VisibleForTesting // for BuildViewTestCase public void setArtifactRoots(PackageRoots packageRoots) { getArtifactFactory().setPackageRoots(packageRoots.getPackageRootLookup()); } - @VisibleForTesting // TODO(janakr): pass the configuration in as a parameter here. public Collection<ConfiguredTarget> getDirectPrerequisitesForTesting( ExtendedEventHandler eventHandler, ConfiguredTarget ct, BuildConfigurationCollection configurations) - throws DependencyResolver.Failure, InvalidConfigurationException, InterruptedException, + throws DependencyResolver.Failure, InvalidConfigurationException, InconsistentAspectOrderException, StarlarkTransition.TransitionException { return Collections2.transform( - getConfiguredTargetAndDataDirectPrerequisitesForTesting( - eventHandler, ct, ct.getConfigurationKey(), configurations), + getConfiguredTargetAndDataDirectPrerequisitesForTesting(eventHandler, ct, configurations), ConfiguredTargetAndData::getConfiguredTarget); } @@ -285,9 +276,8 @@ getConfiguredTargetAndDataDirectPrerequisitesForTesting( ExtendedEventHandler eventHandler, ConfiguredTarget ct, - BuildConfigurationValue.Key configuration, BuildConfigurationCollection configurations) - throws DependencyResolver.Failure, InvalidConfigurationException, InterruptedException, + throws DependencyResolver.Failure, InvalidConfigurationException, InconsistentAspectOrderException, StarlarkTransition.TransitionException { SkyframeExecutorWrappingWalkableGraph walkableGraph = @@ -313,23 +303,18 @@ walkableGraph.getDirectDeps( ConfiguredTargetKey.builder().setConfiguredTarget(ct).build()); - // Turn the keys back into ConfiguredTarget instances, possibly merging in aspects that - // were propagated from the original target. - Collection<ConfiguredTargetAndData> cts = - Streams.stream(directPrerequisites) - .filter(dep -> dep instanceof ConfiguredTargetKey) - .map(dep -> (ConfiguredTargetKey) dep) - .map(configuredTargetKey -> getConfiguredTarget(walkableGraph, configuredTargetKey)) - // For each configured target, add in any aspects from depNodeNames. - .map( - configuredTarget -> - mergeAspects( - walkableGraph, - configuredTarget, - findDependencyKey(dependencyKeys, configuredTarget))) - .collect(toImmutableList()); - - return cts; + // Turn the keys back into ConfiguredTarget instances, possibly merging in aspects that were + // propagated from the original target. + return stream(Iterables.filter(directPrerequisites, ConfiguredTargetKey.class)) + .map(configuredTargetKey -> getConfiguredTarget(walkableGraph, configuredTargetKey)) + // For each configured target, add in any aspects from depNodeNames. + .map( + configuredTarget -> + mergeAspects( + walkableGraph, + configuredTarget, + findDependencyKey(dependencyKeys, configuredTarget))) + .collect(toImmutableList()); } catch (InterruptedException e) { return ImmutableList.of(); } @@ -363,7 +348,7 @@ } @Nullable - private DependencyKey findDependencyKey( + private static DependencyKey findDependencyKey( Multimap<Label, DependencyKey> dependencyKeys, ConfiguredTargetAndData configuredTarget) { // TODO(blaze-configurability): Figure out how to map the ConfiguredTarget back to the correct // DependencyKey when there are more than one. @@ -371,7 +356,7 @@ } // Helper method to find the aspects needed for a target and merge them. - protected ConfiguredTargetAndData mergeAspects( + protected static ConfiguredTargetAndData mergeAspects( WalkableGraph graph, ConfiguredTargetAndData ctd, @Nullable DependencyKey dependencyKey) { if (dependencyKey == null || dependencyKey.getAspects().getUsedAspects().isEmpty()) { return ctd; @@ -400,7 +385,6 @@ } } - @VisibleForTesting public OrderedSetMultimap<DependencyKind, DependencyKey> getDirectPrerequisiteDependenciesForTesting( final ExtendedEventHandler eventHandler, @@ -410,7 +394,7 @@ throws DependencyResolver.Failure, InterruptedException, InconsistentAspectOrderException, StarlarkTransition.TransitionException, InvalidConfigurationException { - Target target = null; + Target target; try { target = skyframeExecutor.getPackageManager().getTarget(eventHandler, ct.getLabel()); } catch (NoSuchPackageException | NoSuchTargetException | InterruptedException e) { @@ -558,7 +542,6 @@ * * <p>Returns {@code null} if something goes wrong. */ - @VisibleForTesting public ConfiguredTarget getConfiguredTargetForTesting( ExtendedEventHandler eventHandler, Label label, BuildConfiguration config) throws StarlarkTransition.TransitionException, InvalidConfigurationException, @@ -571,7 +554,6 @@ return skyframeExecutor.getConfiguredTargetForTesting(eventHandler, label, config, transition); } - @VisibleForTesting ConfiguredTargetAndData getConfiguredTargetAndDataForTesting( ExtendedEventHandler eventHandler, Label label, BuildConfiguration config) throws StarlarkTransition.TransitionException, InvalidConfigurationException, @@ -588,7 +570,6 @@ /** * Returns a RuleContext which is the same as the original RuleContext of the target parameter. */ - @VisibleForTesting public RuleContext getRuleContextForTesting( ConfiguredTarget target, StoredEventHandler eventHandler, @@ -611,7 +592,6 @@ .setLabel(target.getLabel()) .setConfiguration(targetConfig) .build(), - /*isSystemEnv=*/ false, targetConfig.extendedSanityChecks(), targetConfig.allowAnalysisFailures(), eventHandler, @@ -624,7 +604,6 @@ * Creates and returns a rule context that is equivalent to the one that was used to create the * given configured target. */ - @VisibleForTesting public RuleContext getRuleContextForTesting( ExtendedEventHandler eventHandler, ConfiguredTarget configuredTarget, @@ -635,7 +614,7 @@ StarlarkTransition.TransitionException, InvalidExecGroupException { BuildConfiguration targetConfig = skyframeExecutor.getConfiguration(eventHandler, configuredTarget.getConfigurationKey()); - Target target = null; + Target target; try { target = skyframeExecutor.getPackageManager().getTarget(eventHandler, configuredTarget.getLabel()); @@ -740,7 +719,6 @@ } /** Clears the analysis cache as in --discard_analysis_cache. */ - @VisibleForTesting void clearAnalysisCache( Collection<ConfiguredTarget> topLevelTargets, ImmutableSet<AspectKey> topLevelAspects) { skyframeBuildView.clearAnalysisCache(topLevelTargets, topLevelAspects);
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java index b1da3f9..aac74b6 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
@@ -338,9 +338,9 @@ packageOptions, buildLanguageOptions, UUID.randomUUID(), - ImmutableMap.<String, String>of(), + ImmutableMap.of(), tsgm); - skyframeExecutor.setActionEnv(ImmutableMap.<String, String>of()); + skyframeExecutor.setActionEnv(ImmutableMap.of()); useConfiguration(); setUpSkyframe(); this.actionLogBufferPathGenerator = @@ -411,7 +411,7 @@ } protected Iterable<EnvironmentExtension> getEnvironmentExtensions() { - return ImmutableList.<EnvironmentExtension>of(); + return ImmutableList.of(); } protected StarlarkSemantics getStarlarkSemantics() { @@ -452,8 +452,7 @@ optionsParser.setStarlarkOptions(starlarkOptions); BuildOptions buildOptions = ruleClassProvider.createBuildOptions(optionsParser); - return skyframeExecutor.createConfigurations( - reporter, buildOptions, ImmutableSet.<String>of(), false); + return skyframeExecutor.createConfigurations(reporter, buildOptions, ImmutableSet.of(), false); } protected Target getTarget(String label) @@ -559,8 +558,6 @@ * Invalidates all existing packages. Optionally invalidates configurations too. * * <p>Tests should invalidate both unless they have specific reason not to. - * - * @throws InterruptedException */ protected void invalidatePackages(boolean alsoConfigs) throws InterruptedException { skyframeExecutor.invalidateFilesUnderPathForTesting( @@ -605,7 +602,6 @@ * @param starlarkOptions map of Starlark-defined options where the keys are option names (in the * form of label-like strings) and the values are option values * @param args native option name/pair descriptions in command line form (e.g. "--cpu=k8") - * @throws IllegalArgumentException */ protected void useConfiguration(ImmutableMap<String, Object> starlarkOptions, String... args) throws Exception { @@ -624,11 +620,10 @@ } /** - * Creates BuildView using current hostConfig/targetConfig values. - * Ensures that hostConfig is either identical to the targetConfig or has - * 'host' short name. + * Creates BuildView using current hostConfig/targetConfig values. Ensures that hostConfig is + * either identical to the targetConfig or has 'host' short name. */ - protected final void createBuildView() throws Exception { + protected final void createBuildView() { Preconditions.checkNotNull(masterConfig); Preconditions.checkState(getHostConfiguration().equals(getTargetConfiguration()) || getHostConfiguration().isHostConfiguration(), @@ -664,7 +659,6 @@ return null; } }, - /*isSystemEnv=*/ true, /*extendedSanityChecks=*/ false, /*allowAnalysisFailures=*/ false, reporter, @@ -680,7 +674,7 @@ */ protected final Collection<ConfiguredTarget> getDirectPrerequisites(ConfiguredTarget target) throws TransitionException, InvalidConfigurationException, InconsistentAspectOrderException, - Failure, InterruptedException { + Failure { return view.getDirectPrerequisitesForTesting(reporter, target, masterConfig); } @@ -703,7 +697,6 @@ view.getConfiguredTargetAndDataDirectPrerequisitesForTesting( reporter, ctad.getConfiguredTarget(), - ctad.getConfiguredTarget().getConfigurationKey(), masterConfig)) { if (candidate.getConfiguredTarget().getLabel().equals(candidateLabel)) { return candidate; @@ -733,10 +726,10 @@ * comparison. * * <p>Generally, this means they share the same checksum, which is computed by iterating over all - * the individual @Option annotated values contained within the {@link FragmentOption} classes + * the individual @Option annotated values contained within the {@link FragmentOptions} classes * contained within the {@link BuildOptions} inside the given configurations. */ - protected void assertConfigurationsEqual( + protected static void assertConfigurationsEqual( BuildConfiguration config1, BuildConfiguration config2, Set<Class<? extends FragmentOptions>> excludeFragmentOptions) { @@ -747,8 +740,9 @@ .isEqualTo(trimConfiguration(config1.cloneOptions(), excludeFragmentOptions)); } - protected void assertConfigurationsEqual(BuildConfiguration config1, BuildConfiguration config2) { - assertConfigurationsEqual(config1, config2, ImmutableSet.of()); + protected static void assertConfigurationsEqual( + BuildConfiguration config1, BuildConfiguration config2) { + assertConfigurationsEqual(config1, config2, /*excludeFragmentOptions=*/ ImmutableSet.of()); } /** @@ -1099,11 +1093,9 @@ * @param ruleName the name of the rule. * @param lines the text of the rule. * @return the configured target instance for the created rule. - * @throws IOException - * @throws Exception */ protected ConfiguredTarget scratchConfiguredTarget( - String packageName, String ruleName, String... lines) throws IOException, Exception { + String packageName, String ruleName, String... lines) throws Exception { return scratchConfiguredTarget(packageName, ruleName, targetConfig, lines); } @@ -1115,12 +1107,10 @@ * @param config the configuration to use to construct the configured rule. * @param lines the text of the rule. * @return the configured target instance for the created rule. - * @throws IOException - * @throws Exception */ protected ConfiguredTarget scratchConfiguredTarget( String packageName, String ruleName, BuildConfiguration config, String... lines) - throws IOException, Exception { + throws Exception { ConfiguredTargetAndData ctad = scratchConfiguredTargetAndData(packageName, ruleName, config, lines); return ctad == null ? null : ctad.getConfiguredTarget(); @@ -1133,7 +1123,6 @@ * @param rulename the name of the rule. * @param lines the text of the rule. * @return the configured tatarget and target instance for the created rule. - * @throws Exception */ protected ConfiguredTargetAndData scratchConfiguredTargetAndData( String packageName, String rulename, String... lines) throws Exception { @@ -1148,8 +1137,6 @@ * @param config the configuration to use to construct the configured rule. * @param lines the text of the rule. * @return the ConfiguredTargetAndData instance for the created rule. - * @throws IOException - * @throws Exception */ protected ConfiguredTargetAndData scratchConfiguredTargetAndData( String packageName, String ruleName, BuildConfiguration config, String... lines) @@ -1165,8 +1152,6 @@ * @param ruleName the name of the rule. * @param lines the text of the rule. * @return the rule instance for the created rule. - * @throws IOException - * @throws Exception */ protected Rule scratchRule(String packageName, String ruleName, String... lines) throws Exception { @@ -1427,7 +1412,7 @@ } /** Returns the input {@link Artifact}s to the given {@link Action} with the given exec paths. */ - protected List<Artifact> getInputs(Action owner, Collection<String> execPaths) { + protected static List<Artifact> getInputs(Action owner, Collection<String> execPaths) { Set<String> expectedPaths = new HashSet<>(execPaths); List<Artifact> result = new ArrayList<>(); for (Artifact output : owner.getInputs().toList()) { @@ -1664,11 +1649,11 @@ : outputPath; } - protected String fileName(Artifact artifact) { + protected static String fileName(Artifact artifact) { return artifact.getExecPathString(); } - protected String fileName(FileConfiguredTarget target) { + protected static String fileName(FileConfiguredTarget target) { return fileName(target.getArtifact()); } @@ -1722,8 +1707,9 @@ "'" + packageName + ":a.foo' does not produce any " + descriptionPlural); } - protected void assertSrcsValidity(String ruleType, String targetName, boolean expectedError, - String... expectedMessages) throws Exception{ + protected void assertSrcsValidity( + String ruleType, String targetName, boolean expectedError, String... expectedMessages) + throws Exception { ConfiguredTarget target = getConfiguredTarget(targetName); if (expectedError) { assertThat(view.hasErrors(target)).isTrue(); @@ -1757,10 +1743,10 @@ .build(); } - protected static List<String> actionInputsToPaths(NestedSet<? extends ActionInput> actionInputs) { + protected static ImmutableList<String> actionInputsToPaths( + NestedSet<? extends ActionInput> actionInputs) { return ImmutableList.copyOf( - Iterables.transform( - actionInputs.toList(), (actionInput) -> actionInput.getExecPathString())); + Lists.transform(actionInputs.toList(), ActionInput::getExecPathString)); } /** @@ -1774,20 +1760,19 @@ } /** - * Utility method for asserting that a list contains the elements of a - * sublist. This is useful for checking that a list of arguments contains a - * particular set of arguments. + * Utility method for asserting that a list contains the elements of a sublist. This is useful for + * checking that a list of arguments contains a particular set of arguments. */ - protected void assertContainsSublist(List<String> list, List<String> sublist) { + protected static void assertContainsSublist(List<String> list, List<String> sublist) { assertContainsSublist(null, list, sublist); } /** - * Utility method for asserting that a list contains the elements of a - * sublist. This is useful for checking that a list of arguments contains a - * particular set of arguments. + * Utility method for asserting that a list contains the elements of a sublist. This is useful for + * checking that a list of arguments contains a particular set of arguments. */ - protected void assertContainsSublist(String message, List<String> list, List<String> sublist) { + protected static void assertContainsSublist( + String message, List<String> list, List<String> sublist) { if (Collections.indexOfSubList(list, sublist) == -1) { fail((message == null ? "" : (message + ' ')) + "expected: <" + list + "> to contain sublist: <" + sublist + ">"); @@ -1795,10 +1780,10 @@ } protected void assertContainsSelfEdgeEvent(String label) { - assertContainsEvent(Pattern.compile(label + " \\([a-f0-9]+\\) \\[self-edge\\]")); + assertContainsEvent(Pattern.compile(label + " \\([a-f0-9]+\\) \\[self-edge]")); } - protected NestedSet<Artifact> collectRunfiles(ConfiguredTarget target) { + protected static NestedSet<Artifact> collectRunfiles(ConfiguredTarget target) { RunfilesProvider runfilesProvider = target.getProvider(RunfilesProvider.class); if (runfilesProvider != null) { return runfilesProvider.getDefaultRunfiles().getAllArtifacts(); @@ -1807,7 +1792,7 @@ } } - protected NestedSet<Artifact> getFilesToBuild(TransitiveInfoCollection target) { + protected static NestedSet<Artifact> getFilesToBuild(TransitiveInfoCollection target) { return target.getProvider(FileProvider.class).getFilesToBuild(); } @@ -1851,15 +1836,16 @@ return ImmutableList.copyOf(result); } - protected NestedSet<Artifact> getOutputGroup( + protected static NestedSet<Artifact> getOutputGroup( TransitiveInfoCollection target, String outputGroup) { OutputGroupInfo provider = OutputGroupInfo.get(target); return provider == null - ? NestedSetBuilder.<Artifact>emptySet(Order.STABLE_ORDER) + ? NestedSetBuilder.emptySet(Order.STABLE_ORDER) : provider.getOutputGroup(outputGroup); } - protected NestedSet<Artifact.DerivedArtifact> getExtraActionArtifacts(ConfiguredTarget target) { + protected static NestedSet<Artifact.DerivedArtifact> getExtraActionArtifacts( + ConfiguredTarget target) { return target.getProvider(ExtraActionArtifactsProvider.class).getExtraActionArtifacts(); } @@ -1867,15 +1853,15 @@ return getConfiguredTarget(label).getProvider(FilesToRunProvider.class).getExecutable(); } - protected Artifact getExecutable(TransitiveInfoCollection target) { + protected static Artifact getExecutable(TransitiveInfoCollection target) { return target.getProvider(FilesToRunProvider.class).getExecutable(); } - protected NestedSet<Artifact> getFilesToRun(TransitiveInfoCollection target) { + protected static NestedSet<Artifact> getFilesToRun(TransitiveInfoCollection target) { return target.getProvider(FilesToRunProvider.class).getFilesToRun(); } - protected NestedSet<Artifact> getFilesToRun(Label label) throws Exception { + protected NestedSet<Artifact> getFilesToRun(Label label) { return getConfiguredTarget(label, targetConfig) .getProvider(FilesToRunProvider.class).getFilesToRun(); } @@ -1888,7 +1874,7 @@ return getConfiguredTarget(label).getProvider(FilesToRunProvider.class).getRunfilesSupport(); } - protected RunfilesSupport getRunfilesSupport(TransitiveInfoCollection target) { + protected static RunfilesSupport getRunfilesSupport(TransitiveInfoCollection target) { return target.getProvider(FilesToRunProvider.class).getRunfilesSupport(); } @@ -1984,7 +1970,7 @@ boolean doAnalysis, EventBus eventBus) throws Exception { return update( - targets, ImmutableList.<String>of(), keepGoing, loadingPhaseThreads, doAnalysis, eventBus); + targets, ImmutableList.of(), keepGoing, loadingPhaseThreads, doAnalysis, eventBus); } protected AnalysisResult update( @@ -2058,51 +2044,53 @@ return result; } - protected String getErrorMsgNoGoodFiles(String attrName, String ruleType, String ruleName, - String depRuleName) { + protected static String getErrorMsgNoGoodFiles( + String attrName, String ruleType, String ruleName, String depRuleName) { return "in " + attrName + " attribute of " + ruleType + " rule " + ruleName + ": '" + depRuleName + "' does not produce any " + ruleType + " " + attrName + " files"; } - protected String getErrorMsgMisplacedFiles(String attrName, String ruleType, String ruleName, - String fileName) { + protected static String getErrorMsgMisplacedFiles( + String attrName, String ruleType, String ruleName, String fileName) { return "in " + attrName + " attribute of " + ruleType + " rule " + ruleName + ": source file '" + fileName + "' is misplaced here"; } - protected String getErrorNonExistingTarget(String attrName, String ruleType, String ruleName, - String targetName) { + protected static String getErrorNonExistingTarget( + String attrName, String ruleType, String ruleName, String targetName) { return "in " + attrName + " attribute of " + ruleType + " rule " + ruleName + ": target '" + targetName + "' does not exist"; } - protected String getErrorNonExistingRule(String attrName, String ruleType, String ruleName, - String targetName) { + protected static String getErrorNonExistingRule( + String attrName, String ruleType, String ruleName, String targetName) { return "in " + attrName + " attribute of " + ruleType + " rule " + ruleName + ": rule '" + targetName + "' does not exist"; } - protected String getErrorMsgMisplacedRules(String attrName, String ruleType, String ruleName, - String depRuleType, String depRuleName) { + protected static String getErrorMsgMisplacedRules( + String attrName, String ruleType, String ruleName, String depRuleType, String depRuleName) { return "in " + attrName + " attribute of " + ruleType + " rule " + ruleName + ": " + depRuleType + " rule '" + depRuleName + "' is misplaced here"; } - protected String getErrorMsgNonEmptyList(String attrName, String ruleType, String ruleName) { + protected static String getErrorMsgNonEmptyList( + String attrName, String ruleType, String ruleName) { return "in " + attrName + " attribute of " + ruleType + " rule " + ruleName + ": attribute " + "must be non empty"; } - protected String getErrorMsgMandatoryMissing(String attrName, String ruleType) { + protected static String getErrorMsgMandatoryMissing(String attrName, String ruleType) { return "missing value for mandatory attribute '" + attrName + "' in '" + ruleType + "' rule"; } - protected String getErrorMsgWrongAttributeValue(String value, String... expected) { + protected static String getErrorMsgWrongAttributeValue(String value, String... expected) { return String.format("has to be one of %s instead of '%s'", StringUtil.joinEnglishList(ImmutableSet.copyOf(expected), "or", "'"), value); } - protected String getErrorMsgMandatoryProviderMissing(String offendingRule, String providerName) { + protected static String getErrorMsgMandatoryProviderMissing( + String offendingRule, String providerName) { return String.format("'%s' does not have mandatory providers: '%s'", offendingRule, providerName); } @@ -2122,7 +2110,7 @@ String packageName, String... lines) throws Exception { eventCollector.clear(); reporter.removeHandler(failFastHandler); - scratch.file("" + packageName + "/BUILD", lines); + scratch.file(packageName + "/BUILD", lines); return getPackageManager() .getPackage(reporter, PackageIdentifier.createInMainRepo(packageName)); } @@ -2193,7 +2181,7 @@ } @Override - public ImmutableMap<String, Object> getStarlarkDefinedBuiltins() throws InterruptedException { + public ImmutableMap<String, Object> getStarlarkDefinedBuiltins() { throw new UnsupportedOperationException(); } @@ -2427,7 +2415,7 @@ /** Creates instances of {@link ActionExecutionContext} consistent with test case. */ public class ActionExecutionContextBuilder { private MetadataProvider actionInputFileCache = null; - private TreeMap<String, String> clientEnv = new TreeMap<>(); + private final TreeMap<String, String> clientEnv = new TreeMap<>(); private ArtifactExpander artifactExpander = null; private Executor executor = new DummyExecutor(fileSystem, getExecRoot());
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/CompileOnlyTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/CompileOnlyTestCase.java index a3c196d..84f760c 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/CompileOnlyTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/CompileOnlyTestCase.java
@@ -22,7 +22,7 @@ */ public abstract class CompileOnlyTestCase extends BuildViewTestCase { - protected Artifact getArtifactByExecPathSuffix(ConfiguredTarget target, String path) { + protected static Artifact getArtifactByExecPathSuffix(ConfiguredTarget target, String path) { for (Artifact artifact : getOutputGroup(target, OutputGroupInfo.FILES_TO_COMPILE).toList()) { if (artifact.getExecPathString().endsWith(path)) { return artifact;
diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/ResourceTestBase.java b/src/test/java/com/google/devtools/build/lib/rules/android/ResourceTestBase.java index 2fe0e5d..d81d1fe 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/android/ResourceTestBase.java +++ b/src/test/java/com/google/devtools/build/lib/rules/android/ResourceTestBase.java
@@ -210,10 +210,6 @@ return getArtifact(RESOURCE_ROOT, pathString); } - Artifact getOutput(String pathString) { - return getArtifact("outputs", pathString); - } - private Artifact getArtifact(String subdir, String pathString) { Path path = fileSystem.getPath("/" + subdir + "/" + pathString); return new Artifact.SourceArtifact( @@ -244,7 +240,6 @@ .setLabel(dummyTarget.getLabel()) .setConfiguration(targetConfig) .build(), - /*isSystemEnv=*/ false, targetConfig.extendedSanityChecks(), targetConfig.allowAnalysisFailures(), eventHandler, @@ -262,7 +257,7 @@ /** * Assets that the action used to generate the given outputs has the expected inputs and outputs. */ - void assertActionArtifacts( + static void assertActionArtifacts( RuleContext ruleContext, ImmutableList<Artifact> inputs, ImmutableList<Artifact> outputs) { // Actions must have at least one output assertThat(outputs).isNotEmpty();
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/StarlarkCcCommonTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/StarlarkCcCommonTest.java index 173b3ea..194998b 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/StarlarkCcCommonTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/StarlarkCcCommonTest.java
@@ -1479,10 +1479,7 @@ assertThat(userLinkFlags.getImmutableList()) .containsExactly("-la", "-lc2", "-DEP2_LINKOPT", "-lc1", "-lc2", "-DEP1_LINKOPT"); Depset additionalInputs = info.getValue("additional_inputs", Depset.class); - assertThat( - additionalInputs.toList(Artifact.class).stream() - .map(x -> x.getFilename()) - .collect(ImmutableList.toImmutableList())) + assertThat(additionalInputs.toList(Artifact.class).stream().map(Artifact::getFilename)) .containsExactly("b.lds", "d.lds"); Collection<LibraryToLink> librariesToLink = info.getValue("libraries_to_link", Depset.class).toList(LibraryToLink.class); @@ -4661,11 +4658,11 @@ (CcToolchainConfigInfo) target.get(CcToolchainConfigInfo.PROVIDER.getKey()); ImmutableSet<String> featureNames = ccToolchainConfigInfo.getFeatures().stream() - .map(feature -> feature.getName()) + .map(Feature::getName) .collect(ImmutableSet.toImmutableSet()); ImmutableSet<String> actionConfigNames = ccToolchainConfigInfo.getActionConfigs().stream() - .map(actionConfig -> actionConfig.getActionName()) + .map(ActionConfig::getActionName) .collect(ImmutableSet.toImmutableSet()); assertThat(featureNames).containsExactly("no_legacy_features", "custom_feature"); assertThat(actionConfigNames).containsExactly("custom_action"); @@ -4730,11 +4727,11 @@ (CcToolchainConfigInfo) target.get(CcToolchainConfigInfo.PROVIDER.getKey()); ImmutableList<String> featureNames = ccToolchainConfigInfo.getFeatures().stream() - .map(feature -> feature.getName()) + .map(Feature::getName) .collect(ImmutableList.toImmutableList()); ImmutableSet<String> actionConfigNames = ccToolchainConfigInfo.getActionConfigs().stream() - .map(actionConfig -> actionConfig.getActionName()) + .map(ActionConfig::getActionName) .collect(ImmutableSet.toImmutableSet()); // fdo_optimize should not be re-added to the list of features by legacy behavior assertThat(featureNames).containsNoDuplicates(); @@ -4882,7 +4879,7 @@ assertThat(toolchain.getCcTargetOs()).isEqualTo("os"); assertThat( toolchain.getFeatureList().stream() - .map(feature -> feature.getName()) + .map(CToolchain.Feature::getName) .collect(ImmutableList.toImmutableList())) .containsAtLeast("featureone", "sysroot") .inOrder(); @@ -4925,7 +4922,7 @@ assertThat( toolchain.getActionConfigList().stream() - .map(actionConfig -> actionConfig.getActionName()) + .map(CToolchain.ActionConfig::getActionName) .collect(ImmutableList.toImmutableList())) .containsAtLeast("action_one", "action_two") .inOrder(); @@ -5318,7 +5315,7 @@ ConfiguredTarget target = getConfiguredTarget("//foo:starlark_lib"); assertThat( getFilesToBuild(target).toList().stream() - .map(x -> x.getFilename()) + .map(Artifact::getFilename) .collect(ImmutableList.toImmutableList())) .contains("libstarlark_lib.a"); } @@ -5330,12 +5327,12 @@ ConfiguredTarget target = getConfiguredTarget("//foo:starlark_lib"); assertThat( getFilesToBuild(target).toList().stream() - .map(x -> x.getFilename()) + .map(Artifact::getFilename) .collect(ImmutableList.toImmutableList())) .doesNotContain("libstarlark_lib.a"); } - private List<String> getFilenamesToBuild(ConfiguredTarget target) { + private static ImmutableList<String> getFilenamesToBuild(ConfiguredTarget target) { return getFilesToBuild(target).toList().stream() .map(Artifact::getFilename) .collect(ImmutableList.toImmutableList());
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/BUILD b/src/test/java/com/google/devtools/build/lib/skyframe/BUILD index fef712a..7739ef0 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/test/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -8,7 +8,7 @@ filegroup( name = "srcs", testonly = 0, - srcs = glob(["**"]) + ["//src/test/java/com/google/devtools/build/lib/skyframe/trimming:srcs"], + srcs = glob(["**"]), visibility = ["//src/test/java/com/google/devtools/build/lib:__pkg__"], )
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtilTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtilTest.java index 1c588ac..085ba2e 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtilTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtilTest.java
@@ -158,7 +158,7 @@ } } - EvaluationResult<GetPlatformInfoValue> getPlatformInfo(GetPlatformInfoKey key) + private EvaluationResult<GetPlatformInfoValue> getPlatformInfo(GetPlatformInfoKey key) throws InterruptedException { try { // Must re-enable analysis for Skyframe functions that create configured targets. @@ -188,7 +188,7 @@ GetPlatformInfoKey key = (GetPlatformInfoKey) skyKey; try { Map<ConfiguredTargetKey, PlatformInfo> platforms = - PlatformLookupUtil.getPlatformInfo(key.platformKeys(), env, false); + PlatformLookupUtil.getPlatformInfo(key.platformKeys(), env); if (env.valuesMissing()) { return null; } @@ -205,8 +205,8 @@ } } - private static class GetPlatformInfoFunctionException extends SkyFunctionException { - public GetPlatformInfoFunctionException(InvalidPlatformException e) { + private static final class GetPlatformInfoFunctionException extends SkyFunctionException { + GetPlatformInfoFunctionException(InvalidPlatformException e) { super(e, Transience.PERSISTENT); } }
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainTypeLookupUtilTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainTypeLookupUtilTest.java index f85600c..47999e7 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainTypeLookupUtilTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainTypeLookupUtilTest.java
@@ -182,8 +182,8 @@ } } - EvaluationResult<GetToolchainTypeInfoValue> getToolchainTypeInfo(GetToolchainTypeInfoKey key) - throws InterruptedException { + private EvaluationResult<GetToolchainTypeInfoValue> getToolchainTypeInfo( + GetToolchainTypeInfoKey key) throws InterruptedException { try { // Must re-enable analysis for Skyframe functions that create configured targets. skyframeExecutor.getSkyframeBuildView().enableAnalysis(true); @@ -212,7 +212,7 @@ GetToolchainTypeInfoKey key = (GetToolchainTypeInfoKey) skyKey; try { Map<Label, ToolchainTypeInfo> toolchainTypes = - ToolchainTypeLookupUtil.resolveToolchainTypes(env, key.toolchainTypeKeys(), false); + ToolchainTypeLookupUtil.resolveToolchainTypes(env, key.toolchainTypeKeys()); if (env.valuesMissing()) { return null; } @@ -229,8 +229,8 @@ } } - private static class GetToolchainTypeInfoFunctionException extends SkyFunctionException { - public GetToolchainTypeInfoFunctionException(InvalidToolchainTypeException e) { + private static final class GetToolchainTypeInfoFunctionException extends SkyFunctionException { + GetToolchainTypeInfoFunctionException(InvalidToolchainTypeException e) { super(e, Transience.PERSISTENT); } }
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/trimming/BUILD b/src/test/java/com/google/devtools/build/lib/skyframe/trimming/BUILD deleted file mode 100644 index 9636055..0000000 --- a/src/test/java/com/google/devtools/build/lib/skyframe/trimming/BUILD +++ /dev/null
@@ -1,85 +0,0 @@ -# Tests for the trimming support classes. - -load("@rules_java//java:defs.bzl", "java_library", "java_test") - -package( - default_testonly = 1, -) - -filegroup( - name = "srcs", - testonly = 0, - srcs = glob(["**"]), - visibility = ["//src:__subpackages__"], -) - -java_library( - name = "trimmable_test_fragments", - srcs = ["TrimmableTestConfigurationFragments.java"], - visibility = ["//src/test:__subpackages__"], - deps = [ - "//src/main/java/com/google/devtools/build/lib/actions", - "//src/main/java/com/google/devtools/build/lib/actions:artifacts", - "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster", - "//src/main/java/com/google/devtools/build/lib/analysis:config/build_options", - "//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_options", - "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_provider", - "//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/patch_transition", - "//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/transition_factory", - "//src/main/java/com/google/devtools/build/lib/analysis:configured_target", - "//src/main/java/com/google/devtools/build/lib/analysis:file_provider", - "//src/main/java/com/google/devtools/build/lib/analysis:platform_configuration", - "//src/main/java/com/google/devtools/build/lib/analysis:platform_options", - "//src/main/java/com/google/devtools/build/lib/analysis:resolved_toolchain_context", - "//src/main/java/com/google/devtools/build/lib/analysis:test/test_configuration", - "//src/main/java/com/google/devtools/build/lib/analysis/platform", - "//src/main/java/com/google/devtools/build/lib/cmdline", - "//src/main/java/com/google/devtools/build/lib/collect/nestedset", - "//src/main/java/com/google/devtools/build/lib/events", - "//src/main/java/com/google/devtools/build/lib/packages", - "//src/main/java/com/google/devtools/build/lib/rules:core_rules", - "//src/main/java/com/google/devtools/build/lib/rules:repository/bind_rule", - "//src/main/java/com/google/devtools/build/lib/rules:repository/workspace_base_rule", - "//src/main/java/com/google/devtools/build/lib/rules:toolchain_type", - "//src/main/java/com/google/devtools/build/lib/util:filetype", - "//src/main/java/com/google/devtools/common/options", - "//src/main/java/net/starlark/java/annot", - "//src/main/java/net/starlark/java/eval", - "//src/main/java/net/starlark/java/syntax", - "//src/test/java/com/google/devtools/build/lib/analysis/util", - "//src/test/java/com/google/devtools/build/lib/testutil", - "//third_party:guava", - "//third_party:jsr305", - ], -) - -java_library( - name = "test_key", - srcs = ["TestKey.java"], - deps = [ - "//src/main/java/com/google/devtools/build/lib/skyframe/trimming:trimmed_configuration_cache", - "//third_party:auto_value", - "//third_party:guava", - ], -) - -java_test( - name = "TrimmedConfigurationCacheTests", - size = "small", - srcs = [ - "TestKeyTest.java", - "TrimmedConfigurationCacheTest.java", - ], - test_class = "com.google.devtools.build.lib.AllTests", - runtime_deps = ["//src/test/java/com/google/devtools/build/lib:test_runner"], - deps = [ - ":test_key", - "//src/main/java/com/google/devtools/build/lib/skyframe/trimming:trimmed_configuration_cache", - "//third_party:guava", - "//third_party:guava-testlib", - "//third_party:junit4", - "//third_party:truth", - ], -)
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/trimming/TestKey.java b/src/test/java/com/google/devtools/build/lib/skyframe/trimming/TestKey.java deleted file mode 100644 index 45316c1..0000000 --- a/src/test/java/com/google/devtools/build/lib/skyframe/trimming/TestKey.java +++ /dev/null
@@ -1,91 +0,0 @@ -// Copyright 2018 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.build.lib.skyframe.trimming; - -import com.google.auto.value.AutoValue; -import com.google.common.base.Preconditions; -import com.google.common.base.Splitter; -import com.google.common.base.Strings; -import com.google.common.collect.ImmutableMap; -import com.google.common.collect.Sets; -import java.util.Set; -import java.util.regex.Matcher; -import java.util.regex.Pattern; - -/** A simple key suitable for putting into a TrimmedConfigurationCache for testing it. */ -@AutoValue -abstract class TestKey { - abstract String descriptor(); - - abstract ImmutableMap<String, String> configuration(); - - static TestKey create(String descriptor, ImmutableMap<String, String> configuration) { - return new AutoValue_TestKey(descriptor, configuration); - } - - // Test keys look like <config: value, config: value> descriptor - private static final Pattern TEST_KEY_SHAPE = - Pattern.compile("<(?<config>[^>]*)>(?<descriptor>.+)"); - - static TestKey parse(String input) { - Matcher matcher = TEST_KEY_SHAPE.matcher(input.trim()); - Preconditions.checkArgument(matcher.matches()); - return create( - matcher.group("descriptor").trim(), parseConfiguration(matcher.group("config").trim())); - } - - static ImmutableMap<String, String> parseConfiguration(String input) { - if (Strings.isNullOrEmpty(input)) { - return ImmutableMap.of(); - } - return ImmutableMap.copyOf( - Splitter.on(',') - .trimResults() - .withKeyValueSeparator(Splitter.on(':').trimResults()) - .split(input)); - } - - static ConfigurationComparer.Result compareConfigurations( - ImmutableMap<String, String> left, - ImmutableMap<String, String> right) { - Set<String> sharedKeys = Sets.intersection(left.keySet(), right.keySet()); - for (String key : sharedKeys) { - if (!left.get(key).equals(right.get(key))) { - return ConfigurationComparer.Result.DIFFERENT; - } - } - boolean hasLeftOnlyKeys = !Sets.difference(left.keySet(), right.keySet()).isEmpty(); - boolean hasRightOnlyKeys = !Sets.difference(right.keySet(), left.keySet()).isEmpty(); - if (hasLeftOnlyKeys) { - if (hasRightOnlyKeys) { - return ConfigurationComparer.Result.ALL_SHARED_FRAGMENTS_EQUAL; - } else { - return ConfigurationComparer.Result.SUPERSET; - } - } else { - if (hasRightOnlyKeys) { - return ConfigurationComparer.Result.SUBSET; - } else { - return ConfigurationComparer.Result.EQUAL; - } - } - } - - /** Produces a cache suitable for storing TestKeys. */ - static TrimmedConfigurationCache<TestKey, String, ImmutableMap<String, String>> newCache() { - return new TrimmedConfigurationCache<>( - TestKey::descriptor, TestKey::configuration, TestKey::compareConfigurations); - } -}
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/trimming/TestKeyTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/trimming/TestKeyTest.java deleted file mode 100644 index ede356f..0000000 --- a/src/test/java/com/google/devtools/build/lib/skyframe/trimming/TestKeyTest.java +++ /dev/null
@@ -1,152 +0,0 @@ -// Copyright 2018 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.build.lib.skyframe.trimming; - -import static com.google.common.truth.Truth.assertThat; - -import com.google.common.collect.ImmutableMap; -import com.google.common.testing.EqualsTester; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; - -/** Tests for TestKey's parsing functionality. */ -@RunWith(JUnit4.class) -public final class TestKeyTest { - - @Test - public void parseEmptyConfig() throws Exception { - assertThat(TestKey.parse("<> //foo").configuration()).isEmpty(); - assertThat(TestKey.parse("< > //foo").configuration()).isEmpty(); - assertThat(TestKey.parse(" < > //foo").configuration()).isEmpty(); - } - - @Test - public void parseOneElementConfig() throws Exception { - assertThat(TestKey.parse("<A:1> //foo").configuration()).containsExactly("A", "1"); - assertThat(TestKey.parse("< A : 1 > //foo").configuration()).containsExactly("A", "1"); - assertThat(TestKey.parse(" < A : 1 > //foo").configuration()) - .containsExactly("A", "1"); - } - - @Test - public void parseMultiElementConfig() throws Exception { - assertThat(TestKey.parse("<A:1,B:6,C:90> //foo").configuration()) - .containsExactly("A", "1", "B", "6", "C", "90"); - assertThat(TestKey.parse("< A : 1 , B : 6 , C : 90 > //foo").configuration()) - .containsExactly("A", "1", "B", "6", "C", "90"); - assertThat(TestKey.parse(" < A : 1, B: 6, C :90 > //foo").configuration()) - .containsExactly("A", "1", "B", "6", "C", "90"); - } - - @Test - public void parseConfigWithSpaces() throws Exception { - assertThat(TestKey.parse("<An Item: A Value> //foo").configuration()) - .containsExactly("An Item", "A Value"); - } - - @Test - public void parseDescriptor() throws Exception { - assertThat(TestKey.parse("<>//foo").descriptor()).isEqualTo("//foo"); - assertThat(TestKey.parse("<> //foo ").descriptor()).isEqualTo("//foo"); - assertThat(TestKey.parse(" < A : 1, B: 6, C :90 > //foo ").descriptor()) - .isEqualTo("//foo"); - } - - @Test - public void parseDescriptorWithSpaces() throws Exception { - assertThat(TestKey.parse("<>//foo with space").descriptor()).isEqualTo("//foo with space"); - assertThat(TestKey.parse("<> //foo with space ").descriptor()) - .isEqualTo("//foo with space"); - } - - @Test - public void parseMissingConfiguration() throws Exception { - assertThat(TestKey.parse("<>//foo with space").descriptor()).isEqualTo("//foo with space"); - assertThat(TestKey.parse("<> //foo with space ").descriptor()) - .isEqualTo("//foo with space"); - } - - @Test - public void equality() throws Exception { - new EqualsTester() - .addEqualityGroup( - TestKey.parse("<>//foo"), - TestKey.parse(" < > //foo "), - TestKey.create("//foo", ImmutableMap.of())) - .addEqualityGroup( - TestKey.parse("<A:1>//foo"), - TestKey.parse(" < A : 1 > //foo "), - TestKey.create("//foo", ImmutableMap.of("A", "1"))) - .addEqualityGroup( - TestKey.parse("<>//bar"), - TestKey.parse(" < > //bar "), - TestKey.create("//bar", ImmutableMap.of())) - .addEqualityGroup( - TestKey.parse("<A:1>//bar"), - TestKey.parse(" < A : 1 > //bar "), - TestKey.create("//bar", ImmutableMap.of("A", "1"))) - .testEquals(); - } - - @Test - public void compareConfigurations_EqualCases() throws Exception { - assertThat(TestKey.compareConfigurations(ImmutableMap.of(), ImmutableMap.of())) - .isEqualTo(ConfigurationComparer.Result.EQUAL); - assertThat(TestKey.compareConfigurations(ImmutableMap.of("A", "1"), ImmutableMap.of("A", "1"))) - .isEqualTo(ConfigurationComparer.Result.EQUAL); - } - - @Test - public void compareConfigurations_SubsetCases() throws Exception { - assertThat(TestKey.compareConfigurations(ImmutableMap.of(), ImmutableMap.of("A", "1"))) - .isEqualTo(ConfigurationComparer.Result.SUBSET); - assertThat( - TestKey.compareConfigurations( - ImmutableMap.of("A", "1"), ImmutableMap.of("A", "1", "B", "2"))) - .isEqualTo(ConfigurationComparer.Result.SUBSET); - } - - @Test - public void compareConfigurations_SupersetCases() throws Exception { - assertThat(TestKey.compareConfigurations(ImmutableMap.of("A", "1"), ImmutableMap.of())) - .isEqualTo(ConfigurationComparer.Result.SUPERSET); - assertThat( - TestKey.compareConfigurations( - ImmutableMap.of("A", "1", "B", "2"), ImmutableMap.of("A", "1"))) - .isEqualTo(ConfigurationComparer.Result.SUPERSET); - } - - @Test - public void compareConfigurations_AllSharedFragmentsEqualCases() throws Exception { - assertThat(TestKey.compareConfigurations(ImmutableMap.of("A", "1"), ImmutableMap.of("B", "1"))) - .isEqualTo(ConfigurationComparer.Result.ALL_SHARED_FRAGMENTS_EQUAL); - assertThat( - TestKey.compareConfigurations( - ImmutableMap.of("A", "1", "B", "2"), ImmutableMap.of("A", "1", "C", "3"))) - .isEqualTo(ConfigurationComparer.Result.ALL_SHARED_FRAGMENTS_EQUAL); - } - - @Test - public void compareConfigurations_DifferentCases() throws Exception { - assertThat(TestKey.compareConfigurations(ImmutableMap.of("A", "1"), ImmutableMap.of("A", "2"))) - .isEqualTo(ConfigurationComparer.Result.DIFFERENT); - assertThat( - TestKey.compareConfigurations( - ImmutableMap.of("A", "1", "B", "2", "C", "3"), - ImmutableMap.of("A", "2", "B", "2", "D", "4"))) - .isEqualTo(ConfigurationComparer.Result.DIFFERENT); - } -}
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/trimming/TrimmableTestConfigurationFragments.java b/src/test/java/com/google/devtools/build/lib/skyframe/trimming/TrimmableTestConfigurationFragments.java deleted file mode 100644 index 8218e76..0000000 --- a/src/test/java/com/google/devtools/build/lib/skyframe/trimming/TrimmableTestConfigurationFragments.java +++ /dev/null
@@ -1,652 +0,0 @@ -// Copyright 2018 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.build.lib.skyframe.trimming; - -import static com.google.devtools.build.lib.packages.Attribute.attr; - -import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableSet; -import com.google.devtools.build.lib.actions.Artifact; -import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException; -import com.google.devtools.build.lib.analysis.BaseRuleClasses; -import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider; -import com.google.devtools.build.lib.analysis.ConfiguredTarget; -import com.google.devtools.build.lib.analysis.FileProvider; -import com.google.devtools.build.lib.analysis.PlatformConfiguration; -import com.google.devtools.build.lib.analysis.PlatformOptions; -import com.google.devtools.build.lib.analysis.ResolvedToolchainContext; -import com.google.devtools.build.lib.analysis.RuleConfiguredTargetBuilder; -import com.google.devtools.build.lib.analysis.RuleConfiguredTargetFactory; -import com.google.devtools.build.lib.analysis.RuleContext; -import com.google.devtools.build.lib.analysis.Runfiles; -import com.google.devtools.build.lib.analysis.RunfilesProvider; -import com.google.devtools.build.lib.analysis.actions.FileWriteAction; -import com.google.devtools.build.lib.analysis.config.BuildOptions; -import com.google.devtools.build.lib.analysis.config.BuildOptionsView; -import com.google.devtools.build.lib.analysis.config.CoreOptions; -import com.google.devtools.build.lib.analysis.config.Fragment; -import com.google.devtools.build.lib.analysis.config.FragmentOptions; -import com.google.devtools.build.lib.analysis.config.RequiresOptions; -import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition; -import com.google.devtools.build.lib.analysis.config.transitions.TransitionFactory; -import com.google.devtools.build.lib.analysis.platform.ToolchainInfo; -import com.google.devtools.build.lib.analysis.platform.ToolchainTypeInfo; -import com.google.devtools.build.lib.analysis.test.TestConfiguration; -import com.google.devtools.build.lib.analysis.util.MockRule; -import com.google.devtools.build.lib.cmdline.Label; -import com.google.devtools.build.lib.cmdline.LabelSyntaxException; -import com.google.devtools.build.lib.collect.nestedset.Depset; -import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; -import com.google.devtools.build.lib.collect.nestedset.Order; -import com.google.devtools.build.lib.events.EventHandler; -import com.google.devtools.build.lib.packages.AttributeMap; -import com.google.devtools.build.lib.packages.BuildType; -import com.google.devtools.build.lib.packages.ImplicitOutputsFunction; -import com.google.devtools.build.lib.packages.NonconfigurableAttributeMapper; -import com.google.devtools.build.lib.packages.Rule; -import com.google.devtools.build.lib.packages.RuleClass.ToolchainResolutionMode; -import com.google.devtools.build.lib.packages.Type; -import com.google.devtools.build.lib.rules.ToolchainType.ToolchainTypeRule; -import com.google.devtools.build.lib.rules.core.CoreRules; -import com.google.devtools.build.lib.rules.repository.BindRule; -import com.google.devtools.build.lib.rules.repository.WorkspaceBaseRule; -import com.google.devtools.build.lib.testutil.Scratch; -import com.google.devtools.build.lib.util.FileTypeSet; -import com.google.devtools.common.options.Option; -import com.google.devtools.common.options.OptionDefinition; -import com.google.devtools.common.options.OptionDocumentationCategory; -import com.google.devtools.common.options.OptionEffectTag; -import com.google.devtools.common.options.OptionsParser; -import java.io.IOException; -import java.util.List; -import javax.annotation.Nullable; -import net.starlark.java.annot.StarlarkBuiltin; -import net.starlark.java.eval.EvalException; -import net.starlark.java.eval.Starlark; -import net.starlark.java.eval.StarlarkValue; - -/** Set of trimmable fragments for testing automatic trimming. */ -public final class TrimmableTestConfigurationFragments { - - private TrimmableTestConfigurationFragments() { - // Utility class, non-instantiable - } - - public static void installStarlarkRules(Scratch scratch, String path) - throws LabelSyntaxException, IOException { - installStarlarkRules( - scratch, path, Label.parseAbsolute("//:undefined_toolchain_type", ImmutableMap.of())); - } - - public static void installStarlarkRules(Scratch scratch, String path, Label toolchainTypeLabel) - throws LabelSyntaxException, IOException { - scratch.file( - path, - "toolchainTypeLabel = " + Starlark.repr(toolchainTypeLabel), - "def _impl(ctx):", - " ctx.actions.write(ctx.outputs.main, '')", - " files = depset(", - " direct = [ctx.outputs.main],", - " transitive = [dep.files for dep in ctx.attr.deps])", - " return [DefaultInfo(files=files)]", - "alpha_starlark = rule(", - " implementation = _impl,", - " attrs = {'deps': attr.label_list(allow_files=True)},", - " fragments = ['alpha'],", - " outputs = {'main': '%{name}.sa'},", - ")", - "bravo_starlark = rule(", - " implementation = _impl,", - " attrs = {'deps': attr.label_list(allow_files=True)},", - " fragments = ['bravo'],", - " outputs = {'main': '%{name}.sb'},", - ")", - "charlie_starlark = rule(", - " implementation = _impl,", - " attrs = {'deps': attr.label_list(allow_files=True)},", - " fragments = ['charlie'],", - " outputs = {'main': '%{name}.sc'},", - ")", - "delta_starlark = rule(", - " implementation = _impl,", - " attrs = {'deps': attr.label_list(allow_files=True)},", - " fragments = ['delta'],", - " outputs = {'main': '%{name}.sd'},", - ")", - "echo_starlark = rule(", - " implementation = _impl,", - " attrs = {'deps': attr.label_list(allow_files=True)},", - " fragments = ['echo'],", - " outputs = {'main': '%{name}.se'},", - ")", - "platformer_starlark = rule(", - " implementation = _impl,", - " attrs = {'deps': attr.label_list(allow_files=True)},", - " outputs = {'main': '%{name}.sp'},", - ")", - "def _uses_toolchains_impl(ctx):", - " ctx.actions.write(ctx.outputs.main, '')", - " transitive_depsets = [dep.files for dep in ctx.attr.deps]", - " toolchain_deps = ctx.toolchains[toolchainTypeLabel].files", - " files = depset(", - " direct = [ctx.outputs.main],", - " transitive = transitive_depsets + [toolchain_deps])", - " return [DefaultInfo(files=files)]", - "uses_toolchains_starlark = rule(", - " implementation = _uses_toolchains_impl,", - " attrs = {'deps': attr.label_list(allow_files=True)},", - " outputs = {'main': '%{name}.su'},", - " toolchains = [str(toolchainTypeLabel)],", - ")", - "def _toolchain_impl(ctx):", - " ctx.actions.write(ctx.outputs.main, '')", - " files = depset(", - " direct = [ctx.outputs.main],", - " transitive = [dep.files for dep in ctx.attr.deps])", - " return [DefaultInfo(files=files), platform_common.ToolchainInfo(files=files)]", - "toolchain_starlark = rule(", - " implementation = _toolchain_impl,", - " attrs = {'deps': attr.label_list(allow_files=True)},", - " outputs = {'main': '%{name}.st'},", - ")", - "def _group_impl(ctx):", - " files = depset(transitive = [dep.files for dep in ctx.attr.deps])", - " return [DefaultInfo(files=files)]", - "group = rule(", - " implementation = _group_impl,", - " attrs = {'deps': attr.label_list(allow_files=True)},", - ")"); - } - - public static void installFragmentsAndNativeRules(ConfiguredRuleClassProvider.Builder builder) { - installFragmentsAndNativeRules(builder, null); - } - - public static void installFragmentsAndNativeRules( - ConfiguredRuleClassProvider.Builder builder, @Nullable Label toolchainTypeLabel) { - // boilerplate: - builder - // must be set, but it doesn't matter here what it's set to - .setToolsRepository("@") - // must be set, but it doesn't matter here what it's set to - .setRunfilesPrefix("runfiles") - // must be set, but it doesn't matter here what it's set to - .setPrerequisiteValidator((contextBuilder, prerequisite, attribute) -> {}) - // must be set, but it doesn't matter here what it's set to - .setPrelude("//:prelude.bzl") - // must be part of BuildOptions for various reasons e.g. dynamic configs - .addConfigurationOptions(CoreOptions.class) - .addConfigurationFragment(TestConfiguration.class) - // needed for the default workspace - .addRuleDefinition(new WorkspaceBaseRule()) - .addRuleDefinition(new BindRule()) - // needed for our native rules - .addRuleDefinition(new BaseRuleClasses.NativeBuildRule()) - // needed to define toolchains - .addRuleDefinition(new ToolchainTypeRule()) - // needs to be set to something - .addUniversalConfigurationFragment(TestConfiguration.class); - - CoreRules.INSTANCE.init(builder); - - MockRule transitionRule = - () -> - MockRule.ancestor(BaseRuleClasses.NativeBuildRule.class) - .factory(DepsCollectingFactory.class) - .define( - "with_configuration", - (ruleBuilder, env) -> - ruleBuilder - .requiresConfigurationFragments( - AConfig.class, - BConfig.class, - CConfig.class, - DConfig.class, - EConfig.class, - PlatformConfiguration.class) - .cfg(new TestFragmentTransitionFactory()) - .add( - attr("deps", BuildType.LABEL_LIST) - .allowedFileTypes(FileTypeSet.ANY_FILE)) - .add( - attr("alpha", Type.STRING) - .value((String) null) - .nonconfigurable("used in transition")) - .add( - attr("bravo", Type.STRING) - .value((String) null) - .nonconfigurable("used in transition")) - .add( - attr("charlie", Type.STRING) - .value((String) null) - .nonconfigurable("used in transition")) - .add( - attr("delta", Type.STRING) - .value((String) null) - .nonconfigurable("used in transition")) - .add( - attr("echo", Type.STRING) - .value((String) null) - .nonconfigurable("used in transition")) - .add( - attr("platforms", BuildType.NODEP_LABEL_LIST) - .value((List<Label>) null) - .nonconfigurable("used in transition")) - .add( - attr("extra_execution_platforms", Type.STRING_LIST) - .value((List<String>) null) - .nonconfigurable("used in transition")) - .add( - attr("extra_toolchains", Type.STRING_LIST) - .value((List<String>) null) - .nonconfigurable("used in transition"))); - - MockRule alphaRule = - () -> - MockRule.ancestor(BaseRuleClasses.NativeBuildRule.class) - .factory(DepsCollectingFactory.class) - .define( - "alpha_native", - (ruleBuilder, env) -> - ruleBuilder - .add( - attr("deps", BuildType.LABEL_LIST) - .allowedFileTypes(FileTypeSet.ANY_FILE)) - .requiresConfigurationFragments(AConfig.class) - .setImplicitOutputsFunction( - ImplicitOutputsFunction.fromTemplates("%{name}.a"))); - - MockRule bravoRule = - () -> - MockRule.ancestor(BaseRuleClasses.NativeBuildRule.class) - .factory(DepsCollectingFactory.class) - .define( - "bravo_native", - (ruleBuilder, env) -> - ruleBuilder - .add( - attr("deps", BuildType.LABEL_LIST) - .allowedFileTypes(FileTypeSet.ANY_FILE)) - .requiresConfigurationFragments(BConfig.class) - .setImplicitOutputsFunction( - ImplicitOutputsFunction.fromTemplates("%{name}.b"))); - - MockRule charlieRule = - () -> - MockRule.ancestor(BaseRuleClasses.NativeBuildRule.class) - .factory(DepsCollectingFactory.class) - .define( - "charlie_native", - (ruleBuilder, env) -> - ruleBuilder - .add( - attr("deps", BuildType.LABEL_LIST) - .allowedFileTypes(FileTypeSet.ANY_FILE)) - .requiresConfigurationFragments(CConfig.class) - .setImplicitOutputsFunction( - ImplicitOutputsFunction.fromTemplates("%{name}.c"))); - - MockRule deltaRule = - () -> - MockRule.ancestor(BaseRuleClasses.NativeBuildRule.class) - .factory(DepsCollectingFactory.class) - .define( - "delta_native", - (ruleBuilder, env) -> - ruleBuilder - .add( - attr("deps", BuildType.LABEL_LIST) - .allowedFileTypes(FileTypeSet.ANY_FILE)) - .requiresConfigurationFragments(DConfig.class) - .setImplicitOutputsFunction( - ImplicitOutputsFunction.fromTemplates("%{name}.d"))); - - MockRule echoRule = - () -> - MockRule.ancestor(BaseRuleClasses.NativeBuildRule.class) - .factory(DepsCollectingFactory.class) - .define( - "echo_native", - (ruleBuilder, env) -> - ruleBuilder - .add( - attr("deps", BuildType.LABEL_LIST) - .allowedFileTypes(FileTypeSet.ANY_FILE)) - .requiresConfigurationFragments(EConfig.class) - .setImplicitOutputsFunction( - ImplicitOutputsFunction.fromTemplates("%{name}.e"))); - - MockRule platformlessRule = - () -> - MockRule.ancestor(BaseRuleClasses.NativeBuildRule.class) - .factory(DepsCollectingFactory.class) - .define( - "platformless_native", - (ruleBuilder, env) -> - ruleBuilder - .add( - attr("deps", BuildType.LABEL_LIST) - .allowedFileTypes(FileTypeSet.ANY_FILE)) - .useToolchainResolution(ToolchainResolutionMode.DISABLED) - .setImplicitOutputsFunction( - ImplicitOutputsFunction.fromTemplates("%{name}.np"))); - - MockRule platformerRule = - () -> - MockRule.ancestor(BaseRuleClasses.NativeBuildRule.class) - .factory(DepsCollectingFactory.class) - .define( - "platformer_native", - (ruleBuilder, env) -> - ruleBuilder - .add( - attr("deps", BuildType.LABEL_LIST) - .allowedFileTypes(FileTypeSet.ANY_FILE)) - .useToolchainResolution(ToolchainResolutionMode.ENABLED) - .setImplicitOutputsFunction( - ImplicitOutputsFunction.fromTemplates("%{name}.p"))); - - builder - .addConfigurationFragment(AConfig.class) - .addConfigurationFragment(BConfig.class) - .addConfigurationFragment(CConfig.class) - .addConfigurationFragment(DConfig.class) - .addConfigurationFragment(EConfig.class) - .addRuleDefinition(transitionRule) - .addRuleDefinition(alphaRule) - .addRuleDefinition(bravoRule) - .addRuleDefinition(charlieRule) - .addRuleDefinition(deltaRule) - .addRuleDefinition(echoRule) - .addRuleDefinition(platformlessRule) - .addRuleDefinition(platformerRule); - - if (toolchainTypeLabel != null) { - MockRule usesToolchainsRule = - () -> - MockRule.ancestor(BaseRuleClasses.NativeBuildRule.class) - .factory(DepsCollectingFactory.class) - .define( - "uses_toolchains_native", - (ruleBuilder, env) -> - ruleBuilder - .add( - attr("deps", BuildType.LABEL_LIST) - .allowedFileTypes(FileTypeSet.ANY_FILE)) - .useToolchainResolution(ToolchainResolutionMode.ENABLED) - .addRequiredToolchains(toolchainTypeLabel) - .setImplicitOutputsFunction( - ImplicitOutputsFunction.fromTemplates("%{name}.u"))); - builder.addRuleDefinition(usesToolchainsRule); - } - } - - /** Set of test options. */ - public static final class AOptions extends FragmentOptions { - @Option( - name = "alpha", - documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, - effectTags = {OptionEffectTag.NO_OP}, - defaultValue = "0") - public String alpha; - } - - /** Test configuration fragment. */ - @StarlarkBuiltin(name = "alpha", doc = "Test config fragment.") - @RequiresOptions(options = {AOptions.class}) - public static final class AConfig extends Fragment implements StarlarkValue { - private final String value; - - public AConfig(BuildOptions buildOptions) { - this.value = buildOptions.get(AOptions.class).alpha; - } - - @Override - public String getOutputDirectoryName() { - return "A" + value; - } - } - - /** Set of test options. */ - public static final class BOptions extends FragmentOptions { - @Option( - name = "bravo", - documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, - effectTags = {OptionEffectTag.NO_OP}, - defaultValue = "0") - public String bravo; - } - - /** Test configuration fragment. */ - @StarlarkBuiltin(name = "bravo", doc = "Test config fragment.") - @RequiresOptions(options = {BOptions.class}) - public static final class BConfig extends Fragment implements StarlarkValue { - private final String value; - - public BConfig(BuildOptions buildOptions) { - this.value = buildOptions.get(BOptions.class).bravo; - } - - @Override - public String getOutputDirectoryName() { - return "B" + value; - } - } - - /** Set of test options. */ - public static final class COptions extends FragmentOptions { - @Option( - name = "charlie", - documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, - effectTags = {OptionEffectTag.NO_OP}, - defaultValue = "0") - public String charlie; - } - - /** Test configuration fragment. */ - @StarlarkBuiltin(name = "charlie", doc = "Test config fragment.") - @RequiresOptions(options = {COptions.class}) - public static final class CConfig extends Fragment implements StarlarkValue { - private final String value; - - public CConfig(BuildOptions buildOptions) { - this.value = buildOptions.get(COptions.class).charlie; - } - - @Override - public String getOutputDirectoryName() { - return "C" + value; - } - } - - /** Set of test options. */ - public static final class DOptions extends FragmentOptions { - @Option( - name = "delta", - documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, - effectTags = {OptionEffectTag.NO_OP}, - defaultValue = "0") - public String delta; - } - - /** Test configuration fragment. */ - @StarlarkBuiltin(name = "delta", doc = "Test config fragment.") - @RequiresOptions(options = {DOptions.class}) - public static final class DConfig extends Fragment implements StarlarkValue { - private final String value; - - public DConfig(BuildOptions buildOptions) { - this.value = buildOptions.get(DOptions.class).delta; - } - - @Override - public String getOutputDirectoryName() { - return "D" + value; - } - } - - /** Set of test options. */ - public static final class EOptions extends FragmentOptions { - public static final OptionDefinition ECHO = - OptionsParser.getOptionDefinitionByName(EOptions.class, "echo"); - - @Option( - name = "echo", - documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, - effectTags = {OptionEffectTag.NO_OP}, - defaultValue = "0") - public String echo; - } - - /** Test configuration fragment. */ - @StarlarkBuiltin(name = "echo", doc = "Test config fragment.") - @RequiresOptions(options = {EOptions.class}) - public static final class EConfig extends Fragment implements StarlarkValue { - private final String value; - - public EConfig(BuildOptions buildOptions) { - this.value = buildOptions.get(EOptions.class).echo; - } - - @Override - public String getOutputDirectoryName() { - return "E" + value; - } - } - - private static final class TestFragmentTransitionFactory implements TransitionFactory<Rule> { - private static final class SetValuesTransition implements PatchTransition { - private final String alpha; - private final String bravo; - private final String charlie; - private final String delta; - private final String echo; - private final List<Label> platforms; - private final List<String> extraExecutionPlatforms; - private final List<String> extraToolchains; - - public SetValuesTransition( - String alpha, - String bravo, - String charlie, - String delta, - String echo, - List<Label> platforms, - List<String> extraExecutionPlatforms, - List<String> extraToolchains) { - this.alpha = alpha; - this.bravo = bravo; - this.charlie = charlie; - this.delta = delta; - this.echo = echo; - this.platforms = platforms; - this.extraExecutionPlatforms = extraExecutionPlatforms; - this.extraToolchains = extraToolchains; - } - - @Override - public ImmutableSet<Class<? extends FragmentOptions>> requiresOptionFragments() { - return ImmutableSet.of( - AOptions.class, - BOptions.class, - COptions.class, - DOptions.class, - EOptions.class, - PlatformOptions.class); - } - - @Override - public BuildOptions patch(BuildOptionsView target, EventHandler eventHandler) { - BuildOptionsView output = target.clone(); - if (alpha != null) { - output.get(AOptions.class).alpha = alpha; - } - if (bravo != null) { - output.get(BOptions.class).bravo = bravo; - } - if (charlie != null) { - output.get(COptions.class).charlie = charlie; - } - if (delta != null) { - output.get(DOptions.class).delta = delta; - } - if (echo != null) { - output.get(EOptions.class).echo = echo; - } - if (platforms != null) { - output.get(PlatformOptions.class).platforms = platforms; - } - if (extraExecutionPlatforms != null) { - output.get(PlatformOptions.class).extraExecutionPlatforms = extraExecutionPlatforms; - } - if (extraToolchains != null) { - output.get(PlatformOptions.class).extraToolchains = extraToolchains; - } - return output.underlying(); - } - } - - @Override - public PatchTransition create(Rule rule) { - AttributeMap attributes = NonconfigurableAttributeMapper.of(rule); - return new SetValuesTransition( - attributes.get("alpha", Type.STRING), - attributes.get("bravo", Type.STRING), - attributes.get("charlie", Type.STRING), - attributes.get("delta", Type.STRING), - attributes.get("echo", Type.STRING), - attributes.get("platforms", BuildType.NODEP_LABEL_LIST), - attributes.get("extra_execution_platforms", Type.STRING_LIST), - attributes.get("extra_toolchains", Type.STRING_LIST)); - } - } - - /** RuleConfiguredTargetFactory which collects dependencies. */ - public static final class DepsCollectingFactory implements RuleConfiguredTargetFactory { - @Override - public ConfiguredTarget create(RuleContext ruleContext) - throws InterruptedException, RuleErrorException, ActionConflictException { - NestedSetBuilder<Artifact> filesToBuild = NestedSetBuilder.stableOrder(); - filesToBuild.addAll(ruleContext.getOutputArtifacts()); - for (FileProvider dep : ruleContext.getPrerequisites("deps", FileProvider.class)) { - filesToBuild.addTransitive(dep.getFilesToBuild()); - } - for (Artifact artifact : ruleContext.getOutputArtifacts()) { - ruleContext.registerAction( - FileWriteAction.createEmptyWithInputs( - ruleContext.getActionOwner(), - NestedSetBuilder.emptySet(Order.STABLE_ORDER), - artifact)); - } - if (ruleContext.getToolchainContext() != null) { - ResolvedToolchainContext toolchainContext = ruleContext.getToolchainContext(); - for (ToolchainTypeInfo toolchainType : toolchainContext.requiredToolchainTypes()) { - ToolchainInfo toolchainInfo = toolchainContext.forToolchainType(toolchainType); - try { - filesToBuild.addTransitive( - ((Depset) toolchainInfo.getValue("files")).getSet(Artifact.class)); - } catch (EvalException | Depset.TypeException ex) { - throw new AssertionError(ex); - } - } - } - return new RuleConfiguredTargetBuilder(ruleContext) - .setFilesToBuild(filesToBuild.build()) - .setRunfilesSupport(null, null) - .add(RunfilesProvider.class, RunfilesProvider.simple(Runfiles.EMPTY)) - .build(); - } - } -}
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/trimming/TrimmedConfigurationCacheTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/trimming/TrimmedConfigurationCacheTest.java deleted file mode 100644 index e3ae994..0000000 --- a/src/test/java/com/google/devtools/build/lib/skyframe/trimming/TrimmedConfigurationCacheTest.java +++ /dev/null
@@ -1,401 +0,0 @@ -// Copyright 2018 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.build.lib.skyframe.trimming; - -import static com.google.common.truth.Truth.assertThat; -import static com.google.common.truth.Truth8.assertThat; - -import com.google.common.collect.ImmutableMap; -import org.junit.Before; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; - -/** Tests for the TrimmedConfigurationCache. */ -@RunWith(JUnit4.class) -public final class TrimmedConfigurationCacheTest { - - private TrimmedConfigurationCache<TestKey, String, ImmutableMap<String, String>> cache; - - @Before - public void initializeCache() { - cache = TestKey.newCache(); - } - - @Test - public void get_onFreshCache_returnsEmpty() throws Exception { - assertThat(cache.get(TestKey.parse("<A: 1> //foo"))).isEmpty(); - } - - @Test - public void get_afterAddingSubsetCacheEntry_returnsMatchingValue() throws Exception { - TestKey canonicalKey = TestKey.parse("<A: 1, B: 1> //foo"); - cache.putIfAbsent(canonicalKey, TestKey.parseConfiguration("A: 1")); - - assertThat(cache.get(TestKey.parse("<A: 1, C: 1> //foo"))).hasValue(canonicalKey); - } - - @Test - public void get_afterRemovingMatchingCacheEntry_returnsEmpty() throws Exception { - TestKey canonicalKey = TestKey.parse("<A: 1, B: 1> //foo"); - cache.putIfAbsent(canonicalKey, TestKey.parseConfiguration("A: 1")); - cache.remove(canonicalKey); - - assertThat(cache.get(TestKey.parse("<A: 1, B: 2> //foo"))).isEmpty(); - } - - @Test - public void get_afterRemovingCacheEntryWithDifferentConfig_returnsOriginalKey() throws Exception { - TestKey canonicalKey = TestKey.parse("<A: 1, B: 1> //foo"); - cache.putIfAbsent(canonicalKey, TestKey.parseConfiguration("A: 1")); - TestKey removedOtherKey = TestKey.parse("<A: 1, B: 2> //foo"); - cache.remove(removedOtherKey); - - assertThat(cache.get(canonicalKey)).hasValue(canonicalKey); - } - - @Test - public void get_afterRemovingCacheEntryWithDifferentDescriptor_returnsOriginalKey() - throws Exception { - TestKey canonicalKey = TestKey.parse("<A: 1, B: 1> //foo"); - cache.putIfAbsent(canonicalKey, TestKey.parseConfiguration("A: 1")); - TestKey removedOtherKey = TestKey.parse("<A: 1, B: 1> //bar"); - cache.remove(removedOtherKey); - - assertThat(cache.get(canonicalKey)).hasValue(canonicalKey); - } - - @Test - public void get_afterClearingMatchingCacheEntry_returnsEmpty() throws Exception { - cache.putIfAbsent(TestKey.parse("<A: 1, B: 1> //foo"), TestKey.parseConfiguration("A: 1")); - cache.clear(); - - assertThat(cache.get(TestKey.parse("<A: 1, B: 2> //foo"))).isEmpty(); - } - - @Test - public void get_afterRemovingAndReAddingCacheEntry_returnsNewValue() throws Exception { - TestKey oldKey = TestKey.parse("<A: 1, B: 1> //foo"); - ImmutableMap<String, String> trimmedConfiguration = TestKey.parseConfiguration("A: 1"); - cache.putIfAbsent(oldKey, trimmedConfiguration); - cache.remove(oldKey); - TestKey newKey = TestKey.parse("<A: 1, C: 1> //foo"); - cache.putIfAbsent(newKey, trimmedConfiguration); - - assertThat(cache.get(TestKey.parse("<A: 1, D: 1> //foo"))).hasValue(newKey); - } - - @Test - public void get_afterAddingMatchingConfigurationForDifferentDescriptor_returnsEmpty() - throws Exception { - cache.putIfAbsent(TestKey.parse("<A: 1, B: 1> //bar"), TestKey.parseConfiguration("A: 1")); - - assertThat(cache.get(TestKey.parse("<A: 1, B: 1> //foo"))).isEmpty(); - } - - @Test - public void get_afterAddingNonMatchingConfigurationForSameDescriptor_returnsEmpty() - throws Exception { - cache.putIfAbsent(TestKey.parse("<A: 1, B: 1> //foo"), TestKey.parseConfiguration("A: 1")); - - assertThat(cache.get(TestKey.parse("<A: 2, B: 1> //foo"))).isEmpty(); - } - - @Test - public void get_afterAddingAndInvalidatingMatchingCacheEntry_returnsEmpty() throws Exception { - TestKey canonicalKey = TestKey.parse("<A: 1, B: 1> //foo"); - cache.putIfAbsent(canonicalKey, TestKey.parseConfiguration("A: 1")); - cache.invalidate(canonicalKey); - - assertThat(cache.get(TestKey.parse("<A: 1, C: 1> //foo"))).isEmpty(); - } - - @Test - public void get_afterAddingAndInvalidatingAndReAddingMatchingCacheEntry_returnsOriginalValue() - throws Exception { - TestKey oldKey = TestKey.parse("<A: 1, B: 1> //foo"); - ImmutableMap<String, String> trimmedConfiguration = TestKey.parseConfiguration("A: 1"); - cache.putIfAbsent(oldKey, trimmedConfiguration); - cache.invalidate(oldKey); - TestKey newKey = TestKey.parse("<A: 1, C: 1> //foo"); - cache.putIfAbsent(newKey, trimmedConfiguration); - - assertThat(cache.get(TestKey.parse("<A: 1, D: 1> //foo"))).hasValue(oldKey); - } - - @Test - public void get_afterAddingAndInvalidatingAndRevalidatingMatchingCacheEntry_returnsOriginalValue() - throws Exception { - TestKey oldKey = TestKey.parse("<A: 1, B: 1> //foo"); - ImmutableMap<String, String> trimmedConfiguration = TestKey.parseConfiguration("A: 1"); - cache.putIfAbsent(oldKey, trimmedConfiguration); - cache.invalidate(oldKey); - cache.revalidate(oldKey); - - assertThat(cache.get(TestKey.parse("<A: 1, D: 1> //foo"))).hasValue(oldKey); - } - - @Test - public void get_afterMovingKeyToDifferentTrimming_returnsEmpty() throws Exception { - TestKey oldKey = TestKey.parse("<A: 1, B: 1> //foo"); - cache.putIfAbsent(oldKey, TestKey.parseConfiguration("A: 1")); - cache.invalidate(oldKey); - cache.putIfAbsent(oldKey, TestKey.parseConfiguration("B: 1")); - - assertThat(cache.get(TestKey.parse("<A: 1, B: 2> //foo"))).isEmpty(); - } - - @Test(expected = IllegalArgumentException.class) - public void putIfAbsent_forNonSubsetConfiguration_throwsIllegalArgumentException() - throws Exception { - cache.putIfAbsent(TestKey.parse("<A: 1> //foo"), TestKey.parseConfiguration("A: 2")); - } - - @Test - public void putIfAbsent_onFreshCache_returnsInputKey() throws Exception { - TestKey inputKey = TestKey.parse("<A: 1, B: 1> //foo"); - - assertThat(cache.putIfAbsent(inputKey, TestKey.parseConfiguration("A: 1"))).isEqualTo(inputKey); - } - - @Test - public void putIfAbsent_afterRemoving_returnsNewKey() throws Exception { - TestKey oldKey = TestKey.parse("<A: 1, B: 1> //foo"); - ImmutableMap<String, String> trimmedConfiguration = TestKey.parseConfiguration("A: 1"); - cache.putIfAbsent(oldKey, trimmedConfiguration); - cache.remove(oldKey); - TestKey newKey = TestKey.parse("<A: 1, B: 2> //foo"); - - assertThat(cache.putIfAbsent(newKey, trimmedConfiguration)).isEqualTo(newKey); - } - - @Test - public void putIfAbsent_afterAddingEqualConfigurationForDifferentDescriptor_returnsInputKey() - throws Exception { - TestKey oldKey = TestKey.parse("<A: 1, B: 1> //foo"); - ImmutableMap<String, String> trimmedConfiguration = TestKey.parseConfiguration("A: 1"); - cache.putIfAbsent(oldKey, trimmedConfiguration); - TestKey newKey = TestKey.parse("<A: 1, B: 1> //bar"); - - assertThat(cache.putIfAbsent(newKey, trimmedConfiguration)).isEqualTo(newKey); - } - - @Test - public void putIfAbsent_afterAddingNonEqualConfigurationForSameDescriptor_returnsInputKey() - throws Exception { - cache.putIfAbsent(TestKey.parse("<A: 1, B: 1> //foo"), TestKey.parseConfiguration("A: 1")); - TestKey newKey = TestKey.parse("<A: 2, B: 2> //foo"); - - assertThat(cache.putIfAbsent(newKey, TestKey.parseConfiguration("A: 2"))).isEqualTo(newKey); - } - - @Test - public void putIfAbsent_afterAddingEqualConfigurationForSameDescriptor_returnsOriginalKey() - throws Exception { - TestKey oldKey = TestKey.parse("<A: 1, B: 1> //foo"); - ImmutableMap<String, String> trimmedConfiguration = TestKey.parseConfiguration("A: 1"); - cache.putIfAbsent(oldKey, trimmedConfiguration); - - assertThat(cache.putIfAbsent(TestKey.parse("<A: 1, B: 2> //foo"), trimmedConfiguration)) - .isEqualTo(oldKey); - } - - @Test - public void putIfAbsent_afterInvalidatingEqualEntry_returnsOriginalKey() throws Exception { - TestKey oldKey = TestKey.parse("<A: 1, B: 1> //foo"); - ImmutableMap<String, String> trimmedConfiguration = TestKey.parseConfiguration("A: 1"); - cache.putIfAbsent(oldKey, trimmedConfiguration); - cache.invalidate(oldKey); - - assertThat(cache.putIfAbsent(TestKey.parse("<A: 1, B: 2> //foo"), trimmedConfiguration)) - .isEqualTo(oldKey); - } - - @Test - public void putIfAbsent_forKeyAssociatedWithDifferentTrimming_returnsOldKey() throws Exception { - TestKey oldKey = TestKey.parse("<A: 1, B: 1> //foo"); - cache.putIfAbsent(oldKey, TestKey.parseConfiguration("A: 1")); - cache.invalidate(oldKey); - - assertThat(cache.putIfAbsent(oldKey, TestKey.parseConfiguration("B: 1"))).isEqualTo(oldKey); - } - - @Test - public void putIfAbsent_afterMovingPreviousAssociatedKeyToNewTrimming_returnsNewKey() - throws Exception { - TestKey oldKey = TestKey.parse("<A: 1, B: 1> //foo"); - ImmutableMap<String, String> trimmedA1 = TestKey.parseConfiguration("A: 1"); - ImmutableMap<String, String> trimmedB1 = TestKey.parseConfiguration("B: 1"); - cache.putIfAbsent(oldKey, trimmedA1); - cache.invalidate(oldKey); - cache.putIfAbsent(oldKey, trimmedB1); - cache.invalidate(oldKey); - TestKey newKey = TestKey.parse("<A: 1, B: 2> //foo"); - - // This is testing that oldKey is not still associated with trimmedA1, because now it's - // associated with trimmedB1 instead. - assertThat(cache.putIfAbsent(newKey, trimmedA1)).isEqualTo(newKey); - } - - @Test - public void putIfAbsent_afterInvalidatingAndReAddingEqualEntry_returnsOriginalKey() - throws Exception { - TestKey oldKey = TestKey.parse("<A: 1, B: 1> //foo"); - ImmutableMap<String, String> trimmedConfiguration = TestKey.parseConfiguration("A: 1"); - cache.putIfAbsent(oldKey, trimmedConfiguration); - cache.invalidate(oldKey); - cache.putIfAbsent(TestKey.parse("<A: 1, B: 2> //foo"), trimmedConfiguration); - - assertThat(cache.putIfAbsent(TestKey.parse("<A: 1, B: 3> //foo"), trimmedConfiguration)) - .isEqualTo(oldKey); - } - - @Test - public void putIfAbsent_afterInvalidatingAndRevalidatingEqualEntry_returnsOriginalKey() - throws Exception { - TestKey oldKey = TestKey.parse("<A: 1, B: 1> //foo"); - ImmutableMap<String, String> trimmedConfiguration = TestKey.parseConfiguration("A: 1"); - cache.putIfAbsent(oldKey, trimmedConfiguration); - cache.invalidate(oldKey); - cache.revalidate(oldKey); - - assertThat(cache.putIfAbsent(TestKey.parse("<A: 1, B: 2> //foo"), trimmedConfiguration)) - .isEqualTo(oldKey); - } - - @Test - public void putIfAbsent_afterAddingAndInvalidatingSubsetConfiguration_returnsInputKey() - throws Exception { - TestKey oldKey = TestKey.parse("<A: 1, B: 1> //foo"); - ImmutableMap<String, String> trimmedConfiguration = TestKey.parseConfiguration("A: 1"); - cache.putIfAbsent(oldKey, trimmedConfiguration); - cache.invalidate(oldKey); - TestKey newKey = TestKey.parse("<A: 1, B: 2> //foo"); - - assertThat(cache.putIfAbsent(newKey, TestKey.parseConfiguration("A: 1, B: 2"))) - .isEqualTo(newKey); - } - - @Test - public void putIfAbsent_afterAddingAndInvalidatingSupersetConfiguration_returnsInputKey() - throws Exception { - TestKey oldKey = TestKey.parse("<A: 1, B: 1, C: 1> //foo"); - cache.putIfAbsent(oldKey, TestKey.parseConfiguration("A: 1, B: 1")); - cache.invalidate(oldKey); - TestKey newKey = TestKey.parse("<A: 1, B: 1, C: 2> //foo"); - - assertThat(cache.putIfAbsent(newKey, TestKey.parseConfiguration("A: 1"))).isEqualTo(newKey); - } - - @Test - public void invalidate_onEntryNotInCache_doesNotThrow() throws Exception { - // Expect no exception here. - cache.invalidate(TestKey.parse("<A: 1> //foo")); - } - - @Test - public void invalidate_onAlreadyInvalidatedEntry_doesNotThrow() throws Exception { - TestKey oldKey = TestKey.parse("<A: 1, B: 1> //foo"); - cache.putIfAbsent(oldKey, TestKey.parseConfiguration("A: 1")); - cache.invalidate(oldKey); - - // Expect no exception here. - cache.invalidate(oldKey); - } - - @Test - public void invalidate_onRemovedEntry_doesNotThrow() throws Exception { - TestKey oldKey = TestKey.parse("<A: 1, B: 1> //foo"); - cache.putIfAbsent(oldKey, TestKey.parseConfiguration("A: 1")); - cache.remove(oldKey); - - // Expect no exception here. - cache.invalidate(oldKey); - } - - @Test - public void revalidate_onEntryNotInCache_doesNotThrow() throws Exception { - // Expect no exception here. - cache.revalidate(TestKey.parse("<A: 1> //foo")); - } - - @Test - public void revalidate_onAlreadyInvalidatedAndRevalidatedEntry_doesNotThrow() throws Exception { - TestKey oldKey = TestKey.parse("<A: 1, B: 1> //foo"); - cache.putIfAbsent(oldKey, TestKey.parseConfiguration("A: 1")); - cache.invalidate(oldKey); - cache.revalidate(oldKey); - - // Expect no exception here. - cache.revalidate(oldKey); - } - - @Test - public void revalidate_onRemovedEntry_doesNotThrow() throws Exception { - TestKey oldKey = TestKey.parse("<A: 1, B: 1> //foo"); - cache.putIfAbsent(oldKey, TestKey.parseConfiguration("A: 1")); - cache.remove(oldKey); - - // Expect no exception here. - cache.revalidate(oldKey); - } - - @Test - public void remove_onEntryNotInCache_doesNotThrow() throws Exception { - // Expect no exception here. - cache.remove(TestKey.parse("<A: 1> //foo")); - } - - @Test - public void remove_onAlreadyInvalidatedEntry_doesNotThrow() throws Exception { - TestKey oldKey = TestKey.parse("<A: 1, B: 1> //foo"); - cache.putIfAbsent(oldKey, TestKey.parseConfiguration("A: 1")); - cache.invalidate(oldKey); - - // Expect no exception here. - cache.remove(oldKey); - } - - @Test - public void remove_onEntryWithDifferentConfiguration_doesNotThrow() throws Exception { - cache.putIfAbsent(TestKey.parse("<A: 1, B: 1> //foo"), TestKey.parseConfiguration("A: 1")); - - // Expect no exception here. - cache.remove(TestKey.parse("<A: 1, B: 2> //foo")); - } - - @Test - public void remove_onEntryWithDifferentDescriptor_doesNotThrow() throws Exception { - cache.putIfAbsent(TestKey.parse("<A: 1, B: 1> //foo"), TestKey.parseConfiguration("A: 1")); - - // Expect no exception here. - cache.remove(TestKey.parse("<A: 1, B: 1> //bar")); - } - - @Test - public void remove_onRemovedEntry_doesNotThrow() throws Exception { - TestKey oldKey = TestKey.parse("<A: 1, B: 1> //foo"); - cache.putIfAbsent(oldKey, TestKey.parseConfiguration("A: 1")); - cache.remove(oldKey); - - // Expect no exception here. - cache.remove(oldKey); - } - - @Test - public void clear_onEmptyCache_doesNotThrow() throws Exception { - cache.clear(); - } -}