From 86409b7a248d1cb966268451f9aa4db0763c3eb2 Mon Sep 17 00:00:00 2001 From: jhorvitz Date: Mon, 4 Oct 2021 16:55:20 -0700 Subject: [PATCH] Remove code related to trimming the host configuration. The `FragmentClassSet` passed to `BuildConfiguration#clone` is always the full set of fragments, so no trimming is ever performed. PiperOrigin-RevId: 400847795 --- .../analysis/config/BuildConfiguration.java | 57 ------------------ .../build/lib/skyframe/AspectFunction.java | 11 ++-- .../skyframe/ConfiguredTargetFunction.java | 2 +- .../build/lib/skyframe/SkyframeBuildView.java | 59 ++----------------- .../lib/analysis/LateBoundSplitUtil.java | 1 - .../config/BuildConfigurationTest.java | 15 ----- .../genrule/GenRuleConfiguredTargetTest.java | 9 ++- .../ConfigurationsForTargetsTest.java | 2 +- 8 files changed, 15 insertions(+), 141 deletions(-) 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 5d2d77bf26a755..e11d1b1938c454 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 @@ -14,8 +14,6 @@ package com.google.devtools.build.lib.analysis.config; -import static com.google.common.collect.ImmutableSortedMap.toImmutableSortedMap; - import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Suppliers; import com.google.common.collect.ArrayListMultimap; @@ -51,7 +49,6 @@ import java.util.Objects; import java.util.Set; import java.util.TreeMap; -import java.util.function.Function; import java.util.function.Supplier; import net.starlark.java.annot.StarlarkAnnotations; import net.starlark.java.annot.StarlarkBuiltin; @@ -124,28 +121,6 @@ public interface ActionEnvironmentProvider { private final boolean siblingRepositoryLayout; - /** - * Returns true if this configuration is semantically equal to the other, with the possible - * exception that the other has fewer fragments. - * - *

This is useful for trimming: as the same configuration gets "trimmed" while going down a - * dependency chain, it's still the same configuration but loses some of its fragments. So we need - * a more nuanced concept of "equality" than simple reference equality. - */ - // TODO(b/121048710): make this reflect starlark options - public boolean equalsOrIsSupersetOf(BuildConfiguration other) { - return this.equals(other) - || (other != null - // TODO(gregce): add back in output root checking. This requires a better approach to - // configuration-safe output paths. If the parent config has a fragment the child config - // doesn't, it may inject $(FOO) into the output roots. So the child bindir might be - // "bazel-out/arm-linux-fastbuild/bin" while the parent bindir is - // "bazel-out/android-arm-linux-fastbuild/bin". That's pretty awkward to check here. - // && outputRoots.equals(other.outputRoots) - && fragments.values().containsAll(other.fragments.values()) - && buildOptions.getNativeOptions().containsAll(other.buildOptions.getNativeOptions())); - } - /** * Returns {@code true} if this configuration is semantically equal to the other, including * checking that both have the same sets of fragments and options. @@ -259,38 +234,6 @@ public BuildConfiguration( this.commandLineLimits = new CommandLineLimits(options.minParamFileSize); } - /** - * Returns a copy of this configuration only including the given fragments (which the current - * configuration is assumed to have). - */ - public BuildConfiguration clone( - FragmentClassSet fragmentClasses, RuleClassProvider ruleClassProvider) { - ImmutableSortedMap, Fragment> fragmentsMap = - fragments.values().stream() - .filter(fragment -> fragmentClasses.contains(fragment.getClass())) - .collect( - toImmutableSortedMap( - FragmentClassSet.LEXICAL_FRAGMENT_SORTER, - Fragment::getClass, - Function.identity())); - BuildOptions options = - buildOptions.trim(getOptionsClasses(fragmentsMap.keySet(), ruleClassProvider)); - try { - return new BuildConfiguration( - getDirectories(), - fragmentsMap, - fragmentClasses, - options, - reservedActionMnemonics, - actionEnv, - mainRepositoryName, - siblingRepositoryLayout); - } catch (InvalidMnemonicException e) { - throw new IllegalStateException( - "Invalid mnemonic unexpected when cloning: " + this + ", " + fragmentClasses, e); - } - } - /** Returns the config fragment options classes used by the given fragment types. */ public static Set> getOptionsClasses( Iterable> fragmentClasses, RuleClassProvider ruleClassProvider) { 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 fbe50abdd88c55..2633d473afdea4 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 @@ -158,8 +158,7 @@ public static StarlarkDefinedAspect loadStarlarkDefinedAspect( Object starlarkValue = bzlLoadValue.getModule().getGlobal(starlarkValueName); if (starlarkValue == null) { throw new ConversionException( - String.format( - "%s is not exported from %s", starlarkValueName, extensionLabel.toString())); + String.format("%s is not exported from %s", starlarkValueName, extensionLabel)); } if (!(starlarkValue instanceof StarlarkDefinedAspect)) { throw new ConversionException( @@ -296,7 +295,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) if (AliasProvider.isAlias(associatedTarget)) { return createAliasAspect( env, - view.getHostConfiguration(aspectConfiguration), + view.getHostConfiguration(), new TargetAndConfiguration(target, configuration), aspect, key, @@ -427,7 +426,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) .build(), shouldUseToolchainTransition(configuration, aspect.getDefinition()), ruleClassProvider, - view.getHostConfiguration(originalTargetAndAspectConfiguration.getConfiguration()), + view.getHostConfiguration(), transitivePackagesForPackageRootResolution, transitiveRootCauses); } catch (ConfiguredValueCreationException e) { @@ -729,7 +728,7 @@ private AspectValue createAliasAspect( transitivePackagesForPackageRootResolution); } - private AspectValue createAliasAspect( + private static AspectValue createAliasAspect( Environment env, Target originalTarget, AspectKey originalKey, @@ -875,7 +874,7 @@ private AspectValue createAspect( configConditions, toolchainContext, aspectConfiguration, - view.getHostConfiguration(aspectConfiguration), + view.getHostConfiguration(), key); } catch (MissingDepException e) { Preconditions.checkState(env.valuesMissing()); 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 3a6fac2a5cbfc3..b3f319acc115c0 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 @@ -328,7 +328,7 @@ public SkyValue compute(SkyKey key, Environment env) : unloadedToolchainContexts.asToolchainContexts(), DependencyResolver.shouldUseToolchainTransition(configuration, ctgValue.getTarget()), ruleClassProvider, - view.getHostConfiguration(configuration), + view.getHostConfiguration(), transitivePackagesForPackageRootResolution, transitiveRootCauses); if (!transitiveRootCauses.isEmpty()) { 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 a4ea0ebda6b4e6..bf89fd42e7c9ca 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 @@ -59,7 +59,6 @@ import com.google.devtools.build.lib.analysis.config.BuildOptions.OptionsDiff; import com.google.devtools.build.lib.analysis.config.ConfigConditions; import com.google.devtools.build.lib.analysis.config.CoreOptions; -import com.google.devtools.build.lib.analysis.config.FragmentClassSet; import com.google.devtools.build.lib.bugreport.BugReport; import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos; import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.BuildEventId.ConfigurationId; @@ -146,10 +145,6 @@ public final class SkyframeBuildView { // The host configuration containing all fragments used by this build's transitive closure. private BuildConfiguration topLevelHostConfiguration; - // Fragment-limited versions of the host configuration. It's faster to create/cache these here - // than to store them in Skyframe. - private final Map hostConfigurationCache = - Maps.newConcurrentMap(); private BuildConfigurationCollection configurations; @@ -351,13 +346,9 @@ public BuildConfigurationCollection getBuildConfigurationCollection() { /** * Sets the host configuration consisting of all fragments that will be used by the top level * targets' transitive closures. - * - *

This is used to power {@link #getHostConfiguration} during analysis, which computes - * fragment-trimmed host configurations from the top-level one. */ private void setTopLevelHostConfiguration(BuildConfiguration topLevelHostConfiguration) { if (!topLevelHostConfiguration.equals(this.topLevelHostConfiguration)) { - hostConfigurationCache.clear(); this.topLevelHostConfiguration = topLevelHostConfiguration; } } @@ -1117,7 +1108,7 @@ ConfiguredTarget createConfiguredTarget( artifactFactory, target, configuration, - getHostConfiguration(configuration), + topLevelHostConfiguration, configuredTargetKey, prerequisiteMap, configConditions, @@ -1126,55 +1117,13 @@ ConfiguredTarget createConfiguredTarget( } /** - * Returns the host configuration trimmed to the same fragments as the input configuration. If the - * input is null, returns the top-level host configuration. + * Returns the top-level host configuration. * *

This may only be called after {@link #setTopLevelHostConfiguration} has set the correct host * configuration at the top-level. */ - public BuildConfiguration getHostConfiguration(BuildConfiguration config) { - if (config == null) { - return topLevelHostConfiguration; - } - // Currently, a single build doesn't use many different BuildConfiguration instances. Thus, - // having a cache per BuildConfiguration is efficient. It might lead to instances of otherwise - // identical configurations if multiple of these configs use the same fragment classes. However, - // these are cheap especially if there is only a small number of configs. Revisit and turn into - // a cache per FragmentClassSet if configuration trimming results in a much higher number of - // configuration instances. - BuildConfiguration hostConfig = hostConfigurationCache.get(config); - if (hostConfig != null) { - return hostConfig; - } - // TODO(bazel-team): have the fragment classes be those required by the consuming target's - // transitive closure. This isn't the same as the input configuration's fragment classes - - // the latter may be a proper subset of the former. - // - // ConfigurationFactory.getConfiguration provides the reason why: if a declared required - // fragment is evaluated and returns null, it never gets added to the configuration. So if we - // use the configuration's fragments as the source of truth, that excludes required fragments - // that never made it in. - // - // If we're just trimming an existing configuration, this is no big deal (if the original - // configuration doesn't need the fragment, the trimmed one doesn't either). But this method - // trims a host configuration to the same scope as a target configuration. Since their options - // are different, the host instance may actually be able to produce the fragment. So it's - // wrong and potentially dangerous to unilaterally exclude it. - FragmentClassSet fragmentClasses = ruleClassProvider.getConfigurationFragments(); - // TODO(bazel-team): investigate getting the trimmed config from Skyframe instead of cloning. - // This is the only place we instantiate BuildConfigurations outside of Skyframe, This can - // produce surprising effects, such as requesting a configuration that's in the Skyframe cache - // but still produces a unique instance because we don't check that cache. It'd be nice to - // guarantee that *all* instantiations happen through Skyframe. That could, for example, - // guarantee that config1.equals(config2) implies config1 == config2, which is nice for - // verifying we don't accidentally create extra configurations. But unfortunately, - // hostConfigurationCache was specifically created because Skyframe is too slow for this use - // case. So further optimization is necessary to make that viable (proto_library in particular - // contributes to much of the difference). - BuildConfiguration trimmedConfig = - topLevelHostConfiguration.clone(fragmentClasses, ruleClassProvider); - hostConfigurationCache.put(config, trimmedConfig); - return trimmedConfig; + public BuildConfiguration getHostConfiguration() { + return topLevelHostConfiguration; } /** diff --git a/src/test/java/com/google/devtools/build/lib/analysis/LateBoundSplitUtil.java b/src/test/java/com/google/devtools/build/lib/analysis/LateBoundSplitUtil.java index 9244975450a574..683da82038ad7d 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/LateBoundSplitUtil.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/LateBoundSplitUtil.java @@ -14,7 +14,6 @@ package com.google.devtools.build.lib.analysis; - import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.analysis.config.BuildOptions; import com.google.devtools.build.lib.analysis.config.Fragment; diff --git a/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationTest.java b/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationTest.java index 2154d936dee383..502e82fb78a8e0 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationTest.java @@ -19,7 +19,6 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.devtools.build.lib.analysis.config.BuildOptions.MapBackedChecksumCache; import com.google.devtools.build.lib.analysis.config.BuildOptions.OptionsChecksumCache; @@ -202,20 +201,6 @@ public void testGetTransitiveOptionDetails() throws Exception { assertThat(create().getTransitiveOptionDetails().getOptionValue("test_filter")).isNull(); } - @Test - public void testEqualsOrIsSupersetOf() throws Exception { - BuildConfiguration config = create(); - BuildConfiguration trimmedConfig = - config.clone( - FragmentClassSet.of(ImmutableSet.of(CppConfiguration.class)), - analysisMock.createRuleClassProvider()); - BuildConfiguration hostConfig = createHost(); - - assertThat(config.equalsOrIsSupersetOf(trimmedConfig)).isTrue(); - assertThat(config.equalsOrIsSupersetOf(hostConfig)).isFalse(); - assertThat(trimmedConfig.equalsOrIsSupersetOf(config)).isFalse(); - } - @Test public void testConfigFragmentsAreShareableAcrossConfigurations() throws Exception { // Note we can't use any fragments that load files (e.g. CROSSTOOL) because those get diff --git a/src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleConfiguredTargetTest.java b/src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleConfiguredTargetTest.java index 5bf71f059cb318..fda82d36dfb24a 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleConfiguredTargetTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleConfiguredTargetTest.java @@ -44,7 +44,7 @@ public class GenRuleConfiguredTargetTest extends BuildViewTestCase { private static final Pattern SETUP_COMMAND_PATTERN = Pattern.compile(".*/genrule-setup.sh;\\s+(?.*)"); - private void assertCommandEquals(String expected, String command) { + private static void assertCommandEquals(String expected, String command) { // Ensure the command after the genrule setup is correct. Matcher m = SETUP_COMMAND_PATTERN.matcher(command); if (m.matches()) { @@ -417,8 +417,7 @@ public void testToolsAreHostConfiguration() throws Exception { foundSrc = true; break; case "tool": - assertThat(getHostConfiguration().equalsOrIsSupersetOf(getConfiguration(prereq))) - .isTrue(); + assertThat(getHostConfiguration()).isEqualTo(getConfiguration(prereq)); foundTool = true; break; case GENRULE_SETUP_PATH: @@ -535,7 +534,7 @@ private void assertStamped(String target) throws Exception { assertStamped(getConfiguredTarget(target)); } - private void assertStamped(ConfiguredTarget target) throws Exception { + private void assertStamped(ConfiguredTarget target) { Artifact out = getFilesToBuild(target).toList().get(0); List inputs = ActionsTestUtil.baseArtifactNames(getGeneratingAction(out).getInputs()); assertThat(inputs).containsAtLeast("build-info.txt", "build-changelist.txt"); @@ -545,7 +544,7 @@ private void assertNotStamped(String target) throws Exception { assertNotStamped(getConfiguredTarget(target)); } - private void assertNotStamped(ConfiguredTarget target) throws Exception { + private void assertNotStamped(ConfiguredTarget target) { Artifact out = getFilesToBuild(target).toList().get(0); List inputs = ActionsTestUtil.baseArtifactNames(getGeneratingAction(out).getInputs()); assertThat(inputs).doesNotContain("build-info.txt"); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsTest.java index f97b1a38ba13ab..4feefff3652e28 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsTest.java @@ -261,7 +261,7 @@ public void targetDeps() throws Exception { BuildConfiguration topLevelConfiguration = getConfiguration(Iterables.getOnlyElement(update("//a:binary").getTargetsToBuild())); for (ConfiguredTarget dep : deps) { - assertThat(topLevelConfiguration.equalsOrIsSupersetOf(getConfiguration(dep))).isTrue(); + assertThat(topLevelConfiguration).isEqualTo(getConfiguration(dep)); } }