Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Java] Coverage not collected for some manifest jars #21268

Closed
ckilian867 opened this issue Feb 8, 2024 · 5 comments
Closed

[Java] Coverage not collected for some manifest jars #21268

ckilian867 opened this issue Feb 8, 2024 · 5 comments
Assignees
Labels
coverage P1 I'll work on this now. (Assignee required) team-Rules-Java Issues for Java rules type: bug

Comments

@ckilian867
Copy link

Description of the bug:

When the classpath of a test is over the CLASSPATH_LIMIT, Bazel will create a manifest jar that contains a manifest with an entry of the classpath.

The JacocoCoverageRunner accounts for this and will undo the process to find the classpath during coverage collection. However, one of the checks (urls.length == 1) assumes that the manifest jar is the only jar on the classpath.

If a test has a jar in its data, it can also be added to the classpath after the manifest jar. Since the length of the array is greater than 1, the logic is skipped and the coverage collection file is empty.

We were able to resolve this by adding a patch to our fork, but the solution depends on the naming convention of the manifest jar from the stub template and also the ordering of the classpath, which may not be the most robust/futureproof solution. I wanted to raise this issue in case Bazel team had a better way to resolve it.

Which category does this issue belong to?

No response

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

Created reproduction: https://github.com/ckilian867/bazel-coverage-bug

See README for details

Which operating system are you running Bazel on?

PRETTY_NAME="Debian GNU/Linux 10 (buster)" NAME="Debian GNU/Linux" VERSION_ID="10" VERSION="10 (buster)" VERSION_CODENAME=buster ID=debian HOME_URL="https://www.debian.org/" SUPPORT_URL="https://www.debian.org/support" BUG_REPORT_URL="https://bugs.debian.org/"

What is the output of bazel info release?

$ bazel info release release 7.0.0

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 HEAD ?

$ git remote get-url origin; git rev-parse HEAD
https://github.com/ckilian867/bazel-coverage-bug.git
a1570f38065822f548cd279eb7e3bf75257d8a59


### Is this a regression? If yes, please try to identify the Bazel commit where the bug was introduced.


_No response_

### Have you found anything relevant by searching the web?

https://github.com/bazelbuild/bazel/issues/11847

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

_No response_
@fmeum
Copy link
Collaborator

fmeum commented Feb 8, 2024

An idea from the sidelines: Since the stub script knows best whether it created a manifest jar, it could pass along its path as an environment variable or Java system property.

@sgowroji sgowroji added the team-Rules-Java Issues for Java rules label Feb 9, 2024
@hvadehra hvadehra added P1 I'll work on this now. (Assignee required) and removed untriaged labels Feb 9, 2024
@hvadehra
Copy link
Member

hvadehra commented Feb 9, 2024

assigning to @c-mita as this is from 94eeea6

@c-mita
Copy link
Member

c-mita commented Feb 13, 2024

An idea from the sidelines: Since the stub script knows best whether it created a manifest jar, it could pass along its path as an environment variable or Java system property.

Since the original solution already depends on an environment variable being passed along ($JACOCO_IS_JAR_WRAPPED), it could probably just pass the name of the wrapped jar instead.

c-mita added a commit to c-mita/bazel that referenced this issue Feb 15, 2024
bazel-io pushed a commit to bazel-io/bazel that referenced this issue Feb 19, 2024
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 bazelbuild#21268

Closes bazelbuild#21365.

PiperOrigin-RevId: 608333782
Change-Id: I9895689fd9d771c9198e36bef222a9f86ada573e
github-merge-queue bot pushed a commit that referenced this issue Feb 20, 2024
…Runner (#21413)

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

Closes #21365.

Commit
a2ebdf7

PiperOrigin-RevId: 608333782
Change-Id: I9895689fd9d771c9198e36bef222a9f86ada573e

Co-authored-by: Charles Mita <cmita@google.com>
@iancha1992
Copy link
Member

A fix for this issue has been included in Bazel 7.1.0 RC1. Please test out the release candidate and report any issues as soon as possible. Thanks!

@ckilian867
Copy link
Author

ckilian867 commented Feb 27, 2024

Can confirm that this fix works with our repository. Thanks all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
coverage P1 I'll work on this now. (Assignee required) team-Rules-Java Issues for Java rules type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants