Upgrade Jacoco to 0.8.5+ to fix Scala 2.12 lambda code coverage

[gergelyfabian]

ATTENTION! Please read and follow:

• if this is a question about how to build / test / query / deploy using Bazel, or a discussion starter, send it to bazel-discuss@googlegroups.com
• if this is a bug or feature request, fill the form below as best as you can.

Description of the problem / feature request:

When building Scala 2.12 with https://github.com/bazelbuild/rules_scala coverage has a bug (Scala lambdas are not properly detected). This has been tracked down to a bug in Jacoco 0.8.3 + Scala 2.12 (Jacoco did not properly recognize Scala 2.12 lambdas in 0.8.3).
Jacoco 0.8.5 contains a bugfix for this problem.

To fix this issue Jacoco would need to be upgraded from 0.8.3 to 0.8.5 in Bazel.

Feature requests: what underlying problem are you trying to solve with this feature?

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

Reproduction with Bazel:

1. Check out rules_scala repository on master branch.
2. Generate coverage data:
   bazel coverage --extra_toolchains="//scala:code_coverage_toolchain" //test/coverage/...
3. Generated coverage report:
   genhtml -o ${destdir} --ignore-errors source bazel-out/k8-fastbuild/testlogs/test/coverage/test-all/coverage.dat
4. In coverage/D1.scala.gcov.html only the first line is shown as covered

Outside of Bazel:

Download http://search.maven.org/remotecontent?filepath=org/jacoco/jacoco/0.8.5/jacoco-0.8.5.zip and http://search.maven.org/remotecontent?filepath=org/jacoco/jacoco/0.8.3/jacoco-0.8.3.zip.

Take https://github.com/bazelbuild/rules_scala/blob/master/test/coverage/D1.scala and add a main method to it:

  def main(args: Array[String]) {
    D1.veryLongFunctionNameIsHereAaaaaaaaa()
  }

Steps:

# Install Scala 2.12 into ~/opt/scala-2.12.10
# Unpack jacoco archive (in this case 0.8.5).
unzip ~/Downloads/jacoco-0.8.5.zip
mkdir classes
# Added a main method to test/coverage/D1.scala and moved it to src/ folder.
scalac src/D1.scala -d classes
java -javaagent:lib/jacocoagent.jar -cp ~/opt/scala-2.12.10/lib/scala-library.jar:classes D1
java -jar lib/jacococli.jar report jacoco.exec --classfiles classes --sourcefiles src --html report
# Check the Jacoco report (a bit different graphical interface than for LCOV).
firefox report/index.html

1. When using Jacoco 0.8.3:
   a) Missed instructions: 3 of 32
   b) only the first line of veryLongFunctionNameIsHereAaaaaaaaa is covered.
2. When using Jacoco 0.8.5:
   a) Missed instructions: 3 of 295
   b) all lines of veryLongFunctionNameIsHereAaaaaaaaa are covered.

What operating system are you running Bazel on?

Ubuntu 18.04.

What's the output of bazel info release?

release 3.0.0

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

Replace this line with your answer.

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

https://github.com/bazelbuild/rules_scala
056d5921d2c595e7ce2d54a627e8bc68ece7e28d
056d5921d2c595e7ce2d54a627e8bc68ece7e28d

Have you found anything relevant by searching the web?

Rules scala bug report details:
bazelbuild/rules_scala#1056
bazelbuild/rules_scala#1054

Scala 2.12 changes for lambda bytecode:
https://users.scala-lang.org/t/has-a-loop-recurs-comprehension-been-considered-for-scala/4787/21
https://www.scala-lang.org/news/2.12.0/

Jacoco Java lambda support:
https://stackoverflow.com/questions/45674950/jacoco-need-special-handling-fro-lambdas
https://stackoverflow.com/questions/32284326/code-coverage-for-lambda-function
jacoco/jacoco#232

Jacoco Scala lambda fix:
jacoco/jacoco#912
jacoco/jacoco#922

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

More detailed information may be found at:

bazelbuild/rules_scala#1056
bazelbuild/rules_scala#1054

[comius]

JacocoCoverage is attached to java_toolchains. It should be possible to upgrade the version by configuring the toolchain.

[gergelyfabian]

I tried with 0.8.5, generating coverage data works, but I still see the old output (Scala lambdas - for comprehension lines - are not covered).

Here is the example:

https://github.com/gergelyfabian/bazel-scala-example/tree/jacoco_coverage_fix

Steps:

1. tools/JacocoCoverage_jarjar_deploy.jar was built in bazel project, overriding the jacoco jars in third_party/java/jacoco with 0.8.5 (small code fix is necessary in src/java_tools/junitrunner/java/com/google/testing/coverage/BranchDetailAnalyzer.java to remove deprectated Override annotation)
2. In Bazel project run: bazel build src/java_tools/junitrunner/java/com/google/testing/coverage:JacocoCoverage_jarjar_deploy.jar
3. Copy the built file to bazel-scala-example/tools
4. In bazel-scala-example: bazel coverage -t- --extra_toolchains="@io_bazel_rules_scala//scala:code_coverage_toolchain" //...
5. In bazel-scala-example: genhtml -o /tmp/coverage --ignore-errors source bazel-out/k8-fastbuild/testlogs/example-lib/test/coverage.dat

[gergelyfabian]

Now I know the reason is rules_scala is not using the jacocorunner from the Java toolchain, but it has the path to the runner hardcoded.
If I customize rules_scala, to make jacocorunner customizable and provide my definition, then I receive the following error when running coverage:

java.lang.NoClassDefFoundError: org/jacoco/agent/rt/internal_1f1cc91/Offline

It's the same case if I have built JacocoCoverage_jarjar_deploy.jar with jars that I've built from my branch of jacoco (that only has the Scala fixes backported): https://github.com/gergelyfabian/jacoco/tree/0.8.3-scala-lambda-fix.
So I think it's not binary incompatibility, but maybe the way how I built JacocoCoverage_jarjar_deploy.jar missed some files?

If someone could kindly highlight how JacocoCoverage_jarjar_deploy.jar should be properly built from jacoco 0.8.5, 0.8.6 or my jacoco 0.8.3-scala-lambda-fix branch I'd be very grateful.

[gergelyfabian]

Customizable jacocorunner in rules_scala: https://github.com/gergelyfabian/rules_scala/tree/custom_jacocorunner

I guess rules_scala would need a fix, to automatically take jacocorunner from the Java toolchain. CC: @ittaiz

[gergelyfabian]

I managed to make this solution with java_toolchain work.

1. Check out https://github.com/gergelyfabian/jacoco/tree/0.8.3-scala-lambda-fix (has the Scala fixes backported to 0.8.3)
2. Change in org.jacoco.build/pom.xml (Bazel seems to depend on this hard-coded jacoco pkgName):

-                pkgName = buildNumber.substring(buildNumber.length() - 7, buildNumber.length());
+                pkgName = "1f1cc91";

3. Build jacoco (mvn clean install).
4. Uncompress the built jacoco zip file, move and rename jacoco agent jar, org.jacoco.agent jar, org.jacoco.core jar, org.jacoco.report jar to third_party/java/jacoco/org.jacoco.core-0.8.3.jar in Bazel repo.
5. Run in Bazel repo: bazel build src/java_tools/junitrunner/java/com/google/testing/coverage:JacocoCoverage_jarjar_deploy.jar
6. Copy JacocoCoverage_jarjar_deploy.jar to tools/ in this project.
7. Use customized rules_scala from https://github.com/gergelyfabian/rules_scala/tree/custom_jacocorunner as the jacocorunner is hardcoded for the scala_test target (to be able to override that).
8. In bazel-scala-example: bazel coverage -t- --extra_toolchains="@io_bazel_rules_scala//scala:code_coverage_toolchain" //...
9. In bazel-scala-example: genhtml -o /tmp/coverage --ignore-errors source bazel-out/k8-fastbuild/testlogs/example-lib/test/coverage.dat

I believe the only missing part is to extend rules_scala to use jacocorunner from the Java toolchain (to avoid using a forked rules_scala version), or at least to enable scala_test users to override jacocorunner.

[gergelyfabian]

@comius For the rules_scala fix we'd need to do, do you maybe know how to retrieve jacocorunner from java_toolchain with ctx?
We'd like to modify scala_test rule, to automatically take the jacocorunner that has been defined on the java_toolchain.

[ittaiz]

@comius do you know that it's possible or you assume it's possible?
Because what I'm seeing I think you might be wrong.
We (at rules_scala) point to @bazel_tools//tools/jdk:JacocoCoverage which AFAIU actually points to @remote_java_tools//:jacoco_coverage_runner.
IIUC this means that if a rules_scala user overrides their `remote_java_tools with a release that has the needed jacoco (0.8.5 or patched 0.8.3) then that should work.
@gergelyfabian have you tried this already and it failed? If not can you try it and report back?
The java_tools readme has info on how to customize it

[comius]

@comius do you know that it's possible or you assume it's possible?

I'm working on java toolchains. So it's possible to change jacoco_runner on the toolchain, i.e. by defining a custom default_java_toolchain rule and using it.

Problem is that there is currently no Starlark interface so that this setting could be used in Scala directly from the toolchain as it is in Java native rules implementation.

Because what I'm seeing I think you might be wrong.
We (at rules_scala) point to @bazel_tools//tools/jdk:JacocoCoverage which AFAIU actually points to @remote_java_tools//:jacoco_coverage_runner.
IIUC this means that if a rules_scala user overrides their `remote_java_tools with a release that has the needed jacoco (0.8.5 or patched 0.8.3) then that should work.

Having java_tools is an alternative that also works. If somebody prepares it in a bazel branch, I could release it. I'm a bit reluctant to update it on the main branch, because Jacoco 0.8.3 is used internally and changing it is far from trivial. But I'm also reluctant to have an extra branch. But this seems to be the easiest/most usable option at the moment.

@gergelyfabian have you tried this already and it failed? If not can you try it and report back?
The java_tools readme has info on how to customize it

[gergelyfabian]

If somebody prepares it in a bazel branch, I could release it.

@comius, Do you mean:

Starlark interface so that this setting could be used in Scala directly from the toolchain

?

[gergelyfabian]

@gergelyfabian have you tried this already and it failed? If not can you try it and report back?
The java_tools readme has info on how to customize it

@ittaiz I'm not totally sure how I could do that if I already use a solution similar to this one: https://github.com/liucijus/bazel-scala211-jdk8

Can I use a custom java_tool (with custom Jacoco runner) if I also need to ensure my Java toolchain is running on JVM 8?

[ittaiz]

Problem is that there is currently no Starlark interface so that this setting could be used in Scala directly from the toolchain as it is in Java native rules implementation.

How big would that a change be? Exposing to starlark that is. Would you be open to it?

[ittaiz]

Can I use a custom java_tool (with custom Jacoco runner) if I also need to ensure my Java toolchain is running on JVM 8?

I think there should be. I haven't dived into the java_tools zip and so I don't know what exactly is java 8 related.
@comius maybe you know?

[gergelyfabian]

Latest proposal for rules_scala is to enable providing jacocorunner on scala_toolchain (similarly as it can be given for java_toolchain). This would be the way to configure jacocorunner until java_toolchain receives an API to be able to query its settings from Starlark.

Implemented in: bazelbuild/rules_scala#1172

CC: @ittaiz

[gergelyfabian]

Btw. I found out that Jacoco e.g. 0.8.7 seems to be binary incompatible with current Bazel (3.7.1 I checked).
There seems to be a conflict with the asm version used by Jacoco and Bazel. A patched Jacoco 0.8.3 works.

[ittaiz]

@gergelyfabian how big is the jacoco jar? Maybe we should commit the patched jar to rules_scala and have that be the default? If it's not too big maybe you should write in one of the relating issues the full repro and Vaidas or me will commit it (given it's binary maybe it's better it will be done by us)

[gergelyfabian]

I think this may be a good idea. I have a script that I use to generate the jacocorunner jar, so you could use that to build it and then upload it to rules_scala. FYI: I use the same script to create a different version as I have gone further and started applying fixes for Scala itself in Jacoco (e.g branch coverage for case class generated methods and fixes for Jacoco LCOV Formatter in java_tools to respect Jacoco filtering for branch coverage). I could contribute the script to rules_scala and you could use it according to your wishes. Ittai Zeidman <notifications@github.com> ezt írta (időpont: 2020. dec. 19., Szo 6:34):

…

@gergelyfabian <https://github.com/gergelyfabian> how big is the jacoco jar? Maybe we should commit the patched jar to rules_scala and have that be the default? If it's not too big maybe you should write in one of the relating issues the full repro and Vaidas or me will commit it (given it's binary maybe it's better it will be done by us) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11674 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABJGRRJRBUYG5NIOV3BOITTSVQ3OLANCNFSM4OL4D2OA> .

[gergelyfabian]

This script could be also used by anyone to generate their own version of jacocorunner and use it in their workspace (with any further fixes they wish). Fábián Gergely <gergo.fb@gmail.com> ezt írta (időpont: 2020. dec. 19., Szo 8:32):

…

I think this may be a good idea. I have a script that I use to generate the jacocorunner jar, so you could use that to build it and then upload it to rules_scala. FYI: I use the same script to create a different version as I have gone further and started applying fixes for Scala itself in Jacoco (e.g branch coverage for case class generated methods and fixes for Jacoco LCOV Formatter in java_tools to respect Jacoco filtering for branch coverage). I could contribute the script to rules_scala and you could use it according to your wishes. Ittai Zeidman ***@***.***> ezt írta (időpont: 2020. dec. 19., Szo 6:34): > @gergelyfabian <https://github.com/gergelyfabian> how big is the jacoco > jar? Maybe we should commit the patched jar to rules_scala and have that be > the default? If it's not too big maybe you should write in one of the > relating issues the full repro and Vaidas or me will commit it (given it's > binary maybe it's better it will be done by us) > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#11674 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/ABJGRRJRBUYG5NIOV3BOITTSVQ3OLANCNFSM4OL4D2OA> > . >