Pass the name of the classpath manifest jar to JacocoCoverageRunner

[c-mita]

When the java classpath exceeds the limit, we create a manifest JAR
and pass that on the classpath. JacocoCoverageRunner knows how to
extract information from this JAR. But if another JAR ends up on that
classpath, it confuses the coverage runner which is expecting only a
single jar in this case.

This changes the java_stub_template file to export the name of the
created manifest jar so the coverage runner can extract it.

We also change the template file so the relevant exports don't occur
in the middle of a larger function.

It's possible this is somewhat overengineered and that we could
always rely on the manifest jar always being the first one discovered
by the coverage runner, but it is not totally obvious to me that that
will always be true.

Fixes #21268

[c-mita]

Do we need to handle version-skew between Bazel and java_tools? If so, we may need to add a bit more logic to JacocoCoverageRunner?

[hvadehra]

Do we need to handle version-skew between Bazel and java_tools? If so, we may need to add a bit more logic to JacocoCoverageRunner?

Yes, I think we need to. java_tools / rules_java upgrades happen more frequently than bazel upgrades.

[c-mita]

Do we need to handle version-skew between Bazel and java_tools? If so, we may need to add a bit more logic to JacocoCoverageRunner?

Yes, I think we need to. java_tools / rules_java upgrades happen more frequently than bazel upgrades.

Done.

It's not a tested path since the Bazel shell tests always use a fresh build, but I have verified that it should work.

[hvadehra]

Just a quick clarification before I properly review, my understanding is this change is currently doing two things:

1. Export the manifest jar path from the stub template and start using that in JacocoCoverageRunner if set.
2. If the above does not apply (i.e. for Bazel versions not including this change), stop assuming there is only one URL and try to identify the jar from the first URL.

Why are we doing (2)? If we think that fixes things, then we don't need (1). My concern is we're subtly altering behavior for currently released Bazel on a java_tools upgrade. Can we not just do (1) and cherry pick that into 7.1?

[c-mita]

Just a quick clarification before I properly review, my understanding is this change is currently doing two things:

1. Export the manifest jar path from the stub template and start using that in JacocoCoverageRunner if set.
2. If the above does not apply (i.e. for Bazel versions not including this change), stop assuming there is only one URL and try to identify the jar from the first URL.

Why are we doing (2)? If we think that fixes things, then we don't need (1). My concern is we're subtly altering behavior for currently released Bazel on a java_tools upgrade. Can we not just do (1) and cherry pick that into 7.1?

Whilst I may suspect that 2. fixes things in all cases, I cannot prove it. Hence 1.

Just doing 1. breaks things if java_tools is ahead of Blaze, since we're looking for an environment variable that isn't being set and ignoring the one that is. If that's not a problem then 2. is not needed, which is what I originally asked.

[hvadehra]

Sorry, I guess I misunderstood your question about version skew. I meant that in JacocoCoverageRunner we can't assume the new variable will be available/exported.

Assuming we can get this into 7.1, I think it's safer to leave the old behavior as is if the variable isn't set. This should be acceptable as after all, this is long standing (buggy) behavior and not a regression as such. WDYT?

[c-mita]

I don't really get the rationale for that and it doesn't simplify the logic at all. The diff is simply:

@@ -380,7 +380,7 @@ public class JacocoCoverageRunner {
         System.err.println("Classpath JAR " + wrappedJar + " not provided");
         return null;
       }
-    } else if (jarIsWrapped && urls[0].getPath().endsWith("-classpath.jar")) {
+    } else if (jarIsWrapped && urls.length == 1) {
       classPathUrl = urls[0];
     }
     if (classPathUrl != null) {

Regardless, I've done it.

[hvadehra]

Lets not disable the tests. For the new test case that currently only works for java_tools built at HEAD, maybe exit early if JAVA_TOOLS == 'released"?

Also maybe add a TODO to clean up the legacy logic in java_stub_template.txt when we update to a new java_tools version that includes this change?

[c-mita]

Lets not disable the tests. For the new test case that currently only works for java_tools built at HEAD, maybe exit early if JAVA_TOOLS == 'released"?

Also maybe add a TODO to clean up the legacy logic in java_stub_template.txt when we update to a new java_tools version that includes this change?

Good point on the test; done.

[hvadehra]

@bazel-io fork 7.1.0