Adapt bazel-jdt-java-toolchain to latest ecj changes

[trancexpress]

See eclipse-jdt/eclipse.jdt.core#2577, some changes were merged to JDT core to support bazel-jdt-java-toolchain.

bazel-jdt-java-toolchain doesn't seem to support those changes yet though, when using both latest ecj and bazel-jdt-java-toolchain, we get this error:

  Exception in thread "main" java.lang.NoClassDefFoundError: org/eclipse/jdt/internal/compiler/batch/FileSystem$ClasspathAnswer
          at com.google.devtools.build.buildjar.javac.BlazeEcjMain$BlazeEclipseBatchCompiler.getLibraryAccess(BlazeEcjMain.java:173)
          at org.eclipse.jdt.core.compiler.batch/org.eclipse.jdt.internal.compiler.batch.Main.performCompilation(Main.java:4675)
          at org.eclipse.jdt.core.compiler.batch/org.eclipse.jdt.internal.compiler.batch.Main.compile(Main.java:1679)
          at com.google.devtools.build.buildjar.javac.BlazeEcjMain.compile(BlazeEcjMain.java:331)
          at com.google.devtools.build.buildjar.ReducedClasspathJavaLibraryBuilder.compileSources(ReducedClasspathJavaLibraryBuilder.java:58)
          at com.google.devtools.build.buildjar.SimpleJavaLibraryBuilder.compileJavaLibrary(SimpleJavaLibraryBuilder.java:120)
          at com.google.devtools.build.buildjar.SimpleJavaLibraryBuilder.run(SimpleJavaLibraryBuilder.java:128)
          at com.google.devtools.build.buildjar.BazelEcjJavaBuilder.build(BazelEcjJavaBuilder.java:110)
          at com.google.devtools.build.buildjar.BazelEcjJavaBuilder.parseAndBuild(BazelEcjJavaBuilder.java:82)
          at com.google.devtools.build.buildjar.BazelEcjJavaBuilder.main(BazelEcjJavaBuilder.java:70)
  Caused by: java.lang.ClassNotFoundException: org.eclipse.jdt.internal.compiler.batch.FileSystem$ClasspathAnswer
          at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:641)
          at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188)
          at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:526)
          ... 10 more

[guw]

@trancexpress Can you please push a a draft PR so I can verify the changes you did?

[trancexpress]

@guw our changes are to use our ecj jar. We use Eclipse 4.35 with some backports, so its not exactly the ecj from 4.35, but there are no API changes. We don't adjust anything else related to this ticket.

I.e. we do this:

diff --git a/BUILD b/BUILD
index 966bbd2..d97094c 100644
--- a/BUILD
+++ b/BUILD
@@ -21,6 +21,8 @@ java_binary(
     name = "JdtJavaBuilder",
     main_class = "com.google.devtools.build.buildjar.BazelEcjJavaBuilder",
     runtime_deps = ["//compiler:buildjar"],
+    deploy_env = [":ecj_bin"], # keep the ecj classes out of the depoy jar
+    visibility = ["//visibility:public"], # Make it visible so that it can be accessed from smartest
 )
 
 exports_files(
@@ -40,6 +42,6 @@ default_java_toolchain(
 # We have to wrap ecj.jar inside a "java_binary" for "deploy_env" to work.
 java_binary(
     name = "ecj_bin",
-    runtime_deps = ["//compiler/third_party/ecj"],
+    runtime_deps = ["//compiler:ecj"],
     main_class = "org.eclipse.jdt.internal.compiler.batch.Main",
 )
diff --git a/compiler/export/JdtJavaBuilder_deploy.jar b/compiler/export/JdtJavaBuilder_deploy.jar
index ea5ebf8..246eb95 100755
Binary files a/compiler/export/JdtJavaBuilder_deploy.jar and b/compiler/export/JdtJavaBuilder_deploy.jar differ

And then to to build our software, we use:

default_java_toolchain(
    name = "ecj_java_toolchain21",
    forcibly_disable_header_compilation = True,
    java_runtime = "@local_jdk21",  # JDK to use for compilation and toolchain's tools execution
    javabuilder = ["@bazel_jdt_java_toolchain//compiler/export:JdtJavaBuilder"],
    # set the required release
    javacopts = ["--release 21"],
    jvm_opts = [
        "--module-path",
        "$(location @ecj//:ecj.jar)",
    ],
    misc = DEFAULT_JAVACOPTS + [
        "-XSFmax_stdouterr_bytes=9000000",
    ],
    tools = ["@ecj//:ecj.jar"],
)

We don't have other actual changes. Or am I missing something? Are you expecting some other changes on our end?

What we initially wanted was to not have patches on top of JDT. Is that possible after the changes to JDT from the referred PR, or do we need more? See again:

• Create a ticket to to eclipse.org to integrate the ecj patch #27
• Provide ability to track dependencies during batch compilation (#2529) eclipse-jdt/eclipse.jdt.core#2577

[trancexpress]

@guw just to clarify, we don't have a fix for the issue here. Do you have time to look into it?

[guw]

@trancexpress Steps to update ECJ are described in the README. If you follow those steps it should work.

[trancexpress]

@guw we don't want to patch/update ECJ. The goal of eclipse-jdt/eclipse.jdt.core#2577 was to avoid this patching?

[iloveeclipse]

@trancexpress Steps to update ECJ are described in the README. If you follow those steps it should work.

Gunnar, if I see it right, readme says that patched ecj is needed to

As well as additional modification to support:
Collection of used dependencies during compilation (FileSystem.nameEnvironmentListener, see commit history)

Does it mean, bazel can't use "out of the box" ecj at all?

Background for my question: we at Advantest are using Bazel for building one application and also use ecj nightly builds to validate latest ecj/JDT state. So what we want is to have ability to just "drop in" latest ecj build to bazel and use that, and not some (ancient) ecj version shipped with some bazel release.

[guw]

@guw we don't want to patch/update ECJ. The goal of eclipse-jdt/eclipse.jdt.core#2577 was to avoid this patching?

That is correct. However, ECJ has not been updated to the unpatched version yet. Thus, an update to the unpatched version is necessary. Once updated I expect the build to fail with compilation issues (hence my pointer to follow the steps). This likely requires modifications to

bazel-jdt-java-toolchain/compiler/src/main/buildjar/com/google/devtools/build/buildjar/javac/BlazeEcjMain.java

Line 173 in fd37288

nameEnvironment.nameEnvironmentListener = this::recordNameEnvironmentAnswer;

One thing I am not sure about is whether ClasspathAnswer is still necessary to track the origin an answer came from or if there is an alternate way.

[trancexpress]

Note that we are not using ecj_collect_used_deps so I'm not sure we can test that the functionality still works - it is changed here...

[guw]

It might make sense to add a test for that. What do you think @rsalvador?

[rsalvador]

It might make sense to add a test for that. What do you think @rsalvador?

Yes, I agree.

[trancexpress]

@guw while trying to use the newest version of the toolchain, we have some issue with dependencies. The classpath passed to ecj seems to have [-**/*] suffixes for a lot of libraries. This is coming from: com.google.devtools.build.buildjar.javac.BlazeEcjMain.setLocations(List<String>, BlazeJavacArguments, DependencyModule)

And in particular this code:

+		if (!arguments.classPath().isEmpty()) {
+			ImmutableSet<Path> directJars = dependencyModule.directJars();
+			ecjArguments.add("-classpath");
+			ecjArguments.add(arguments.classPath().stream()
+					.map(p -> directJars.contains(p) ? p.toString() : format("%s[-**/*]", p.toString()))
+					.collect(joining(":")));
+		}

What is the purpose of this code?

We have problems for the following project structure:

• Project A has a lot of re-exported dependencies.
• Projects B and C depend on project A for those re-exported dependencies, instead of having the many dependencies in their respective .classpath files.

The old builder produces a normal classpath, the new one produces a lot of entries ending on [**/*]. So the transitive dependencies from the project that re-exports dependencies doesn't work. Are we supposed to generate all the dependencies in the .bzl build file now?

[guw]

@trancexpress Bazel has a similar concept. Did you declare the exports attribute on the java_library exporting the dependencies?

[trancexpress]

@trancexpress Bazel has a similar concept. Did you declare the exports attribute on the java_library exporting the dependencies?

No, they are empty. Do they need to mirror the dependencies and then we are good?