Bazel: --strict_java_deps=warn regression in 0.18x

[ittaiz]

Description of the problem / feature request:

We have code which is being compiled via java_common.compile.
0.17.2 running with --strict_java_deps=warn emits a warning (warning: [strict] Using type org.atmosphere.wasync.impl.AtmosphereClient from an indirect dependency (TOOL_INFO: "external/org_atmosphere_wasync/wasync-2.1.5.jar").) we parse that and add the relevant buildozer strict-deps instructions.
In 0.18.1 (and in 0.19.0) running with --strict_java_deps=warn does not emit this warning but fails (error: symbol not found org.atmosphere.wasync.impl.AtmosphereClient import org.atmosphere.wasync.impl.AtmosphereClient; )
In 0.18.1- I looked in the relevant bazel-out folder and I see that in foo_java-hjar.jar-0.params the external dependency is missing while in foo_java.jar-0.params it exists.
In 0.17.2- the external dependency exists in both of the above files.
This leads me to suspect something has changed in the classpath being sent to javac when building the ijar (maybe it's turbine and not javac, not sure).

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

I'm working on a repro. Not sure it surfaces easily in an extracted piece but I'll try.
I will also try to see if I can repro this on java_library because we can always suspect the starlark middleman (java_common.compile)

cc @cushon @iirina

[ittaiz]

Ok, this happens exactly the same when I have a java_library there.
Trying to carve out a minimal example.
Might be interesting to add that the build fails when I run bazel build //some/path/foo/... on //some/path/foo:some-target but passes when I run only //some/path/foo:some-target.
There are other sub-packages under foo and other packages.

[ittaiz]

there's a chance this is related to our setup. Still looking. I'll update later

[cushon]

Hard to say for sure without a repro, but I have a few notes:

error: symbol not found org.atmosphere.wasync.impl.AtmosphereClient import org.atmosphere.wasync.impl.AtmosphereClient;

I think that's a turbine error, not a javac error. Strict deps diagnostics in header compilation (hjar) actions are best effort. JavaBuilder is the source of truth for strict deps diagnostics.

I looked in the relevant bazel-out folder and I see that in foo_java-hjar.jar-0.params the external dependency is missing while in foo_java.jar-0.params it exists.

There are some optimizations to prune or repackage transitive dependencies to reduce the number of inputs to hjar actions. They don't affect correctness or the output of the action, but they could affect strict deps diagnostics. Again, the diagnostics from the JavaBuilder / non-hjar action for that target should have worked.

passes when I run only //some/path/foo:some-target.

Note that building a java_library doesn't result in the hjar action being executed; the hjar is only created if there's a downstream library depending on the target. For the repro you might need to create another target that depends on the one whose hjar you want to be built.

[ittaiz]

ok, fairly convinced this isn't related to our setup and is a real issue.
Still working on carving out a repro but I'm very close.
Just in case it will trigger an aha moment (or an "it's obvious you're doing something wrong" moment) for @cushon then I'll mention that we're running with --java_toolchain --host_java_toolchain --host_javabase --javabase all pointing to an oracle jdk 8.
Could 0.18.1 have removed strict-deps support for jdk 8?

[ittaiz]

oh, just saw your reply now. thanks. I indeed created another downstream target.
Will update soon I hope

[ittaiz]

ok, that was lengthy, unpleasant and tedious but we have a carved out minimal repro :)
https://github.com/ittaiz/strict-deps-regression-18.1
bazel build //... with 0.17.2 => build completes successfully with a turbine warning + strict-deps warning
bazel build //... with 0.18.1 => build fails to complete
Note that you should symlink your jdk to /usr/local/lib/jvm/java-8-latest since that's where the java_runtime is pointing to and for me that is

java version "1.8.0_131"
Java(TM) SE Runtime Environment (build 1.8.0_131-b11)
Java HotSpot(TM) 64-Bit Server VM (build 25.131-b11, mixed mode)

[ittaiz]

I'll note that we saw this also on 0.19,0 but I think the repro needs to be changed for this (I think the bootclasspath of the default_java_toolchain changed but I'm not sure)
Also this is blocking our upgrade to 0.19.0 (to be more precise we'll probably rollback our upgrade on Sunday).
Thanks for your help!

[cushon]

Building with --keep_going shows that the error is being reported by the turbine action, and that the corresponding JavaBuilder action logs the expected strict deps warning.

Without --keep_going you won't get the warning if the turbine action finishes first, and also that the warning gets cached so it won't show up on rebuilds.

$ ./bazel-0.18.1-linux-x86_64 clean
$ ./bazel-0.18.1-linux-x86_64 build \
  --keep_going \
   --define=ABSOLUTE_JAVABASE=$JAVA8_HOME \
  --javabase=@bazel_tools//tools/jdk:absolute_javabase \
  --host_javabase=@bazel_tools//tools/jdk:absolute_javabase \
  --output_filter= --strict_java_deps=warn  \
  ...
...
ERROR: /tmp/tmp.kRw22ndQCR/strict-deps-regression-18.1/BUILD.bazel:11:1: Couldn't build file libautomation-hjar.jar: Compiling Java headers libautomation-hjar.jar (1 files) failed (Exit 1)

DefaultAtmosphere.java:4: error: symbol not found org.atmosphere.wasync.impl.AtmosphereClient
import org.atmosphere.wasync.impl.AtmosphereClient;
       ^

INFO: From Building libautomation.jar (1 source file):
DefaultAtmosphere.java:8: warning: [strict] Using type org.atmosphere.wasync.impl.AtmosphereClient from an indirect dependency (TOOL_INFO: "@org_atmosphere_wasync//:org_atmosphere_wasync"). See command below **
    private final AtmosphereClient client = ClientFactory.getDefault().newClient(AtmosphereClient.class);
                  ^
 ** Please add the following dependencies: 
  @org_atmosphere_wasync//:org_atmosphere_wasync to //:automation 
 ** You can use the following buildozer command: 
buildozer 'add deps @org_atmosphere_wasync//:org_atmosphere_wasync' //:automation 

The turbine action relies on the invariants enforced by strict deps to optimize its classpath. This is related to b5099a245ced48f3badb43e93766691bc223bf93 and 12e27f7b016acf1538b3c14c11bf828ca499fcc0.

Adding the missing dep allows the header compilation action to succeed:

diff --git a/BUILD.bazel b/BUILD.bazel
index 2b7297f..5befff5 100644
--- a/BUILD.bazel
+++ b/BUILD.bazel
@@ -15,6 +15,7 @@ java_library(
     ],
     deps = [
         ":main_dependencies",
+        "@org_atmosphere_wasync//jar",
     ],
 )

[ittaiz]

I’ll take a look soon but why does it fail (and so need -k)? In 0.17.2 the
build completes successfully while in 0.18.1 it fails.
On Sat, 3 Nov 2018 at 0:49 Liam Miller-Cushon [e-mail]
wrote:

Building with --keep_going shows that the error is being reported by the
turbine action, and that the corresponding JavaBuilder action logs the
expected strict deps warning.

Without --keep_going you won't get the warning if the turbine action
finishes first, and also that the warning gets cached so it won't show up
on rebuilds.

$ ./bazel-0.18.1-linux-x86_64 clean
$ ./bazel-0.18.1-linux-x86_64 build \
--keep_going \
--define=ABSOLUTE_JAVABASE=$JAVA8_HOME \
--javabase=@bazel_tools//tools/jdk:absolute_javabase \
--host_javabase=@bazel_tools//tools/jdk:absolute_javabase \
--output_filter= --strict_java_deps=warn \
...
...
ERROR: /tmp/tmp.kRw22ndQCR/strict-deps-regression-18.1/BUILD.bazel:11:1: Couldn't build file libautomation-hjar.jar: Compiling Java headers libautomation-hjar.jar (1 files) failed (Exit 1)

DefaultAtmosphere.java:4: error: symbol not found org.atmosphere.wasync.impl.AtmosphereClient
import org.atmosphere.wasync.impl.AtmosphereClient;
^

INFO: From Building libautomation.jar (1 source file):
DefaultAtmosphere.java:8: warning: [strict] Using type org.atmosphere.wasync.impl.AtmosphereClient from an indirect dependency (TOOL_INFO: "@org_atmosphere_wasync//:org_atmosphere_wasync"). See command below *
private final AtmosphereClient client = ClientFactory.getDefault().newClient(AtmosphereClient.class);
^
* Please add the following dependencies:
@org_atmosphere_wasync//:org_atmosphere_wasync to //:automation
** You can use the following buildozer command:
buildozer 'add deps @org_atmosphere_wasync//:org_atmosphere_wasync' //:automation

The turbine action relies on the invariants enforced by strict deps to
optimize its classpath. This is related to b5099a2
https://github.com/bazelbuild/bazel/commit/b5099a245ced48f3badb43e93766691bc223bf93
and 12e27f7
https://github.com/bazelbuild/bazel/commit/12e27f7b016acf1538b3c14c11bf828ca499fcc0
.

Adding the missing dep allows the header compilation action to succeed:

diff --git a/BUILD.bazel b/BUILD.bazel
index 2b7297f..5befff5 100644--- a/BUILD.bazel+++ b/BUILD.bazel@@ -15,6 +15,7 @@ java_library(
],
deps = [
":main_dependencies",+ "@org_atmosphere_wasync//jar",
],
)

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/bazelbuild/bazel/issues/6580#issuecomment-435531468,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABUIF9z2fipmAiYianQTZ1rYnF6nhuLfks5urMwKgaJpZM4YLNSX
.

[cushon]

My previous comment mentions that "the turbine action relies on the invariants enforced by strict deps to optimize its classpath." If there are missing direct deps, the optimization may result in 'missing symbol' errors that weren't present before.

[ittaiz]

I’m starting to understand (forgive me if I’m still putting in the pieces).

1. The build fails because you’ve removed strict-deps fallback to javac.
2. We don’t get the message because of turbine being able to work on
   sources instead of bytecode (since it means it doesn’t have to be after the
   strict-deps action execution wise).
3. Turbine changed to assume strict-deps already ran?

I hope I’m building up an understanding. I’ll try to run with -k and see I
can work with the outputs.

I hope 0.19.0 won’t pose additional difficulties since I think the
platformclasspath part changed as well.

On Sat, 3 Nov 2018 at 6:33 ittai zeidman [e-mail] wrote:

I’ll take a look soon but why does it fail (and so need -k)? In 0.17.2 the
build completes successfully while in 0.18.1 it fails.
On Sat, 3 Nov 2018 at 0:49 Liam Miller-Cushon [e-mail]
wrote:

Building with --keep_going shows that the error is being reported by the
turbine action, and that the corresponding JavaBuilder action logs the
expected strict deps warning.

Without --keep_going you won't get the warning if the turbine action
finishes first, and also that the warning gets cached so it won't show up
on rebuilds.

$ ./bazel-0.18.1-linux-x86_64 clean
$ ./bazel-0.18.1-linux-x86_64 build \
--keep_going \
--define=ABSOLUTE_JAVABASE=$JAVA8_HOME \
--javabase=@bazel_tools//tools/jdk:absolute_javabase \
--host_javabase=@bazel_tools//tools/jdk:absolute_javabase \
--output_filter= --strict_java_deps=warn \
...
...
ERROR: /tmp/tmp.kRw22ndQCR/strict-deps-regression-18.1/BUILD.bazel:11:1: Couldn't build file libautomation-hjar.jar: Compiling Java headers libautomation-hjar.jar (1 files) failed (Exit 1)

DefaultAtmosphere.java:4: error: symbol not found org.atmosphere.wasync.impl.AtmosphereClient
import org.atmosphere.wasync.impl.AtmosphereClient;
^

INFO: From Building libautomation.jar (1 source file):
DefaultAtmosphere.java:8: warning: [strict] Using type org.atmosphere.wasync.impl.AtmosphereClient from an indirect dependency (TOOL_INFO: "@org_atmosphere_wasync//:org_atmosphere_wasync"). See command below *
private final AtmosphereClient client = ClientFactory.getDefault().newClient(AtmosphereClient.class);
^
* Please add the following dependencies:
@org_atmosphere_wasync//:org_atmosphere_wasync to //:automation
** You can use the following buildozer command:
buildozer 'add deps @org_atmosphere_wasync//:org_atmosphere_wasync' //:automation

The turbine action relies on the invariants enforced by strict deps to
optimize its classpath. This is related to b5099a2
https://github.com/bazelbuild/bazel/commit/b5099a245ced48f3badb43e93766691bc223bf93
and 12e27f7
https://github.com/bazelbuild/bazel/commit/12e27f7b016acf1538b3c14c11bf828ca499fcc0
.

Adding the missing dep allows the header compilation action to succeed:

diff --git a/BUILD.bazel b/BUILD.bazel
index 2b7297f..5befff5 100644--- a/BUILD.bazel+++ b/BUILD.bazel@@ -15,6 +15,7 @@ java_library(
],
deps = [
":main_dependencies",+ "@org_atmosphere_wasync//jar",
],
)

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/bazelbuild/bazel/issues/6580#issuecomment-435531468,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABUIF9z2fipmAiYianQTZ1rYnF6nhuLfks5urMwKgaJpZM4YLNSX
.

[cushon]

The build fails because you’ve removed strict-deps fallback to javac.

Right. With --strict_java_deps=error the build would have failed either way.

it means it doesn’t have to be after the strict-deps action execution wise

Right, the turbine and JavaBuilder actions happen in parallel, unlike ijar which runs after JavaBuilder. That's basically the point of header compilation: we can move the slower JavaBuilder actions off the critical path. Scheduling turbine actions after JavaBuilder would make it easier to report good strict deps diagnostics, but regress performance.

I’ll try to run with -k and see I can work with the outputs.

Are you using --strict_java_deps=warn in production, or just for debugging? (e.g. it can be useful to fix a bunch of strict deps errors at once, instead of having the build fail on the first one.)

[ittaiz]

@cushon If I run with -k 0.18.1 indeed prints the message (though I think the change from success to failure of Bazel itself should be documented).
Problem is that if I try to run the repro with 0.19.0 I get

ERROR: /Users/ittaiz/workspace/bazel-playground/strict-deps-regression-18.1/toolchains/BUILD:3:1: no such target '@bazel_tools//tools/jdk:platformclasspath8': target 'platformclasspath8' not declared in package 'tools/jdk' (did you mean 'platformclasspath'?) defined by /private/var/tmp/_bazel_ittaiz/52bfd4a4cc3853242b36dda37b15b15d/external/bazel_tools/tools/jdk/BUILD and referenced by '//toolchains:default_java_toolchain'

and if I change the default_java_toolchain to depend on platformclasspath (and not platformclasspath8) then I get

ERROR: /Users/ittaiz/workspace/bazel-playground/strict-deps-regression-18.1/BUILD.bazel:3:1: Building libmain_dependencies.jar (1 source file) failed: Worker process did not return a WorkResponse:

---8<---8<--- Start of log, file at /private/var/tmp/_bazel_ittaiz/52bfd4a4cc3853242b36dda37b15b15d/bazel-workers/worker-0-Javac.log ---8<---8<---
Unrecognized VM option 'CompactStrings'
Error: Could not create the Java Virtual Machine.
Error: A fatal exception has occurred. Program will exit.
---8<---8<--- End of log ---8<---8<---

Which is strange since I think I'm passing all the relevant flags (via .bazelrc)

[ittaiz]

again missed your message before posting (sorry, busy with micro experiments).
We have an area where we run with warn and it's essential for us to fix massive amounts of strict-deps (automatic migration from maven to bazel) but I'm fairly confident we can run with -k.
The bigger problem is the 0.19.0 from above.

[cushon]

This should work with 0.19:

diff --git a/toolchains/BUILD b/toolchains/BUILD
index 84d899e..3bc04a8 100644
--- a/toolchains/BUILD
+++ b/toolchains/BUILD
@@ -1,9 +1,10 @@
-load("@bazel_tools//tools/jdk:default_java_toolchain.bzl", "default_java_toolchain")
+load("@bazel_tools//tools/jdk:default_java_toolchain.bzl", "default_java_toolchain", "JDK8_JVM_OPTS", "DEFAULT_JAVACOPTS")

 default_java_toolchain(
     name = "default_java_toolchain",
-    bootclasspath = ["@bazel_tools//tools/jdk:platformclasspath8"],
-    misc = [
+    bootclasspath = ["@bazel_tools//tools/jdk:platformclasspath"],
+    jvm_opts = JDK8_JVM_OPTS,
+    misc = DEFAULT_JAVACOPTS + [
         "-XepDisableAllChecks",
     ],
     source_version = "8",

The important part is jvm_opts = JDK8_JVM_OPTS,; the implicit defaults now assume a JDK 9 --host_javabase. I recommend DEFAULT_JAVACOPTS too, but it should work without it.

[ittaiz]

Thanks! Indeed generates it.
I think both of these changes weren't in the release notes, am I mistaken?
The frequency of java changes since 15 are hard to keep up and without big bold announcements it's even harder :(
Also,
Is there any relevance to strict-deps warn? Do the above changes mean it will fail often (always?)?

Again, I really appreciate your help on this.

[cushon]

You're right that there weren't relnotes for the direct classpath changes, and maybe there should have been. It was supposed to be functionally equivalent and only affect performance, but I didn't think through the effect on --strict_java_deps=warn.

Is there any relevance to strict-deps warn? Do the above changes mean it will fail often (always?)?

Yeah, this is definitely a regression for --strict_java_deps=warn :/ I think --keep_going is a good work-around for your use-case, and I'll defer to @iirina and @lberki on what the level of support is for using --strict_java_deps=warn in production.

[ittaiz]

Thanks!
Not sure “production” is the right categorization as this is a non
experimental feature that effectively stopped working. Also for developer
machines.
I have to say that for our specific use case I think we can use -k but for
developer machines and rapid iterations maybe this is still desired.
On Sat, 3 Nov 2018 at 7:50 Liam Miller-Cushon [e-mail]
wrote:

You're right that there weren't relnotes for the direct classpath changes,
and maybe there should have been. It was supposed to be functionally
equivalent and only affect performance, but I didn't think through the
effect on --strict_java_deps=warn.

Is there any relevance to strict-deps warn? Do the above changes mean it
will fail often (always?)?

Yeah, this is definitely a regression for --strict_java_deps=warn :/ I
think --keep_going is a good work-around for your use-case, and I'll
defer to @iirina https://github.com/iirina and @lberki
https://github.com/lberki on what the level of support is for using
--strict_java_deps=warn in production.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/bazelbuild/bazel/issues/6580#issuecomment-435563085,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABUIF2jbTYI28I8eonxUZIqJy6zWe_nxks5urS65gaJpZM4YLNSX
.

[ittaiz]

Actually we had jvm_opts with the literal but I removed it from the example since I thought it might just be extra noise. We also have javac = ["@bazel_tools//third_party/java/jdk/langtools:javac_jar"] in the full example, is that good/bad/indifferent?

[cushon]

java_toolchain.javac is a legacy attribute and shouldn't have an effect either way.

The canonical --host_javabase=<jdk8>-compatible toolchain is @bazel_tools//tools/jdk:toolchain_hostjdk8:

https://github.com/bazelbuild/bazel/blob/f8be43cebfe5d9ee80e838c883fb305969ece743/tools/jdk/BUILD#L209-L215

[lberki]

I would actually prefer not having --strict_java_deps=warn at all -- warnings have a tendency to be ignored, so in my experience, their utility is very limited.

[cushon]

--strict_java_deps=warn is useful for scraping all of the add_dep commands from a build so you can fix them all at once. But with header compilation enabled you can also do that using --keep_going, so maybe we should encourage that approach and deprecate --strict_java_deps.

[ittaiz]

We’re fairly certain we have a strong need for this in a cross repo setting
where we have two repositories with strict-deps infractions and the
dependency graph travels back and forth between the repositories.
My plan was to run Bazel with warn on both repositories and apply the fixes
for each and then switch to error.
Am I mistaken and I can achieve the same result now?
Cc @or-shachar
On Tue, 6 Nov 2018 at 19:30 Liam Miller-Cushon [e-mail]
wrote:

--strict_java_deps=warn is useful for scraping all of the add_dep
commands from a build so you can fix them all at once. But with header
compilation enabled you can also do that using --keep_going, so maybe we
should encourage that approach and deprecate --strict_java_deps.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/bazelbuild/bazel/issues/6580#issuecomment-436338742,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABUIF8C57Ps3zugu3wIzA-zujIqQM7cUks5uscdKgaJpZM4YLNSX
.

[cushon]

Am I mistaken and I can achieve the same result now?

I think you might be able to achieve the same result using --keep_going instead. Can you try that experiment?

[or-shachar]

We always run with the --keep-going but we get errors on strict deps issues of external repositories

[cushon]

The need for per-workspace configuration has come up before, e.g. in https://github.com/bazelbuild/bazel/issues/6353#issuecomment-429092469. I'm not sure if there are any plans in that area.

[jin]

@lberki could you add a priority to this issue, please?