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

mauriciogg · 2023-01-18T00:14:53Z

Conditionally enable worker and multiples support for resource processing actions using flags
--define=persistent_android_resource_processor
--define=persistent_multiplex_android_resource_processor -> this flag will implicitly enable the persistent_android_resource_processor

Disable warnings for busybox actions Warnings go to stdout/stderr which makes it incompatible with workers
Force use of param files

fixes: #30

…ctions - Disable warnings for busybox actions Warnings go to stdout/stderr which makes it incompatible with workers

ted-xie

TL;DR we need to be able to optionally enable/disable workers and multiplex workers. It would also be ideal to add a test for this, but we haven't open-sourced the tests yet, so it's acceptable to omit for now.

ted-xie · 2023-04-19T16:59:23Z

rules/busybox.bzl

        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,


Is it possible to enable worker mode conditionally?

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

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?

ted-xie · 2023-04-19T16:59:44Z

rules/busybox.bzl

        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,
+        supports_multiplex_workers = True,


Is it possible to enable multiplex worker mode conditionally?

ted-xie · 2023-04-19T17:00:08Z

rules/busybox.bzl

@@ -388,16 +393,20 @@ def _package(
    if java_package:
        args.add("--packageForR", java_package)

+    _disable_warnings(args)


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

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

ted-xie · 2023-04-19T17:00:51Z

rules/busybox.bzl

@@ -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")


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?

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

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 😄)

ted-xie · 2023-05-05T18:16:30Z

Would you mind rebasing this PR? There are merge conflicts with pre-alpha HEAD now.

ted-xie

LMK if my proposed changes make sense to you. I'll also discuss this more internally.

ted-xie · 2023-05-12T20:47:27Z

rules/busybox.bzl

@@ -118,6 +119,12 @@ def _make_resources_flag(
        ],
    )

+def _disable_warnings(ctx, args):
+    if (_flags.get(ctx).persistent_android_resource_processor or


It turns out that we have some internal test rules that directly call into the busybox. It was a pain to manually add _flags = [...] to each of those rules, so I decided to add an early-fail here:

if not hasattr(ctx, "_flags"): return

ted-xie · 2023-05-12T20:48:46Z

rules/busybox.bzl

+    enable_workers = _flags.get(ctx).persistent_android_resource_processor
+    multiplex_workers = _flags.get(ctx).persistent_multiplex_android_resource_processor
+    kwargs["supports_workers"] = enable_workers or multiplex_workers
+    kwargs["supports_multiplex_workers"] = multiplex_workers 


I put these in an if-block:

enable_workers = False multiplex_workers = False if hasattr(ctx, "_flags"): enable_workers = [...] multiplex_workers = [...] kwargs[...] kwargs[...]

I also discussed this internally: there is a general preference that for flag-related features, we use the existing design pattern here: https://github.com/bazelbuild/rules_android/blob/pre-alpha/rules/dex.bzl#L49-L52

So instead of doing bazel build [...] --define=persistent_android_resource_processor, you can use the existing flags: bazel build [...] --persistent_android_dex_desugar etc.

mauriciogg · 2023-05-23T20:19:56Z

LMK if my proposed changes make sense to you. I'll also discuss this more internally.

makes sense. I deleted the use of the flags and replaced with ctx.fragments.android.persistent_android_resource_processor and ctx.fragments.android.persistent_multiplex_android_resource_processor

maybe it makes sense to replace this logic and add the starlark actions to the expansion in https://github.com/bazelbuild/bazel/blob/56fd3dc889717a8428843137e18016c906129924/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java#L863

ted-xie · 2023-05-23T20:37:01Z

The updated changes look good.

At a high level, we'd like to move as much Android-related stuff out of bazel as possible (which is really one of the major motivations behind having this separate Android rules codebase), so I think it's better for now to keep all the logic inside rules_android. @ahumesky, thoughts?

ted-xie · 2023-05-23T21:02:30Z

I'm actually getting this error when integrating into google3:

File "rules/busybox.bzl", line 122, column 30, in _disable_warnings
                if (ctx.fragments.android.persistent_android_resource_processor or
Error: 'android' value has no field or method 'persistent_android_resource_processor'

@mauriciogg Have you tested this change out locally to see if it works with Bazel? I don't see persistent_android_resource_processor in AndroidConfigurationApi.java, which is where flags are publicly exposed and ultimately hooked up to ctx.fragments. Example here.

I think to merge this change, we'll first have to expose persistent_android_resource_processor to AndroidConfigurationApi, wait for that change to propagate to both a Bazel prerelease and a Blaze release, and only then will this cleanly integrate with internal code.

ted-xie · 2023-05-24T19:40:36Z

rules/busybox.bzl

@@ -118,6 +118,12 @@ def _make_resources_flag(
        ],
    )

+def _disable_warnings(ctx, args):
+    if (ctx.fragments.android.persistent_android_resource_processor or


I changed these fragment flags to ctx.fragments.android.persistent_busybox_tools and ctx.fragments.android.persistent_multiplex_busybox_tools internally and tests are passing. Those flags also seem more appropriate than the resource_processor ones; what do you think?

Unfortunately the fragments.android data structure can't accept Void expansion flags such as persistent_android_resource_processor, and changing the return type of that flag handler to boolean causes some other kind of sanity check error. I think in Bazel/Blaze, expansion flags must return Void, so there's somewhat of an impedance mismatch.

ted-xie · 2024-01-24T20:33:13Z

This has been merged.

nkoroste · 2024-01-26T21:11:06Z

Looks like this was reverted f49f2e3

ted-xie · 2024-01-29T18:26:03Z

Yes - the root cause was that in google3, there can be spaces in manifest_values, i.e. manifest_values = {"versionName": "a b c d"}. There was an issue with this PR where those spaces were not properly quoted, which was causing internal app builds to fail. If you sed all of the multiline usages in busybox.bzl and replace with shell instead (see doc here), it should fix the problem. I'm trying that out internally and wil attempt to un-revert this change.

…port multiplex workers in busybox actions bazelbuild#64 Closes bazelbuild#64 COPYBARA_INTEGRATE_REVIEW=bazelbuild#64 from mauriciogg:mgalindo-busybox-workers 761207e PiperOrigin-RevId: 601199913 Change-Id: I5ecc0b549b405f27fcde3582bf2494ce3c3a9e8d

[SNAP FORK][UPSTREAMED] enable support multiplex workers in busybox a…

1508984

…ctions - Disable warnings for busybox actions Warnings go to stdout/stderr which makes it incompatible with workers

ted-xie requested changes Apr 19, 2023

View reviewed changes

Mauricio Galindo added 4 commits April 24, 2023 14:13

Add flag to enable resource processr worker mode

981477e

only disable warning of worker mode is enabled

e243693

Fix issues

6f9998f

Add flag for multiplex workers

55662a8

Merge branch 'pre-alpha' into mgalindo-busybox-workers

6c0f0cf

ted-xie requested changes May 12, 2023

View reviewed changes

Use persistent android workers from fragment config

69a010c

Remove unused args

761207e

ted-xie requested changes May 24, 2023

View reviewed changes

copybara-service bot closed this in f53c8c9 Jan 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mauriciogg commented Jan 18, 2023 •

edited

Loading

ted-xie left a comment

ted-xie Apr 19, 2023

mauriciogg Apr 24, 2023

ted-xie May 15, 2023

mauriciogg May 23, 2023

ted-xie Apr 19, 2023

mauriciogg Apr 24, 2023

ted-xie Apr 19, 2023

mauriciogg Apr 24, 2023

ted-xie Apr 19, 2023

Bencodes Apr 19, 2023

ted-xie Apr 19, 2023

ted-xie commented May 5, 2023

ted-xie left a comment •

edited

Loading

ted-xie May 12, 2023

ted-xie May 12, 2023

ted-xie May 15, 2023

mauriciogg commented May 23, 2023

ted-xie commented May 23, 2023

ted-xie commented May 23, 2023

ted-xie May 24, 2023

ted-xie commented Jan 24, 2024

nkoroste commented Jan 26, 2024

ted-xie commented Jan 29, 2024

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

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

Conversation

mauriciogg commented Jan 18, 2023 • edited Loading

ted-xie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ted-xie commented May 5, 2023

ted-xie left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauriciogg commented May 23, 2023

ted-xie commented May 23, 2023

ted-xie commented May 23, 2023

Choose a reason for hiding this comment

ted-xie commented Jan 24, 2024

nkoroste commented Jan 26, 2024

ted-xie commented Jan 29, 2024

mauriciogg commented Jan 18, 2023 •

edited

Loading

ted-xie left a comment •

edited

Loading