From 5bae50aa8479096a2038cdf7b3a4cf744a84de6a Mon Sep 17 00:00:00 2001 From: gregce Date: Thu, 17 Aug 2017 23:23:47 +0200 Subject: Remove BuildConfiguration.useDynamicConfigurations. This is always true. Part of the static config cleanup effort. PiperOrigin-RevId: 165628823 --- .../devtools/build/lib/analysis/BuildViewTest.java | 28 +++++++--------------- .../build/lib/analysis/util/AnalysisTestCase.java | 2 +- .../build/lib/analysis/util/AnalysisTestUtil.java | 25 +++++++++---------- .../build/lib/analysis/util/BuildViewTestCase.java | 12 ++++------ .../lib/analysis/util/ConfigurationTestCase.java | 4 +--- .../lib/skyframe/ConfigurationsForTargetsTest.java | 20 +++++----------- 6 files changed, 31 insertions(+), 60 deletions(-) (limited to 'src/test/java/com/google/devtools/build/lib') diff --git a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java index 72c18f2ae3..183c8c05f7 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java @@ -17,7 +17,6 @@ package com.google.devtools.build.lib.analysis; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertWithMessage; import static com.google.devtools.build.lib.packages.Attribute.attr; -import static com.google.devtools.build.lib.testutil.MoreAsserts.assertEventCount; import static com.google.devtools.build.lib.testutil.MoreAsserts.assertEventCountAtLeast; import static org.junit.Assert.fail; @@ -393,21 +392,12 @@ public class BuildViewTest extends BuildViewTestBase { Iterable targets = getView().getDirectPrerequisiteDependenciesForTesting( reporter, top, getBuildConfigurationCollection()).values(); - Dependency innerDependency; - Dependency fileDependency; - if (top.getConfiguration().useDynamicConfigurations()) { - innerDependency = - Dependency.withTransitionAndAspects( - Label.parseAbsolute("//package:inner"), - Attribute.ConfigurationTransition.NONE, - AspectCollection.EMPTY); - } else { - innerDependency = - Dependency.withConfiguration( - Label.parseAbsolute("//package:inner"), - getTargetConfiguration()); - } - fileDependency = + Dependency innerDependency = + Dependency.withTransitionAndAspects( + Label.parseAbsolute("//package:inner"), + Attribute.ConfigurationTransition.NONE, + AspectCollection.EMPTY); + Dependency fileDependency = Dependency.withNullConfiguration( Label.parseAbsolute("//package:file")); @@ -541,11 +531,9 @@ public class BuildViewTest extends BuildViewTestBase { // the transitive target closure) and in the normal configured target cycle detection path. // So we get an additional instance of this check (which varies depending on whether Skyframe // loading phase is enabled). - // TODO(gregce): refactor away this variation. Note that the duplicate doesn't make it into + // TODO(gregce): Fix above and uncomment the below. Note that the duplicate doesn't make it into // real user output (it only affects tests). - if (!getTargetConfiguration().useDynamicConfigurations()) { - assertEventCount(3, eventCollector); - } + // assertEventCount(3, eventCollector); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java index ff655baad9..5c88265159 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java @@ -286,7 +286,7 @@ public abstract class AnalysisTestCase extends FoundationTestCase { throws InterruptedException { BuildConfiguration targetConfig = Iterables.getOnlyElement(masterConfig.getTargetConfigurations()); - if (useDynamicVersionIfEnabled && targetConfig.useDynamicConfigurations()) { + if (useDynamicVersionIfEnabled) { return skyframeExecutor.getConfigurationForTesting( reporter, targetConfig.fragmentClasses(), targetConfig.getOptions()); } else { diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestUtil.java b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestUtil.java index e04eae3c67..7e2400da9a 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestUtil.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestUtil.java @@ -432,20 +432,17 @@ public final class AnalysisTestUtil { hostConfiguration.getMiddlemanDirectory(RepositoryName.MAIN).getPath().toString(), "internal(host)"); - if (targetConfiguration.useDynamicConfigurations()) { - // With dynamic configurations, the output paths that bin, genfiles, etc. refer to may - // or may not include the C++-contributed pieces. e.g. they may be - // bazel-out/gcc-X-glibc-Y-k8-fastbuild/ or they may be bazel-out/fastbuild/. This code - // adds support for the non-C++ case, too. - Map prunedRootMap = new HashMap<>(); - for (Map.Entry root : rootMap.entrySet()) { - prunedRootMap.put( - OUTPUT_PATH_CPP_PREFIX_PATTERN.matcher(root.getKey()).replaceFirst(""), - root.getValue() - ); - } - rootMap.putAll(prunedRootMap); - } + // The output paths that bin, genfiles, etc. refer to may or may not include the C++-contributed + // pieces. e.g. they may be bazel-out/gcc-X-glibc-Y-k8-fastbuild/ or they may be + // bazel-out/fastbuild/. This code adds support for the non-C++ case, too. + Map prunedRootMap = new HashMap<>(); + for (Map.Entry root : rootMap.entrySet()) { + prunedRootMap.put( + OUTPUT_PATH_CPP_PREFIX_PATTERN.matcher(root.getKey()).replaceFirst(""), + root.getValue() + ); + } + rootMap.putAll(prunedRootMap); Set files = new LinkedHashSet<>(); for (Artifact artifact : artifacts) { 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 1f0a7a4e99..c00b06e08f 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 @@ -1304,16 +1304,12 @@ public abstract class BuildViewTestCase extends FoundationTestCase { BuildConfiguration config; try { config = getConfiguredTarget(label).getConfiguration(); + config = view.getDynamicConfigurationForTesting(getTarget(label), config, reporter); } catch (LabelSyntaxException e) { throw new IllegalArgumentException(e); - } - if (targetConfig.useDynamicConfigurations()) { - try { - config = view.getDynamicConfigurationForTesting(getTarget(label), config, reporter); - } catch (Exception e) { - //TODO(b/36585204): Clean this up - throw new RuntimeException(e); - } + } catch (Exception e) { + //TODO(b/36585204): Clean this up + throw new RuntimeException(e); } return new ConfiguredTargetKey(makeLabel(label), config); } diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/ConfigurationTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/ConfigurationTestCase.java index f01405e538..c2c4c30138 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/ConfigurationTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/ConfigurationTestCase.java @@ -56,7 +56,6 @@ import com.google.devtools.common.options.OptionEffectTag; import com.google.devtools.common.options.Options; import com.google.devtools.common.options.OptionsBase; import com.google.devtools.common.options.OptionsParser; -import java.util.Collection; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -196,9 +195,8 @@ public abstract class ConfigurationTestCase extends FoundationTestCase { public void assertConfigurationsHaveUniqueOutputDirectories( BuildConfigurationCollection configCollection) throws Exception { - Collection allConfigs = configCollection.getAllConfigurations(); Map outputPaths = new HashMap<>(); - for (BuildConfiguration config : allConfigs) { + for (BuildConfiguration config : configCollection.getTargetConfigurations()) { if (config.isActionsEnabled()) { BuildConfiguration otherConfig = outputPaths.get( config.getOutputDirectory(RepositoryName.MAIN)); 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 46c1cf99ca..0ea433908a 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 @@ -315,20 +315,12 @@ public class ConfigurationsForTargetsTest extends AnalysisTestCase { dep1.getConfiguration().getCpu(), dep2.getConfiguration().getCpu())) .containsExactly("armeabi-v7a", "k8"); - // We don't care what order split deps are listed, but it must be deterministic. Static and - // dynamic configurations happen to apply different orders (static: same order as the split - // transition definition, dynamic: ConfiguredTargetFunction.DYNAMIC_SPLIT_DEP_ORDERING). - // That's okay because of the "we don't care what order" principle. The primary value of this - // test is to check against the new dynamic code, which will soon replace the static code - // anyway. And the static code is already well-tested through all other Blaze tests. And - // checking its order would be a lot uglier. So we only worry about the dynamic case here. - if (getTargetConfiguration().useDynamicConfigurations()) { - assertThat( - ConfiguredTargetFunction.DYNAMIC_SPLIT_DEP_ORDERING.compare( - Dependency.withConfiguration(dep1.getLabel(), dep1.getConfiguration()), - Dependency.withConfiguration(dep2.getLabel(), dep2.getConfiguration()))) - .isLessThan(0); - } + // We don't care what order split deps are listed, but it must be deterministic. + assertThat( + ConfiguredTargetFunction.DYNAMIC_SPLIT_DEP_ORDERING.compare( + Dependency.withConfiguration(dep1.getLabel(), dep1.getConfiguration()), + Dependency.withConfiguration(dep2.getLabel(), dep2.getConfiguration()))) + .isLessThan(0); } } } -- cgit v1.2.3