Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[6.2.0]Fix worker and multiplex workers for DexBuilder and Desugar actions #17965

Merged
merged 1 commit into from
Apr 19, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<String> 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",
Expand Down Expand Up @@ -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",
Expand All @@ -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",
Expand Down Expand Up @@ -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.
*
* <p>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",
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -1128,7 +1149,6 @@ public FragmentOptions getHost() {
private final ImmutableList<String> targetDexoptsThatPreventIncrementalDexing;
private final ImmutableList<String> dexoptsSupportedInDexMerger;
private final ImmutableList<String> dexoptsSupportedInDexSharder;
private final boolean useWorkersWithDexbuilder;
private final boolean desugarJava8;
private final boolean desugarJava8Libs;
private final boolean checkDesugarDeps;
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -1319,12 +1342,6 @@ public ImmutableList<String> 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;
Expand Down Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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;
}
Expand All @@ -660,6 +664,21 @@ private static Set<Set<String>> aspectDexopts(RuleContext ruleContext) {
normalizeDexopts(getAndroidConfig(ruleContext).getDexoptsSupportedInIncrementalDexing()));
}

/** Creates the execution requires for the DexBuilder and Desugar actions */
private static ImmutableMap.Builder<String, String> createDexingDesugaringExecRequirements(
RuleContext ruleContext) {
final ImmutableMap.Builder<String, String> 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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,13 +94,6 @@ public interface AndroidConfigurationApi extends StarlarkValue {
documented = false)
ImmutableList<String> getTargetDexoptsThatPreventIncrementalDexing();

@StarlarkMethod(
name = "use_workers_with_dexbuilder",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's cherry-pick your GraveyardOptions commit into this PR as well for completeness.

9bfc2b7

structField = true,
doc = "",
documented = false)
boolean useWorkersWithDexbuilder();

@StarlarkMethod(name = "desugar_java8", structField = true, doc = "", documented = false)
boolean desugarJava8();

Expand Down Expand Up @@ -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,
Expand Down
22 changes: 18 additions & 4 deletions src/test/shell/bazel/android/desugarer_integration_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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"
2 changes: 0 additions & 2 deletions tools/android/BUILD.tools
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down