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

Isolate bazel_worker_java dependencies #7

Open
Bencodes opened this issue Dec 3, 2024 · 10 comments
Open

Isolate bazel_worker_java dependencies #7

Bencodes opened this issue Dec 3, 2024 · 10 comments
Assignees

Comments

@Bencodes
Copy link

Bencodes commented Dec 3, 2024

We see lots of warnings in our console builds because of these rules relying on the default rules_jvm_external maven namespaces.

The solution here would be to move these maven dependencies into their own namespace.

(15:01:37) DEBUG: /private/var/tmp/_bazel_blee/499a001013731d09bffd82f8601a3161/external/rules_jvm_external+/private/extensions/maven.bzl:155:14: The maven repository 'maven' is used in two different bazel modules, originally in 'protobuf' and now in 'bazel_worker_java'
(15:01:37) DEBUG: /private/var/tmp/_bazel_blee/499a001013731d09bffd82f8601a3161/external/rules_jvm_external+/private/extensions/maven.bzl:155:14: The maven repository 'maven' is used in two different bazel modules, originally in 'protobuf' and now in 'bazel_worker_java'
(15:01:37) DEBUG: /private/var/tmp/_bazel_blee/499a001013731d09bffd82f8601a3161/external/rules_jvm_external+/private/extensions/maven.bzl:155:14: The maven repository 'maven' is used in two different bazel modules, originally in 'protobuf' and now in 'bazel_worker_java'
(15:01:37) DEBUG: /private/var/tmp/_bazel_blee/499a001013731d09bffd82f8601a3161/external/rules_jvm_external+/private/extensions/maven.bzl:155:14: The maven repository 'maven' is used in two different bazel modules, originally in 'protobuf' and now in 'bazel_worker_java'
(15:01:37) DEBUG: /private/var/tmp/_bazel_blee/499a001013731d09bffd82f8601a3161/external/rules_jvm_external+/private/extensions/maven.bzl:155:14: The maven repository 'maven' is used in two different bazel modules, originally in 'protobuf' and now in 'bazel_worker_java'
(15:01:37) DEBUG: /private/var/tmp/_bazel_blee/499a001013731d09bffd82f8601a3161/external/rules_jvm_external+/private/extensions/maven.bzl:155:14: The maven repository 'maven' is used in two different bazel modules, originally in 'protobuf' and now in 'bazel_worker_java'

And as a result of using the default shared namespace:

(15:01:41) DEBUG: /private/var/tmp/_bazel_blee/499a001013731d09bffd82f8601a3161/external/rules_jvm_external+/private/rules/coursier.bzl:774:18: Found duplicate artifact versions
    org.mockito:mockito-core has multiple versions 4.3.1, 5.4.0
    com.google.truth:truth has multiple versions 1.1.2, 1.4.0
    com.google.code.gson:gson has multiple versions 2.8.9, 2.10.1
    com.google.errorprone:error_prone_annotations has multiple versions 2.5.1, 2.23.0
    com.google.guava:guava has multiple versions 32.0.1-jre, 33.0.0-jre
@meteorcloudy
Copy link
Member

We could use a different maven namespace for this module, but wouldn't there be risks of conflict classes? If your target A depends on bazel-worker-api, which depends on guava, and your target also directly depends on a different version of guava from another maven namespace.

@jin @shs96c What is the recommended best practice?

@jin
Copy link
Member

jin commented Dec 5, 2024

Yes, care has to be taken when using multiple namespaces (https://github.com/bazel-contrib/rules_jvm_external?tab=readme-ov-file#multiple-maven_install-declarations-for-isolated-artifact-version-trees) that the dep classes from the different namespaces don't make it into the same top level target, i.e. enforcing the one-version rule at the target level.

Is there warning too verbose? We can consider improving it if that's the main concern.

@Bencodes
Copy link
Author

Bencodes commented Dec 6, 2024

Is there warning too verbose? We can consider improving it if that's the main concern.

The messages are definitely verbose. Any time you run a Bazel command where it has to bring up the server for the first time, the console gets hit with a flood of warnings from rules_jvm_external all caused by this worker using the default namespace. I only pasted a subset of them above as an example.

@shs96c
Copy link

shs96c commented Dec 6, 2024

This can be fixed by updating to v29.1 of protobuf. Relevant issues are protocolbuffers/protobuf#19477 (comment) and protocolbuffers/protobuf#19451 (comment)

@meteorcloudy
Copy link
Member

The problem also exists in this repo:

use_repo(maven, "maven")

Should we also rename it?

@shs96c
Copy link

shs96c commented Dec 8, 2024

I would suggest doing so if the repos are independent of each other.

@meteorcloudy
Copy link
Member

meteorcloudy commented Dec 9, 2024

Say if I have a target:

java_binary(
  name = "foo",
  srcs = ["Foo.java"],
  deps = [
     "@bazel_worker_java//src/main/java/com/google/devtools/build/lib
/worker:work_request_handlers",
     "@maven//:com_google_guava_guava",
  ],
)

If we rename the maven install for bazel_woker_java, wouldn't two different guava versions ended up in foo's transitive dependencies? Is that in general fine with Java?

@jin
Copy link
Member

jin commented Dec 11, 2024

There are two options:

  1. bazel_worker_java contributes its own jar deps into the @maven namespace. This will contribute to the users jars for resolving into a single version per dep.
  2. bazel_worker_java uses its own namespace, and make sure that the targets it exposes do not pass on the transitive runtime classpath to the user.

I don't think renaming is the right solution, because

is meant to be depended upon by users' java tooling targets, which means :work_request_handlers's deps should also be contributed into default user @maven namespace, so that there's only one resolved version per jar in the runtime classpath of those worker tools.

I'll work on a separate patch to reduce the noise.

@meteorcloudy
Copy link
Member

@jin Thanks for the clarification!

@jin
Copy link
Member

jin commented Dec 11, 2024

bazel-contrib/rules_jvm_external#1295 will fix the noisy logs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants