support multiplex worker for jetify #911
Conversation
SupSaiYaJin
force-pushed
the
jetify-worker
branch
2 times, most recently
from
8c1a0fa
to
e96ba32
Compare
SupSaiYaJin
force-pushed
the
jetify-worker
branch
from
e96ba32
to
72d2715
Compare
| visibility = ["//visibility:public"], | ||
| runtime_deps = [ | ||
| ":jetifier_jars", | ||
| ":jetifier_worker_wrapper" |
There was a problem hiding this comment.
Depending on WorkRequestHandler as an implementation detail of the android_tools probably isn't the best way to add worker support here.
There is an issue open already against Bazel to have this worker implementation exposed or distributed to some place like maven. bazelbuild/bazel#14556
There was a problem hiding this comment.
Is it acceptable to copy WorkRequestHandler to this repo for the time being?
There was a problem hiding this comment.
That's one option, although also not ideal. There is an implementation in the aswb plugin that's much simpler that we could pull into rules_jvm_external if we need to.
@SupSaiYaJin Are you using the jetify_include_list attribute on maven_install to limit the number of maven dependencies being Jetified to only those that need it? The screenshot in the description shows okio being Jetified which looks incorrect to me.
On a related note, AndroidX has been the default for 6+ years now. We should probably be figuring out how to remove Jetifier entirely from rules_jvm_external instead of expanding support. People might even be unknowingly using it and experiencing slower builds as a result when it's in fact not needed. Alternatively we could switch the behavior to only run on the specific maven coords listed in jetify_include_list by default.
There was a problem hiding this comment.
Agree on that, we do use jetify_include_list to limit the libs. Besides even putting aside jetifier, this change could benefit all java_binary actions if they are stateless, for example
They could be disabled by default and enabled with --strategy if it fits consuming project needs.
I also found BazelMultiplexWorker with a nicer API BazelMultiplexWorker.run(args, (compilerArgs, out) -> );
| http_archive( | ||
| name = "android_tools", | ||
| sha256 = "1afa4b7e13c82523c8b69e87f8d598c891ec7e2baa41d9e24e08becd723edb4d", | ||
| url = "https://mirror.bazel.build/bazel_android_tools/android_tools_pkg-0.27.0.tar.gz", |
There was a problem hiding this comment.
This will also need to be added to the MODULE.bazel
| @@ -0,0 +1,46 @@ | |||
| package com.github.bazelbuild.rules_jvm_external; | |||
There was a problem hiding this comment.
This file should live under //private/tools/java rather than in a new top-level directory.
|
Thank you for the PR. Making things more efficient is definitely a Good Thing! |
|
This PR and some other PRs and conversations recently have highlighted that we should really be keeping the focus of this ruleset tight. In #924 we deprecate In addition, it's not clear that people understand how limited the work it does is: it's only needed in a handful of cases. For example, the Square For these reasons, although I very much appreciate the PR and the work it represents, and I thank you for it, I shall be closing this PR. If you feel this needs further discussion, I'd be happy to chat on the |
This PR uses WorkRequestHandler in android_tools wrapping jetify-standalone to support multiplex worker which will significantly improves the performance of Jetifying especially during first-time compilation without cache.
not using worker:
using worker: