Skip to content

Commit

Permalink
Move resource busybox actions to BusyBoxActionBuilder and AndroidData…
Browse files Browse the repository at this point in the history 
…Context

BusyBoxActionBuilder makes much cleaner action builders while making it harder
to do Bad Things (like collapsing NestedSets in analysis, or adding an artifact
to one of the command line and the inputs but not both).

Also:
- In merging, simplify the code somewhat by removing unneeded conditionals -
  for example, the parsed merging action will always be built given the current
  code.
- In the few builders where we aren't doing so already, parameter files should
  always be shell quoted and always be used when the OS is Windows. The BusyBox
  should always support the former and require the latter (although
  sufficiently large inputs may have masked this by triggering parameter files
  in Windows anyway).
- In the builder for linking (within validation), no longer collapse the
  NestedSet of transitiveCompiledSymbols when adding them to the inputs. Using
  the new BusyBoxActionBuilder code, trying to do this would throw an
  IllegalStateException.

RELNOTES: none
PiperOrigin-RevId: 197728382
  • Loading branch information
@asteinb
asteinb authored and Copybara-Service committed May 23, 2018
1 parent 9dc32e2 commit 9d5c323
Show file tree
Hide file tree
Showing 20 changed files with 388 additions and 855 deletions.
6 changes: 2 additions & 4 deletions src/main/java/com/google/devtools/build/lib/rules/android/AarImport.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,7 @@ public ConfiguredTarget create(RuleContext ruleContext)
final AndroidDataContext dataContext = androidSemantics.makeContextForNative(ruleContext);
final ResourceApk resourceApk;
if (AndroidResources.decoupleDataProcessing(dataContext)) {
StampedAndroidManifest manifest =
AndroidManifest.forAarImport(androidManifestArtifact);
StampedAndroidManifest manifest = AndroidManifest.forAarImport(androidManifestArtifact);

boolean neverlink = JavaCommon.isNeverLink(ruleContext);
ValidatedAndroidResources validatedResources =
Expand All @@ -122,6 +121,7 @@ public ConfiguredTarget create(RuleContext ruleContext)
resourceApk =
androidManifest.packAarWithDataAndResources(
ruleContext,
dataContext,
AndroidAssets.forAarImport(assets),
AndroidResources.forAarImport(resources),
ResourceDependencies.fromRuleDeps(ruleContext, JavaCommon.isNeverLink(ruleContext)),
Expand Down Expand Up @@ -168,8 +168,6 @@ public ConfiguredTarget create(RuleContext ruleContext)
.addCompileTimeJarAsFullJar(mergedJar)
.build());



JavaConfiguration javaConfig = ruleContext.getFragment(JavaConfiguration.class);
// TODO(cnsun): need to pass the transitive classpath too to emit add dep command.
NestedSet<Artifact> directDeps = getCompileTimeJarsFromCollection(targets, /*isStrict=*/ true);
Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,7 @@ private static RuleConfiguredTargetBuilder init(
resourceApk =
applicationManifest.packBinaryWithDataAndResources(
ruleContext,
dataContext,
ruleContext.getImplicitOutputArtifact(AndroidRuleClasses.ANDROID_RESOURCES_APK),
resourceDeps,
ruleContext.getImplicitOutputArtifact(AndroidRuleClasses.ANDROID_R_TXT),
Expand Down
2 changes: 2 additions & 0 deletions src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinaryMobileInstall.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ static MobileInstallResourceApks createMobileInstallResourceApks(
.addMobileInstallStubApplication(ruleContext)
.packIncrementalBinaryWithDataAndResources(
ruleContext,
dataContext,
ruleContext.getImplicitOutputArtifact(
AndroidRuleClasses.ANDROID_INCREMENTAL_RESOURCES_APK),
resourceDeps,
Expand All @@ -114,6 +115,7 @@ static MobileInstallResourceApks createMobileInstallResourceApks(
.createSplitManifest(ruleContext, "android_resources", false)
.packIncrementalBinaryWithDataAndResources(
ruleContext,
dataContext,
getMobileInstallArtifact(ruleContext, "android_resources.ap_"),
resourceDeps,
ruleContext.getExpander().withDataLocations().tokenized("nocompress_extensions"),
Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/rules/android/AndroidLibrary.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,7 @@ public ConfiguredTarget create(RuleContext ruleContext)
resourceApk =
applicationManifest.packLibraryWithDataAndResources(
ruleContext,
dataContext,
resourceDeps,
ruleContext.getImplicitOutputArtifact(AndroidRuleClasses.ANDROID_R_TXT),
ruleContext.getImplicitOutputArtifact(AndroidRuleClasses.ANDROID_MERGED_SYMBOLS),
Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/rules/android/AndroidLocalTestBase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ public ConfiguredTarget create(RuleContext ruleContext)
resourceApk =
applicationManifest.packBinaryWithDataAndResources(
ruleContext,
dataContext,
ruleContext.getImplicitOutputArtifact(AndroidRuleClasses.ANDROID_RESOURCES_APK),
resourceDependencies,
ruleContext.getImplicitOutputArtifact(AndroidRuleClasses.ANDROID_R_TXT),
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/rules/android/AndroidManifestMergeHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,4 +71,4 @@ public static void createMergeManifestAction(
.addCommandLine(commandLine.build())
.build(context));
}
}
}
229 changes: 70 additions & 159 deletions ...java/com/google/devtools/build/lib/rules/android/AndroidResourceMergingActionBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,21 +14,10 @@
package com.google.devtools.build.lib.rules.android;

import com.google.common.base.Preconditions;
import com.google.common.base.Strings;
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.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.analysis.configuredtargets.RuleConfiguredTarget.Mode;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.rules.android.AndroidDataConverter.JoinerType;
import com.google.devtools.build.lib.util.OS;
import java.util.ArrayList;
import java.util.List;
import javax.annotation.Nullable;

/**
Expand All @@ -52,9 +41,6 @@ public class AndroidResourceMergingActionBuilder {
.withArtifact(CompiledMergableAndroidData::getCompiledSymbols)
.build();

private final RuleContext ruleContext;
private final AndroidSdkProvider sdk;

// Inputs
private CompiledMergableAndroidData primary;
private ResourceDependencies dependencies;
Expand All @@ -70,12 +56,6 @@ public class AndroidResourceMergingActionBuilder {
private boolean throwOnResourceConflict;
private boolean useCompiledMerge;

/** @param ruleContext The RuleContext that was used to create the SpawnAction.Builder. */
public AndroidResourceMergingActionBuilder(RuleContext ruleContext) {
this.ruleContext = ruleContext;
this.sdk = AndroidSdkProvider.fromRuleContext(ruleContext);
}

/**
* The primary resource for merging. This resource will overwrite any resource or data value in
* the transitive closure.
Expand Down Expand Up @@ -132,172 +112,102 @@ public AndroidResourceMergingActionBuilder setUseCompiledMerge(boolean useCompil
return this;
}

private NestedSetBuilder<Artifact> createInputsForBuilder(CustomCommandLine.Builder builder) {
// Use a FluentIterable to avoid flattening the NestedSets
NestedSetBuilder<Artifact> inputs = NestedSetBuilder.naiveLinkOrder();

builder.addExecPath("--androidJar", sdk.getAndroidJar());
inputs.add(sdk.getAndroidJar());

Preconditions.checkNotNull(primary.getManifest());
builder.addExecPath("--primaryManifest", primary.getManifest());
inputs.add(primary.getManifest());

if (!Strings.isNullOrEmpty(customJavaPackage)) {
// Sets an alternative java package for the generated R.java
// this allows android rules to generate resources outside of the java{,tests} tree.
builder.add("--packageForR", customJavaPackage);
}

if (throwOnResourceConflict) {
builder.add("--throwOnResourceConflict");
}

return inputs;
private BusyBoxActionBuilder createInputsForBuilder(BusyBoxActionBuilder builder) {
return builder
.addAndroidJar()
.addInput("--primaryManifest", primary.getManifest())
.maybeAddFlag("--packageForR", customJavaPackage)
.maybeAddFlag("--throwOnResourceConflict", throwOnResourceConflict);
}

private void buildCompiledResourceMergingAction(
CustomCommandLine.Builder builder,
List<Artifact> outputs,
ActionConstructionContext context) {
NestedSetBuilder<Artifact> inputs = createInputsForBuilder(builder);

private void buildCompiledResourceMergingAction(BusyBoxActionBuilder builder) {
Preconditions.checkNotNull(primary);
builder.add("--primaryData", RESOURCE_CONTAINER_TO_ARG_FOR_COMPILED.map(primary));
inputs.addAll(primary.getArtifacts());
inputs.add(primary.getCompiledSymbols());

createInputsForBuilder(builder)
.addInput(
"--primaryData",
RESOURCE_CONTAINER_TO_ARG_FOR_COMPILED.map(primary),
Iterables.concat(
primary.getArtifacts(), ImmutableList.of(primary.getCompiledSymbols())));

if (dependencies != null) {
RESOURCE_CONTAINER_TO_ARG_FOR_COMPILED.addDepsToCommandLine(
builder,
dependencies.getDirectResourceContainers(),
dependencies.getTransitiveResourceContainers());
inputs.addTransitive(dependencies.getTransitiveResources());
inputs.addTransitive(dependencies.getTransitiveAssets());
inputs.addTransitive(dependencies.getTransitiveCompiledSymbols());
builder
.addTransitiveFlag(
"--data",
dependencies.getTransitiveResourceContainers(),
RESOURCE_CONTAINER_TO_ARG_FOR_COMPILED)
.addTransitiveFlag(
"--directData",
dependencies.getDirectResourceContainers(),
RESOURCE_CONTAINER_TO_ARG_FOR_COMPILED)
.addTransitiveInputValues(dependencies.getTransitiveResources())
.addTransitiveInputValues(dependencies.getTransitiveAssets())
.addTransitiveInputValues(dependencies.getTransitiveCompiledSymbols());
}

SpawnAction.Builder spawnActionBuilder = new SpawnAction.Builder();
ParamFileInfo.Builder compiledParamFileInfo = ParamFileInfo.builder(ParameterFileType.UNQUOTED);
compiledParamFileInfo.setUseAlways(OS.getCurrent() == OS.WINDOWS);
// Create the spawn action.
ruleContext.registerAction(
spawnActionBuilder
.useDefaultShellEnvironment()
.addTransitiveInputs(inputs.build())
.addOutputs(ImmutableList.copyOf(outputs))
.addCommandLine(builder.build(), compiledParamFileInfo.build())
.setExecutable(
ruleContext.getExecutablePrerequisite("$android_resources_busybox", Mode.HOST))
.setProgressMessage("Merging compiled Android resources for %s", ruleContext.getLabel())
.setMnemonic("AndroidCompiledResourceMerger")
.build(context));
builder.buildAndRegister("Merging compiled Android resources", "AndroidCompiledResourceMerger");
}

private void buildParsedResourceMergingAction(
CustomCommandLine.Builder builder,
List<Artifact> outputs,
ActionConstructionContext context) {
NestedSetBuilder<Artifact> inputs = createInputsForBuilder(builder);

private void buildParsedResourceMergingAction(BusyBoxActionBuilder builder) {
Preconditions.checkNotNull(primary);
builder.add("--primaryData", RESOURCE_CONTAINER_TO_ARG.map(primary));
inputs.addAll(primary.getArtifacts());
inputs.add(primary.getSymbols());

createInputsForBuilder(builder)
.addInput(
"--primaryData",
RESOURCE_CONTAINER_TO_ARG.map(primary),
Iterables.concat(primary.getArtifacts(), ImmutableList.of(primary.getSymbols())));

if (dependencies != null) {
RESOURCE_CONTAINER_TO_ARG.addDepsToCommandLine(
builder,
dependencies.getDirectResourceContainers(),
dependencies.getTransitiveResourceContainers());
inputs.addTransitive(dependencies.getTransitiveResources());
inputs.addTransitive(dependencies.getTransitiveAssets());
inputs.addTransitive(dependencies.getTransitiveSymbolsBin());
builder
.addTransitiveFlag(
"--data", dependencies.getTransitiveResourceContainers(), RESOURCE_CONTAINER_TO_ARG)
.addTransitiveFlag(
"--directData", dependencies.getDirectResourceContainers(), RESOURCE_CONTAINER_TO_ARG)
.addTransitiveInputValues(dependencies.getTransitiveResources())
.addTransitiveInputValues(dependencies.getTransitiveAssets())
.addTransitiveInputValues(dependencies.getTransitiveSymbolsBin());
}

SpawnAction.Builder spawnActionBuilder = new SpawnAction.Builder();
ParamFileInfo.Builder paramFileInfo = ParamFileInfo.builder(ParameterFileType.SHELL_QUOTED);
// Some flags (e.g. --mainData) may specify lists (or lists of lists) separated by special
// characters (colon, semicolon, hashmark, ampersand) that don't work on Windows, and quoting
// semantics are very complicated (more so than in Bash), so let's just always use a parameter
// file.
// TODO(laszlocsomor), TODO(corysmith): restructure the Android BusyBux's flags by deprecating
// list-type and list-of-list-type flags that use such problematic separators in favor of
// multi-value flags (to remove one level of listing) and by changing all list separators to a
// platform-safe character (= comma).
paramFileInfo.setUseAlways(OS.getCurrent() == OS.WINDOWS);

// Create the spawn action.
ruleContext.registerAction(
spawnActionBuilder
.useDefaultShellEnvironment()
.addTransitiveInputs(inputs.build())
.addOutputs(ImmutableList.copyOf(outputs))
.addCommandLine(builder.build(), paramFileInfo.build())
.setExecutable(
ruleContext.getExecutablePrerequisite("$android_resources_busybox", Mode.HOST))
.setProgressMessage("Merging Android resources for %s", ruleContext.getLabel())
.setMnemonic("AndroidResourceMerger")
.build(context));
builder.buildAndRegister("Merging Android resources", "AndroidResourceMerger");
}

private void build(RuleContext context) {
CustomCommandLine.Builder parsedMergeBuilder =
new CustomCommandLine.Builder().add("--tool").add("MERGE").add("--");
CustomCommandLine.Builder compiledMergeBuilder =
new CustomCommandLine.Builder().add("--tool").add("MERGE_COMPILED").add("--");
List<Artifact> parsedMergeOutputs = new ArrayList<>();
List<Artifact> compiledMergeOutputs = new ArrayList<>();
private void build(AndroidDataContext dataContext) {
BusyBoxActionBuilder parsedMergeBuilder = BusyBoxActionBuilder.create(dataContext, "MERGE");
BusyBoxActionBuilder compiledMergeBuilder =
BusyBoxActionBuilder.create(dataContext, "MERGE_COMPILED");

if (mergedResourcesOut != null) {
parsedMergeBuilder.addExecPath("--resourcesOutput", mergedResourcesOut);
parsedMergeOutputs.add(mergedResourcesOut);
}
parsedMergeBuilder.addOutput("--resourcesOutput", mergedResourcesOut);

// TODO(corysmith): Move the data binding parsing out of the merging pass to enable faster
// aapt2 builds.
if (dataBindingInfoZip != null) {
parsedMergeBuilder.addExecPath("--dataBindingInfoOut", dataBindingInfoZip);
parsedMergeOutputs.add(dataBindingInfoZip);
}

CustomCommandLine.Builder jarAndManifestBuilder =
useCompiledMerge ? compiledMergeBuilder : parsedMergeBuilder;
List<Artifact> jarAndManifestOutputs =
useCompiledMerge ? compiledMergeOutputs : parsedMergeOutputs;
parsedMergeBuilder.maybeAddOutput("--dataBindingInfoOut", dataBindingInfoZip);

if (classJarOut != null) {
jarAndManifestBuilder.addExecPath("--classJarOutput", classJarOut);
jarAndManifestBuilder.addLabel("--targetLabel", ruleContext.getLabel());
jarAndManifestOutputs.add(classJarOut);
}
(useCompiledMerge ? compiledMergeBuilder : parsedMergeBuilder)
.addOutput("--classJarOutput", classJarOut)
.addLabelFlag("--targetLabel")

// For now, do manifest processing to remove placeholders that aren't handled by the legacy
// manifest merger. Remove this once enough users migrate over to the new manifest merger.
if (manifestOut != null) {
jarAndManifestBuilder.addExecPath("--manifestOutput", manifestOut);
jarAndManifestOutputs.add(manifestOut);
}
// For now, do manifest processing to remove placeholders that aren't handled by the legacy
// manifest merger. Remove this once enough users migrate over to the new manifest merger.
.maybeAddOutput("--manifestOutput", manifestOut);

if (!compiledMergeOutputs.isEmpty()) {
buildCompiledResourceMergingAction(compiledMergeBuilder, compiledMergeOutputs, context);
if (useCompiledMerge) {
buildCompiledResourceMergingAction(compiledMergeBuilder);
}

if (!parsedMergeOutputs.isEmpty()) {
buildParsedResourceMergingAction(parsedMergeBuilder, parsedMergeOutputs, context);
}
// Always make an action for merging parsed resources - the merged resources are still created
// this way.
buildParsedResourceMergingAction(parsedMergeBuilder);
}

public ResourceContainer build(RuleContext ruleContext, ResourceContainer resourceContainer) {
withPrimary(resourceContainer).build(ruleContext);
public ResourceContainer build(
AndroidDataContext dataContext, ResourceContainer resourceContainer) {
withPrimary(resourceContainer).build(dataContext);

// Return the full set of processed transitive dependencies.
ResourceContainer.Builder result = resourceContainer.toBuilder();
if (classJarOut != null) {
// ensure the classJar is propagated if it exists. Otherwise, AndroidCommon tries to make it.
// TODO(corysmith): Centralize the class jar generation.
result.setJavaClassJar(classJarOut);
}

result.setJavaClassJar(classJarOut);

if (manifestOut != null) {
result.setManifest(manifestOut);
}
Expand All @@ -307,8 +217,9 @@ public ResourceContainer build(RuleContext ruleContext, ResourceContainer resour
return result.build();
}

public MergedAndroidResources build(RuleContext ruleContext, ParsedAndroidResources parsed) {
withPrimary(parsed).build(ruleContext);
public MergedAndroidResources build(
AndroidDataContext dataContext, ParsedAndroidResources parsed) {
withPrimary(parsed).build(dataContext);

return MergedAndroidResources.of(
parsed,
Expand Down
Loading

0 comments on commit 9d5c323

Please sign in to comment.