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 367f5da856abf6..472f6a83aa9e2e 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 @@ java_library( ":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 077e6523122c9e..0801c6065fb6e4 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. * *

Package design

* - *

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. + *

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. * - *

Configurations the inputs to a build come from two sources: the - * intrinsic inputs, specified in the BUILD file, are called targets. - * The environmental inputs, coming from the build tool, the command-line, or - * configuration files, are called the configuration. Only when a - * target and a configuration are combined is there sufficient information to - * perform a build.

+ *

Configurations the inputs to a build come from two sources: the intrinsic inputs, + * specified in the BUILD file, are called targets. The environmental inputs, coming from + * the build tool, the command-line, or configuration files, are called the configuration. + * Only when a target and a configuration are combined is there sufficient information to perform a + * build. * - *

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. + *

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. + * + *

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: * - *

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: *

* - *

See also {@link ConfiguredTarget} which documents some important - * invariants. + *

See also {@link ConfiguredTarget} which documents some important invariants. */ public class BuildView { private static final GoogleLogger logger = GoogleLogger.forEnclosingClass(); @@ -154,12 +149,11 @@ public class BuildView { 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 @@ private ArtifactFactory getArtifactFactory() { @VisibleForTesting static LinkedHashSet filterTestsByTargets( Collection targets, Set

The artifact is stored so that we can deduplicate artifacts created multiple times. */ - private Map artifacts; + private Map 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 actions = new ArrayList<>(); + private final List actions = new ArrayList<>(); public CachingAnalysisEnvironment( ArtifactFactory artifactFactory, ActionKeyContext actionKeyContext, ActionLookupKey owner, - boolean isSystemEnv, boolean extendedSanityChecks, boolean allowAnalysisFailures, ExtendedEventHandler errorEventListener, @@ -109,14 +101,12 @@ public CachingAnalysisEnvironment( 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 @@ private static StringBuilder shortDescription(ActionAnalysisMetadata action) { */ public void verifyGeneratedArtifactHaveActions(Target target) { Collection orphanArtifacts = getOrphanArtifactMap().values(); - List checkedActions = null; + List checkedActions; if (!orphanArtifacts.isEmpty()) { checkedActions = Lists.newArrayListWithCapacity(actions.size()); for (ActionAnalysisMetadata action : actions) { @@ -244,10 +234,6 @@ public ActionKeyContext getActionKeyContext() { @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 @@ public Artifact.DerivedArtifact getDerivedArtifact( 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 @@ public SpecialArtifact getTreeArtifact(PathFragment rootRelativePath, ArtifactRo Preconditions.checkState(enabled); return (SpecialArtifact) dedupAndTrackArtifactAndOrigin( - artifactFactory.getTreeArtifact(rootRelativePath, root, getOwner()), + artifactFactory.getTreeArtifact(rootRelativePath, root, owner), extendedSanityChecks ? new Throwable() : null); } @@ -313,7 +299,7 @@ public SpecialArtifact getSymlinkArtifact(PathFragment rootRelativePath, Artifac Preconditions.checkState(enabled); return (SpecialArtifact) dedupAndTrackArtifactAndOrigin( - artifactFactory.getSymlinkArtifact(rootRelativePath, root, getOwner()), + artifactFactory.getSymlinkArtifact(rootRelativePath, root, owner), extendedSanityChecks ? new Throwable() : null); } @@ -327,13 +313,13 @@ public Artifact.DerivedArtifact getFilesetArtifact( 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 @@ public SkyFunction.Environment getSkyframeEnv() { } @Override - public StarlarkSemantics getStarlarkSemantics() throws InterruptedException { + public StarlarkSemantics getStarlarkSemantics() { return starlarkBuiltinsValue.starlarkSemantics; } @Override - public ImmutableMap getStarlarkDefinedBuiltins() throws InterruptedException { + public ImmutableMap 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 ad63bb92140364..fbd8e11f81c5af 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 @@ -768,14 +768,6 @@ public boolean trimConfigurations() { return options.configsMode == CoreOptions.ConfigsMode.ON; } - /** - * 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; - } - /** * >Experimental feature: if true, qualifying outputs use path prefixes based on their * content instead of the traditional blaze-out/$CPU-$COMPILATION_MODE. 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 92ef5509c4a873..bb123e896a81fa 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 Map labelizeStarlarkOptions(Map sta public static BuildOptions getDefaultBuildOptionsForFragments( List> fragmentClasses) { - ArrayList 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 @@ public Builder addStarlarkOption(Label key, Object value) { 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 key) { fragmentOptions.remove(key); @@ -974,82 +965,6 @@ private boolean isEmpty() { && extraSecondStarlarkOptions.isEmpty(); } - /** - * 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 @@ void clearCachedReconstructedForTesting() { cachedReconstructed = new SoftReference<>(null); } - private boolean hasChangeToStarlarkOptionUnchangedIn(OptionsDiffForReconstruction that) { - Set

Also collects all propagating aspects in correct order. */ - private ImmutableMap getSkyKeysForAspectsAndCollectAspectPath( - ImmutableList keys, - ImmutableList.Builder aspectPathBuilder) { + private static ImmutableMap getSkyKeysForAspectsAndCollectAspectPath( + ImmutableList keys, ImmutableList.Builder aspectPathBuilder) { HashMap result = new HashMap<>(); for (AspectKey key : keys) { buildSkyKeys(key, result, aspectPathBuilder); @@ -568,7 +559,9 @@ private ImmutableMap getSkyKeysForAspectsAndCollectAsp return ImmutableMap.copyOf(result); } - private void buildSkyKeys(AspectKey key, HashMap result, + private static void buildSkyKeys( + AspectKey key, + HashMap result, ImmutableList.Builder aspectPathBuilder) { if (result.containsKey(key.getAspectDescriptor())) { return; @@ -657,7 +650,7 @@ private AspectValue createAspect( 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 165cf989e68e34..0ad6577eaec0ae 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 @@ filegroup( "//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 @@ java_library( "TopLevelActionLookupConflictFindingFunction.java", "ToplevelStarlarkAspectFunction.java", "TransitiveTargetFunction.java", - "TrimmedConfigurationProgressReceiver.java", "WorkspaceFileFunction.java", "actiongraph/ActionGraphDump.java", "actiongraph/v2/ActionGraphDump.java", @@ -299,7 +297,6 @@ java_library( "//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 950693aae752a7..5f03dc0a87dd0a 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 @@ public SkyValue compute(SkyKey key, Environment env) throws ConfiguredTargetFunc env, resolver, ctgValue, - ImmutableList.of(), + ImmutableList.of(), configConditions.asProviders(), unloadedToolchainContexts == null ? null @@ -521,8 +521,7 @@ static ToolchainCollection computeUnloadedToolchainCon 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 @@ static ToolchainCollection computeUnloadedToolchainCon .configurationKey(toolchainConfig) .requiredToolchainTypeLabels(execGroup.requiredToolchains()) .execConstraintLabels(execGroup.execCompatibleWith()) - .shouldSanityCheckConfiguration(configuration.trimConfigurationsRetroactively()) .build()); } @@ -610,7 +608,6 @@ public static ImmutableSet

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. - * - *

Consider a {@code } as a pair of {@code } and {@code }. The - * descriptor describes what the key builds, while the configuration describes how to build it. - * - *

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. - * - *

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. - * - *

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. - * - *

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.). - * - *

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. - * - *

This class expects to be used in ways which are consistent with trimming. That is to say: - * - *

- * - *

If used as described above, this class is thread-safe. - */ -public final class TrimmedConfigurationCache { - - // ======== 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 descriptorExtractor; - private final Function configurationExtractor; - - private final ConfigurationComparer configurationComparer; - - private volatile ConcurrentHashMap< - DescriptorT, ConcurrentHashMap>> - 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. - * - *

{@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 descriptorExtractor, - Function configurationExtractor, - ConfigurationComparer 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. - * - *

Note that this is not referring to a proper subset; it's quite possible for a key - * to "trim" to a configuration equal to its configuration. That is, without anything being - * removed. - * - *

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. - * - *

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 get(KeyT input) { - DescriptorT descriptor = getDescriptorFor(input); - ConcurrentHashMap> 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> entry : trimmingsOfDescriptor.entrySet()) { - ConfigurationT trimmedConfig = entry.getKey(); - KeyAndState 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 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. - * - *

The trimmed configuration must be a subset of the input key's configuration. Otherwise, - * {@link IllegalArgumentException} will be thrown. - * - *

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. - * - *

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> trimmingsOfDescriptor = - descriptors.computeIfAbsent(getDescriptorFor(canonicalKey), unused -> newDescriptorMap()); - KeyAndState 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> 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. - * - *

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. - * - *

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. - * - *

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. - * - *

This undoes the effects of {@link #invalidate(KeyT)}, allowing the key to be returned from - * {@link #get(KeyT)} again. - * - *

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. - * - *

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. - * - *

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> 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. - * - *

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. - * - *

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> transformation) { - while (!updateEntryIfNoConflicts(key, transformation)) {} - } - - /** - * Finds the entry in the cache where the given key is canonical and updates or removes it. - * - *

Only one attempt is made, and if there's a collision with another change, false is returned - * and the map is not changed. - * - *

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> transformation) { - DescriptorT descriptor = getDescriptorFor(key); - ConcurrentHashMap> trimmingsOfDescriptor = - descriptors.get(descriptor); - if (trimmingsOfDescriptor == null) { - // There are no entries at all for this descriptor. - return true; - } - - for (Entry> entry : trimmingsOfDescriptor.entrySet()) { - KeyAndState currentValue = entry.getValue(); - if (currentValue.getKey().equals(key)) { - KeyAndState 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. - * - *

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>> - 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> 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 a7c1bef22a3877..f7c236ac8a6b35 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 @@ java_library( "//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 @@ java_library( "//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 3464e5f1f91ccc..00000000000000 --- 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 starlarkOptions = - new ImmutableMap.Builder<>(); - private final ImmutableList.Builder> fragments = - new ImmutableList.Builder<>(); - private final ImmutableList.Builder nativeOptions = new ImmutableList.Builder<>(); - - public OptionsBuilder withNativeFragment( - Class 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 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 3f8b306cabd260..293a0b777752ff 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 @@ public BuildViewForTesting( this.skyframeBuildView = skyframeExecutor.getSkyframeBuildView(); } - @VisibleForTesting public Set getSkyframeEvaluatedActionLookupKeyCountForTesting() { Set actionLookupKeys = populateActionLookupKeyMapAndGetDiff(); Preconditions.checkState( @@ -188,7 +185,6 @@ private Set populateActionLookupKeyMapAndGetDiff() { /** * Returns whether the given configured target has errors. */ - @VisibleForTesting public boolean hasErrors(ConfiguredTarget configuredTarget) { return configuredTarget == null; } @@ -223,8 +219,7 @@ public AnalysisResult update( 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 @@ public ArtifactFactory getArtifactFactory() { * 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 @@ public BuildConfiguration getConfigurationForTesting( * 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 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 @@ public Collection getDirectPrerequisitesForTesting( 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 @@ public Collection getDirectPrerequisitesForTesting( 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 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 @@ private static ConfiguredTargetAndData getConfiguredTarget( } @Nullable - private DependencyKey findDependencyKey( + private static DependencyKey findDependencyKey( Multimap 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 @@ private DependencyKey findDependencyKey( } // 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 @@ protected ConfiguredTargetAndData mergeAspects( } } - @VisibleForTesting public OrderedSetMultimap getDirectPrerequisiteDependenciesForTesting( final ExtendedEventHandler eventHandler, @@ -410,7 +394,7 @@ protected ConfiguredTargetAndData mergeAspects( 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 @@ private ConfigurationTransition getTopLevelTransitionForTarget( * *

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 @@ public ConfiguredTarget getConfiguredTargetForTesting( return skyframeExecutor.getConfiguredTargetForTesting(eventHandler, label, config, transition); } - @VisibleForTesting ConfiguredTargetAndData getConfiguredTargetAndDataForTesting( ExtendedEventHandler eventHandler, Label label, BuildConfiguration config) throws StarlarkTransition.TransitionException, InvalidConfigurationException, @@ -588,7 +570,6 @@ ConfiguredTargetAndData getConfiguredTargetAndDataForTesting( /** * 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 @@ public RuleContext getRuleContextForTesting( .setLabel(target.getLabel()) .setConfiguration(targetConfig) .build(), - /*isSystemEnv=*/ false, targetConfig.extendedSanityChecks(), targetConfig.allowAnalysisFailures(), eventHandler, @@ -624,7 +604,6 @@ public RuleContext getRuleContextForTesting( * 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 @@ public RuleContext getRuleContextForTesting( 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 @@ public RuleContext getRuleContextForTesting( } /** Clears the analysis cache as in --discard_analysis_cache. */ - @VisibleForTesting void clearAnalysisCache( Collection topLevelTargets, ImmutableSet 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 b1da3f99160c67..aac74b6f7ed62c 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 @@ public void initializeSkyframeExecutor(boolean doPackageLoadingChecks) throws Ex packageOptions, buildLanguageOptions, UUID.randomUUID(), - ImmutableMap.of(), + ImmutableMap.of(), tsgm); - skyframeExecutor.setActionEnv(ImmutableMap.of()); + skyframeExecutor.setActionEnv(ImmutableMap.of()); useConfiguration(); setUpSkyframe(); this.actionLogBufferPathGenerator = @@ -411,7 +411,7 @@ protected final PackageFactory getPackageFactory() { } protected Iterable getEnvironmentExtensions() { - return ImmutableList.of(); + return ImmutableList.of(); } protected StarlarkSemantics getStarlarkSemantics() { @@ -452,8 +452,7 @@ protected final BuildConfigurationCollection createConfigurations( optionsParser.setStarlarkOptions(starlarkOptions); BuildOptions buildOptions = ruleClassProvider.createBuildOptions(optionsParser); - return skyframeExecutor.createConfigurations( - reporter, buildOptions, ImmutableSet.of(), false); + return skyframeExecutor.createConfigurations(reporter, buildOptions, ImmutableSet.of(), false); } protected Target getTarget(String label) @@ -559,8 +558,6 @@ protected void invalidatePackages() throws InterruptedException { * Invalidates all existing packages. Optionally invalidates configurations too. * *

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 @@ protected Iterable getDefaultsForConfiguration() { * @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 starlarkOptions, String... args) throws Exception { @@ -624,11 +620,10 @@ protected void useConfiguration(String... args) throws Exception { } /** - * 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 @@ public SkyFunctionName functionName() { return null; } }, - /*isSystemEnv=*/ true, /*extendedSanityChecks=*/ false, /*allowAnalysisFailures=*/ false, reporter, @@ -680,7 +674,7 @@ public SkyFunctionName functionName() { */ protected final Collection getDirectPrerequisites(ConfiguredTarget target) throws TransitionException, InvalidConfigurationException, InconsistentAspectOrderException, - Failure, InterruptedException { + Failure { return view.getDirectPrerequisitesForTesting(reporter, target, masterConfig); } @@ -703,7 +697,6 @@ protected final ConfiguredTargetAndData getConfiguredTargetAndDataDirectPrerequi view.getConfiguredTargetAndDataDirectPrerequisitesForTesting( reporter, ctad.getConfiguredTarget(), - ctad.getConfiguredTarget().getConfigurationKey(), masterConfig)) { if (candidate.getConfiguredTarget().getLabel().equals(candidateLabel)) { return candidate; @@ -733,10 +726,10 @@ private static BuildOptions trimConfiguration( * comparison. * *

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> excludeFragmentOptions) { @@ -747,8 +740,9 @@ protected void assertConfigurationsEqual( .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 @@ public void rewriteWorkspace(String... lines) throws Exception { * @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 @@ protected ConfiguredTarget scratchConfiguredTarget( * @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 @@ protected ConfiguredTarget scratchConfiguredTarget( * @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 @@ protected ConfiguredTargetAndData scratchConfiguredTargetAndData( * @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 @@ protected ConfiguredTargetAndData scratchConfiguredTargetAndData( * @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 @@ private Artifact getPackageRelativeDerivedArtifact( } /** Returns the input {@link Artifact}s to the given {@link Action} with the given exec paths. */ - protected List getInputs(Action owner, Collection execPaths) { + protected static List getInputs(Action owner, Collection execPaths) { Set expectedPaths = new HashSet<>(execPaths); List result = new ArrayList<>(); for (Artifact output : owner.getInputs().toList()) { @@ -1664,11 +1649,11 @@ protected String stripCppPrefixForTrimmedConfigs(String outputPath) { : 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 @@ protected void assertSrcsValidityForRuleType(String packageName, String ruleType "'" + 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 @@ private ConfiguredTargetKey makeConfiguredTargetKey(String label) { .build(); } - protected static List actionInputsToPaths(NestedSet actionInputs) { + protected static ImmutableList actionInputsToPaths( + NestedSet actionInputs) { return ImmutableList.copyOf( - Iterables.transform( - actionInputs.toList(), (actionInput) -> actionInput.getExecPathString())); + Lists.transform(actionInputs.toList(), ActionInput::getExecPathString)); } /** @@ -1774,20 +1760,19 @@ protected void assertSameContentsWithCommonElements(Iterable artifacts, } /** - * 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 list, List sublist) { + protected static void assertContainsSublist(List list, List 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 list, List sublist) { + protected static void assertContainsSublist( + String message, List list, List sublist) { if (Collections.indexOfSubList(list, sublist) == -1) { fail((message == null ? "" : (message + ' ')) + "expected: <" + list + "> to contain sublist: <" + sublist + ">"); @@ -1795,10 +1780,10 @@ protected void assertContainsSublist(String message, List list, List collectRunfiles(ConfiguredTarget target) { + protected static NestedSet collectRunfiles(ConfiguredTarget target) { RunfilesProvider runfilesProvider = target.getProvider(RunfilesProvider.class); if (runfilesProvider != null) { return runfilesProvider.getDefaultRunfiles().getAllArtifacts(); @@ -1807,7 +1792,7 @@ protected NestedSet collectRunfiles(ConfiguredTarget target) { } } - protected NestedSet getFilesToBuild(TransitiveInfoCollection target) { + protected static NestedSet getFilesToBuild(TransitiveInfoCollection target) { return target.getProvider(FileProvider.class).getFilesToBuild(); } @@ -1851,15 +1836,16 @@ protected ImmutableList getFilesToBuildActions(ConfiguredTarget target) return ImmutableList.copyOf(result); } - protected NestedSet getOutputGroup( + protected static NestedSet getOutputGroup( TransitiveInfoCollection target, String outputGroup) { OutputGroupInfo provider = OutputGroupInfo.get(target); return provider == null - ? NestedSetBuilder.emptySet(Order.STABLE_ORDER) + ? NestedSetBuilder.emptySet(Order.STABLE_ORDER) : provider.getOutputGroup(outputGroup); } - protected NestedSet getExtraActionArtifacts(ConfiguredTarget target) { + protected static NestedSet getExtraActionArtifacts( + ConfiguredTarget target) { return target.getProvider(ExtraActionArtifactsProvider.class).getExtraActionArtifacts(); } @@ -1867,15 +1853,15 @@ protected Artifact getExecutable(String label) throws Exception { 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 getFilesToRun(TransitiveInfoCollection target) { + protected static NestedSet getFilesToRun(TransitiveInfoCollection target) { return target.getProvider(FilesToRunProvider.class).getFilesToRun(); } - protected NestedSet getFilesToRun(Label label) throws Exception { + protected NestedSet getFilesToRun(Label label) { return getConfiguredTarget(label, targetConfig) .getProvider(FilesToRunProvider.class).getFilesToRun(); } @@ -1888,7 +1874,7 @@ protected RunfilesSupport getRunfilesSupport(String label) throws Exception { 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 @@ protected AnalysisResult update(List targets, boolean doAnalysis, EventBus eventBus) throws Exception { return update( - targets, ImmutableList.of(), keepGoing, loadingPhaseThreads, doAnalysis, eventBus); + targets, ImmutableList.of(), keepGoing, loadingPhaseThreads, doAnalysis, eventBus); } protected AnalysisResult update( @@ -2058,51 +2044,53 @@ public static Set