From 1ad8a8815e5d0f646f12fbcb2ec53a79fb8dcb79 Mon Sep 17 00:00:00 2001 From: asteinb Date: Mon, 7 May 2018 11:39:09 -0700 Subject: Stop collapsing NestedSet of deps in RClassGenerator This collapsing should be avoided when possible, and punted on until execution when needed. RELNOTES: none PiperOrigin-RevId: 195695290 --- .../android/RClassGeneratorActionBuilder.java | 72 +++++++--------------- .../build/lib/rules/android/AndroidBinaryTest.java | 44 +++++++++++++ 2 files changed, 66 insertions(+), 50 deletions(-) (limited to 'src') diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/RClassGeneratorActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/android/RClassGeneratorActionBuilder.java index 016aee5866..01dc6f549b 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/RClassGeneratorActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/RClassGeneratorActionBuilder.java @@ -13,26 +13,22 @@ // limitations under the License. package com.google.devtools.build.lib.rules.android; -import com.google.common.base.Function; import com.google.common.base.Strings; -import com.google.common.collect.FluentIterable; import com.google.common.collect.ImmutableList; -import com.google.common.collect.Iterables; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.ParamFileInfo; import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType; import com.google.devtools.build.lib.analysis.RuleContext; -import com.google.devtools.build.lib.analysis.actions.CustomCommandLine; +import com.google.devtools.build.lib.analysis.actions.CustomCommandLine.Builder; import com.google.devtools.build.lib.analysis.actions.CustomCommandLine.VectorArg; import com.google.devtools.build.lib.analysis.actions.SpawnAction; import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget.Mode; -import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; -import com.google.devtools.build.lib.collect.nestedset.Order; import com.google.devtools.build.lib.rules.android.AndroidConfiguration.AndroidAaptVersion; +import com.google.devtools.build.lib.rules.android.AndroidDataConverter.JoinerType; import java.util.ArrayList; import java.util.List; -import javax.annotation.Nullable; +import java.util.function.Function; /** Builds up the spawn action for $android_rclass_generator. */ public class RClassGeneratorActionBuilder { @@ -77,7 +73,7 @@ public class RClassGeneratorActionBuilder { } private void build(Artifact rTxt, ProcessedAndroidManifest manifest) { - CustomCommandLine.Builder builder = new CustomCommandLine.Builder(); + Builder builder = new Builder(); // Set the busybox tool. builder.add("--tool").add("GENERATE_BINARY_R").add("--"); @@ -98,19 +94,20 @@ public class RClassGeneratorActionBuilder { builder.add("--packageForR", manifest.getPackage()); } if (dependencies != null) { - // TODO(corysmith): Remove NestedSet as we are already flattening it. - Iterable depResources = dependencies.getResourceContainers(); - if (!Iterables.isEmpty(depResources)) { + if (!dependencies.getResourceContainers().isEmpty()) { builder.addAll( VectorArg.addBefore("--library") - .each( - ImmutableList.copyOf( - Iterables.transform(depResources, chooseDepsToArg(version))))); - inputs.addTransitive( - NestedSetBuilder.wrap( - Order.NAIVE_LINK_ORDER, - FluentIterable.from(depResources) - .transformAndConcat(chooseDepsToArtifacts(version)))); + .each(dependencies.getResourceContainers()) + .mapped( + AndroidDataConverter.builder(JoinerType.COLON_COMMA) + .with(chooseDepsToArg(version)) + .build())); + if (version == AndroidAaptVersion.AAPT2) { + inputs.addTransitive(dependencies.getTransitiveAapt2RTxt()); + } else { + inputs.addTransitive(dependencies.getTransitiveRTxt()); + } + inputs.addTransitive(dependencies.getTransitiveManifests()); } } builder.addExecPath("--classJarOutput", classJarOut); @@ -134,39 +131,14 @@ public class RClassGeneratorActionBuilder { .build(ruleContext)); } - private static Artifact chooseRTxt(ValidatedAndroidData container, AndroidAaptVersion version) { - return version == AndroidAaptVersion.AAPT2 ? container.getAapt2RTxt() : container.getRTxt(); - } - - private static Function> chooseDepsToArtifacts( - final AndroidAaptVersion version) { - return new Function>() { - @Override - public NestedSet apply(ValidatedAndroidData container) { - NestedSetBuilder artifacts = NestedSetBuilder.naiveLinkOrder(); - addIfNotNull(chooseRTxt(container, version), artifacts); - addIfNotNull(container.getManifest(), artifacts); - return artifacts.build(); - } - - private void addIfNotNull(@Nullable Artifact artifact, NestedSetBuilder artifacts) { - if (artifact != null) { - artifacts.add(artifact); - } - } - }; - } - private static Function chooseDepsToArg( final AndroidAaptVersion version) { - return new Function() { - @Override - public String apply(ValidatedAndroidData container) { - Artifact rTxt = chooseRTxt(container, version); - return (rTxt != null ? rTxt.getExecPath() : "") - + "," - + (container.getManifest() != null ? container.getManifest().getExecPath() : ""); - } + return container -> { + Artifact rTxt = + version == AndroidAaptVersion.AAPT2 ? container.getAapt2RTxt() : container.getRTxt(); + return (rTxt != null ? rTxt.getExecPath() : "") + + "," + + (container.getManifest() != null ? container.getManifest().getExecPath() : ""); }; } } diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java index 1dd3e80eed..3a9bb90190 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java @@ -1958,6 +1958,50 @@ public class AndroidBinaryTest extends AndroidBuildViewTestCase { "--packageForR", "com.binary.custom"); } + @Test + public void testUseRClassGeneratorMultipleDeps() throws Exception { + scratch.file( + "java/r/android/BUILD", + "android_library(name = 'lib1',", + " manifest = 'AndroidManifest.xml',", + " resource_files = glob(['res1/**']),", + " )", + "android_library(name = 'lib2',", + " manifest = 'AndroidManifest.xml',", + " resource_files = glob(['res2/**']),", + " )", + "android_binary(name = 'r',", + " manifest = 'AndroidManifest.xml',", + " resource_files = glob(['res/**']),", + " deps = [':lib1', ':lib2'],", + " )"); + ConfiguredTargetAndData binary = getConfiguredTargetAndData("//java/r/android:r"); + Artifact jar = getResourceClassJar(binary); + assertThat(getGeneratingAction(jar).getMnemonic()).isEqualTo("RClassGenerator"); + List args = getGeneratingSpawnActionArgs(jar); + + AndroidResourcesInfo resourcesInfo = + binary.getConfiguredTarget().get(AndroidResourcesInfo.PROVIDER); + assertThat(resourcesInfo.getTransitiveAndroidResources()).hasSize(2); + ValidatedAndroidData firstDep = resourcesInfo.getTransitiveAndroidResources().toList().get(0); + ValidatedAndroidData secondDep = resourcesInfo.getTransitiveAndroidResources().toList().get(1); + + assertThat(args) + .containsAllOf( + "--primaryRTxt", + "--primaryManifest", + "--library", + firstDep.getRTxt().getExecPathString() + + "," + + firstDep.getManifest().getExecPathString(), + "--library", + secondDep.getRTxt().getExecPathString() + + "," + + secondDep.getManifest().getExecPathString(), + "--classJarOutput") + .inOrder(); + } + @Test public void testNoCrunchBinaryOnly() throws Exception { scratch.file("java/r/android/BUILD", -- cgit v1.2.3