diff options
4 files changed, 173 insertions, 60 deletions
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 f76a5b8b0e..aa565155a0 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 @@ -17,7 +17,6 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Strings.isNullOrEmpty; import com.google.common.base.Optional; -import com.google.common.base.Predicate; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; @@ -68,6 +67,7 @@ import com.google.devtools.build.lib.util.Preconditions; import com.google.devtools.build.lib.vfs.PathFragment; import java.util.ArrayList; +import java.util.Collection; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -1118,9 +1118,8 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory { Artifact classesDex = getDxArtifact(ruleContext, "classes.dex.zip"); Artifact jarToDex = getDxArtifact(ruleContext, "classes.jar"); createShuffleJarAction(ruleContext, true, (Artifact) null, ImmutableList.of(jarToDex), - common, attributes, (Artifact) null); - createDexMergerAction(ruleContext, "off", jarToDex, classesDex, (Artifact) null, - /* minimalMainDex */ false); + common, dexopts, attributes, (Artifact) null); + createDexMergerAction(ruleContext, "off", jarToDex, classesDex, (Artifact) null, dexopts); return new DexingOutput(classesDex, binaryJar, ImmutableList.of(classesDex)); } else { // By *not* writing a zip we get dx to drop resources on the floor. @@ -1152,6 +1151,7 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory { finalJarIsDerived ? proguardedJar : null, shards, common, + dexopts, attributes, mainDexList); @@ -1168,7 +1168,7 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory { // instead of being a conventional Jar file with .class files. String multidexStrategy = mainDexList != null && i == 1 ? "minimal" : "best_effort"; createDexMergerAction(ruleContext, multidexStrategy, shard, shardDex, (Artifact) null, - /* minimalMainDex */ false); + dexopts); } else { AndroidCommon.createDexAction( ruleContext, shard, shardDex, dexopts, /* multidex */ true, (Artifact) null); @@ -1199,10 +1199,8 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory { if (incrementalDexing.contains(AndroidBinaryType.MULTIDEX_UNSHARDED)) { Artifact jarToDex = AndroidBinary.getDxArtifact(ruleContext, "classes.jar"); createShuffleJarAction(ruleContext, true, (Artifact) null, ImmutableList.of(jarToDex), - common, attributes, (Artifact) null); - createDexMergerAction(ruleContext, "minimal", jarToDex, classesDex, mainDexList, - // unlike dexopts.contains(), this works even for "--a --b" in one string - Iterables.any(dexopts, FlagMatcher.MINIMAL_MAIN_DEX)); + common, dexopts, attributes, (Artifact) null); + createDexMergerAction(ruleContext, "minimal", jarToDex, classesDex, mainDexList, dexopts); } else { // Because the dexer also places resources into this zip, we also need to create a cleanup // action that removes all non-.dex files before staging for apk building. @@ -1229,7 +1227,7 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory { AndroidCommon.getAndroidConfig(ruleContext).getIncrementalDexingBinaries(); if (!result.isEmpty() && Iterables.any(dexopts, - new FlagMatcher(AndroidCommon + new DexArchiveAspect.FlagMatcher(AndroidCommon .getAndroidConfig(ruleContext) .getTargetDexoptsThatPreventIncrementalDexing()))) { result = ImmutableSet.of(); @@ -1243,24 +1241,20 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory { Artifact inputJar, Artifact classesDex, @Nullable Artifact mainDexList, - boolean minimalMainDex) { - checkArgument(!minimalMainDex || mainDexList != null, "expected mainDexList"); + Collection<String> dexopts) { SpawnAction.Builder dexmerger = new SpawnAction.Builder() .setExecutable(ruleContext.getExecutablePrerequisite("$dexmerger", Mode.HOST)) .addArgument("--input") .addInputArgument(inputJar) .addArgument("--output") .addOutputArgument(classesDex) + .addArguments(DexArchiveAspect.incrementalDexingOptions(ruleContext, dexopts)) .addArgument("--multidex=" + multidexStrategy) .setMnemonic("DexMerger") .setProgressMessage("Assembling dex files into " + classesDex.prettyPrint()); - if (ruleContext.getConfiguration().isCodeCoverageEnabled()) { - // Match what we do in AndroidCommon.createDexAction - dexmerger.addArgument("--nolocals"); // TODO(bazel-team): Still needed? See createDexAction - } if (mainDexList != null) { dexmerger.addArgument("--main-dex-list").addInputArgument(mainDexList); - if (minimalMainDex) { + if (dexopts.contains("--minimal-main-dex")) { dexmerger.addArgument("--minimal-main-dex"); } } @@ -1272,7 +1266,7 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory { * dex archives for the Jars produced by the binary target itself. */ private static DexArchiveProvider collectDexArchives( - RuleContext ruleContext, AndroidCommon common) { + RuleContext ruleContext, AndroidCommon common, List<String> dexopts) { DexArchiveProvider.Builder result = new DexArchiveProvider.Builder() // Use providers from all attributes that declare DexArchiveAspect .addTransitiveProviders( @@ -1287,7 +1281,7 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory { Artifact dexArchive = ruleContext.getDerivedArtifact( jarPath.replaceName(jarPath.getBaseName() + ".dex.zip"), jar.getRoot()); - DexArchiveAspect.createDexArchiveAction(ruleContext, jar, dexArchive); + DexArchiveAspect.createDexArchiveAction(ruleContext, jar, dexArchive, dexopts); result.addDexArchive(dexArchive, jar); } return result.build(); @@ -1299,6 +1293,7 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory { @Nullable Artifact proguardedJar, List<Artifact> shards, AndroidCommon common, + List<String> dexopts, JavaTargetAttributes attributes, @Nullable Artifact mainDexList) throws InterruptedException { @@ -1341,7 +1336,8 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory { // Use dex archives instead of their corresponding Jars wherever we can. At this point // there should be very few or no Jar files that still end up in shards. The dexing // step below will have to deal with those in addition to merging .dex files together. - classpath = Iterables.transform(classpath, collectDexArchives(ruleContext, common)); + classpath = Iterables + .transform(classpath, collectDexArchives(ruleContext, common, dexopts)); shardCommandLine.add("--split_dexed_classes"); } shardCommandLine.addBeforeEachExecPath("--input_jar", classpath); @@ -1723,25 +1719,4 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory { return ruleContext.getUniqueDirectoryArtifact("_dx", baseName, ruleContext.getBinOrGenfilesDirectory()); } - - private static class FlagMatcher implements Predicate<String> { - static final FlagMatcher MINIMAL_MAIN_DEX = - new FlagMatcher(ImmutableList.of("--minimal-main-dex")); - - private final ImmutableList<String> matching; - - FlagMatcher(ImmutableList<String> matching) { - this.matching = matching; - } - - @Override - public boolean apply(String input) { - for (String match : matching) { - if (input.contains(match)) { - return true; - } - } - return false; - } - } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java index 606a35e9a0..3966eee33f 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java @@ -262,11 +262,21 @@ public class AndroidConfiguration extends BuildConfiguration.Fragment { category = "semantics", help = "dx flags that that prevent incremental dexing for binary targets that list any of " + "the flags listed here in their 'dexopts' attribute, which are ignored with " - + "incremental dexing. Defaults to --no-locals for safety but can in general be used " + + "incremental dexing (superseding --dexopts_supported_in_incremental_dexing). " + + "Defaults to --no-locals for safety but can in general be used " + "to make sure the listed dx flags are honored, with additional build latency. " + "Please notify us if you find yourself needing this flag.") public List<String> nonIncrementalPerTargetDexopts; + // Do not use on the command line. + // This flag is intended to be updated as we add supported flags to the incremental dexing tools + @Option(name = "dexopts_supported_in_incremental_dexing", + converter = Converters.CommaSeparatedOptionListConverter.class, + defaultValue = "", + category = "hidden", + help = "dx flags supported in incremental dexing.") + public List<String> dexoptsSupportedInIncrementalDexing; + @Option(name = "experimental_allow_android_library_deps_without_srcs", defaultValue = "true", category = "undocumented", @@ -345,6 +355,7 @@ public class AndroidConfiguration extends BuildConfiguration.Fragment { private final boolean useJackForDexing; private final boolean jackSanityChecks; private final ImmutableSet<AndroidBinaryType> incrementalDexingBinaries; + private final ImmutableList<String> dexoptsSupportedInIncrementalDexing; private final ImmutableList<String> targetDexoptsThatPreventIncrementalDexing; private final boolean allowAndroidLibraryDepsWithoutSrcs; private final boolean useAndroidResourceShrinking; @@ -364,6 +375,8 @@ public class AndroidConfiguration extends BuildConfiguration.Fragment { } else { this.incrementalDexingBinaries = options.dexingStrategy.binaryTypes; } + this.dexoptsSupportedInIncrementalDexing = + ImmutableList.copyOf(options.dexoptsSupportedInIncrementalDexing); this.targetDexoptsThatPreventIncrementalDexing = ImmutableList.copyOf(options.nonIncrementalPerTargetDexopts); this.allowAndroidLibraryDepsWithoutSrcs = options.allowAndroidLibraryDepsWithoutSrcs; @@ -418,8 +431,15 @@ public class AndroidConfiguration extends BuildConfiguration.Fragment { } /** - * Regardless of {@link #getIncrementalDexing}, incremental dexing must not be used for binaries - * that list any of these flags in their {@code dexopts} attribute. + * dx flags supported in incremental dexing actions. + */ + public ImmutableList<String> getDexoptsSupportedInIncrementalDexing() { + return dexoptsSupportedInIncrementalDexing; + } + + /** + * Regardless of {@link #getIncrementalDexingBinaries}, incremental dexing must not be used for + * binaries that list any of these flags in their {@code dexopts} attribute. */ public ImmutableList<String> getTargetDexoptsThatPreventIncrementalDexing() { return targetDexoptsThatPreventIncrementalDexing; diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidRuleClasses.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidRuleClasses.java index a6a612a42a..600a68ec3a 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidRuleClasses.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidRuleClasses.java @@ -584,7 +584,7 @@ public final class AndroidRuleClasses { .allowedRuleClasses(ALLOWED_DEPENDENCIES) .allowedFileTypes() .aspect(androidNeverlinkAspect) - .aspect(dexArchiveAspect) + .aspect(dexArchiveAspect, DexArchiveAspect.PARAM_EXTRACTOR) .aspect(jackAspect)) // Proguard rule specifying master list of classes to keep during legacy multidexing. .add(attr("$build_incremental_dexmanifest", LABEL).cfg(HOST).exec() @@ -614,7 +614,9 @@ public final class AndroidRuleClasses { Subject to <a href="make-variables.html">"Make variable"</a> substitution and <a href="common-definitions.html#sh-tokenization">Bourne shell tokenization</a>. <!-- #END_BLAZE_RULE.ATTRIBUTE --> */ - .add(attr("dexopts", STRING_LIST)) + .add(attr("dexopts", STRING_LIST) + // Read by DexArchiveAspect's attribute extractor + .nonconfigurable("AspectParameters don't support configurations.")) /* <!-- #BLAZE_RULE($android_binary_base).ATTRIBUTE(dex_shards) --> Number of shards dexing should be decomposed into. This is makes dexing much faster at the expense of app installation and startup time. The diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/DexArchiveAspect.java b/src/main/java/com/google/devtools/build/lib/rules/android/DexArchiveAspect.java index 06698e10ef..bf2ad53fe3 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/DexArchiveAspect.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/DexArchiveAspect.java @@ -18,9 +18,15 @@ import static com.google.devtools.build.lib.packages.Attribute.ConfigurationTran import static com.google.devtools.build.lib.packages.Attribute.attr; import static com.google.devtools.build.lib.packages.BuildType.LABEL; import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST; +import static com.google.devtools.build.lib.rules.android.AndroidCommon.getAndroidConfig; +import static com.google.devtools.build.lib.syntax.Type.STRING_LIST; import static java.nio.charset.StandardCharsets.ISO_8859_1; +import com.google.common.base.Function; +import com.google.common.base.Joiner; +import com.google.common.base.Predicate; 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.ParameterFile; import com.google.devtools.build.lib.analysis.ConfiguredAspect; @@ -34,16 +40,45 @@ import com.google.devtools.build.lib.analysis.actions.SpawnAction; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.packages.AspectDefinition; import com.google.devtools.build.lib.packages.AspectParameters; +import com.google.devtools.build.lib.packages.AttributeMap; import com.google.devtools.build.lib.packages.NativeAspectClass; +import com.google.devtools.build.lib.packages.NonconfigurableAttributeMapper; +import com.google.devtools.build.lib.packages.Rule; import com.google.devtools.build.lib.rules.java.JavaCommon; import com.google.devtools.build.lib.rules.java.JavaCompilationArgsProvider; import com.google.devtools.build.lib.rules.java.JavaRuntimeJarProvider; +import java.util.ArrayList; +import java.util.Collection; +import java.util.List; +import java.util.Set; +import java.util.TreeSet; + +import javax.annotation.Nullable; + /** * Aspect to {@link DexArchiveProvider build .dex Archives} from Jars. */ public final class DexArchiveAspect extends NativeAspectClass implements ConfiguredAspectFactory { public static final String NAME = "DexArchiveAspect"; + /** + * Function that returns a {@link Rule}'s {@code dexopts} attribute for use by this aspect. + * Must be provided when attaching this aspect to a rule. + */ + static final Function<Rule, AspectParameters> PARAM_EXTRACTOR = + new Function<Rule, AspectParameters>() { + @Nullable + @Override + public AspectParameters apply(Rule rule) { + AttributeMap attributes = NonconfigurableAttributeMapper.of(rule); + AspectParameters.Builder result = new AspectParameters.Builder(); + for (String opt : attributes.get("dexopts", STRING_LIST)) { + result.addAttribute("dexopts", opt); + } + return result.build(); + } + }; + private static final String ASPECT_DEXBUILDER_PREREQ = "$dex_archive_dexbuilder"; private static final ImmutableList<String> TRANSITIVE_ATTRIBUTES = ImmutableList.of("deps", "exports", "runtime_deps"); @@ -71,7 +106,7 @@ public final class DexArchiveAspect extends NativeAspectClass implements Configu @Override public ConfiguredAspect create(ConfiguredTarget base, RuleContext ruleContext, AspectParameters params) throws InterruptedException { - if (AndroidCommon.getAndroidConfig(ruleContext).getIncrementalDexingBinaries().isEmpty()) { + if (getAndroidConfig(ruleContext).getIncrementalDexingBinaries().isEmpty()) { // Dex archives will never be used, so don't bother setting them up. return new ConfiguredAspect.Builder(NAME, ruleContext).build(); } @@ -87,8 +122,9 @@ public final class DexArchiveAspect extends NativeAspectClass implements Configu DexArchiveProvider.Builder result = createArchiveProviderBuilderFromDeps(ruleContext); JavaRuntimeJarProvider jarProvider = base.getProvider(JavaRuntimeJarProvider.class); if (jarProvider != null) { + Collection<String> incrementalDexopts = aspectDexopts(ruleContext, params); for (Artifact jar : jarProvider.getRuntimeJars()) { - Artifact dexArchive = createDexArchiveAction(ruleContext, jar); + Artifact dexArchive = createDexArchiveAction(ruleContext, jar, incrementalDexopts); result.addDexArchive(dexArchive, jar); } } @@ -109,8 +145,15 @@ public final class DexArchiveAspect extends NativeAspectClass implements Configu return result; } - private static Artifact createDexArchiveAction(RuleContext ruleContext, Artifact jar) { - Artifact result = AndroidBinary.getDxArtifact(ruleContext, jar.getFilename() + ".dex.zip"); + private static Artifact createDexArchiveAction(RuleContext ruleContext, Artifact jar, + Collection<String> incrementalDexopts) { + // When DexArchiveAspect is applied to the dependencies of multiple android_binary rules, + // multiple aspects could end up being applied to shared dependencies. This is a problem because + // the dexopts from the different android_binary rules could be different, leading to different + // artifacts. So, the artifacts must be uniquely named. Here, it is convenient to distinguish + // them by the flags that were used for creating the artifacts. + String filename = jar.getFilename() + Joiner.on("").join(incrementalDexopts) + ".dex.zip"; + Artifact result = AndroidBinary.getDxArtifact(ruleContext, filename); // Aspect must use attribute name for dexbuilder prereq that's different from the prerequisite // declared on AndroidBinaryBaseRule because the two prereq's can otherwise name-clash when // android_binary targets are built as part of an android_test: building android_test causes @@ -119,7 +162,7 @@ public final class DexArchiveAspect extends NativeAspectClass implements Configu // RuleContext.getExecutablePrerequisite would fail with "$dexbuilder produces multiple prereqs" // (note they both resolve to the same artifact but that doesn't seem to prevent the exception // from being thrown). - createDexArchiveAction(ruleContext, ASPECT_DEXBUILDER_PREREQ, jar, result); + createDexArchiveAction(ruleContext, ASPECT_DEXBUILDER_PREREQ, jar, result, incrementalDexopts); return result; } @@ -129,20 +172,23 @@ public final class DexArchiveAspect extends NativeAspectClass implements Configu * {@link #getDefinition} does it for {@link DexArchiveAspect} under a different name. */ // Package-private method for use in AndroidBinary - static void createDexArchiveAction(RuleContext ruleContext, Artifact jar, Artifact dexArchive) { - createDexArchiveAction(ruleContext, "$dexbuilder", jar, dexArchive); + static void createDexArchiveAction(RuleContext ruleContext, Artifact jar, Artifact dexArchive, + Collection<String> tokenizedDexopts) { + createDexArchiveAction(ruleContext, "$dexbuilder", jar, dexArchive, + incrementalDexingOptions(ruleContext, tokenizedDexopts)); } + /** + * Creates a dexbuilder action with the given input, output, and flags. Flags must have been + * filtered and normalized to a set that the dexbuilder tool can understand. + */ private static void createDexArchiveAction(RuleContext ruleContext, String dexbuilderPrereq, - Artifact jar, Artifact dexArchive) { + Artifact jar, Artifact dexArchive, Collection<String> incrementalDexopts) { // Write command line arguments into a params file for compatibility with WorkerSpawnStrategy CustomCommandLine.Builder args = new CustomCommandLine.Builder() .addExecPath("--input_jar", jar) - .addExecPath("--output_zip", dexArchive); - if (ruleContext.getConfiguration().isCodeCoverageEnabled()) { - // Match what we do in AndroidCommon.createDexAction - args.add("--nolocals"); // TODO(bazel-team): Still needed? See createDexAction - } + .addExecPath("--output_zip", dexArchive) + .add(incrementalDexopts); Artifact paramFile = ruleContext.getDerivedArtifact( ParameterFile.derivePath(dexArchive.getRootRelativePath()), dexArchive.getRoot()); @@ -161,7 +207,77 @@ public final class DexArchiveAspect extends NativeAspectClass implements Configu .addInput(paramFile) .addOutput(dexArchive) .setMnemonic("DexBuilder") - .setProgressMessage("Dexing " + jar.prettyPrint()); + .setProgressMessage( + "Dexing " + jar.prettyPrint() + " with applicable dexopts " + incrementalDexopts); ruleContext.registerAction(dexbuilder.build(ruleContext)); } + + private static List<String> aspectDexopts(RuleContext ruleContext, AspectParameters params) { + return incrementalDexingOptions(ruleContext, + // Extract dexopts attribute similar to how AndroidBinary does it + // (minus $(location) expansion) + tokenizeAndExpandMakeVars(ruleContext, "dexopts", params.getAttribute("dexopts"))); + } + + /** + * Derives options to use in incremental dexing actions from the given context and dx flags, where + * the latter typically come from a {@code dexopts} attribute on a top-level target. This method + * only works reliably if the given dexopts were tokenized, e.g., using + * {@link RuleContext#getTokenizedStringListAttr}. + */ + static List<String> incrementalDexingOptions(RuleContext ruleContext, Iterable<String> dexopts) { + // Use TreeSet to drop duplicates and get fixed (sorted) order. Fixed order is important so + // we generate one dex archive per set of flag in create() method, regardless of how those flags + // are listed in all the top-level targets being built. + Set<String> args = new TreeSet<>(); + if (ruleContext.getConfiguration().isCodeCoverageEnabled()) { + // Match what we do in AndroidCommon.createDexAction + args.add("--nolocals"); // TODO(bazel-team): Still needed? See createDexAction + } + Iterables.addAll(args, + Iterables.transform( + Iterables.filter( + dexopts, + new FlagMatcher( + getAndroidConfig(ruleContext).getDexoptsSupportedInIncrementalDexing())), + FlagConverter.DX_TO_DEXBUILDER)); + return ImmutableList.copyOf(args); + } + + private static Collection<String> tokenizeAndExpandMakeVars(RuleContext ruleContext, + String attributeName, Collection<String> original) { + List<String> result = new ArrayList<>(); + for (String value : original) { + ruleContext.tokenizeAndExpandMakeVars(result, attributeName, value); + } + return result; + } + + private enum FlagConverter implements Function<String, String> { + DX_TO_DEXBUILDER; + + @Override + public String apply(String input) { + return input.replace("--no-", "--no"); + } + } + + static class FlagMatcher implements Predicate<String> { + + private final ImmutableList<String> matching; + + FlagMatcher(ImmutableList<String> matching) { + this.matching = matching; + } + + @Override + public boolean apply(String input) { + for (String match : matching) { + if (input.contains(match)) { + return true; + } + } + return false; + } + } } |