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

support multiplex worker for jetify #911

Closed
wants to merge 1 commit into from
Closed
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
6 changes: 6 additions & 0 deletions WORKSPACE
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,12 @@ load("@io_bazel_stardoc//:setup.bzl", "stardoc_repositories")

stardoc_repositories()

http_archive(
name = "android_tools",
sha256 = "1afa4b7e13c82523c8b69e87f8d598c891ec7e2baa41d9e24e08becd723edb4d",
url = "https://mirror.bazel.build/bazel_android_tools/android_tools_pkg-0.27.0.tar.gz",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will also need to be added to the MODULE.bazel

)

http_archive(
name = "io_bazel_rules_kotlin",
sha256 = "946747acdbeae799b085d12b240ec346f775ac65236dfcf18aa0cd7300f6de78",
Expand Down
6 changes: 6 additions & 0 deletions private/rules/jetifier.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,18 @@ def _jetify_impl(ctx):
jetify_args.add("-o", jetified_outfile)
jetify_args.add("-i", artifact)
jetify_args.add("-timestampsPolicy", "keepPrevious")
jetify_args.use_param_file("@%s", use_always = True)
jetify_args.set_param_file_format("multiline")
ctx.actions.run(
mnemonic = "Jetify",
inputs = [artifact],
outputs = [jetified_outfile],
progress_message = "Jetifying {}".format(artifact.owner),
executable = ctx.executable._jetifier,
execution_requirements = {
"supports-workers" : "1",
"supports-multiplex-workers": "1",
},
arguments = [jetify_args],
)
outfiles.append(jetified_outfile)
Expand Down
18 changes: 17 additions & 1 deletion third_party/jetifier/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,27 @@ java_import(
jars = glob(["jetifier-standalone/lib/*.jar"]),
)

java_import(
name = "all_android_tools",
jars = ["@android_tools//:all_android_tools_deploy.jar"],
)

java_library(
name = "jetifier_worker_wrapper",
visibility = ["//visibility:public"],
srcs = glob(["**/*.java"]),
deps = [
"//third_party/jetifier:jetifier_jars",
":all_android_tools",
],
)

java_binary(
name = "jetifier",
main_class = "com.android.tools.build.jetifier.standalone.Main",
main_class = "com.github.bazelbuild.rules_jvm_external.JetifierWorkerWrapper",
visibility = ["//visibility:public"],
runtime_deps = [
":jetifier_jars",
":jetifier_worker_wrapper"
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

Is it acceptable to copy WorkRequestHandler to this repo for the time being?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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

],
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
package com.github.bazelbuild.rules_jvm_external;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file should live under //private/tools/java rather than in a new top-level directory.


import com.android.tools.build.jetifier.standalone.Main;
import com.google.devtools.build.lib.worker.ProtoWorkerMessageProcessor;
import com.google.devtools.build.lib.worker.WorkRequestHandler;

import java.io.IOException;
import java.io.PrintWriter;
import java.time.Duration;
import java.util.List;

/**
* A Wrapper for jetifier-standalone to support multiplex worker
*/
public class JetifierWorkerWrapper {

public static void main(String[] args) {
if (args.length == 1 && args[0].equals("--persistent_worker")) {
WorkRequestHandler workerHandler =
new WorkRequestHandler.WorkRequestHandlerBuilder(
JetifierWorkerWrapper::jetifier,
System.err,
new ProtoWorkerMessageProcessor(System.in, System.out))
.setCpuUsageBeforeGc(Duration.ofSeconds(10))
.build();
int exitCode = 1;
try {
workerHandler.processRequests();
exitCode = 0;
} catch (IOException e) {
System.err.println(e.getMessage());
} finally {
// Prevent hanging threads from keeping the worker alive.
System.exit(exitCode);
}
} else {
Main.main(args);
}
}

private static int jetifier(List<String> args, PrintWriter pw) {
Main.main(args.toArray(new String[0]));
return 0;
}

}