From 687550660db90b007741e1f67a8092b08945f8e3 Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 21 Oct 2015 14:24:18 +0000 Subject: Trying again with checking for the presence of the "resources" attribute and deduplication of transitive and direct dependencies in the AndroidProcessingAction. Change the resource dependency handling to separate between the transitive and direct resources from libraries. This slightly increases the complexity of resource propagation. The initial algorithm was to simply merge all transitive ResourceContainers together with any new ResourceContainer and propagate them via the AndroidResourcesProvider. The new algorithm is encapsulated inside a new ResourceDependencies class which works as follows: 1. Collect resources from the deps into transitive and direct NestedSets 2. If a rule provides a ResourceContainer, merge the transitive and direct into a new transitive set 3. Export the provider This results having a rule without resources "forward" the merged sets of transitive and direct resources to the next rule. -- MOS_MIGRATED_REVID=105960655 --- .../lib/analysis/actions/CustomCommandLine.java | 24 +++ .../build/lib/rules/android/AndroidBinary.java | 43 ++-- .../build/lib/rules/android/AndroidCommon.java | 39 +--- .../build/lib/rules/android/AndroidLibrary.java | 18 +- .../android/AndroidResourcesProcessorBuilder.java | 237 ++++++++++++--------- .../rules/android/AndroidResourcesProvider.java | 49 +++-- .../lib/rules/android/ApplicationManifest.java | 65 +++--- .../build/lib/rules/android/ResourceApk.java | 40 +++- .../lib/rules/android/ResourceDependencies.java | 184 ++++++++++++++++ .../build/lib/actions/CustomCommandLineTest.java | 7 + .../android/AndroidResourceProcessingAction.java | 32 ++- 11 files changed, 511 insertions(+), 227 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/rules/android/ResourceDependencies.java (limited to 'src') diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java index 8eed18804e..584944f0e9 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java @@ -129,6 +129,22 @@ public final class CustomCommandLine extends CommandLine { } } + private static final class JoinStringsArg extends ArgvFragment { + + private final String delimiter; + private final Iterable strings; + + private JoinStringsArg(String delimiter, Iterable strings) { + this.delimiter = delimiter; + this.strings = CollectionUtils.makeImmutable(strings); + } + + @Override + void eval(ImmutableList.Builder builder) { + builder.add(Joiner.on(delimiter).join(strings)); + } + } + /** * Arguments that intersperse strings between the items in a sequence. There are two forms of * interspersing, and either may be used by this implementation: @@ -261,6 +277,14 @@ public final class CustomCommandLine extends CommandLine { return this; } + public Builder addJoinStrings(String arg, String delimiter, Iterable strings) { + if (arg != null && strings != null) { + arguments.add(new ObjectArg(arg)); + arguments.add(new JoinStringsArg(delimiter, strings)); + } + return this; + } + public Builder addJoinExecPaths(String arg, String delimiter, Iterable artifacts) { if (arg != null && artifacts != null) { arguments.add(new ObjectArg(arg)); diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java index 876d796fc8..6c8fd51b0b 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java @@ -49,7 +49,6 @@ import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; import com.google.devtools.build.lib.packages.BuildType; import com.google.devtools.build.lib.packages.TriState; import com.google.devtools.build.lib.rules.RuleConfiguredTargetFactory; -import com.google.devtools.build.lib.rules.android.AndroidResourcesProvider.ResourceContainer; import com.google.devtools.build.lib.rules.android.AndroidRuleClasses.MultidexMode; import com.google.devtools.build.lib.rules.cpp.CcToolchainProvider; import com.google.devtools.build.lib.rules.cpp.CppHelper; @@ -97,11 +96,19 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory { AndroidCommon androidCommon = new AndroidCommon( ruleContext, javaCommon, true /* asNeverLink */, true /* exportDeps */); try { - RuleConfiguredTargetBuilder builder = - init(ruleContext, filesBuilder, - AndroidCommon.getTransitiveResourceContainers(ruleContext, true), - javaCommon, androidCommon, javaSemantics, androidSemantics, - ImmutableList.of("deps")); + ResourceDependencies resourceDeps = LocalResourceContainer.definesAndroidResources( + ruleContext.attributes()) + ? ResourceDependencies.fromRuleDeps(ruleContext) + : ResourceDependencies.fromRuleResourceAndDeps(ruleContext); + RuleConfiguredTargetBuilder builder = init( + ruleContext, + filesBuilder, + resourceDeps, + javaCommon, + androidCommon, + javaSemantics, + androidSemantics, + ImmutableList.of("deps")); if (builder == null) { return null; } @@ -116,7 +123,7 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory { private static RuleConfiguredTargetBuilder init( RuleContext ruleContext, NestedSetBuilder filesBuilder, - NestedSet resourceContainers, + ResourceDependencies resourceDeps, JavaCommon javaCommon, AndroidCommon androidCommon, JavaSemantics javaSemantics, @@ -183,11 +190,11 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory { throw new RuleConfigurationException(); } applicationManifest = androidSemantics.getManifestForRule(ruleContext) - .mergeWith(ruleContext, resourceContainers); + .mergeWith(ruleContext, resourceDeps); resourceApk = applicationManifest.packWithDataAndResources( ruleContext.getImplicitOutputArtifact(AndroidRuleClasses.ANDROID_RESOURCES_APK), ruleContext, - resourceContainers, + resourceDeps, null, /* Artifact rTxt */ null, /* Artifact symbolsTxt */ ruleContext.getTokenizedStringListAttr("resource_configuration_filters"), @@ -201,7 +208,7 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory { .packWithDataAndResources(ruleContext .getImplicitOutputArtifact(AndroidRuleClasses.ANDROID_INCREMENTAL_RESOURCES_APK), ruleContext, - resourceContainers, + resourceDeps, null, /* Artifact rTxt */ null, /* Artifact symbolsTxt */ ruleContext.getTokenizedStringListAttr("resource_configuration_filters"), @@ -215,7 +222,7 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory { .createSplitManifest(ruleContext, "android_resources", false) .packWithDataAndResources(getDxArtifact(ruleContext, "android_resources.ap_"), ruleContext, - resourceContainers, + resourceDeps, null, /* Artifact rTxt */ null, /* Artifact symbolsTxt */ ruleContext.getTokenizedStringListAttr("resource_configuration_filters"), @@ -229,13 +236,13 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory { // Retrieve the resources from the resources attribute on the android_binary rule // and recompile them if necessary. applicationManifest = ApplicationManifest.fromResourcesRule(ruleContext).mergeWith( - ruleContext, resourceContainers); + ruleContext, resourceDeps); // Always recompiling resources causes AndroidTest to fail in certain circumstances. - if (shouldRegenerate(ruleContext, resourceContainers)) { + if (shouldRegenerate(ruleContext, resourceDeps)) { resourceApk = applicationManifest.packWithResources( ruleContext.getImplicitOutputArtifact(AndroidRuleClasses.ANDROID_RESOURCES_APK), ruleContext, - resourceContainers, + resourceDeps, true, getProguardConfigArtifact(ruleContext, "")); } else { @@ -248,7 +255,7 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory { ruleContext.getImplicitOutputArtifact( AndroidRuleClasses.ANDROID_INCREMENTAL_RESOURCES_APK), ruleContext, - resourceContainers, + resourceDeps, false, getProguardConfigArtifact(ruleContext, "incremental")); @@ -256,7 +263,7 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory { .createSplitManifest(ruleContext, "android_resources", false) .packWithResources(getDxArtifact(ruleContext, "android_resources.ap_"), ruleContext, - resourceContainers, + resourceDeps, false, getProguardConfigArtifact(ruleContext, "incremental_split")); } @@ -1272,8 +1279,8 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory { * */ public static boolean shouldRegenerate(RuleContext ruleContext, - Iterable resourceContainers) { - return Iterables.size(resourceContainers) > 1 + ResourceDependencies resourceDeps) { + return Iterables.size(resourceDeps.getResources()) > 1 || ruleContext.attributes().isAttributeValueExplicitlySpecified("densities") || ruleContext.attributes().isAttributeValueExplicitlySpecified( "resource_configuration_filters") diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidCommon.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidCommon.java index 37328e6b64..b8e5a41000 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidCommon.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidCommon.java @@ -97,7 +97,6 @@ public class AndroidCommon { private Artifact genClassJar; private Artifact genSourceJar; - private NestedSet transitiveResources; private boolean asNeverLink; private boolean exportDeps; private Artifact manifestProtoOutput; @@ -261,36 +260,11 @@ public class AndroidCommon { return jackCompilationHelper.compileAsDex(mode, mainDexList, proguardSpecs); } - public static NestedSet getTransitiveResourceContainers( - RuleContext ruleContext, boolean withDeps) { - // Traverse through all android_library targets looking for resources - NestedSetBuilder resourcesBuilder = NestedSetBuilder.naiveLinkOrder(); - List attributes = new ArrayList<>(); - attributes.add("resources"); - if (withDeps) { - attributes.add("deps"); - } - - for (String attribute : attributes) { - if (!ruleContext.attributes().has(attribute, BuildType.LABEL) - && !ruleContext.attributes().has(attribute, BuildType.LABEL_LIST)) { - continue; - } - - for (AndroidResourcesProvider resources : - ruleContext.getPrerequisites(attribute, Mode.TARGET, AndroidResourcesProvider.class)) { - resourcesBuilder.addTransitive(resources.getTransitiveAndroidResources()); - } - } - - return resourcesBuilder.build(); - } - private void compileResources( JavaSemantics javaSemantics, JavaCompilationArtifacts.Builder artifactsBuilder, JavaTargetAttributes.Builder attributes, - NestedSet resourceContainers, + ResourceDependencies resourceDeps, ResourceContainer updatedResources) throws InterruptedException { Artifact binaryResourcesJar = ruleContext.getImplicitOutputArtifact(JavaSemantics.JAVA_BINARY_CLASS_JAR); @@ -301,7 +275,7 @@ public class AndroidCommon { // Repackages the R.java for each dependency package and places the resultant jars // before the dependency libraries to ensure that the generated resource ids are // correct. - createJarJarActions(attributes, resourceContainers, + createJarJarActions(attributes, resourceDeps.getResources(), updatedResources.getJavaPackage(), binaryResourcesJar); } @@ -397,12 +371,11 @@ public class AndroidCommon { JavaTargetAttributes.Builder attributes = init( androidSemantics, - resourceApk.getTransitiveResources(), extraSourcesBuilder.build()); JavaCompilationArtifacts.Builder artifactsBuilder = new JavaCompilationArtifacts.Builder(); if (resourceApk.isLegacy()) { compileResources(javaSemantics, artifactsBuilder, attributes, - resourceApk.getTransitiveResources(), resourceApk.getPrimaryResource()); + resourceApk.getResourceDependencies(), resourceApk.getPrimaryResource()); } JavaCompilationHelper helper = initAttributes(attributes, javaSemantics); @@ -433,12 +406,9 @@ public class AndroidCommon { private JavaTargetAttributes.Builder init( AndroidSemantics androidSemantics, - NestedSet transitiveResources, Collection extraArtifacts) { - this.transitiveResources = transitiveResources; javaCommon.initializeJavacOpts(androidSemantics.getJavacArguments()); JavaTargetAttributes.Builder attributes = javaCommon.initCommon(extraArtifacts); - attributes.setBootClassPath(ImmutableList.of( AndroidSdkProvider.fromRuleContext(ruleContext).getAndroidJar())); return attributes; @@ -608,8 +578,7 @@ public class AndroidCommon { new JavaRuntimeJarProvider(javaCommon.getJavaCompilationArtifacts().getRuntimeJars())) .add(RunfilesProvider.class, RunfilesProvider.simple(runfiles)) .add( - AndroidResourcesProvider.class, - new AndroidResourcesProvider(ruleContext.getLabel(), transitiveResources)) + AndroidResourcesProvider.class, resourceApk.toResourceProvider(ruleContext.getLabel())) .add( AndroidIdeInfoProvider.class, createAndroidIdeInfoProvider(ruleContext, androidSemantics, idlHelper, diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidLibrary.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidLibrary.java index bb90189242..0eec811d57 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidLibrary.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidLibrary.java @@ -62,8 +62,6 @@ public abstract class AndroidLibrary implements RuleConfiguredTargetFactory { List deps = ruleContext.getPrerequisites("deps", Mode.TARGET); checkResourceInlining(ruleContext); - NestedSet transitiveResources = - AndroidCommon.getTransitiveResourceContainers(ruleContext, true); NestedSetBuilder transitiveAars = collectTransitiveAars(ruleContext); NestedSet transitiveNativeLibraries = AndroidCommon.collectTransitiveNativeLibraries(deps); @@ -84,7 +82,8 @@ public abstract class AndroidLibrary implements RuleConfiguredTargetFactory { try { resourceApk = applicationManifest.packWithDataAndResources( ruleContext.getImplicitOutputArtifact(AndroidRuleClasses.ANDROID_RESOURCES_APK), - ruleContext, transitiveResources, + ruleContext, + ResourceDependencies.fromRuleDeps(ruleContext), ruleContext.getImplicitOutputArtifact(AndroidRuleClasses.ANDROID_R_TXT), ruleContext.getImplicitOutputArtifact(AndroidRuleClasses.ANDROID_SYMBOLS_TXT), ImmutableList.of(), /* configurationFilters */ @@ -102,7 +101,8 @@ public abstract class AndroidLibrary implements RuleConfiguredTargetFactory { return null; } } else { - resourceApk = ResourceApk.fromTransitiveResources(transitiveResources); + resourceApk = ResourceApk.fromTransitiveResources( + ResourceDependencies.fromRuleResourceAndDeps(ruleContext)); } JavaTargetAttributes javaTargetAttributes = androidCommon.init( @@ -129,7 +129,7 @@ public abstract class AndroidLibrary implements RuleConfiguredTargetFactory { transitiveAars.add(aar); } else if (AndroidCommon.getAndroidResources(ruleContext) != null) { primaryResources = Iterables.getOnlyElement( - AndroidCommon.getAndroidResources(ruleContext).getTransitiveAndroidResources()); + AndroidCommon.getAndroidResources(ruleContext).getDirectAndroidResources()); aar = new Aar(aarOut, primaryResources.getManifest()); transitiveAars.add(aar); } else { @@ -157,7 +157,7 @@ public abstract class AndroidLibrary implements RuleConfiguredTargetFactory { .setSourceJarOut(resourceContainer.getJavaSourceJar()) .setJavaPackage(resourceContainer.getJavaPackage()) .withPrimary(resourceContainer) - .withDependencies(transitiveResources) + .withDependencies(resourceApk.getResourceDependencies()) .setDebug( ruleContext.getConfiguration().getCompilationMode() != CompilationMode.OPT) .build(ruleContext); @@ -172,8 +172,8 @@ public abstract class AndroidLibrary implements RuleConfiguredTargetFactory { .build(ruleContext); RuleConfiguredTargetBuilder builder = new RuleConfiguredTargetBuilder(ruleContext); - androidCommon.addTransitiveInfoProviders(builder, androidSemantics, - definesLocalResources ? resourceApk : null, null, ImmutableList.of()); + androidCommon.addTransitiveInfoProviders(builder, androidSemantics, resourceApk, null, + ImmutableList.of()); androidSemantics.addTransitiveInfoProviders( builder, ruleContext, javaCommon, androidCommon, null); @@ -220,7 +220,7 @@ public abstract class AndroidLibrary implements RuleConfiguredTargetFactory { } ResourceContainer container = Iterables.getOnlyElement( - resources.getTransitiveAndroidResources()); + resources.getDirectAndroidResources()); if (container.getConstantsInlined() && !container.getArtifacts(ResourceType.RESOURCES).isEmpty()) { diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidResourcesProcessorBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidResourcesProcessorBuilder.java index 0fa9ecd291..adaf1baab5 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidResourcesProcessorBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidResourcesProcessorBuilder.java @@ -14,9 +14,11 @@ package com.google.devtools.build.lib.rules.android; import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Function; import com.google.common.base.Functions; import com.google.common.base.Joiner; 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.common.collect.Iterators; @@ -24,7 +26,11 @@ import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.analysis.RuleConfiguredTarget.Mode; import com.google.devtools.build.lib.analysis.RuleContext; import com.google.devtools.build.lib.analysis.actions.ActionConstructionContext; +import com.google.devtools.build.lib.analysis.actions.CustomCommandLine; import com.google.devtools.build.lib.analysis.actions.SpawnAction; +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.AndroidResourcesProvider.ResourceContainer; import com.google.devtools.build.lib.rules.android.AndroidResourcesProvider.ResourceType; @@ -32,13 +38,25 @@ import java.util.ArrayList; import java.util.Collections; import java.util.List; +import javax.annotation.Nullable; + /** * Builder for creating resource processing action. */ public class AndroidResourcesProcessorBuilder { + private static final ResourceContainerToArtifacts RESOURCE_CONTAINER_TO_ARTIFACTS = + new ResourceContainerToArtifacts(false); + + private static final ResourceContainerToArtifacts RESOURCE_DEP_TO_ARTIFACTS = + new ResourceContainerToArtifacts(true); + + private static final ResourceContainerToArg RESOURCE_CONTAINER_TO_ARG = + new ResourceContainerToArg(false); + private static final ResourceContainerToArg RESOURCE_DEP_TO_ARG = + new ResourceContainerToArg(true); private ResourceContainer primary; - private List dependencies = Collections.emptyList(); + private ResourceDependencies dependencies; private Artifact proguardOut; private Artifact rTxtOut; private Artifact sourceJarOut; @@ -75,8 +93,8 @@ public class AndroidResourcesProcessorBuilder { return this; } - public AndroidResourcesProcessorBuilder withDependencies(Iterable nestedSet) { - this.dependencies = ImmutableList.copyOf(nestedSet); + public AndroidResourcesProcessorBuilder withDependencies(ResourceDependencies resourceDeps) { + this.dependencies = resourceDeps; return this; } @@ -131,160 +149,188 @@ public class AndroidResourcesProcessorBuilder { return this; } - private void addResourceContainer(List inputs, List args, - ResourceContainer container) { - Iterables.addAll(inputs, container.getArtifacts()); - inputs.add(container.getManifest()); - inputs.add(container.getRTxt()); - - args.add(String.format("%s:%s:%s:%s:%s", - convertRoots(container, ResourceType.RESOURCES), - convertRoots(container, ResourceType.ASSETS), - container.getManifest().getExecPathString(), - container.getRTxt() == null ? "" : container.getRTxt().getExecPath(), - container.getSymbolsTxt() == null ? "" : container.getSymbolsTxt().getExecPath() - )); + private static class ResourceContainerToArg implements Function { + private boolean includeSymbols; + + public ResourceContainerToArg(boolean includeSymbols) { + this.includeSymbols = includeSymbols; + } + + @Override + public String apply(ResourceContainer container) { + StringBuilder builder = new StringBuilder(); + builder.append(convertRoots(container, ResourceType.RESOURCES)) + .append(":") + .append(convertRoots(container, ResourceType.ASSETS)) + .append(":") + .append(container.getManifest().getExecPathString()); + if (includeSymbols) { + builder.append(":") + .append(container.getRTxt() == null ? "" : container.getRTxt().getExecPath()) + .append(":") + .append( + container.getSymbolsTxt() == null ? "" : container.getSymbolsTxt().getExecPath()); + } + return builder.toString(); + } } - private void addPrimaryResourceContainer(List inputs, List args, - ResourceContainer container) { - Iterables.addAll(inputs, container.getArtifacts()); - inputs.add(container.getManifest()); - - // no R.txt, because it will be generated from this action. - args.add(String.format("%s:%s:%s", - convertRoots(container, ResourceType.RESOURCES), - convertRoots(container, ResourceType.ASSETS), - container.getManifest().getExecPathString() - )); + private static class ResourceContainerToArtifacts + implements Function> { + + private boolean includeSymbols; + + public ResourceContainerToArtifacts(boolean includeSymbols) { + this.includeSymbols = includeSymbols; + } + + @Override + public NestedSet apply(ResourceContainer container) { + NestedSetBuilder artifacts = NestedSetBuilder.naiveLinkOrder(); + addIfNotNull(container.getManifest(), artifacts); + if (includeSymbols) { + addIfNotNull(container.getRTxt(), artifacts); + addIfNotNull(container.getSymbolsTxt(), artifacts); + } + artifacts.addAll(container.getArtifacts()); + return artifacts.build(); + } + + private void addIfNotNull(@Nullable Artifact artifact, NestedSetBuilder artifacts) { + if (artifact != null) { + artifacts.add(artifact); + } + } } @VisibleForTesting public static String convertRoots(ResourceContainer container, ResourceType resourceType) { - return Joiner.on("#").join( - Iterators.transform( - container.getRoots(resourceType).iterator(), Functions.toStringFunction())); + return Joiner.on("#").join(Iterators.transform( + container.getRoots(resourceType).iterator(), Functions.toStringFunction())); } public ResourceContainer build(ActionConstructionContext context) { List outs = new ArrayList<>(); - List ins = new ArrayList<>(); - List args = new ArrayList<>(); - - args.add("--aapt"); - args.add(sdk.getAapt().getExecutable().getExecPathString()); + CustomCommandLine.Builder builder = new CustomCommandLine.Builder(); - Iterables.addAll(ins, - ruleContext.getExecutablePrerequisite("$android_resources_processor", Mode.HOST) + builder.addExecPath("--aapt", sdk.getAapt().getExecutable()); + // Use a FluentIterable to avoid flattening the NestedSets + NestedSetBuilder inputs = NestedSetBuilder.naiveLinkOrder(); + inputs.addAll(ruleContext.getExecutablePrerequisite("$android_resources_processor", Mode.HOST) .getRunfilesSupport() .getRunfilesArtifactsWithoutMiddlemen()); - args.add("--annotationJar"); - args.add(sdk.getAnnotationsJar().getExecPathString()); - ins.add(sdk.getAnnotationsJar()); - args.add("--androidJar"); - args.add(sdk.getAndroidJar().getExecPathString()); - ins.add(sdk.getAndroidJar()); - - args.add("--primaryData"); - addPrimaryResourceContainer(ins, args, primary); - if (!dependencies.isEmpty()) { - args.add("--data"); - List data = new ArrayList<>(); - for (ResourceContainer container : dependencies) { - addResourceContainer(ins, data, container); + builder.addExecPath("--annotationJar", sdk.getAnnotationsJar()); + inputs.add(sdk.getAnnotationsJar()); + + builder.addExecPath("--androidJar", sdk.getAndroidJar()); + inputs.add(sdk.getAndroidJar()); + + builder.add("--primaryData").add(RESOURCE_CONTAINER_TO_ARG.apply(primary)); + inputs.addTransitive(RESOURCE_CONTAINER_TO_ARTIFACTS.apply(primary)); + + if (dependencies != null) { + // TODO(bazel-team): Find an appropriately lazy method to deduplicate the dependencies between + // the direct and transitive data. + // Add transitive data inside an unmodifiableIterable to ensure it won't be expanded until + // iteration. + if (!dependencies.getTransitiveResources().isEmpty()) { + builder.addJoinStrings("--data", ",", + Iterables.unmodifiableIterable( + Iterables.transform(dependencies.getTransitiveResources(), RESOURCE_DEP_TO_ARG))); } - args.add(Joiner.on(",").join(data)); + // Add direct data inside an unmodifiableIterable to ensure it won't be expanded until + // iteration. + if (!dependencies.getDirectResources().isEmpty()) { + builder.addJoinStrings("--directData", ",", + Iterables.unmodifiableIterable( + Iterables.transform(dependencies.getDirectResources(), RESOURCE_DEP_TO_ARG))); + } + // This flattens the nested set. Since each ResourceContainer needs to be transformed into + // Artifacts, and the NestedSetBuilder.wrap doesn't support lazy Iterator evaluation + // and SpawnActionBuilder.addInputs evaluates Iterables, it becomes necessary to make the + // best effort and let it get flattened. + inputs.addTransitive( + NestedSetBuilder.wrap( + Order.NAIVE_LINK_ORDER, + FluentIterable.from(dependencies.getResources()) + .transformAndConcat(RESOURCE_DEP_TO_ARTIFACTS))); } if (rTxtOut != null) { - args.add("--rOutput"); - args.add(rTxtOut.getExecPathString()); + builder.addExecPath("--rOutput", rTxtOut); outs.add(rTxtOut); // If R.txt is not null, dependency R.javas will not be regenerated from the R.txt found in // the deps, which means the resource processor needs to be told it is creating a library so // that it will generate the R.txt. - args.add("--packageType"); - args.add("LIBRARY"); + builder.add("--packageType").add("LIBRARY"); } + if (symbolsTxt != null) { - args.add("--symbolsTxtOut"); - args.add(symbolsTxt.getExecPathString()); + builder.addExecPath("--symbolsTxtOut", symbolsTxt); outs.add(symbolsTxt); } if (sourceJarOut != null) { - args.add("--srcJarOutput"); - args.add(sourceJarOut.getExecPathString()); + builder.addExecPath("--srcJarOutput", sourceJarOut); outs.add(sourceJarOut); } if (proguardOut != null) { - args.add("--proguardOutput"); - args.add(proguardOut.getExecPathString()); + builder.addExecPath("--proguardOutput", proguardOut); outs.add(proguardOut); } if (apkOut != null) { - args.add("--packagePath"); - args.add(apkOut.getExecPathString()); + builder.addExecPath("--packagePath", apkOut); outs.add(apkOut); } if (!resourceConfigs.isEmpty()) { - args.add("--resourceConfigs"); - args.add(Joiner.on(',').join(resourceConfigs)); + builder.addJoinStrings("--resourceConfigs", ",", resourceConfigs); } if (!densities.isEmpty()) { - args.add("--densities"); - args.add(Joiner.on(',').join(densities)); + builder.addJoinStrings("--densities", "'", densities); } if (!uncompressedExtensions.isEmpty()) { - args.add("--uncompressedExtensions"); - args.add(Joiner.on(',').join(uncompressedExtensions)); + builder.addJoinStrings("--uncompressedExtensions", ",", uncompressedExtensions); } if (!assetsToIgnore.isEmpty()) { - args.add("--assetsToIgnore"); - args.add( - Joiner.on(',').join(assetsToIgnore)); + builder.addJoinStrings("--assetsToIgnore", ",", assetsToIgnore); } if (debug) { - args.add("--debug"); + builder.add("--debug"); } if (versionCode != null) { - args.add("--versionCode"); - args.add(versionCode); + builder.add("--versionCode").add(versionCode); } if (versionName != null) { - args.add("--versionName"); - args.add(versionName); + builder.add("--versionName").add(versionName); } if (applicationId != null) { - args.add("--applicationId"); - args.add(applicationId); + builder.add("--applicationId").add(applicationId); } if (!Strings.isNullOrEmpty(customJavaPackage)) { // Sets an alternative java package for the generated R.java // this is allows android rules to generate resources outside of the java{,tests} tree. - args.add("--packageForR"); - args.add(customJavaPackage); + builder.add("--packageForR").add(customJavaPackage); } // Create the spawn action. - ruleContext.registerAction(this.spawnActionBuilder - .addTool(sdk.getAapt()) - .addInputs(ImmutableList.copyOf(ins)) - .addOutputs(ImmutableList.copyOf(outs)) - .addArguments(ImmutableList.copyOf(args)) - .setExecutable( - ruleContext.getExecutablePrerequisite("$android_resources_processor", Mode.HOST)) - .setProgressMessage("Processing resources") - .setMnemonic("AndroidAapt") - .build(context)); + ruleContext.registerAction( + this.spawnActionBuilder + .addTool(sdk.getAapt()) + .addTransitiveInputs(inputs.build()) + .addOutputs(ImmutableList.copyOf(outs)) + .setCommandLine(builder.build()) + .setExecutable( + ruleContext.getExecutablePrerequisite("$android_resources_processor", Mode.HOST)) + .setProgressMessage("Processing resources") + .setMnemonic("AndroidAapt") + .build(context)); // Return the full set of processed transitive dependencies. - return new ResourceContainer( - primary.getLabel(), + return new ResourceContainer(primary.getLabel(), primary.getJavaPackage(), primary.getRenameManifestPackage(), primary.getConstantsInlined(), @@ -293,8 +339,7 @@ public class AndroidResourcesProcessorBuilder { // for this resource processing action (in case of just creating an R.txt or // proguard merging), reuse the primary resource from the dependencies. apkOut != null ? apkOut : primary.getApk(), - primary.getManifest(), - sourceJarOut, + primary.getManifest(), sourceJarOut, primary.getArtifacts(ResourceType.ASSETS), primary.getArtifacts(ResourceType.RESOURCES), primary.getRoots(ResourceType.ASSETS), diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidResourcesProvider.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidResourcesProvider.java index e2fda51aba..24e3174a42 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidResourcesProvider.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidResourcesProvider.java @@ -28,43 +28,50 @@ import java.util.Objects; import javax.annotation.Nullable; /** - * A provider that supplies Android resources from its transitive closure. + * A provider that supplies ResourceContainers from its transitive closure. */ @Immutable public final class AndroidResourcesProvider implements TransitiveInfoProvider { - private final Label label; private final NestedSet transitiveAndroidResources; + private final NestedSet directAndroidResources; - public AndroidResourcesProvider(Label label, - NestedSet transitiveAndroidResources) { + public AndroidResourcesProvider( + Label label, NestedSet transitiveAndroidResources, + NestedSet directAndroidResources) { this.label = label; + this.directAndroidResources = directAndroidResources; this.transitiveAndroidResources = transitiveAndroidResources; } /** * Returns the label that is associated with this piece of information. - * - *

- * This is usually the label of the target that provides the information. */ public Label getLabel() { return label; } /** - * Returns transitive Android resources (APK, assets, etc.). + * Returns the transitive ResourceContainers for the label. */ public NestedSet getTransitiveAndroidResources() { return transitiveAndroidResources; } + /** + * Returns the immediate ResourceContainers for the label. + */ + public NestedSet getDirectAndroidResources() { + return directAndroidResources; + } + /** * The type of resource in question: either asset or a resource. */ public enum ResourceType { - ASSETS("assets"), RESOURCES("resources"); + ASSETS("assets"), + RESOURCES("resources"); private final String attribute; @@ -83,7 +90,6 @@ public final class AndroidResourcesProvider implements TransitiveInfoProvider { */ @Immutable public static final class ResourceContainer { - private final Label label; private final String javaPackage; private final String renameManifestPackage; @@ -102,10 +108,8 @@ public final class AndroidResourcesProvider implements TransitiveInfoProvider { public ResourceContainer(Label label, String javaPackage, @Nullable String renameManifestPackage, - boolean constantsInlined, - Artifact apk, - Artifact manifest, - Artifact javaSourceJar, + boolean constantsInlined, Artifact apk, + Artifact manifest, Artifact javaSourceJar, ImmutableList assets, ImmutableList resources, ImmutableList assetsRoots, @@ -184,7 +188,7 @@ public final class AndroidResourcesProvider implements TransitiveInfoProvider { @Override public int hashCode() { - return Objects.hashCode(label); + return Objects.hash(label, rTxt, symbolsTxt); } @Override @@ -196,7 +200,20 @@ public final class AndroidResourcesProvider implements TransitiveInfoProvider { return false; } ResourceContainer other = (ResourceContainer) obj; - return label.equals(other.label); + return Objects.equals(label, other.label) + && Objects.equals(rTxt, other.rTxt) + && Objects.equals(symbolsTxt, other.symbolsTxt); + } + + @Override + public String toString() { + return String.format( + "ResourceContainer [label=%s, javaPackage=%s, renameManifestPackage=%s," + + " constantsInlined=%s, apk=%s, manifest=%s, assets=%s, resources=%s, assetsRoots=%s," + + " resourcesRoots=%s, manifestExported=%s, javaSourceJar=%s, rTxt=%s, symbolsTxt=%s]", + label, javaPackage, renameManifestPackage, constantsInlined, apk, manifest, assets, + resources, assetsRoots, resourcesRoots, manifestExported, javaSourceJar, rTxt, + symbolsTxt); } } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/ApplicationManifest.java b/src/main/java/com/google/devtools/build/lib/rules/android/ApplicationManifest.java index 3c14b8ec6e..42ffbae043 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/ApplicationManifest.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/ApplicationManifest.java @@ -26,9 +26,6 @@ import com.google.devtools.build.lib.analysis.TransitiveInfoCollection; import com.google.devtools.build.lib.analysis.actions.FileWriteAction; import com.google.devtools.build.lib.analysis.actions.SpawnAction; import com.google.devtools.build.lib.analysis.config.CompilationMode; -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.AndroidResourcesProvider.ResourceContainer; import com.google.devtools.build.lib.rules.android.AndroidResourcesProvider.ResourceType; import com.google.devtools.build.lib.rules.android.LocalResourceContainer.Builder.InvalidAssetPath; @@ -48,7 +45,7 @@ public final class ApplicationManifest { throw new RuleConfigurationException(); } return new ApplicationManifest(Iterables.getOnlyElement( - resources.getTransitiveAndroidResources()) + resources.getDirectAndroidResources()) .getManifest()); } @@ -91,7 +88,7 @@ public final class ApplicationManifest { AndroidResourcesProvider resourcesProvider = AndroidCommon.getAndroidResources(ruleContext); if (resourcesProvider != null) { ResourceContainer resourceContainer = Iterables.getOnlyElement( - resourcesProvider.getTransitiveAndroidResources()); + resourcesProvider.getDirectAndroidResources()); return resourceContainer.getRenameManifestPackage(); } else { return null; @@ -168,9 +165,10 @@ public final class ApplicationManifest { } public ApplicationManifest mergeWith(RuleContext ruleContext, - Iterable resourceContainers) { - if (!Iterables.isEmpty(getMergeeManifests(resourceContainers))) { - Iterable exportedManifests = getMergeeManifests(resourceContainers); + ResourceDependencies resourceDeps) { + Iterable mergeeManifests = getMergeeManifests(resourceDeps.getResources()); + if (!Iterables.isEmpty(mergeeManifests)) { + Iterable exportedManifests = mergeeManifests; Artifact outputManifest = ruleContext.getUniqueDirectoryArtifact( ruleContext.getRule().getName() + "_merged", "AndroidManifest.xml", ruleContext.getBinOrGenfilesDirectory()); @@ -198,7 +196,7 @@ public final class ApplicationManifest { public ResourceApk packWithAssets( Artifact resourceApk, RuleContext ruleContext, - NestedSet resourceContainers, + ResourceDependencies resourceDeps, Artifact rTxt, boolean incremental, Artifact proguardCfg) throws InterruptedException { @@ -214,7 +212,7 @@ public final class ApplicationManifest { return createApk(resourceApk, ruleContext, - resourceContainers, + resourceDeps, rTxt, null, /* configurationFilters */ ImmutableList.of(), /* uncompressedExtensions */ @@ -237,7 +235,7 @@ public final class ApplicationManifest { public ResourceApk packWithDataAndResources( Artifact resourceApk, RuleContext ruleContext, - NestedSet resourceContainers, + ResourceDependencies resourceDeps, Artifact rTxt, Artifact symbolsTxt, List configurationFilters, @@ -264,7 +262,7 @@ public final class ApplicationManifest { return createApk(resourceApk, ruleContext, - resourceContainers, + resourceDeps, rTxt, symbolsTxt, configurationFilters, @@ -287,7 +285,7 @@ public final class ApplicationManifest { private ResourceApk createApk(Artifact resourceApk, RuleContext ruleContext, - NestedSet resourceContainers, + ResourceDependencies resourceDeps, Artifact rTxt, Artifact symbolsTxt, List configurationFilters, @@ -305,7 +303,10 @@ public final class ApplicationManifest { .withROutput(rTxt) .withSymbolsFile(symbolsTxt) .buildFromRule(ruleContext, resourceApk), - resourceContainers, + resourceDeps.getResources(), // TODO(bazel-team): Figure out if we really need to check + // the ENTIRE transitive closure, or just the direct dependencies. Given that each rule with + // resources would check for inline resources, we can rely on the previous rule to have + // checked its dependencies. ruleContext); AndroidResourcesProcessorBuilder builder = @@ -316,7 +317,7 @@ public final class ApplicationManifest { .setJavaPackage(resourceContainer.getJavaPackage()) .setDebug(ruleContext.getConfiguration().getCompilationMode() != CompilationMode.OPT) .withPrimary(resourceContainer) - .withDependencies(resourceContainers) + .withDependencies(resourceDeps) .setDensities(densities) .setProguardOut(proguardCfg) .setApplicationId(applicationId) @@ -331,16 +332,9 @@ public final class ApplicationManifest { } ResourceContainer processed = builder.build(ruleContext); - NestedSet transitiveResources = - NestedSetBuilder.naiveLinkOrder() - // TODO(bazel-team): If this is replaced with .addTransitive(), a few tests fail. - // Investigate. - .addAll(resourceContainers) - .add(processed) - .build(); return new ResourceApk( - resourceApk, processed.getJavaSourceJar(), transitiveResources, processed, manifest, + resourceApk, processed.getJavaSourceJar(), resourceDeps, processed, manifest, proguardCfg, false); } @@ -363,15 +357,7 @@ public final class ApplicationManifest { /** Uses the resource apk from the resources attribute, as opposed to recompiling. */ public ResourceApk useCurrentResources(RuleContext ruleContext, Artifact proguardCfg) { ResourceContainer resourceContainer = Iterables.getOnlyElement( - AndroidCommon.getAndroidResources(ruleContext).getTransitiveAndroidResources()); - NestedSet resourceContainers = - NestedSetBuilder.emptySet(Order.NAIVE_LINK_ORDER); - - NestedSet transitiveResources = - NestedSetBuilder.naiveLinkOrder() - .addAll(resourceContainers) - .add(resourceContainer) - .build(); + AndroidCommon.getAndroidResources(ruleContext).getDirectAndroidResources()); new AndroidAaptActionHelper( ruleContext, @@ -381,7 +367,7 @@ public final class ApplicationManifest { return new ResourceApk( resourceContainer.getApk(), null /* javaSrcJar */, - transitiveResources, + ResourceDependencies.empty(), resourceContainer, manifest, proguardCfg, @@ -398,7 +384,7 @@ public final class ApplicationManifest { public ResourceApk packWithResources( Artifact resourceApk, RuleContext ruleContext, - NestedSet resourceContainers, + ResourceDependencies resourceDeps, boolean createSource, Artifact proguardCfg) throws InterruptedException { @@ -406,10 +392,15 @@ public final class ApplicationManifest { ruleContext.getPrerequisite("resources", Mode.TARGET); ResourceContainer resourceContainer = Iterables.getOnlyElement( resourcesPrerequisite.getProvider(AndroidResourcesProvider.class) - .getTransitiveAndroidResources()); + .getDirectAndroidResources()); + // It's ugly, but flattening now is more performant given the rest of the checks. + List resourceContainers = + ImmutableList.builder() + //.add(resourceContainer) + .addAll(resourceDeps.getResources()).build(); // Dealing with Android library projects - if (Iterables.size(resourceContainers) > 1) { + if (Iterables.size(resourceDeps.getResources()) > 1) { if (resourceContainer.getConstantsInlined() && !resourceContainer.getArtifacts(ResourceType.RESOURCES).isEmpty()) { ruleContext.ruleError("This android_binary depends on an android_library, so the" @@ -470,7 +461,7 @@ public final class ApplicationManifest { aaptActionHelper.createGenerateProguardAction(proguardCfg); return new ResourceApk(resourceApk, updatedResources.getJavaSourceJar(), - resourceContainers, updatedResources, manifest, proguardCfg, true); + resourceDeps, updatedResources, manifest, proguardCfg, true); } public Artifact getManifest() { diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/ResourceApk.java b/src/main/java/com/google/devtools/build/lib/rules/android/ResourceApk.java index 920f4cd68f..8a14507c95 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/ResourceApk.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/ResourceApk.java @@ -14,7 +14,7 @@ package com.google.devtools.build.lib.rules.android; import com.google.devtools.build.lib.actions.Artifact; -import com.google.devtools.build.lib.collect.nestedset.NestedSet; +import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; import com.google.devtools.build.lib.rules.android.AndroidResourcesProvider.ResourceContainer; @@ -31,7 +31,7 @@ public class ResourceApk { // to do this. @Nullable private final Artifact resourceApk; // The .ap_ file @Nullable private final Artifact resourceJavaSrcJar; // Source jar containing R.java and friends - private final NestedSet transitiveResources; + private final ResourceDependencies resourceDeps; @Nullable private final ResourceContainer primaryResource; @Nullable private final Artifact manifest; // The non-binary XML version of AndroidManifest.xml @Nullable private final Artifact resourceProguardConfig; @@ -40,14 +40,14 @@ public class ResourceApk { public ResourceApk( @Nullable Artifact resourceApk, @Nullable Artifact resourceJavaSrcJar, - NestedSet transitiveResources, + ResourceDependencies resourceDeps, @Nullable ResourceContainer primaryResource, @Nullable Artifact manifest, @Nullable Artifact resourceProguardConfig, boolean legacy) { this.resourceApk = resourceApk; this.resourceJavaSrcJar = resourceJavaSrcJar; - this.transitiveResources = transitiveResources; + this.resourceDeps = resourceDeps; this.primaryResource = primaryResource; this.manifest = manifest; this.resourceProguardConfig = resourceProguardConfig; @@ -74,16 +74,36 @@ public class ResourceApk { return legacy; } - public NestedSet getTransitiveResources() { - return transitiveResources; - } - public static ResourceApk fromTransitiveResources( - NestedSet transitiveResources) { - return new ResourceApk(null, null, transitiveResources, null, null, null, false); + ResourceDependencies resourceDeps) { + return new ResourceApk(null, null, resourceDeps, null, null, null, false); } public Artifact getResourceProguardConfig() { return resourceProguardConfig; } + + public ResourceDependencies getResourceDependencies() { + return resourceDeps; + } + + /** + * Creates an provider from the resources in the ResourceApk. + * + *

If the ResourceApk was created from transitive resources, the provider will effectively + * contain the "forwarded" resources: The merged transitive and merged direct dependencies of this + * library. + * + *

If the ResourceApk was generated from a "resources" attribute, it will contain the + * "resources" container in the direct dependencies and the rest as transitive. + * + *

If the ResourceApk was generated from local resources, that will be the direct dependencies and + * the rest will be transitive. + */ + public AndroidResourcesProvider toResourceProvider(Label label) { + if (primaryResource == null) { + return resourceDeps.toProvider(label); + } + return resourceDeps.toProvider(label, primaryResource); + } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/ResourceDependencies.java b/src/main/java/com/google/devtools/build/lib/rules/android/ResourceDependencies.java new file mode 100644 index 0000000000..a0766e32df --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/rules/android/ResourceDependencies.java @@ -0,0 +1,184 @@ +// Copyright 2015 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.rules.android; + +import com.google.devtools.build.lib.analysis.RuleConfiguredTarget.Mode; +import com.google.devtools.build.lib.analysis.RuleContext; +import com.google.devtools.build.lib.cmdline.Label; +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.packages.BuildType; +import com.google.devtools.build.lib.rules.android.AndroidResourcesProvider.ResourceContainer; + +/** + * Represents a container for the {@link ResourceContainer}s for a given library. This is + * abstraction simplifies the process of managing and exporting the direct and transitive resource + * dependencies of an android rule, as well as providing type safety. + * + *

The transitive and direct dependencies are not guaranteed to be disjoint. If a + * library is included in both the transitive and direct dependencies, it will appear twice. This + * requires consumers to manage duplicated resources gracefully. + */ +public class ResourceDependencies { + /** + * Contains all the transitive resources that are not generated by the direct ancestors of the + * current rule. + */ + private final NestedSet transitiveResources; + /** + * Contains all the direct dependencies of the current target. Since a given direct dependency can + * act as a "forwarding" library, collecting all the direct resource from it's dependencies + * and providing them as "direct" dependencies to maintain merge order, this uses a NestedSet to + * properly maintain ordering and ease of merging. + */ + private final NestedSet directResources; + + public static ResourceDependencies fromRuleResources(RuleContext ruleContext) { + if (!hasResourceAttribute(ruleContext)) { + return empty(); + } + + NestedSetBuilder transitiveDependencies = NestedSetBuilder.naiveLinkOrder(); + NestedSetBuilder directDependencies = NestedSetBuilder.naiveLinkOrder(); + extractFromAttribute("resources", ruleContext, transitiveDependencies, directDependencies); + return new ResourceDependencies(transitiveDependencies.build(), directDependencies.build()); + } + + public static ResourceDependencies fromRuleDeps(RuleContext ruleContext) { + NestedSetBuilder transitiveDependencies = NestedSetBuilder.naiveLinkOrder(); + NestedSetBuilder directDependencies = NestedSetBuilder.naiveLinkOrder(); + extractFromAttribute("deps", ruleContext, transitiveDependencies, directDependencies); + return new ResourceDependencies(transitiveDependencies.build(), directDependencies.build()); + } + + public static ResourceDependencies fromRuleResourceAndDeps(RuleContext ruleContext) { + NestedSetBuilder transitiveDependencies = NestedSetBuilder.naiveLinkOrder(); + NestedSetBuilder directDependencies = NestedSetBuilder.naiveLinkOrder(); + if (hasResourceAttribute(ruleContext)) { + extractFromAttribute("resources",ruleContext, transitiveDependencies, directDependencies); + } + if (directDependencies.isEmpty()) { + // There are no resources, so this library will forward the direct and transitive dependencies + // without changes. + extractFromAttribute("deps", ruleContext, transitiveDependencies, directDependencies); + } else { + // There are resources, so the direct dependencies and the transitive will be merged into + // the transitive dependencies. This maintains the relationship of the resources being + // directly on the rule. + extractFromAttribute("deps", ruleContext, transitiveDependencies, transitiveDependencies); + } + return new ResourceDependencies(transitiveDependencies.build(), directDependencies.build()); + } + + private static void extractFromAttribute(String attribute, + RuleContext ruleContext, NestedSetBuilder builderForTransitive, + NestedSetBuilder builderForDirect) { + for (AndroidResourcesProvider resources : + ruleContext.getPrerequisites(attribute, Mode.TARGET, AndroidResourcesProvider.class)) { + builderForTransitive.addTransitive(resources.getTransitiveAndroidResources()); + builderForDirect.addTransitive(resources.getDirectAndroidResources()); + } + } + + /** + * Check for the existence of a "resources" attribute. + * + *

The existence of the resources attribute is not guaranteed on for all android rules, so it + * is necessary to check for it. + * + * @param ruleContext The context to check. + * @return True if the ruleContext has resources, otherwise, false. + */ + private static boolean hasResourceAttribute(RuleContext ruleContext) { + return ruleContext.attributes().has("resources", BuildType.LABEL); + } + + @Override + public String toString() { + return "ResourceDependencies [transitiveResources=" + transitiveResources + ", directResources=" + + directResources + "]"; + } + + /** + * Creates an empty ResourceDependencies instance. This is used when an AndroidResources rule + * is the only resource dependency. The most common case is the AndroidTest rule. + */ + public static ResourceDependencies empty() { + return new ResourceDependencies( + NestedSetBuilder.emptySet(Order.NAIVE_LINK_ORDER), + NestedSetBuilder.emptySet(Order.NAIVE_LINK_ORDER)); + } + + public ResourceDependencies( + NestedSet transitiveResources, + NestedSet directResources) { + this.transitiveResources = transitiveResources; + this.directResources = directResources; + } + + /** + * Creates a new AndroidResourcesProvider with the supplied ResourceContainer as the direct dep. + * + *

When a library produces a new resource container the AndroidResourcesProvider should use + * that container as a the direct dependency for that library. This makes the consuming rule + * to identify the new container and merge appropriately. The previous direct dependencies are + * then added to the transitive dependencies. + * + * @param label The label of the library exporting this provider. + * @param newDirectResource The new direct dependency for AndroidResourcesProvider + * @return A provider with the current resources and label. + */ + public AndroidResourcesProvider toProvider(Label label, ResourceContainer newDirectResource) { + return new AndroidResourcesProvider( + label, + NestedSetBuilder.naiveLinkOrder() + .addTransitive(transitiveResources) + .addTransitive(directResources) + .build(), + NestedSetBuilder.naiveLinkOrder().add(newDirectResource).build()); + } + + /** + * Create a new AndroidResourcesProvider from the dependencies of this library. + * + *

When a library doesn't export resources it should simply forward the current transitive and + * direct resources to the consuming rule. This allows the consuming rule to make decisions about + * the resource merging as if this library didn't exist. + * + * @param label The label of the library exporting this provider. + * @return A provider with the current resources and label. + */ + public AndroidResourcesProvider toProvider(Label label) { + return new AndroidResourcesProvider(label, transitiveResources, directResources); + } + + /** + * Provides an NestedSet of the direct and transitive resources. + */ + public Iterable getResources() { + return NestedSetBuilder.naiveLinkOrder() + .addTransitive(directResources) + .addTransitive(transitiveResources) + .build(); + } + + public NestedSet getTransitiveResources() { + return transitiveResources; + } + + public NestedSet getDirectResources() { + return directResources; + } +} diff --git a/src/test/java/com/google/devtools/build/lib/actions/CustomCommandLineTest.java b/src/test/java/com/google/devtools/build/lib/actions/CustomCommandLineTest.java index d94780f2ff..a61790d645 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/CustomCommandLineTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/CustomCommandLineTest.java @@ -68,6 +68,13 @@ public class CustomCommandLineTest { assertEquals(ImmutableList.of("--arg", "a", "b"), cl.arguments()); } + @Test + public void testArtifactJoinStringArgs() { + CustomCommandLine cl = CustomCommandLine.builder().addJoinStrings("--path", ":", + ImmutableList.of("foo", "bar")).build(); + assertEquals(ImmutableList.of("--path", "foo:bar"), cl.arguments()); + } + @Test public void testArtifactExecPathArgs() { CustomCommandLine cl = CustomCommandLine.builder().addExecPath("--path", artifact1).build(); diff --git a/src/tools/android/java/com/google/devtools/build/android/AndroidResourceProcessingAction.java b/src/tools/android/java/com/google/devtools/build/android/AndroidResourceProcessingAction.java index 5d8ef6c27c..c621ba7095 100644 --- a/src/tools/android/java/com/google/devtools/build/android/AndroidResourceProcessingAction.java +++ b/src/tools/android/java/com/google/devtools/build/android/AndroidResourceProcessingAction.java @@ -17,6 +17,7 @@ package com.google.devtools.build.android; import com.google.common.base.Joiner; import com.google.common.base.Stopwatch; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; import com.google.common.hash.Hashing; import com.google.devtools.build.android.Converters.DependencyAndroidDataListConverter; import com.google.devtools.build.android.Converters.ExistingPathConverter; @@ -44,6 +45,7 @@ import java.nio.file.FileSystems; import java.nio.file.Files; import java.nio.file.Path; import java.util.Collection; +import java.util.LinkedHashSet; import java.util.List; import java.util.concurrent.TimeUnit; import java.util.logging.Logger; @@ -137,11 +139,21 @@ public class AndroidResourceProcessingAction { defaultValue = "", converter = DependencyAndroidDataListConverter.class, category = "input", - help = "Additional Data dependencies. These values will be used if not defined in the " + help = "Transitive Data dependencies. These values will be used if not defined in the " + "primary resources. The expected format is " - + "resources[#resources]:assets[#assets]:manifest:r.txt:symbols.txt" - + "[,resources[#resources]:assets[#assets]:manifest:r.txt:symbols.txt]") - public List data; + + "resources[#resources]:assets[#assets]:manifest:r.txt:symbols.bin" + + "[,resources[#resources]:assets[#assets]:manifest:r.txt:symbols.bin]") + public List transitiveData; + + @Option(name = "directData", + defaultValue = "", + converter = DependencyAndroidDataListConverter.class, + category = "input", + help = "Direct Data dependencies. These values will be used if not defined in the " + + "primary resources. The expected format is " + + "resources[#resources]:assets[#assets]:manifest:r.txt:symbols.bin" + + "[,resources[#resources]:assets[#assets]:manifest:r.txt:symbols.bin]") + public List directData; @Option(name = "rOutput", defaultValue = "null", @@ -298,11 +310,19 @@ public class AndroidResourceProcessingAction { new PackedResourceTarExpander(expandedOut, working), new FileDeDuplicator(Hashing.murmur3_128(), deduplicatedOut, working)); + // Resources can appear in both the direct dependencies and transitive -- use a set to + // ensure depeduplication. + List data = + ImmutableSet.builder() + .addAll(options.directData) + .addAll(options.transitiveData) + .build() + .asList(); final AndroidBuilder builder = sdkTools.createAndroidBuilder(); final MergedAndroidData mergedData = resourceProcessor.mergeData( options.primaryData, - options.data, + data, mergedResources, mergedAssets, modifiers, @@ -329,7 +349,7 @@ public class AndroidResourceProcessingAction { options.versionCode, options.versionName, filteredData, - options.data, + data, working.resolve("manifest"), generatedSources, options.packagePath, -- cgit v1.2.3