From 76ad4a9b8e1b5a5cc5ed5edaad9b438cd9d8ef66 Mon Sep 17 00:00:00 2001 From: Ben Lee Date: Wed, 19 Apr 2023 14:21:30 -0700 Subject: [PATCH] [6.2.0]Fix worker and multiplex workers for DexBuilder and Desugar actions (#17965) --- .../lib/bazel/rules/BazelRulesModule.java | 9 +++ .../rules/android/AndroidConfiguration.java | 69 +++++++++++++------ .../lib/rules/android/DexArchiveAspect.java | 31 +++++++-- .../android/AndroidConfigurationApi.java | 21 ++++-- .../android/desugarer_integration_test.sh | 22 ++++-- tools/android/BUILD.tools | 2 - 6 files changed, 114 insertions(+), 40 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java index 885a610400a4bf..0fd6672a0b67c7 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java @@ -273,6 +273,15 @@ public static class BuildGraveyardOptions extends OptionsBase { help = "Deprecated no-op. Use --experimental_dynamic_local_load_factor instead.") @Deprecated public boolean skipFirstBuild; + + @Option( + name = "use_workers_with_dexbuilder", + defaultValue = "true", + documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, + effectTags = {OptionEffectTag.EXECUTION}, + help = "This option is deprecated and has no effect.") + @Deprecated + public boolean useWorkersWithDexbuilder; } /** This is where deprecated Bazel-specific options only used by the build command go to die. */ 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 a3fb4898797c46..19476edcc8952e 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 @@ -533,14 +533,6 @@ public static class Options extends FragmentOptions { help = "dx flags supported in tool that groups classes for inclusion in final .dex files.") public List dexoptsSupportedInDexSharder; - @Option( - name = "use_workers_with_dexbuilder", - defaultValue = "true", - documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, - effectTags = {OptionEffectTag.EXECUTION}, - help = "Whether dexbuilder supports being run in local worker mode.") - public boolean useWorkersWithDexbuilder; - @Option( name = "experimental_android_rewrite_dexes_with_rex", defaultValue = "false", @@ -905,10 +897,11 @@ public static class Options extends FragmentOptions { }, help = "Enable persistent Android dex and desugar actions by using workers.", expansion = { + "--internal_persistent_android_dex_desugar", "--strategy=Desugar=worker", "--strategy=DexBuilder=worker", }) - public Void persistentDexDesugar; + public Void persistentAndroidDexDesugar; @Option( name = "persistent_multiplex_android_dex_desugar", @@ -921,10 +914,9 @@ public static class Options extends FragmentOptions { help = "Enable persistent multiplexed Android dex and desugar actions by using workers.", expansion = { "--persistent_android_dex_desugar", - "--modify_execution_info=Desugar=+supports-multiplex-workers", - "--modify_execution_info=DexBuilder=+supports-multiplex-workers", + "--internal_persistent_multiplex_android_dex_desugar", }) - public Void persistentMultiplexDexDesugar; + public Void persistentMultiplexAndroidDexDesugar; @Option( name = "persistent_multiplex_android_tools", @@ -974,6 +966,36 @@ public static class Options extends FragmentOptions { help = "Tracking flag for when multiplexed busybox workers are enabled.") public boolean persistentMultiplexBusyboxTools; + /** + * We use this option to decide when to enable workers for busybox tools. This flag is also a + * guard against enabling workers using nothing but --persistent_android_resource_processor. + * + *

Consequently, we use this option to decide between param files or regular command line + * parameters. If we're not using workers or on Windows, there's no need to always use param + * files for I/O performance reasons. + */ + @Option( + name = "internal_persistent_android_dex_desugar", + documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, + effectTags = { + OptionEffectTag.HOST_MACHINE_RESOURCE_OPTIMIZATIONS, + OptionEffectTag.EXECUTION, + }, + defaultValue = "false", + help = "Tracking flag for when dexing and desugaring workers are enabled.") + public boolean persistentDexDesugar; + + @Option( + name = "internal_persistent_multiplex_android_dex_desugar", + documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, + effectTags = { + OptionEffectTag.HOST_MACHINE_RESOURCE_OPTIMIZATIONS, + OptionEffectTag.EXECUTION, + }, + defaultValue = "false", + help = "Tracking flag for when multiplexed dexing and desugaring workers are enabled.") + public boolean persistentMultiplexDexDesugar; + @Option( name = "experimental_remove_r_classes_from_instrumentation_test_jar", defaultValue = "true", @@ -1100,7 +1122,6 @@ public FragmentOptions getHost() { host.dexoptsSupportedInIncrementalDexing = dexoptsSupportedInIncrementalDexing; host.dexoptsSupportedInDexMerger = dexoptsSupportedInDexMerger; host.dexoptsSupportedInDexSharder = dexoptsSupportedInDexSharder; - host.useWorkersWithDexbuilder = useWorkersWithDexbuilder; host.manifestMerger = manifestMerger; host.manifestMergerOrder = manifestMergerOrder; host.allowAndroidLibraryDepsWithoutSrcs = allowAndroidLibraryDepsWithoutSrcs; @@ -1128,7 +1149,6 @@ public FragmentOptions getHost() { private final ImmutableList targetDexoptsThatPreventIncrementalDexing; private final ImmutableList dexoptsSupportedInDexMerger; private final ImmutableList dexoptsSupportedInDexSharder; - private final boolean useWorkersWithDexbuilder; private final boolean desugarJava8; private final boolean desugarJava8Libs; private final boolean checkDesugarDeps; @@ -1155,6 +1175,8 @@ public FragmentOptions getHost() { private final boolean dataBindingAndroidX; private final boolean persistentBusyboxTools; private final boolean persistentMultiplexBusyboxTools; + private final boolean persistentDexDesugar; + private final boolean persistentMultiplexDexDesugar; private final boolean filterRJarsFromAndroidTest; private final boolean removeRClassesFromInstrumentationTestJar; private final boolean alwaysFilterDuplicateClassesFromAndroidTest; @@ -1184,7 +1206,6 @@ public AndroidConfiguration(BuildOptions buildOptions) throws InvalidConfigurati ImmutableList.copyOf(options.nonIncrementalPerTargetDexopts); this.dexoptsSupportedInDexMerger = ImmutableList.copyOf(options.dexoptsSupportedInDexMerger); this.dexoptsSupportedInDexSharder = ImmutableList.copyOf(options.dexoptsSupportedInDexSharder); - this.useWorkersWithDexbuilder = options.useWorkersWithDexbuilder; this.desugarJava8 = options.desugarJava8; this.desugarJava8Libs = options.desugarJava8Libs; this.checkDesugarDeps = options.checkDesugarDeps; @@ -1216,6 +1237,8 @@ public AndroidConfiguration(BuildOptions buildOptions) throws InvalidConfigurati this.dataBindingAndroidX = options.dataBindingAndroidX; this.persistentBusyboxTools = options.persistentBusyboxTools; this.persistentMultiplexBusyboxTools = options.persistentMultiplexBusyboxTools; + this.persistentDexDesugar = options.persistentDexDesugar; + this.persistentMultiplexDexDesugar = options.persistentMultiplexDexDesugar; this.filterRJarsFromAndroidTest = options.filterRJarsFromAndroidTest; this.removeRClassesFromInstrumentationTestJar = options.removeRClassesFromInstrumentationTestJar; @@ -1319,12 +1342,6 @@ public ImmutableList getTargetDexoptsThatPreventIncrementalDexing() { return targetDexoptsThatPreventIncrementalDexing; } - /** Whether to assume the dexbuilder tool supports local worker mode. */ - @Override - public boolean useWorkersWithDexbuilder() { - return useWorkersWithDexbuilder; - } - @Override public boolean desugarJava8() { return desugarJava8; @@ -1473,6 +1490,16 @@ public boolean persistentMultiplexBusyboxTools() { return persistentMultiplexBusyboxTools; } + @Override + public boolean persistentDexDesugar() { + return persistentDexDesugar; + } + + @Override + public boolean persistentMultiplexDexDesugar() { + return persistentMultiplexDexDesugar; + } + @Override public boolean incompatibleUseToolchainResolution() { return incompatibleUseToolchainResolution; 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 d0ca9d2091ad0f..34c16773bb9a25 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 @@ -28,6 +28,7 @@ import com.google.common.base.Predicate; import com.google.common.base.Predicates; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.common.collect.Sets; @@ -526,7 +527,10 @@ private static Artifact createDesugarAction( .addOutput(result) .setMnemonic("Desugar") .setProgressMessage("Desugaring %s for Android", jar.prettyPrint()) - .setExecutionInfo(ExecutionRequirements.WORKER_MODE_ENABLED); + .setExecutionInfo( + createDexingDesugaringExecRequirements(ruleContext) + .putAll(ExecutionRequirements.WORKER_MODE_ENABLED) + .buildKeepingLast()); // SpawnAction.Builder.build() is documented as being safe for re-use. So we can call build here // to get the action's inputs for vetting path stripping safety, then call it again later to @@ -618,8 +622,11 @@ static Artifact createDexArchiveAction( .useDefaultShellEnvironment() .setExecutable(ruleContext.getExecutablePrerequisite(dexbuilderPrereq)) .setExecutionInfo( - TargetUtils.getExecutionInfo( - ruleContext.getRule(), ruleContext.isAllowTagsPropagation())) + createDexingDesugaringExecRequirements(ruleContext) + .putAll( + TargetUtils.getExecutionInfo( + ruleContext.getRule(), ruleContext.isAllowTagsPropagation())) + .buildKeepingLast()) // WorkerSpawnStrategy expects the last argument to be @paramfile .addInput(jar) .addOutput(dexArchive) @@ -648,9 +655,6 @@ static Artifact createDexArchiveAction( dexbuilder .addCommandLine(args.build(), ParamFileInfo.builder(UNQUOTED).setUseAlways(true).build()) .stripOutputPaths(stripOutputPaths); - if (getAndroidConfig(ruleContext).useWorkersWithDexbuilder()) { - dexbuilder.setExecutionInfo(ExecutionRequirements.WORKER_MODE_ENABLED); - } ruleContext.registerAction(dexbuilder.build(ruleContext)); return dexArchive; } @@ -660,6 +664,21 @@ private static Set> aspectDexopts(RuleContext ruleContext) { normalizeDexopts(getAndroidConfig(ruleContext).getDexoptsSupportedInIncrementalDexing())); } + /** Creates the execution requires for the DexBuilder and Desugar actions */ + private static ImmutableMap.Builder createDexingDesugaringExecRequirements( + RuleContext ruleContext) { + final ImmutableMap.Builder executionInfo = ImmutableMap.builder(); + AndroidConfiguration androidConfiguration = getAndroidConfig(ruleContext); + if (androidConfiguration.persistentDexDesugar()) { + executionInfo.putAll(ExecutionRequirements.WORKER_MODE_ENABLED); + if (androidConfiguration.persistentMultiplexDexDesugar()) { + executionInfo.putAll(ExecutionRequirements.WORKER_MULTIPLEX_MODE_ENABLED); + } + } + + return executionInfo; + } + /** * 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 diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/android/AndroidConfigurationApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/android/AndroidConfigurationApi.java index c29241be6f5bc7..6c5f1aed09b358 100644 --- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/android/AndroidConfigurationApi.java +++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/android/AndroidConfigurationApi.java @@ -94,13 +94,6 @@ public interface AndroidConfigurationApi extends StarlarkValue { documented = false) ImmutableList getTargetDexoptsThatPreventIncrementalDexing(); - @StarlarkMethod( - name = "use_workers_with_dexbuilder", - structField = true, - doc = "", - documented = false) - boolean useWorkersWithDexbuilder(); - @StarlarkMethod(name = "desugar_java8", structField = true, doc = "", documented = false) boolean desugarJava8(); @@ -238,6 +231,20 @@ public interface AndroidConfigurationApi extends StarlarkValue { documented = false) boolean persistentMultiplexBusyboxTools(); + @StarlarkMethod( + name = "persistent_android_dex_desugar", + structField = true, + doc = "", + documented = false) + boolean persistentDexDesugar(); + + @StarlarkMethod( + name = "persistent_multiplex_android_dex_desugar", + structField = true, + doc = "", + documented = false) + boolean persistentMultiplexDexDesugar(); + @StarlarkMethod( name = "get_output_directory_name", structField = true, diff --git a/src/test/shell/bazel/android/desugarer_integration_test.sh b/src/test/shell/bazel/android/desugarer_integration_test.sh index c9d3a1f7161293..ff5563af729d5a 100755 --- a/src/test/shell/bazel/android/desugarer_integration_test.sh +++ b/src/test/shell/bazel/android/desugarer_integration_test.sh @@ -124,10 +124,24 @@ function test_java_8_android_binary_worker_strategy() { setup_android_sdk_support create_java_8_android_binary - bazel build \ - --strategy=Desugar=worker \ - --desugar_for_android //java/bazel:bin \ - || fail "build failed" + assert_build //java/bazel:bin \ + --persistent_android_dex_desugar \ + --worker_verbose &> $TEST_log + expect_log "Created new non-sandboxed Desugar worker (id [0-9]\+)" + expect_log "Created new non-sandboxed DexBuilder worker (id [0-9]\+)" +} + +function test_java_8_android_binary_multiplex_worker_strategy() { + create_new_workspace + setup_android_sdk_support + create_java_8_android_binary + + assert_build //java/bazel:bin \ + --experimental_worker_multiplex \ + --persistent_multiplex_android_dex_desugar \ + --worker_verbose &> $TEST_log + expect_log "Created new non-sandboxed Desugar multiplex-worker (id [0-9]\+)" + expect_log "Created new non-sandboxed DexBuilder multiplex-worker (id [0-9]\+)" } run_suite "Android desugarer integration tests" diff --git a/tools/android/BUILD.tools b/tools/android/BUILD.tools index 7fa51a1fe46739..9d5fd1e5333daf 100644 --- a/tools/android/BUILD.tools +++ b/tools/android/BUILD.tools @@ -47,8 +47,6 @@ java_binary( runtime_deps = ["//src/tools/android/java/com/google/devtools/build/android/r8"], ) -# NOTE: d8 dex builder doesn't support the persistent worker mode at the moment. To use this config, -# without a build error, --nouse_workers_with_dexbuilder flag must also be specified. config_setting( name = "d8_incremental_dexing", values = {