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.

Sign up for GitHub

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

[SNAP FORK][UPSTREAMED] enable support multiplex workers in busybox actions #64

Closed
wants to merge 8 commits into from
Closed

[SNAP FORK][UPSTREAMED] enable support multiplex workers in busybox actions #64

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1508984
[SNAP FORK][UPSTREAMED] enable support multiplex workers in busybox a…
Sep 19, 2022
981477e
Add flag to enable resource processr worker mode
Apr 24, 2023
e243693
only disable warning of worker mode is enabled
Apr 24, 2023
6f9998f
Fix issues
Apr 24, 2023
55662a8
Add flag for multiplex workers
Apr 24, 2023
6c0f0cf
Merge branch 'pre-alpha' into mgalindo-busybox-workers
ted-xie May 12, 2023
69a010c
Use persistent android workers from fragment config
May 23, 2023
761207e
Remove unused args
May 23, 2023
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
77 changes: 69 additions & 8 deletions rules/busybox.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,10 @@ def _make_resources_flag(
],
)

def _disable_warnings(args):
# Disable warnings - this are output to stdin/stderr which breaks worker mode
args.add("--logWarnings=false")
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens to warnings in worker mode? I think we'd still want to know about them. Would they get dumped to a non-stdout/stderr file?

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this is just mirroring the existing behavior inside BusyBoxActionBuilder https://github.com/bazelbuild/bazel/blob/84c1ed430405b154b6e9eb2c28281f450e250eff/src/main/java/com/google/devtools/build/lib/rules/android/BusyBoxActionBuilder.java#L369

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense I suppose, but I'm still curious where the warnings go even in the native implementation. We can shelf this discussion for later since it's slightly out of scope (and no one has complained about this yet 😄)


def _path(f):
return f.path

Expand Down Expand Up @@ -281,7 +285,8 @@ def _package(
transitive_input_files = []

args = ctx.actions.args()
args.use_param_file("@%s")
args.use_param_file("@%s", use_always=True)
args.set_param_file_format("multiline")
args.add("--tool", "AAPT2_PACKAGE")
args.add("--")
args.add("--aapt2", aapt.executable)
Expand Down Expand Up @@ -388,16 +393,20 @@ def _package(
if java_package:
args.add("--packageForR", java_package)

_disable_warnings(args)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should disable warnings conditionally based on if worker mode is enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Default nos is false. Can be enabled using --define=persistent_android_resource_processor=true


_java.run(
ctx = ctx,
host_javabase = host_javabase,
executable = busybox,
tools = [aapt],
tools = [aapt, busybox],
arguments = [args],
inputs = depset(input_files, transitive = transitive_input_files),
outputs = output_files,
mnemonic = "PackageAndroidResources",
progress_message = "Packaging Android Resources in %s" % ctx.label,
supports_workers = True,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to enable worker mode conditionally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Default nos is false. Can be enabled using --define=persistent_android_resource_processor=true

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks to me like the supports_workers and supports_multiplex_workers arguments to java_run() no longer do anything. Maybe we can remove these from the PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

supports_multiplex_workers = True,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to enable multiplex worker mode conditionally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

)

def _parse(
Expand All @@ -418,7 +427,8 @@ def _parse(
host_javabase: Target. The host javabase.
"""
args = ctx.actions.args()
args.use_param_file("@%s")
args.use_param_file("@%s", use_always=True)
args.set_param_file_format("multiline")
args.add("--tool", "PARSE")
args.add("--")
args.add(
Expand All @@ -430,6 +440,8 @@ def _parse(
)
args.add("--output", out_symbols)

_disable_warnings(args)

_java.run(
ctx = ctx,
host_javabase = host_javabase,
Expand All @@ -439,6 +451,8 @@ def _parse(
outputs = [out_symbols],
mnemonic = "ParseAndroidResources",
progress_message = "Parsing Android Resources in %s" % out_symbols.short_path,
supports_workers = True,
supports_multiplex_workers = True,
)

def _make_merge_assets_flags(resources_node):
Expand Down Expand Up @@ -484,7 +498,8 @@ def _merge_assets(
host_javabase: Target. The host javabase.
"""
args = ctx.actions.args()
args.use_param_file("@%s")
args.use_param_file("@%s", use_always=True)
args.set_param_file_format("multiline")
args.add("--tool", "MERGE_ASSETS")
args.add("--")
args.add("--assetsOutput", out_assets_zip)
Expand All @@ -510,10 +525,13 @@ def _merge_assets(
join_with = "&",
)

_disable_warnings(args)

_java.run(
ctx = ctx,
host_javabase = host_javabase,
executable = busybox,
tools = [busybox],
arguments = [args],
inputs = depset(
assets + [symbols],
Expand All @@ -523,6 +541,8 @@ def _merge_assets(
mnemonic = "MergeAndroidAssets",
progress_message =
"Merging Android Assets in %s" % out_assets_zip.short_path,
supports_workers = True,
supports_multiplex_workers = True,
)

def _validate_and_link(
Expand Down Expand Up @@ -562,7 +582,8 @@ def _validate_and_link(

# Retrieves the list of files at runtime when a directory is passed.
args = ctx.actions.args()
args.use_param_file("@%s")
args.use_param_file("@%s", use_always=True)
args.set_param_file_format("multiline")
args.add("--tool", "LINK_STATIC_LIBRARY")
args.add("--")
args.add("--aapt2", aapt.executable)
Expand All @@ -587,6 +608,8 @@ def _validate_and_link(
args.add("--staticLibraryOut", out_file)
output_files.append(out_file)

_disable_warnings(args)

_java.run(
ctx = ctx,
host_javabase = host_javabase,
Expand All @@ -598,6 +621,8 @@ def _validate_and_link(
mnemonic = "LinkAndroidResources",
progress_message =
"Linking Android Resources in " + out_file.short_path,
supports_workers = True,
supports_multiplex_workers = True,
)

def _compile(
Expand Down Expand Up @@ -627,7 +652,8 @@ def _compile(

# Retrieves the list of files at runtime when a directory is passed.
args = ctx.actions.args()
args.use_param_file("@%s")
args.use_param_file("@%s", use_always=True)
args.set_param_file_format("multiline")
args.add("--tool", "COMPILE_LIBRARY_RESOURCES")
args.add("--")
args.add("--aapt2", aapt.executable)
Expand All @@ -641,6 +667,8 @@ def _compile(
)
args.add("--output", out_file)

_disable_warnings(args)

_java.run(
ctx = ctx,
host_javabase = host_javabase,
Expand All @@ -651,6 +679,8 @@ def _compile(
outputs = [out_file],
mnemonic = "CompileAndroidResources",
progress_message = "Compiling Android Resources in %s" % out_file.short_path,
supports_workers = True,
supports_multiplex_workers = True,
)

def _make_merge_compiled_flags(resources_node_info):
Expand Down Expand Up @@ -703,7 +733,8 @@ def _merge_compiled(
transitive_input_files = []

args = ctx.actions.args()
args.use_param_file("@%s")
args.use_param_file("@%s", use_always=True)
args.set_param_file_format("multiline")
args.add("--tool", "MERGE_COMPILED")
args.add("--")
args.add("--classJarOutput", out_class_jar)
Expand Down Expand Up @@ -742,16 +773,21 @@ def _merge_compiled(
)
transitive_input_files.append(transitive_compiled_resources)

_disable_warnings(args)

_java.run(
ctx = ctx,
host_javabase = host_javabase,
executable = busybox,
tools = [busybox],
arguments = [args],
inputs = depset(input_files, transitive = transitive_input_files),
outputs = output_files,
mnemonic = "StarlarkMergeCompiledAndroidResources",
progress_message =
"Merging compiled Android Resources in " + out_class_jar.short_path,
supports_workers = True,
supports_multiplex_workers = True,
)

def _escape_mv(s):
Expand Down Expand Up @@ -812,6 +848,7 @@ def _merge_manifests(
# Args for busybox
args = ctx.actions.args()
args.use_param_file("@%s", use_always = True)
args.set_param_file_format("multiline")
args.add("--tool", "MERGE_MANIFEST")
args.add("--")
if manifest:
Expand All @@ -834,15 +871,20 @@ def _merge_manifests(
args.add("--log", out_log_file)
outputs.append(out_log_file)

_disable_warnings(args)

_java.run(
ctx = ctx,
host_javabase = host_javabase,
executable = busybox,
tools = [busybox],
arguments = [args],
inputs = depset(directs, transitive = transitives),
outputs = outputs,
mnemonic = "MergeManifests",
progress_message = "Merging Android Manifests in %s" % out_file.short_path,
supports_workers = True,
supports_multiplex_workers = True,
)

def _process_databinding(
Expand Down Expand Up @@ -877,13 +919,17 @@ def _process_databinding(
res_dirs = _get_unique_res_dirs(resource_files)

args = ctx.actions.args()
args.use_param_file("@%s", use_always=True)
args.set_param_file_format("multiline")
args.add("--tool", "PROCESS_DATABINDING")
args.add("--")
args.add("--output_resource_directory", databinding_resources_dirname)
args.add_all(res_dirs, before_each = "--resource_root")
args.add("--dataBindingInfoOut", out_databinding_info)
args.add("--appId", java_package)

_disable_warnings(args)

_java.run(
ctx = ctx,
host_javabase = host_javabase,
Expand All @@ -893,6 +939,8 @@ def _process_databinding(
outputs = [out_databinding_info] + out_databinding_processed_resources,
mnemonic = "StarlarkProcessDatabinding",
progress_message = "Processing data binding",
supports_workers = True,
supports_multiplex_workers = True,
)

def _make_generate_binay_r_flags(resources_node):
Expand Down Expand Up @@ -931,6 +979,8 @@ def _generate_binary_r(
host_javabase: A Target. The host javabase.
"""
args = ctx.actions.args()
args.use_param_file("@%s", use_always=True)
args.set_param_file_format("multiline")
args.add("--tool", "GENERATE_BINARY_R")
args.add("--")
args.add("--primaryRTxt", r_txt)
Expand All @@ -950,17 +1000,21 @@ def _generate_binary_r(
# TODO(b/154003916): support transitive "--library transitive_r_txt_path,transitive_manifest_path" flags
args.add("--classJarOutput", out_class_jar)
args.add("--targetLabel", str(ctx.label))
args.use_param_file("@%s")

_disable_warnings(args)

_java.run(
ctx = ctx,
host_javabase = host_javabase,
executable = busybox,
tools = [busybox],
arguments = [args],
inputs = depset([r_txt, manifest], transitive = transitive_r_txts + transitive_manifests),
outputs = [out_class_jar],
mnemonic = "StarlarkRClassGenerator",
progress_message = "Generating R classes",
supports_workers = True,
supports_multiplex_workers = True,
)

def _make_aar(
Expand Down Expand Up @@ -995,6 +1049,8 @@ def _make_aar(
when a resource conflict occurs.
"""
args = ctx.actions.args()
args.use_param_file("@%s", use_always=True)
args.set_param_file_format("multiline")
args.add("--tool", "GENERATE_AAR")
args.add("--")
args.add(
Expand All @@ -1014,10 +1070,13 @@ def _make_aar(
if should_throw_on_conflict:
args.add("--throwOnResourceConflict")

_disable_warnings(args)

_java.run(
ctx = ctx,
host_javabase = host_javabase,
executable = busybox,
tools = [busybox],
arguments = [args],
inputs = (
resource_files +
Expand All @@ -1028,6 +1087,8 @@ def _make_aar(
outputs = [out_aar],
mnemonic = "StarlarkAARGenerator",
progress_message = "Generating AAR package for %s" % ctx.label,
supports_workers = True,
supports_multiplex_workers = True,
)

busybox = struct(
Expand Down
12 changes: 11 additions & 1 deletion rules/java.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,7 +431,17 @@ def _run(

args["arguments"] = jvm_flags + [jar_args] + args.get("arguments", default = [])

ctx.actions.run(**args)
mnemonic = args.get("mnemonic")
supports_workers = args.pop("supports_workers", False)
supports_multiplex_workers = args.pop("supports_multiplex_workers", False)
execution_requirements = args.pop("execution_requirements", dict())
if supports_workers:
execution_requirements["worker-key-mnemonic"] = mnemonic
execution_requirements["supports-workers"] = "1"
if supports_multiplex_workers:
execution_requirements["supports-multiplex-workers"] = "1"

ctx.actions.run(execution_requirements = execution_requirements, **args)

java = struct(
compile = _compile,
Expand Down