-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Fix Java coverage collection with Java 8 runtime #17338
Conversation
7ea67a4
to
b34bd8c
Compare
`JacocoRunner` is run with the target runtime, not the exec runtime, and thus needs to be compatible with a Java 8 runtime.
b34bd8c
to
bdf23de
Compare
I confirmed locally that the issue is fixed. Setting up an integration test for this would require JDK 8 to be available in CI, which as far as I can tell isn't available on most CI runners anymore. |
@cushon Could you review? |
This makes sense to me. I'm not sure how owns this stuff, @comius is there anyone you want to loop in for this? |
@bazel-io flag |
@bazel-io fork 6.1.0 |
LGTM from my side, but WANT_LGTM=c-mita also |
@c-mita just in case |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Optional<ImmutableSet>::isEmpty
was confusing anyway.
`JacocoCoverageRunner` is run with the target runtime, not the exec runtime, and thus needs to be compatible with a Java 8 runtime. Fixes #17165 Closes #17338. PiperOrigin-RevId: 509123073 Change-Id: I41d7255e1f3c3dd3864082899d5dc9ab2cc82027 Co-authored-by: Fabian Meumertzheim <[email protected]>
JacocoCoverageRunner
is run with the target runtime, not the exec runtime, and thus needs to be compatible with a Java 8 runtime.Fixes #17165