Introduce persistent_multiplex_android_resource_processor expansion flag #15950
Conversation
| @@ -123,7 +123,7 @@ function test_persistent_multiplex_resource_processor() { | |||
| setup_font_resources | |||
|
|
|||
| assert_build //java/bazel:bin --persistent_android_resource_processor \ | |||
| --experimental_persistent_multiplex_busybox_tools | |||
| --persistent_multiplex_android_resource_processor | |||
There was a problem hiding this comment.
With this approach we also wouldn't need to add a unique test case just for DexBuilder multiplex workers as it would be covered by the persistent_multiplex_android_resource_processor expansion here.
There was a problem hiding this comment.
we can consolidate in a followup change
Bencodes
force-pushed
the
introduce-persistent_multiplex_android_resource_processor-expansion-flag
branch
from
ace04d3
to
43d6f74
Compare
sgowroji
added
team-Android
|
@larsrc-google is this something you'd be interested in merging to clean up the multiplex worker flags on the Android side? |
| }) | ||
| public Void persistentResourceProcessor; | ||
|
|
||
| @Option( | ||
| name = "persistent_multiplex_android_resource_processor", |
There was a problem hiding this comment.
If this is meant to be used just while rolling the multiplex implementation, it ought to be an experimental_ flag. If not, why not just set support-multiplex-workers in BusyBoxActionBuilder?
There was a problem hiding this comment.
I wouldn't be against just setting support-multiplex-workers everywhere but that might be on the riskier side given that 6.x will be the first release where there's actual multiplex support for all of the tools that the busybox wrapper runs.
There was a problem hiding this comment.
I don't think we want the flag around forever, so what is the testing strategy?
There was a problem hiding this comment.
Addressing a few things:
- There's a test for this in resource_processing_integration_test.sh, so I think that's covered
- I think setting
supports-multiplex-workersin BusyBoxActionBuilder would also turn this on internally, and I don't think we want to sign up for figuring out how to roll that out right now. I think we should consider setting workers to be on for all this by default, but that can be a separate change - I'm ok with leaving this without
experimental_, because this isn't really a new thing that's being actively changed or developed, and it's not an API or similar
There was a problem hiding this comment.
I realized that with desugar and dexing in the list, this isn't strictly about resource processing. Maybe a more accurate name would be --persistent_multiplex_android_tools
Similarly, adding desugar and dexing to --persistent_android_resource_processor changes the behavior of the flag, and seems to make that name less accurate. We can't easily rename that though flag since it's been around for a while already. The solution would seem to be to introduce --persistent_android_dex_desugar.
If that sounds good I can make the change while importing this PR
There was a problem hiding this comment.
Yeah so even though we were trying to simplify things, I seem to be now proposing:
--persistent_android_resource_processor
--persistent_multiplex_android_resource_processor
--persistent_android_dex_desugar
--persistent_multiplex_android_dex_desugar
--persistent_multiplex_android_tools
--persistent_android_resource_processor already exists and --persistent_multiplex_android_tools adds all the previous ones (directly or indirectly).
But this seems to be the most accurate and safest.
| }) | ||
| public Void persistentResourceProcessor; | ||
|
|
||
| @Option( | ||
| name = "persistent_multiplex_android_resource_processor", |
There was a problem hiding this comment.
Addressing a few things:
- There's a test for this in resource_processing_integration_test.sh, so I think that's covered
- I think setting
supports-multiplex-workersin BusyBoxActionBuilder would also turn this on internally, and I don't think we want to sign up for figuring out how to roll that out right now. I think we should consider setting workers to be on for all this by default, but that can be a separate change - I'm ok with leaving this without
experimental_, because this isn't really a new thing that's being actively changed or developed, and it's not an API or similar
| @@ -123,7 +123,7 @@ function test_persistent_multiplex_resource_processor() { | |||
| setup_font_resources | |||
|
|
|||
| assert_build //java/bazel:bin --persistent_android_resource_processor \ | |||
| --experimental_persistent_multiplex_busybox_tools | |||
| --persistent_multiplex_android_resource_processor | |||
There was a problem hiding this comment.
we can consolidate in a followup change
|
I'll start importing this |
- Moved `GenerateDataBindingBaseClasses` to `ResourceProcessorBusyBox` since Android databinding classes were already a dependency there. - `ProcessDatabinding` is already part of `ResourceProcessorBusyBox` but it was not enabled in `persistent_android_resource_processor` expansion flag. Added that as well. This change is multiplex compatible and probably could use expansion flags as done here #15950. Results: In a synthetic generated project with 30 modules here https://github.com/arunkumar9t2/bazel-playground, the worker can be turned off/on with `.bazel.sh build //android/databinding_workers:android_data_binding --strategy=GenerateDataBindingBaseClasses=[sandboxed|worker]` and worker is about 9 to 10% faster. Closes #16067. PiperOrigin-RevId: 557906961 Change-Id: I085d8a8e92d1882834ad90db31767ab9c7d2e5cf
Alternative approach to #15321 (comment) that leverages an expansion CLI flag instead of proliferating unique experimental multiplex flags.