Modify UberRDotJavaUtil.getAndroidResourceDeps() so it no longer needs a DependencyGraph. Summary: This makes `UberRDotJavaUtil` more general, which will make it easier to repurpose for use by a standalone `aapt package` rule. Test Plan: Sandcastle builds.
diff --git a/src/com/facebook/buck/android/AndroidBinaryRule.java b/src/com/facebook/buck/android/AndroidBinaryRule.java index e5a1bc3..595afd2 100644 --- a/src/com/facebook/buck/android/AndroidBinaryRule.java +++ b/src/com/facebook/buck/android/AndroidBinaryRule.java
@@ -38,7 +38,6 @@ import com.facebook.buck.rules.Buildable; import com.facebook.buck.rules.BuildableContext; import com.facebook.buck.rules.BuildableProperties; -import com.facebook.buck.rules.DependencyGraph; import com.facebook.buck.rules.DoNotUseAbstractBuildable; import com.facebook.buck.rules.InstallableBuildRule; import com.facebook.buck.rules.RuleKey; @@ -477,11 +476,10 @@ steps.add(new MkdirAndSymlinkFileStep(getManifest().resolve(context).toString(), getAndroidManifestXml())); - final AndroidTransitiveDependencies transitiveDependencies = findTransitiveDependencies( - context.getDependencyGraph()); + final AndroidTransitiveDependencies transitiveDependencies = findTransitiveDependencies(); final AndroidDexTransitiveDependencies dexTransitiveDependencies = - findDexTransitiveDependencies(context.getDependencyGraph()); + findDexTransitiveDependencies(); // Add the steps for the aapt_package command. This method returns data that the ApkBuilder // needs to create a final, unsigned APK. @@ -866,13 +864,13 @@ return Optional.of(destination); } - public AndroidTransitiveDependencies findTransitiveDependencies(DependencyGraph graph) { - return getTransitiveDependencyGraph().findDependencies(getAndroidResourceDepsInternal(graph)); + public AndroidTransitiveDependencies findTransitiveDependencies() { + return getTransitiveDependencyGraph().findDependencies(getAndroidResourceDepsInternal()); } - public AndroidDexTransitiveDependencies findDexTransitiveDependencies(DependencyGraph graph) { + public AndroidDexTransitiveDependencies findDexTransitiveDependencies() { return getTransitiveDependencyGraph().findDexDependencies( - getAndroidResourceDepsInternal(graph), + getAndroidResourceDepsInternal(), buildRulesToExcludeFromDex); } @@ -880,9 +878,8 @@ * @return a list of {@link HasAndroidResourceDeps}s that should be passed, in order, to {@code aapt} * when generating the {@code R.java} files for this APK. */ - protected ImmutableList<HasAndroidResourceDeps> getAndroidResourceDepsInternal( - DependencyGraph graph) { - return UberRDotJavaUtil.getAndroidResourceDeps(this, graph); + protected ImmutableList<HasAndroidResourceDeps> getAndroidResourceDepsInternal() { + return UberRDotJavaUtil.getAndroidResourceDeps(this); } private boolean canSkipAaptResourcePackaging() {
diff --git a/src/com/facebook/buck/android/AndroidInstrumentationApk.java b/src/com/facebook/buck/android/AndroidInstrumentationApk.java index 356b035..4b791bb 100644 --- a/src/com/facebook/buck/android/AndroidInstrumentationApk.java +++ b/src/com/facebook/buck/android/AndroidInstrumentationApk.java
@@ -25,7 +25,6 @@ import com.facebook.buck.rules.BuildRuleParams; import com.facebook.buck.rules.BuildRuleResolver; import com.facebook.buck.rules.BuildRuleType; -import com.facebook.buck.rules.DependencyGraph; import com.facebook.buck.rules.InstallableBuildRule; import com.facebook.buck.rules.RuleKey; import com.facebook.buck.rules.SourcePath; @@ -105,13 +104,12 @@ } @Override - protected ImmutableList<HasAndroidResourceDeps> getAndroidResourceDepsInternal( - DependencyGraph graph) { + protected ImmutableList<HasAndroidResourceDeps> getAndroidResourceDepsInternal() { // Filter out the AndroidResourceRules that are needed by this APK but not the APK under test. ImmutableSet<HasAndroidResourceDeps> originalResources = ImmutableSet.copyOf( - UberRDotJavaUtil.getAndroidResourceDeps(apkUnderTest, graph)); + UberRDotJavaUtil.getAndroidResourceDeps(apkUnderTest)); ImmutableList<HasAndroidResourceDeps> instrumentationResources = - UberRDotJavaUtil.getAndroidResourceDeps(this, graph); + UberRDotJavaUtil.getAndroidResourceDeps(this); // Include all of the instrumentation resources first, in their original order. ImmutableList.Builder<HasAndroidResourceDeps> allResources = ImmutableList.builder();
diff --git a/src/com/facebook/buck/android/AndroidManifest.java b/src/com/facebook/buck/android/AndroidManifest.java index fb7270d..bc85177 100644 --- a/src/com/facebook/buck/android/AndroidManifest.java +++ b/src/com/facebook/buck/android/AndroidManifest.java
@@ -116,10 +116,9 @@ * @return a list of {@link AndroidResourceRule}s that should be passed, * in order, to {@code aapt} when generating the {@code R.java} files for this APK. */ - private ImmutableList<HasAndroidResourceDeps> getAndroidResourceDeps( - DependencyGraph graph) { + private ImmutableList<HasAndroidResourceDeps> getAndroidResourceDeps(DependencyGraph graph) { BuildRule self = graph.findBuildRuleByTarget(buildTarget); - return UberRDotJavaUtil.getAndroidResourceDeps(self, graph); + return UberRDotJavaUtil.getAndroidResourceDeps(self); } public static Builder newManifestMergeRuleBuilder(AbstractBuildRuleBuilderParams params) {
diff --git a/src/com/facebook/buck/android/AndroidResourceRule.java b/src/com/facebook/buck/android/AndroidResourceRule.java index e4a6e58..1e228c9 100644 --- a/src/com/facebook/buck/android/AndroidResourceRule.java +++ b/src/com/facebook/buck/android/AndroidResourceRule.java
@@ -172,7 +172,7 @@ // Searching through the deps, find any additional res directories to pass to aapt. ImmutableList<HasAndroidResourceDeps> androidResourceDeps = UberRDotJavaUtil.getAndroidResourceDeps( - this, context.getDependencyGraph()); + this); Set<String> resDirectories = ImmutableSet.copyOf( Iterables.transform(androidResourceDeps, GET_RES_FOR_RULE));
diff --git a/src/com/facebook/buck/android/UberRDotJavaUtil.java b/src/com/facebook/buck/android/UberRDotJavaUtil.java index ebf81c1..a725aa3 100644 --- a/src/com/facebook/buck/android/UberRDotJavaUtil.java +++ b/src/com/facebook/buck/android/UberRDotJavaUtil.java
@@ -16,6 +16,9 @@ package com.facebook.buck.android; +import com.facebook.buck.graph.DefaultImmutableDirectedAcyclicGraph; +import com.facebook.buck.graph.ImmutableDirectedAcyclicGraph; +import com.facebook.buck.graph.MutableDirectedGraph; import com.facebook.buck.graph.TopologicalSort; import com.facebook.buck.java.JavacInMemoryStep; import com.facebook.buck.java.JavacOptions; @@ -24,7 +27,6 @@ import com.facebook.buck.rules.BuildDependencies; import com.facebook.buck.rules.BuildRule; import com.facebook.buck.rules.BuildRuleType; -import com.facebook.buck.rules.DependencyGraph; import com.facebook.buck.step.Step; import com.facebook.buck.step.fs.MakeCleanDirectoryStep; import com.facebook.buck.step.fs.WriteFileStep; @@ -39,6 +41,8 @@ import com.google.common.collect.Maps; import com.google.common.collect.Sets; +import java.util.Collection; +import java.util.Collections; import java.util.Iterator; import java.util.Map; import java.util.Set; @@ -122,13 +126,55 @@ /** * Finds the transitive set of {@code rule}'s {@link AndroidResourceRule} dependencies with * non-null {@code res} directories, which can also include {@code rule} itself. - * This set will be returned as an {@link ImmutableList} with the rules topologically sorted as - * determined by {@code graph}. Rules will be ordered from least dependent to most dependent. + * This set will be returned as an {@link ImmutableList} with the rules topologically sorted. + * Rules will be ordered from least dependent to most dependent. + */ + public static ImmutableList<HasAndroidResourceDeps> getAndroidResourceDeps(BuildRule rule) { + return getAndroidResourceDeps(Collections.singleton(rule)); + } + + /** + * Finds the transitive set of {@code rules}' {@link AndroidResourceRule} dependencies with + * non-null {@code res} directories, which can also include any of the {@code rules} themselves. + * This set will be returned as an {@link ImmutableList} with the rules topologically sorted. + * Rules will be ordered from least dependent to most dependent. */ public static ImmutableList<HasAndroidResourceDeps> getAndroidResourceDeps( - BuildRule rule, - DependencyGraph graph) { - final Set<HasAndroidResourceDeps> allAndroidResourceRules = findAllAndroidResourceDeps(rule); + Collection<BuildRule> rules) { + // This visitor finds all AndroidResourceRules that are reachable from the specified rules via + // rules with types in the TRAVERSABLE_TYPES collection. It also builds up the dependency graph + // that was traversed to find the AndroidResourceRules. + final MutableDirectedGraph<BuildRule> mutableGraph = new MutableDirectedGraph<>(); + final ImmutableSet.Builder<HasAndroidResourceDeps> androidResources = ImmutableSet.builder(); + AbstractDependencyVisitor visitor = new AbstractDependencyVisitor(rules) { + + @Override + public ImmutableSet<BuildRule> visit(BuildRule rule) { + mutableGraph.addNode(rule); + + if (rule instanceof HasAndroidResourceDeps) { + HasAndroidResourceDeps androidResourceRule = (HasAndroidResourceDeps)rule; + if (androidResourceRule.getRes() != null) { + androidResources.add(androidResourceRule); + } + } + + // Only certain types of rules should be considered as part of this traversal. + BuildRuleType type = rule.getType(); + ImmutableSet<BuildRule> depsToVisit = maybeVisitAllDeps(rule, + TRAVERSABLE_TYPES.contains(type)); + for (BuildRule dep : depsToVisit) { + mutableGraph.addEdge(rule, dep); + } + return depsToVisit; + } + + }; + visitor.start(); + + final Set<HasAndroidResourceDeps> allAndroidResourceRules = androidResources.build(); + final ImmutableDirectedAcyclicGraph<BuildRule> graph = + new DefaultImmutableDirectedAcyclicGraph<>(mutableGraph); // Now that we have the transitive set of AndroidResourceRules, we need to return them in // topologically sorted order. This is critical because the order in which -S flags are passed @@ -160,30 +206,6 @@ } }; - private static ImmutableSet<HasAndroidResourceDeps> findAllAndroidResourceDeps(BuildRule buildRule) { - final ImmutableSet.Builder<HasAndroidResourceDeps> androidResources = ImmutableSet.builder(); - AbstractDependencyVisitor visitor = new AbstractDependencyVisitor(buildRule) { - - @Override - public ImmutableSet<BuildRule> visit(BuildRule rule) { - if (rule instanceof HasAndroidResourceDeps) { - HasAndroidResourceDeps androidResourceRule = (HasAndroidResourceDeps)rule; - if (androidResourceRule.getRes() != null) { - androidResources.add(androidResourceRule); - } - } - - // Only certain types of rules should be considered as part of this traversal. - BuildRuleType type = rule.getType(); - return maybeVisitAllDeps(rule, TRAVERSABLE_TYPES.contains(type)); - } - - }; - visitor.start(); - - return androidResources.build(); - } - /** * Aggregate information about a list of {@link AndroidResourceRule}s. */
diff --git a/src/com/facebook/buck/command/Project.java b/src/com/facebook/buck/command/Project.java index a6c2e90..2f88689 100644 --- a/src/com/facebook/buck/command/Project.java +++ b/src/com/facebook/buck/command/Project.java
@@ -350,7 +350,7 @@ if (srcRule instanceof AndroidBinaryRule) { AndroidBinaryRule androidBinary = (AndroidBinaryRule)srcRule; AndroidDexTransitiveDependencies binaryDexTransitiveDependencies = - androidBinary.findDexTransitiveDependencies(dependencyGraph); + androidBinary.findDexTransitiveDependencies(); noDxJarsBuilder.addAll(binaryDexTransitiveDependencies.noDxClasspathEntries); } @@ -360,7 +360,7 @@ ImmutableSet<String> noDxJars = noDxJarsBuilder.build(); // Update module dependencies to apply scope="PROVIDED", where appropriate. - markNoDxJarsAsProvided(modules, noDxJars, dependencyGraph); + markNoDxJarsAsProvided(modules, noDxJars); return modules; } @@ -664,10 +664,7 @@ * to the android_binary that <em>does not</em> list the library in its {@code no_dx} list. */ @VisibleForTesting - static void markNoDxJarsAsProvided( - List<Module> modules, - Set<String> noDxJars, - DependencyGraph dependencyGraph) { + static void markNoDxJarsAsProvided(List<Module> modules, Set<String> noDxJars) { Map<String, String> intelliJLibraryNameToJarPath = Maps.newHashMap(); for (String jarPath : noDxJars) { String libraryName = getIntellijNameForBinaryJar(jarPath); @@ -683,7 +680,7 @@ if (module.srcRule instanceof AndroidBinaryRule) { AndroidBinaryRule androidBinaryRule = (AndroidBinaryRule)module.srcRule; AndroidDexTransitiveDependencies dexTransitiveDependencies = - androidBinaryRule.findDexTransitiveDependencies(dependencyGraph); + androidBinaryRule.findDexTransitiveDependencies(); classpathEntriesToDex = Sets.newHashSet(Sets.intersection(noDxJars, dexTransitiveDependencies.classpathEntriesToDex)); } else {
diff --git a/src/com/facebook/buck/java/DefaultJavaLibraryRule.java b/src/com/facebook/buck/java/DefaultJavaLibraryRule.java index b9c9fee..952f695 100644 --- a/src/com/facebook/buck/java/DefaultJavaLibraryRule.java +++ b/src/com/facebook/buck/java/DefaultJavaLibraryRule.java
@@ -518,8 +518,7 @@ // If this rule depends on AndroidResourceRules, then we need to generate the R.java files that // this rule needs in order to be able to compile itself. - androidResourceDeps = UberRDotJavaUtil.getAndroidResourceDeps(this, - context.getDependencyGraph()); + androidResourceDeps = UberRDotJavaUtil.getAndroidResourceDeps(this); boolean dependsOnAndroidResourceRules = !androidResourceDeps.isEmpty(); if (dependsOnAndroidResourceRules) { UberRDotJavaUtil.createDummyRDotJavaFiles(androidResourceDeps, buildTarget, commands);
diff --git a/src/com/facebook/buck/java/JavaTestRule.java b/src/com/facebook/buck/java/JavaTestRule.java index dc091d8..76efdb3 100644 --- a/src/com/facebook/buck/java/JavaTestRule.java +++ b/src/com/facebook/buck/java/JavaTestRule.java
@@ -153,8 +153,7 @@ // androidResourceDeps will be null if this test is re-run without being rebuilt. if (androidResourceDeps == null) { - androidResourceDeps = UberRDotJavaUtil.getAndroidResourceDeps(this, - buildContext.getDependencyGraph()); + androidResourceDeps = UberRDotJavaUtil.getAndroidResourceDeps(this); } ImmutableList.Builder<Step> steps = ImmutableList.builder();
diff --git a/test/com/facebook/buck/android/AndroidBinaryRuleTest.java b/test/com/facebook/buck/android/AndroidBinaryRuleTest.java index 26277f4..ed8849f 100644 --- a/test/com/facebook/buck/android/AndroidBinaryRuleTest.java +++ b/test/com/facebook/buck/android/AndroidBinaryRuleTest.java
@@ -36,7 +36,6 @@ import com.facebook.buck.rules.BuildContext; import com.facebook.buck.rules.BuildRule; import com.facebook.buck.rules.BuildRuleResolver; -import com.facebook.buck.rules.DependencyGraph; import com.facebook.buck.rules.FakeAbstractBuildRuleBuilderParams; import com.facebook.buck.rules.FileSourcePath; import com.facebook.buck.rules.SourcePath; @@ -45,7 +44,6 @@ import com.facebook.buck.step.fs.MakeCleanDirectoryStep; import com.facebook.buck.step.fs.MkdirAndSymlinkFileStep; import com.facebook.buck.testutil.MoreAsserts; -import com.facebook.buck.testutil.RuleMap; import com.facebook.buck.util.DirectoryTraversal; import com.facebook.buck.util.DirectoryTraverser; import com.facebook.buck.util.MorePaths; @@ -104,11 +102,10 @@ .setKeystore(addKeystoreRule(ruleResolver)) .setPackageType("debug")); - DependencyGraph graph = RuleMap.createGraphFromBuildRules(ruleResolver); AndroidTransitiveDependencies transitiveDependencies = - androidBinary.findTransitiveDependencies(graph); + androidBinary.findTransitiveDependencies(); AndroidDexTransitiveDependencies dexTransitiveDependencies = - androidBinary.findDexTransitiveDependencies(graph); + androidBinary.findDexTransitiveDependencies(); ImmutableList.Builder<Step> commands = ImmutableList.builder(); BuildContext context = createMock(BuildContext.class);
diff --git a/test/com/facebook/buck/android/AndroidResourceRuleTest.java b/test/com/facebook/buck/android/AndroidResourceRuleTest.java index 2480d94..2a95529 100644 --- a/test/com/facebook/buck/android/AndroidResourceRuleTest.java +++ b/test/com/facebook/buck/android/AndroidResourceRuleTest.java
@@ -25,12 +25,10 @@ import com.facebook.buck.rules.BuildRule; import com.facebook.buck.rules.BuildRuleParams; import com.facebook.buck.rules.BuildRuleResolver; -import com.facebook.buck.rules.DependencyGraph; import com.facebook.buck.rules.FakeAbstractBuildRuleBuilderParams; import com.facebook.buck.rules.FakeBuildRuleParams; import com.facebook.buck.rules.FileSourcePath; import com.facebook.buck.testutil.MoreAsserts; -import com.facebook.buck.testutil.RuleMap; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSortedSet; @@ -125,8 +123,7 @@ .addDep(BuildTargetFactory.newInstance("//:c")) .addDep(BuildTargetFactory.newInstance("//:d"))); - DependencyGraph graph = RuleMap.createGraphFromBuildRules(ruleResolver); - ImmutableList<HasAndroidResourceDeps> deps = UberRDotJavaUtil.getAndroidResourceDeps(a, graph); + ImmutableList<HasAndroidResourceDeps> deps = UberRDotJavaUtil.getAndroidResourceDeps(a); // Note that a topological sort for a DAG is not guaranteed to be unique. In this particular // case, there are two possible valid outcomes. @@ -157,8 +154,7 @@ .addDep(BuildTargetFactory.newInstance("//:a")) .addDep(BuildTargetFactory.newInstance("//:c"))); - DependencyGraph graph2 = RuleMap.createGraphFromBuildRules(ruleResolver); - ImmutableList<HasAndroidResourceDeps> deps2 = UberRDotJavaUtil.getAndroidResourceDeps(e, graph2); + ImmutableList<HasAndroidResourceDeps> deps2 = UberRDotJavaUtil.getAndroidResourceDeps(e); assertTrue( String.format( "Topological sort %s should be either %s or %s", deps, validResult1, validResult2),
diff --git a/test/com/facebook/buck/android/AndroidTransitiveDependencyGraphTest.java b/test/com/facebook/buck/android/AndroidTransitiveDependencyGraphTest.java index 799f08c..54a8539 100644 --- a/test/com/facebook/buck/android/AndroidTransitiveDependencyGraphTest.java +++ b/test/com/facebook/buck/android/AndroidTransitiveDependencyGraphTest.java
@@ -26,10 +26,8 @@ import com.facebook.buck.model.BuildTargetPattern; import com.facebook.buck.rules.BuildRule; import com.facebook.buck.rules.BuildRuleResolver; -import com.facebook.buck.rules.DependencyGraph; import com.facebook.buck.rules.FakeAbstractBuildRuleBuilderParams; import com.facebook.buck.rules.FileSourcePath; -import com.facebook.buck.testutil.RuleMap; import com.facebook.buck.util.BuckConstant; import com.google.common.collect.ImmutableSet; @@ -107,10 +105,9 @@ .setKeystore(keystoreTarget)); // Verify that the correct transitive dependencies are found. - DependencyGraph graph = RuleMap.createGraphFromBuildRules(ruleResolver); - AndroidTransitiveDependencies transitiveDeps = binaryRule.findTransitiveDependencies(graph); + AndroidTransitiveDependencies transitiveDeps = binaryRule.findTransitiveDependencies(); AndroidDexTransitiveDependencies dexTransitiveDeps = - binaryRule.findDexTransitiveDependencies(graph); + binaryRule.findDexTransitiveDependencies(); assertEquals( "Because guava was passed to no_dx, it should not be in the classpathEntriesToDex list", ImmutableSet.of("third_party/jsr-305/jsr305.jar"), @@ -188,9 +185,8 @@ .setKeystore(keystoreTarget) .addClasspathDep(androidLibraryTarget)); - DependencyGraph dependencyGraph = RuleMap.createGraphFromBuildRules(ruleResolver); AndroidDexTransitiveDependencies androidTransitiveDeps = androidBinaryRule - .findDexTransitiveDependencies(dependencyGraph); + .findDexTransitiveDependencies(); assertEquals( "Classpath entries should include facebook/base but not keystore/base.", ImmutableSet.of(BuckConstant.GEN_DIR + "/java/com/facebook/base/lib__base__output/base.jar"),
diff --git a/test/com/facebook/buck/java/DefaultJavaLibraryRuleTest.java b/test/com/facebook/buck/java/DefaultJavaLibraryRuleTest.java index 44ec6e5..9681815 100644 --- a/test/com/facebook/buck/java/DefaultJavaLibraryRuleTest.java +++ b/test/com/facebook/buck/java/DefaultJavaLibraryRuleTest.java
@@ -461,10 +461,7 @@ .addSrc("java/src/com/libtwo/Foo.java") .addDep(BuildTargetFactory.newInstance("//:libone"))); - DependencyGraph graph = RuleMap.createGraphFromBuildRules(ruleResolver); - BuildContext buildContext = EasyMock.createMock(BuildContext.class); - expect(buildContext.getDependencyGraph()).andReturn(graph); expect(buildContext.getBuildDependencies()).andReturn(BuildDependencies.FIRST_ORDER_ONLY) .times(2); JavaPackageFinder javaPackageFinder = EasyMock.createMock(JavaPackageFinder.class);