Open honnix opened 7 months ago
Alternatively, this can be done similarly as https://github.com/bazelbuild/bazel/blob/5d5da86dd7a6ad94e755489e743fdd58c480a116/src/main/starlark/builtins_bzl/common/java/proto/java_proto_library.bzl#L121
@ejona86 Any thoughts on plumbing through additional javac options in this manner?
FWIW, we applied the following patch and it worked as expected:
diff --git a/java_grpc_library.bzl b/java_grpc_library.bzl
index ed9900917..ec11f2453 100644
--- a/java_grpc_library.bzl
+++ b/java_grpc_library.bzl
@@ -104,13 +104,15 @@ def _java_rpc_library_impl(ctx):
)
deps_java_info = java_common.merge([dep[JavaInfo] for dep in ctx.attr.deps])
+ java_toolchain = toolchain.java_toolchain[java_common.JavaToolchainInfo]
java_info = java_common.compile(
ctx,
- java_toolchain = toolchain.java_toolchain[java_common.JavaToolchainInfo],
+ java_toolchain = java_toolchain,
source_jars = [srcjar],
output = ctx.outputs.jar,
output_source_jar = ctx.outputs.srcjar,
+ javac_opts = java_toolchain._compatible_javacopts.get("grpc", depset()),
plugins = [plugin[JavaPluginInfo] for plugin in toolchain.java_plugins],
deps = [
java_common.make_non_strict(deps_java_info),
And this is how we configure toolchain:
default_java_toolchain(
...
compatible_javacopts = {
"grpc": [...],
},
...
)
I'm really not up-to-date on how toolchains are configured. It seems this sort of option could maybe be in java_rpc_toolchain, although right now grpc_java_library uses a hard-coded toolchain.
I'm generally very fine with copying proto_java_library, but _compatible_javacopts is private API.
What type of javac options are you wanting to set? Linter, warnings, or something else?
@ejona86 It could be the case having a toolchain configured globally to target one Java version, while for proto/grpc to target another version, for compatibility reasons. It could also be disabling or enabling for example -Werror
.
but _compatible_javacopts is private API
Yeah that is very valid concern.
Looks like compatible_javacopts
in java_toolchain
is also private. "Internal API, do not use!":
https://bazel.build/reference/be/java#java_toolchain.compatible_javacopts
having a toolchain configured globally to target one Java version, while for proto/grpc to target another version, for compatibility reasons
Although in that case "the Bazel way" would have a full new toolchain, not just javacopts. Right?
It could also be disabling or enabling for example -Werror.
If our generated code is producing a warning, I'd be interested in fixing that. And there's not much point in enabling -Werror
only for the generated code.
Are either of these issues a problem you are actually having?
Although in that case "the Bazel way" would have a full new toolchain, not just javacopts. Right?
Yeah that could be an alternative if java_grpc_library
could accept a Java toolchain. I think right now it uses whatever resolved by Bazel via @bazel_tools//tools/jdk:current_java_toolchain
if I'm not mistaken. Or we could perhaps create a full java_grpc_library_toolchain and pass it in here https://github.com/grpc/grpc-java/blob/34e241a60e7dd54195daaec3a16decae0dd702cc/java_grpc_library.bzl#L134.
If our generated code is producing a warning, I'd be interested in fixing that. And there's not much point in enabling -Werror only for the generated code.
It could be a temporary deprecation warning.
Are either of these issues a problem you are actually having?
We sort of have both, but primarily the first case where we need to compile proto+grpc targeting a lower version of Java.
Is your feature request related to a problem?
java_grpc_library
passes the resolved java toolchain tojava_common.compile
to compile generated code. However, there are cases that users might need to further customizejavac_opts
, where the customization is not suitable to be done directly on the toolchain itself.Describe the solution you'd like
_java_grpc_library
and_java_lite_grpc_library
(used byjava_grpc_library
) expose an attribute to receive customizedjavac_opts
and pass it down tojava_common.compile
.Describe alternatives you've considered
I was not able to find a reasonable alternative but I could have missed something.
Additional context