diff options
author | Greg Estren <gregce@google.com> | 2016-12-07 16:47:02 +0000 |
---|---|---|
committer | Kristina Chodorow <kchodorow@google.com> | 2016-12-07 22:20:15 +0000 |
commit | dc388246ff56d4cdd95fbeebba7f78ff3097e0c4 (patch) | |
tree | ed3f31fddfdf7677dfd3bc26ef553a386cf81f45 /src | |
parent | 13dc56add07ce31580b924dc9289507a00518e54 (diff) |
Provide deterministic order for split configured deps (roll forward part 2).
Also:
- Make ConfiguredTargetFunction.getDynamicConfigurations more readable.
- Add a bit more testing coverage for configured dep resolution.
This is a roll forward of commit 7505d94c19727e3100ac5e16a960bff2cb324f23. The original changed failed for two
reasons:
1) Windows-only: "ppc" wasn't recognized as a valid cpu:
https://github.com/bazelbuild/bazel/issues/2191
2) Bazel requires android_binary's "manifest" attribute to be "AndroidManifest.xml":
https://www.google.com/url?sa=D&q=http%3A%2F%2Fci.bazel.io%2Fjob%2Fbazel-tests%2FBAZEL_VERSION%3DHEAD%2CPLATFORM_NAME%3Dubuntu_15.10-x86_64%2FlastCompletedBuild%2FtestReport%2F
This version uses "armeabi-v7a" instead of "ppc" and "AndroidManifest.xml"
in the splitDeps test.
--
PiperOrigin-RevId: 141313454
MOS_MIGRATED_REVID=141313454
Diffstat (limited to 'src')
-rw-r--r-- | src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java | 97 | ||||
-rw-r--r-- | src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsTest.java | 64 |
2 files changed, 130 insertions, 31 deletions
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 df1b7c94a5..c6051e715c 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 @@ -80,6 +80,8 @@ import com.google.devtools.build.skyframe.ValueOrException; import com.google.devtools.build.skyframe.ValueOrException2; import java.util.ArrayList; import java.util.Collection; +import java.util.Collections; +import java.util.Comparator; import java.util.HashSet; import java.util.Iterator; import java.util.LinkedHashMap; @@ -485,15 +487,23 @@ final class ConfiguredTargetFunction implements SkyFunction { ctgValue.getConfiguration().fragmentClasses(); BuildOptions ctgOptions = ctgValue.getConfiguration().getOptions(); - // Stores the trimmed versions of each dependency. This method must preserve the original label - // ordering of each attribute. For example, if originalDeps.get("data") is [":a", ":b"], the - // trimmed variant must also be [":a", ":b"] in the same order. Because we may not actualize - // the results in order (some results need Skyframe-evaluated configurations while others can - // be computed trivially), we dump them all into this map, then as a final step iterate through - // the original list and pluck out values from here for the final value. + // Stores the dynamically configured versions of each dependency. This method must preserve the + // original label ordering of each attribute. For example, if originalDeps.get("data") is + // [":a", ":b"], the dynamic variant must also be [":a", ":b"] in the same order. Because we may + // not actualize the results in order (some results need Skyframe-evaluated configurations while + // others can be computed trivially), we dump them all into this map, then as a final step + // iterate through the original list and pluck out values from here for the final value. + // + // For split transitions, originaldeps.get("data") = [":a", ":b"] can produce the output + // [":a"<config1>, ":a"<config2>, ..., ":b"<config1>, ":b"<config2>, ...]. All instances of ":a" + // still appear before all instances of ":b". But the [":a"<config1>, ":a"<config2>"] subset may + // be in any (deterministic) order. In particular, this may not be the same order as + // SplitTransition.split. If needed, this code can be modified to use that order, but that + // involves more runtime in performance-critical code, so we won't make that change without a + // clear need. // // This map is used heavily by all builds. Inserts and gets should be as fast as possible. - Multimap<AttributeAndLabel, Dependency> trimmedDeps = LinkedHashMultimap.create(); + Multimap<AttributeAndLabel, Dependency> dynamicDeps = LinkedHashMultimap.create(); // Performance optimization: This method iterates over originalDeps twice. By storing // AttributeAndLabel instances in this list, we avoid having to recreate them the second time @@ -540,7 +550,7 @@ final class ConfiguredTargetFunction implements SkyFunction { if (transition == Attribute.ConfigurationTransition.NONE) { // The dep uses the same exact configuration. putOnlyEntry( - trimmedDeps, + dynamicDeps, attributeAndLabel, Dependency.withConfigurationAndAspects( dep.getLabel(), ctgValue.getConfiguration(), dep.getAspects())); @@ -552,7 +562,7 @@ final class ConfiguredTargetFunction implements SkyFunction { // to incur multiple host transitions. So we aggressively optimize to avoid hurting // analysis time. putOnlyEntry( - trimmedDeps, + dynamicDeps, attributeAndLabel, Dependency.withConfigurationAndAspects( dep.getLabel(), hostConfiguration, dep.getAspects())); @@ -564,7 +574,6 @@ final class ConfiguredTargetFunction implements SkyFunction { FragmentsAndTransition transitionKey = new FragmentsAndTransition(depFragments, transition); List<BuildOptions> toOptions = transitionsMap.get(transitionKey); if (toOptions == null) { - ImmutableList.Builder<BuildOptions> toOptionsBuilder = ImmutableList.builder(); toOptions = getDynamicTransitionOptions(ctgOptions, transition, depFragments, ruleClassProvider, !sameFragments); transitionsMap.put(transitionKey, toOptions); @@ -575,7 +584,7 @@ final class ConfiguredTargetFunction implements SkyFunction { if (sameFragments && toOptions.size() == 1 && Iterables.getOnlyElement(toOptions).equals(ctgOptions)) { putOnlyEntry( - trimmedDeps, + dynamicDeps, attributeAndLabel, Dependency.withConfigurationAndAspects( dep.getLabel(), ctgValue.getConfiguration(), dep.getAspects())); @@ -607,9 +616,9 @@ final class ConfiguredTargetFunction implements SkyFunction { Dependency resolvedDep = Dependency.withConfigurationAndAspects(originalDep.getLabel(), trimmedConfig.getConfiguration(), originalDep.getAspects()); if (attr.attribute.hasSplitConfigurationTransition()) { - trimmedDeps.put(attr, resolvedDep); + dynamicDeps.put(attr, resolvedDep); } else { - putOnlyEntry(trimmedDeps, attr, resolvedDep); + putOnlyEntry(dynamicDeps, attr, resolvedDep); } } } @@ -617,21 +626,7 @@ final class ConfiguredTargetFunction implements SkyFunction { throw new DependencyEvaluationException(e); } - // Re-assemble the output map with the same value ordering (e.g. each attribute's dep labels - // appear in the same order) as the input. - Iterator<AttributeAndLabel> iterator = attributesAndLabels.iterator(); - OrderedSetMultimap<Attribute, Dependency> result = OrderedSetMultimap.create(); - for (Map.Entry<Attribute, Dependency> depsEntry : originalDeps.entries()) { - AttributeAndLabel attrAndLabel = iterator.next(); - if (depsEntry.getValue().hasStaticConfiguration()) { - result.put(attrAndLabel.attribute, depsEntry.getValue()); - } else { - Collection<Dependency> trimmedAttrDeps = trimmedDeps.get(attrAndLabel); - Verify.verify(!trimmedAttrDeps.isEmpty()); - result.putAll(depsEntry.getKey(), trimmedAttrDeps); - } - } - return result; + return sortDynamicallyConfiguredDeps(originalDeps, dynamicDeps, attributesAndLabels); } /** @@ -734,6 +729,52 @@ final class ConfiguredTargetFunction implements SkyFunction { } } + /** + * Determines the output ordering of each <attribute, depLabel> -> + * [dep<config1>, dep<config2>, ...] collection produced by a split transition. + */ + @VisibleForTesting + static final Comparator<Dependency> DYNAMIC_SPLIT_DEP_ORDERING = + new Comparator<Dependency>() { + @Override + public int compare(Dependency d1, Dependency d2) { + return d1.getConfiguration().getMnemonic().compareTo(d2.getConfiguration().getMnemonic()); + } + }; + + /** + * Helper method for {@link #getDynamicConfigurations}: returns a copy of the output deps + * using the same key and value ordering as the input deps. + * + * @param originalDeps the input deps with the ordering to preserve + * @param dynamicDeps the unordered output deps + * @param attributesAndLabels collection of <attribute, depLabel> pairs guaranteed to match + * the ordering of originalDeps.entries(). This is a performance optimization: see + * {@link #getDynamicConfigurations#attributesAndLabels} for details. + */ + private static OrderedSetMultimap<Attribute, Dependency> sortDynamicallyConfiguredDeps( + OrderedSetMultimap<Attribute, Dependency> originalDeps, + Multimap<AttributeAndLabel, Dependency> dynamicDeps, + ArrayList<AttributeAndLabel> attributesAndLabels) { + Iterator<AttributeAndLabel> iterator = attributesAndLabels.iterator(); + OrderedSetMultimap<Attribute, Dependency> result = OrderedSetMultimap.create(); + for (Map.Entry<Attribute, Dependency> depsEntry : originalDeps.entries()) { + AttributeAndLabel attrAndLabel = iterator.next(); + if (depsEntry.getValue().hasStaticConfiguration()) { + result.put(attrAndLabel.attribute, depsEntry.getValue()); + } else { + Collection<Dependency> dynamicAttrDeps = dynamicDeps.get(attrAndLabel); + Verify.verify(!dynamicAttrDeps.isEmpty()); + if (dynamicAttrDeps.size() > 1) { + List<Dependency> sortedSplitList = new ArrayList<>(dynamicAttrDeps); + Collections.sort(sortedSplitList, DYNAMIC_SPLIT_DEP_ORDERING); + dynamicAttrDeps = sortedSplitList; + } + result.putAll(depsEntry.getKey(), dynamicAttrDeps); + } + } + return result; + } /** * Merges the each direct dependency configured target with the aspects associated with it. 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 e6bfebc8ac..3dda587c2e 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 @@ -27,6 +27,7 @@ import com.google.common.collect.ListMultimap; import com.google.common.collect.Multimap; import com.google.common.collect.SetMultimap; import com.google.devtools.build.lib.analysis.ConfiguredTarget; +import com.google.devtools.build.lib.analysis.Dependency; import com.google.devtools.build.lib.analysis.DependencyResolver; import com.google.devtools.build.lib.analysis.TargetAndConfiguration; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; @@ -50,7 +51,8 @@ import com.google.devtools.build.skyframe.SkyFunctionException; import com.google.devtools.build.skyframe.SkyFunctionName; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; -import java.util.Collection; +import java.util.List; + import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -209,12 +211,12 @@ public class ConfigurationsForTargetsTest extends AnalysisTestCase { * * <p>Throws an exception if the attribute can't be found. */ - private Collection<ConfiguredTarget> getConfiguredDeps(String targetLabel, String attrName) + private List<ConfiguredTarget> getConfiguredDeps(String targetLabel, String attrName) throws Exception { Multimap<Attribute, ConfiguredTarget> allDeps = getConfiguredDeps(targetLabel); for (Attribute attribute : allDeps.keySet()) { if (attribute.getName().equals(attrName)) { - return allDeps.get(attribute); + return ImmutableList.copyOf(allDeps.get(attribute)); } } throw new AssertionError( @@ -261,6 +263,62 @@ public class ConfigurationsForTargetsTest extends AnalysisTestCase { assertThat(genIn.getConfiguration()).isNull(); } + @Test + public void targetDeps() throws Exception { + scratch.file( + "a/BUILD", + "cc_library(name = 'dep1', srcs = ['dep1.cc'])", + "cc_library(name = 'dep2', srcs = ['dep2.cc'])", + "cc_binary(name = 'binary', srcs = ['main.cc'], deps = [':dep1', ':dep2'])"); + List<ConfiguredTarget> deps = getConfiguredDeps("//a:binary", "deps"); + assertThat(deps).hasSize(2); + for (ConfiguredTarget dep : deps) { + assertThat(getTargetConfiguration().equalsOrIsSupersetOf(dep.getConfiguration())).isTrue(); + } + } + + @Test + public void hostDeps() throws Exception { + scratch.file( + "a/BUILD", + "cc_binary(name = 'host_tool', srcs = ['host_tool.cc'])", + "genrule(name = 'gen', srcs = [], cmd = '', outs = ['gen.out'], tools = [':host_tool'])"); + ConfiguredTarget toolDep = Iterables.getOnlyElement(getConfiguredDeps("//a:gen", "tools")); + assertThat(toolDep.getConfiguration().isHostConfiguration()).isTrue(); + } + + @Test + public void splitDeps() throws Exception { + scratch.file( + "java/a/BUILD", + "cc_library(name = 'lib', srcs = ['lib.cc'])", + "android_binary(name='a', manifest = 'AndroidManifest.xml', deps = [':lib'])"); + useConfiguration("--fat_apk_cpu=k8,armeabi-v7a"); + List<ConfiguredTarget> deps = getConfiguredDeps("//java/a:a", "deps"); + assertThat(deps).hasSize(2); + ConfiguredTarget dep1 = deps.get(0); + ConfiguredTarget dep2 = deps.get(1); + assertThat( + ImmutableList.<String>of( + 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); + } + } + /** Runs the same test with trimmed dynamic configurations. */ @TestSpec(size = Suite.SMALL_TESTS) @RunWith(JUnit4.class) |