Upgrade JaCoCo from 0.8.7 to 0.8.9-SNAPSHOT

[somethingvague]

Description of the feature request:

Please upgrade to the latest JaCoCo distribution which adds official support for Java 17/18 and experimental support for Java 19 classes.

Happy to try workarounds if there are any suggestions.

What underlying problem are you trying to solve with this feature?

Instrumentation for Java 19 targets

Which operating system are you running Bazel on?

Debian GNU/Linux

What is the output of bazel info release?

release 5.3.1

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

No response

What's the output of git remote get-url origin; git rev-parse master; git rev-parse HEAD ?

This is a internal corporate git repository hosted on GitLab.

Have you found anything relevant by searching the web?

https://www.jacoco.org/jacoco/trunk/doc/changes.html

Any other information, logs, or outputs that you want to share?

java.io.IOException: Error while instrumenting bazel-out/k8-fastbuild/bin/<path/to/Class>.
        at org.jacoco.core.instr.Instrumenter.instrumentError(Instrumenter.java:161)
        at org.jacoco.core.instr.Instrumenter.instrument(Instrumenter.java:110)
        at org.jacoco.core.instr.Instrumenter.instrument(Instrumenter.java:135)
        at org.jacoco.core.instr.Instrumenter.instrument(Instrumenter.java:155)
        at com.google.devtools.build.buildjar.instrumentation.JacocoInstrumentationProcessor$1.visitFile(JacocoInstrumentationProcessor.java:141)
        at com.google.devtools.build.buildjar.instrumentation.JacocoInstrumentationProcessor$1.visitFile(JacocoInstrumentationProcessor.java:113)
        at java.base/java.nio.file.Files.walkFileTree(Files.java:2810)
        at java.base/java.nio.file.Files.walkFileTree(Files.java:2881)
        at com.google.devtools.build.buildjar.instrumentation.JacocoInstrumentationProcessor.instrumentRecursively(JacocoInstrumentationProcessor.java:111)
        at com.google.devtools.build.buildjar.instrumentation.JacocoInstrumentationProcessor.processRequest(JacocoInstrumentationProcessor.java:85)
        at com.google.devtools.build.buildjar.SimpleJavaLibraryBuilder.buildJar(SimpleJavaLibraryBuilder.java:151)
        at com.google.devtools.build.buildjar.SimpleJavaLibraryBuilder.run(SimpleJavaLibraryBuilder.java:120)
        at com.google.devtools.build.buildjar.BazelJavaBuilder.build(BazelJavaBuilder.java:105)
        at com.google.devtools.build.buildjar.BazelJavaBuilder.parseAndBuild(BazelJavaBuilder.java:85)
        at com.google.devtools.build.buildjar.BazelJavaBuilder.main(BazelJavaBuilder.java:69)
Caused by: java.lang.IllegalArgumentException: Unsupported class file major version 63
        at org.objectweb.asm.ClassReader.<init>(ClassReader.java:199)
        at org.objectweb.asm.ClassReader.<init>(ClassReader.java:180)
        at org.objectweb.asm.ClassReader.<init>(ClassReader.java:166)
        at org.jacoco.core.internal.instr.InstrSupport.classReaderFor(InstrSupport.java:280)
        at org.jacoco.core.instr.Instrumenter.instrument(Instrumenter.java:76)
        at org.jacoco.core.instr.Instrumenter.instrument(Instrumenter.java:108)
        ... 13 more

[comius]

cc @cushon

[cushon]

Caused by: java.lang.IllegalArgumentException: Unsupported class file major version 63
        at org.objectweb.asm.ClassReader.<init>(ClassReader.java:199)

Is the jacoco in Bazel using a vendored ASM, or is the immediate problem actually that we need a newer ASM?

Either way, upgrading to the latest versions SGTM. Anyone want to send a PR?

[somethingvague]

Hi @cushon

Apologies for the delayed response.

I noticed that the latest release version of jacoco is 0.8.8 which should also be fine, it does not require an upgrade to ASM.

I started making the first commit (see somethingvague@fa2b2cd) and then I noticed the two patches under /third_party/java/jacoco which appear to have been applied to pre-existing 0.8.7 jars.

I think the process is

1. check out 0.8.8 jacoco release tag and make equivalent changes to the patches.
2. maven install to create the patched JARs (any particular bytecode version?)
3. check in the patched JARs along with the diffs for documentation. Are these diffs just for reference, do they need to be separate files and do they need to be a specific format?

Please confirm.

Thanks.

[cushon]

I don't know exactly how this was done for Bazel in 21e2794, but that sounds about right to me.

I think it might be OK to drop 0001-Disable-use-of-constant-dynamic-bytecode.patch at this point.

The patches could be applied with something like:

$ git checkout v0.8.8
$ git am ~/src/bazel/third_party/java/jacoco/*.patch

It doesn't look like there are merge conflicts, but if there were I would apply them against v0.8.7 instead and then use git to do the rebase onto v0.8.8.

And then I would use git format-patch v0.8.8 to regenerate the patches.

[somethingvague]

Thanks for the tips.

I think an ASM upgrade might be needed after all, however I am struggling to verify the changes I am making to my fork. The process I'm following is:

1. Add the Jacoco and ASM jars and update any references I find
2. Build //src:bazel-dev
3. Execute bazel-bin/src/bazel-dev against a sample project which has a similar configuration as my work setting (remote_java_repository with a default_java_toolchain)

I can see that bazel-bin/src/java_tools/buildjar/JavaBuilder_deploy.jar contains the updated JARs but when I run coverage against my sample project Bazel internally runs external/remote_java_tools/java_tools/JavaBuilder_deploy.jar which has the older asm@9.2 and jacoco@0.8.7 packed inside.

I tried configuring the toolchain with NONPREBUILT_TOOLCHAIN_CONFIGURATION but the outcome was the same.

Is there a way to have Bazel use the JavaBuilder that is being built in my source?

[cushon]

Is there a way to have Bazel use the JavaBuilder that is being built in my source?

Yes, there's one more step that's need to use a newer build of JavaBuilder: bazelbuild/java_tools#43 (comment)

[somethingvague]

Apologies for the edits, my branch was incorrect.

If I upgrade both JaCoCo and ASM I can at least get to a test failure rather than a build failure, please see the error below. Interestingly if I upgrade ASM only the test passes and I get a .dat output. I assume that JaCoCo would be unable to interpret Java 19 though so I'm still keen to get the upgrade working.

A script to reproduce the error described in this repo's README. It clones this fork of bazelbuild containing asm@9.3 and jacoco@0.8.8 with build_tools updated

I believe the null pointer passed down the stack is from reading the class name in BranchDetailAnalyzer here but at this point I am guessing. There is quite a lot of Bazel machinery here, I'm wondering how much of it is solving internal use cases and if it's worth us trying to write a vanilla rule which executes jacoco, what do you think?

java.io.IOException: Error while analyzing repro/App.class.uninstrumented.
	at com.google.testing.coverage.BranchDetailAnalyzer.analyzerError(BranchDetailAnalyzer.java:110)
	at com.google.testing.coverage.BranchDetailAnalyzer.analyzeClass(BranchDetailAnalyzer.java:71)
	at com.google.testing.coverage.JacocoCoverageRunner.analyzeBranch(JacocoCoverageRunner.java:209)
	at com.google.testing.coverage.JacocoCoverageRunner.create(JacocoCoverageRunner.java:131)
	at com.google.testing.coverage.JacocoCoverageRunner$2.run(JacocoCoverageRunner.java:568)
Caused by: java.lang.NullPointerException: Cannot invoke "String.equals(Object)" because "owner" is null
	at org.jacoco.core.internal.analysis.filter.AbstractMatcher.nextIsField(AbstractMatcher.java:92)
	at org.jacoco.core.internal.analysis.filter.AssertFilter$Matcher.matchGet(AssertFilter.java:57)
	at org.jacoco.core.internal.analysis.filter.AssertFilter.filter(AssertFilter.java:33)
	at org.jacoco.core.internal.analysis.filter.Filters.filter(Filters.java:59)
	at com.google.testing.coverage.MethodProbesMapper.accept(MethodProbesMapper.java:123)
	at org.jacoco.core.internal.flow.ClassProbesAdapter$2.visitEnd(ClassProbesAdapter.java:91)
	at com.google.testing.coverage.jarjar.org.objectweb.asm.ClassReader.readMethod(ClassReader.java:1518)
	at com.google.testing.coverage.jarjar.org.objectweb.asm.ClassReader.accept(ClassReader.java:744)
	at com.google.testing.coverage.jarjar.org.objectweb.asm.ClassReader.accept(ClassReader.java:424)
	at com.google.testing.coverage.BranchDetailAnalyzer.mapProbes(BranchDetailAnalyzer.java:119)
	at com.google.testing.coverage.BranchDetailAnalyzer.analyzeClass(BranchDetailAnalyzer.java:76)
	at com.google.testing.coverage.BranchDetailAnalyzer.analyzeClass(BranchDetailAnalyzer.java:106)
	at com.google.testing.coverage.BranchDetailAnalyzer.analyzeClass(BranchDetailAnalyzer.java:69)
	... 3 more

[somethingvague]

Any thoughts on this issue please?

[somethingvague]

I see the first PR #17514 was approved even though I have not been able to verify that these versions work with Bazel (see #16412 (comment)). I also notice the PR has unsuccessful checks. Do you have any feedback on those?

[cushon]

Sorry, the mechanics of the upgrade look good, I didn't realize it wasn't passing tests yet.

I am not sure if this is relevant or not, but internally we're using a newer jaococ version than the last tagged release (specifically 4d09eceb478338680ca87c38e8b282035a3b5abd), there are some changes to support JDK 19 that haven't been released yet. We're also using ASM 9.4. I'd be curious to see if using the latest versions makes a difference.

[somethingvague]

somethingvague@2e3f566 on the forked bazel repro branch upgrades jacoco to jacoco/jacoco@4d09ece and ASM 9.4. Unfortunately I get the same error in #16412 (comment) when running on the test project.

Unsure if it's anything to do with the java toolchain or bazel configuration on the test project. Would you mind taking a look please?

[cushon]

I was able to reproduce and have a possible workaround, can you verify this resolves the NPE for you?

diff --git a/src/java_tools/junitrunner/java/com/google/testing/coverage/ClassProbesMapper.java b/src/java_tools/junitrunner/java/com/google/testing/coverage/ClassProbesMapper.java
index 0633e1acf2..25c5d105e4 100644
--- a/src/java_tools/junitrunner/java/com/google/testing/coverage/ClassProbesMapper.java
+++ b/src/java_tools/junitrunner/java/com/google/testing/coverage/ClassProbesMapper.java
@@ -52,7 +52,7 @@ public class ClassProbesMapper extends ClassProbesVisitor implements IFilterCont
   public ClassProbesMapper(String className) {
     classLineToBranchExp = new TreeMap<Integer, BranchExp>();
     stringPool = new StringPool();
-    className = stringPool.get(className);
+    this.className = stringPool.get(className);
   }

   @Override

[somethingvague]

Thanks for this, it resolves the NPE.

I have updated PR #17514 to include JaCoCo v0.8.8, ASM 9.4, + the fix in #16412 (comment).

I have confirmed it works with the subsequent BUILD.java_tools changes and the remote_java_tools repository overridden. Please let me know the next steps.

[somethingvague]

Thanks for merging, PR 2 of 3 raised #17785

[somethingvague]

jdk20 GA release is tomorrow.

Given that the latest JaCoCo snapshot build supports jdk20/21 and that snapshot builds are considered safe, should we instead take the latest snapshot now or would you prefer to do another cycle after this issue is closed out?

The master branch of JaCoCo is automatically built and published. Due to the test driven development approach every build is considered fully functional
https://www.jacoco.org/jacoco/trunk/doc/changes.html

We intend to follow the JDK release cycle, do you think it's reasonable for us to do that and have coverage? Also, do you think the mechanics of the 3 stage change to upgrade JaCoCo could be optimized in future?

[cushon]

Personally I would be fine with trying JaCoCo snapshot builds.

Also, do you think the mechanics of the 3 stage change to upgrade JaCoCo could be optimized in future?

@comius do you have any thoughts on options for streamlining the update process? (The 3-stage process referred to is the one documented in https://github.com/bazelbuild/bazel/blob/28001f6109ede0bd28ec00eb8d1a59d30cdb3bde/third_party/java/jacoco/README.md)

[hvadehra]

streamlining the update process

There is an ongoing effort (by @meteorcloudy ) to empty third_party and fetch the deps from maven:

bazel/WORKSPACE

Line 650 in 2a3ab5c

maven_install(

However, for now this is only being done for dependencies that only require class jars . Doing this for dependencies that also need source jars, while possible, is yet to be figured out. Once that happens though, upgrading versions can happen in a single PR with other changes.

[cushon]

That sounds great, although there may still be some added complexity here since we're carrying patches for modifications to JaCoCo.

[somethingvague]

@hvadehra makes sense, thanks for explaining.

@cushon I was able to verify that Bazel HEAD + JaCoCo 0.8.8 produces coverage data for Java 20 features built with a JDK 20 toolchain, though I'm not confident the coverage data is meaningful. Should we push ahead with upgrading JaCoCo to 0.8.9-SNAPSHOT on #17836? (I've also confirmed this version works)

[meteorcloudy]

@cushon Can you elaborate a bit more on how those patches are applied to the jar files? Do we still need those patches after upgrading to a new JaCoco version? Can we upstream those changes?

Here are the current jar files left in third_party, understanding how they are used will be helpful for me to move them to rules_jvm_external:

fpcloudy@pcloudy-macbookpro3:~/workspace/bazel (master)
$ find third_party | grep "\.jar$"
third_party/asm/asm-analysis-9.4.jar
third_party/asm/asm-tree-9.4-sources.jar
third_party/asm/asm-analysis-9.4-sources.jar
third_party/asm/asm-9.4.jar
third_party/asm/asm-commons-9.4.jar
third_party/asm/asm-util-9.4-sources.jar
third_party/asm/asm-tree-9.4.jar
third_party/asm/asm-9.4-sources.jar
third_party/asm/asm-util-9.4.jar
third_party/asm/asm-commons-9.4-sources.jar
third_party/java/jacoco/org.jacoco.ant-0.8.7-sources.jar
third_party/java/jacoco/org.jacoco.core-0.8.8.jar
third_party/java/jacoco/jacocoagent-0.8.7.jar
third_party/java/jacoco/org.jacoco.core-0.8.7-sources.jar
third_party/java/jacoco/org.jacoco.ant-0.8.8-nodeps.jar
third_party/java/jacoco/org.jacoco.agent-0.8.8-sources.jar
third_party/java/jacoco/org.jacoco.report-0.8.8.jar
third_party/java/jacoco/org.jacoco.agent-0.8.7.jar
third_party/java/jacoco/org.jacoco.ant-0.8.8-sources.jar
third_party/java/jacoco/org.jacoco.agent-0.8.7-sources.jar
third_party/java/jacoco/org.jacoco.ant-0.8.7-nodeps.jar
third_party/java/jacoco/org.jacoco.ant-0.8.8.jar
third_party/java/jacoco/org.jacoco.core-0.8.8-sources.jar
third_party/java/jacoco/org.jacoco.ant-0.8.7.jar
third_party/java/jacoco/org.jacoco.report-0.8.8-sources.jar
third_party/java/jacoco/org.jacoco.report-0.8.7.jar
third_party/java/jacoco/org.jacoco.agent-0.8.8.jar
third_party/java/jacoco/org.jacoco.report-0.8.7-sources.jar
third_party/java/jacoco/org.jacoco.core-0.8.7.jar
third_party/java/jacoco/jacocoagent-0.8.8.jar
third_party/java/proguard/proguard6.2.2/lib/retrace.jar
third_party/java/proguard/proguard6.2.2/lib/proguard.jar
third_party/java/proguard/proguard6.2.2/lib/proguardgui.jar
third_party/java/android_databinding/v3_4_0/exec.jar
third_party/ijar/test/jar-with-manifest.jar
third_party/ijar/test/records/records.jar
third_party/ijar/test/sealed/sealed.jar
third_party/ijar/test/jar-without-manifest.jar
third_party/ijar/test/jar-with-manifest-and-target-label.jar
third_party/ijar/test/libwrongcentraldir.jar
third_party/ijar/test/nestmates/nestmates.jar
third_party/turbine/turbine_direct.jar

[cushon]

Can you elaborate a bit more on how those patches are applied to the jar files?

I would do something like #16412 (comment). I think @comius did the first Bazel update that used those patches in a0dec28 and may have additional context.

Can we upstream those changes?

I tried: jacoco/jacoco#1151

Do we still need those patches after upgrading to a new JaCoco version?

JaCoCo hasn't changed. At this point support for condy should be more ubiquitous, so I think it's worth trying to drop that patch.

[somethingvague]

I've confirmed 0.8.9-SNAPSHOT with updated BUILD.java_tools works against my repro project so I've updated #17836.

Also, I noticed Bazel pre-release 7.0.0-pre.20230316.2 does not include c22f29b updating BUILD.java_tools to JaCoCo 0.8.8, when do you think this might be released?

[somethingvague]

I see that the latest pre-release includes the change. I notice that Bazel 7 is targeted for late 2023 https://bazel.build/about/roadmap#bazel_70_release , is there a chance of this being released in a 6.x version?