add dep commands don't work for java_proto_library

[cushon]

Description

The suggested fix for Strict Java Deps errors adds the wrong target for dependencies on java_proto_librarys. It suggests adding a dep on the underlying proto_library target, instead of on the java_proto_library target.

Repro

First, download: https://gist.github.com/cushon/0241dafdb608b7e2f37c475d3304aa18

Building fails with a strict deps error:

$ bazel build :b
...
B.java:2: error: [strict] Using type com.test.proto.P from an indirect dependency (TOOL_INFO: "//:p_proto wrapped in java_proto_library"). See command below **
  com.test.proto.P.Message m;
                ^
 ** Please add the following dependencies: 
  //:p_proto  to //:b 
 ** You can use the following buildozer command: 
buildozer 'add deps //:p_proto ' //:b 

The suggested fix is to add //p:proto, which does not fix the problem:

$ buildozer 'add deps //:p_proto ' //:b 
fixed ./sjdproto/BUILD
$ bazel build :b
B.java:2: error: [strict] Using type com.test.proto.P from an indirect dependency (TOOL_INFO: "//:p_proto wrapped in java_proto_library"). See command below **
  com.test.proto.P.Message m;

The correct dep to add is //:p_java_proto:

$ buildozer 'add deps //:p_java_proto ' //:b 
$ bazel build :b
...
INFO: Build completed successfully

What's the output of bazel info release?

$ bazel info release
release 0.11.1

[ittaiz]

Doesn’t this stem from “stamping” the outputs on the creation site? Like with java exports?

…

On Tue, 10 Apr 2018 at 9:19 Liam Miller-Cushon ***@***.***> wrote: Description The suggested fix for Strict Java Deps errors adds the wrong target for dependencies on java_proto_librarys. It suggests adding a dep on the underlying proto_library target, instead of on the java_proto_library target. Repro First, download: https://gist.github.com/cushon/0241dafdb608b7e2f37c475d3304aa18 Building fails with a strict deps error: $ bazel build :b ... B.java:2: error: [strict] Using type com.test.proto.P from an indirect dependency (TOOL_INFO: "//:p_proto wrapped in java_proto_library"). See command below ** com.test.proto.P.Message m; ^ ** Please add the following dependencies: //:p_proto to //:b ** You can use the following buildozer command: buildozer 'add deps //:p_proto ' //:b The suggested fix is to add //p:proto, which does not fix the problem: $ buildozer 'add deps //:p_proto ' //:b fixed ./sjdproto/BUILD $ bazel build :b B.java:2: error: [strict] Using type com.test.proto.P from an indirect dependency (TOOL_INFO: "//:p_proto wrapped in java_proto_library"). See command below ** com.test.proto.P.Message m; The correct dep to add is //:p_java_proto: $ buildozer 'add deps //:p_java_proto ' //:b $ bazel build :b ... INFO: Build completed successfully What's the output of bazel info release? $ bazel info release release 0.11.1 — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#4990>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIF3FXr82jtyfCyvn3LZLIjT5Wy-tgks5tnE7_gaJpZM4TNqaT> .

[cushon]

No, the current handling of j_p_l (and exports) predates and was unchanged by stamping.

The issue here is that the special-casing of j_p_l @tomlu described in the thread [1] doesn't work with Bazel.

[1] https://groups.google.com/d/msg/bazel-discuss/mt2llfwzmac/epJDzdY9CAAJ

[ittaiz]

got you.

…

On Tue, Apr 10, 2018 at 7:54 PM Liam Miller-Cushon ***@***.***> wrote: No, the current handling of j_p_l (and exports) predates and was unchanged by stamping. The issue here is that the special-casing of j_p_l @tomlu <https://github.com/tomlu> described in the thread [1] doesn't work with Bazel. [1] https://groups.google.com/d/msg/bazel-discuss/mt2llfwzmac/epJDzdY9CAAJ — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#4990 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIF0LnN7y9nEKhGG6tVNopLM4go1h1ks5tnOO4gaJpZM4TNqaT> .

[github-actions]

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 3 years. It will be closed in the next 14 days unless any other activity occurs or one of the following labels is added: "not stale", "awaiting-bazeler". Please reach out to the triage team (@bazelbuild/triage) if you think this issue is still relevant or you are interested in getting the issue resolved.

[cheister]

Commenting to reset stale check, This is still an issue.

[github-actions]

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 90 days unless any other activity occurs. If you think this issue is still relevant and should stay open, please post any comment here and the issue will no longer be marked as stale.

[evis]

It's still relevant.

[evis]

@cushon are there any plans to fix this issue? Or maybe, some tips for potential contributors, how to fix it? Links to relevant code, etc.

We'd like to turn on strict_deps_mode in our repo. But we can't do it with this issue. When we turn on strict_deps_mode, then there are lots of false-positive suggestions to add proto_library targets.

[cushon]

At Google we have a separate add_dep utility that Strict Java Deps uses for the suggested fix in the diagnostic, instead of using buildozer directly. add_dep is a simple wrapper around buildozer that handles a few special cases for these fixes, including handling the java_proto_library rules.

There's some discussion in #17805 about another case the current buildozer suggested fixes don't handle, which the add_dep wrapper does.

The long term plan here was always to open-source the add_dep wrapper.

[cushon]

I took a first pass at open-sourcing the tool in bazelbuild/buildtools#1269

Demo:

add_dep "//:p_proto java_proto_library" //:b
...
Running bazel query: visible(//:b, //:p_proto + //:b)
Loading: 0 packages loaded
Finished query in 2502ms
ADDING: //:p_java_proto (resolved from //:p_proto)
finished in 4507ms [added 1/1 dep(s)]

$ git diff
diff --git a/BUILD b/BUILD
index c2ac9c7..3efb234 100644
--- a/BUILD
+++ b/BUILD
@@ -19,6 +19,7 @@ java_library(
     srcs = ["B.java"],
     deps = [
         ":a",
+        ":p_java_proto",
         ":p_proto",
     ],
 )