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.

Sign up for GitHub

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

Jump to bottom

Allow Java coverage collection for external targets #16268

Closed
wants to merge 1 commit into from
Closed

Allow Java coverage collection for external targets #16268

wants to merge 1 commit into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Sep 13, 2022 •
edited
Loading

  • Removes a hardcoded filter that results in the LCOV merger filtering
    out coverage of external targets even if these are matched by
    --instrumentation_filter.
  • Lets LazyWritePathsFileAction write out exec rather than
    root-relative paths, as advertised by its javadocs, so that the paths
    match those that the LCOV mergers filters by.

Work towards #16228
Work towards #16208

@fmeum
Allow Java coverage collection for external targets
7f7d616 
* Removes a hardcoded filters that result in the LCOV merger filtering
  out coverage of external targets even if these are matched by
  `--instrumentation_filter`.
* Lets `LazyWritePathsFileAction` write out exec rather than
  root-relative paths, as advertised by its javadocs, so that the paths
  match those that the LCOV mergers filters by.
@fmeum fmeum force-pushed the 16228-external-coverage-java branch from 29fd88f to 7f7d616 Compare October 12, 2022 22:03
@fmeum fmeum changed the title WIP: Allow Java coverage collection for external targets Allow Java coverage collection for external targets Oct 12, 2022
@fmeum fmeum marked this pull request as ready for review October 12, 2022 22:12
@fmeum fmeum requested a review from lberki as a code owner October 12, 2022 22:12
@ShreeM01 ShreeM01 added team-Rules-Server Issues for serverside rules included with Bazel awaiting-review PR is awaiting review from an assigned reviewer labels Oct 12, 2022
@lberki lberki requested review from c-mita and removed request for lberki October 13, 2022 06:47
@fmeum
Copy link
Collaborator Author

fmeum commented Oct 25, 2022

@c-mita Friendly ping

@fmeum
Copy link
Collaborator Author

fmeum commented Oct 25, 2022

@bazel-io flag

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Oct 25, 2022
@ShreeM01
Copy link
Contributor

ShreeM01 commented Nov 2, 2022

Hello @c-mita, As I could see we are good to merge the PR, does it require any further review? Thanks!

@meteorcloudy
Copy link
Member

@bazel-io fork 6.0.0

@bazel-io bazel-io mentioned this pull request Nov 2, 2022
Closed
@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Nov 2, 2022
@c-mita c-mita added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Nov 2, 2022
@c-mita
Copy link
Member

c-mita commented Nov 2, 2022

Hello @c-mita, As I could see we are good to merge the PR, does it require any further review? Thanks!

Just forgot to update the labels.

@copybara-service copybara-service bot closed this in 8f95651 Nov 2, 2022
@sgowroji sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Nov 2, 2022
@fmeum fmeum deleted the 16228-external-coverage-java branch November 2, 2022 19:52
fmeum added a commit to fmeum/bazel that referenced this pull request Nov 2, 2022
@fmeum
Allow Java coverage collection for external targets
6871da3 
* Removes a hardcoded filter that results in the LCOV merger filtering
  out coverage of external targets even if these are matched by
  `--instrumentation_filter`.
* Lets `LazyWritePathsFileAction` write out exec rather than
  root-relative paths, as advertised by its javadocs, so that the paths
  match those that the LCOV mergers filters by.

Work towards bazelbuild#16228
Work towards bazelbuild#16208

Closes bazelbuild#16268.

PiperOrigin-RevId: 485580267
Change-Id: I830d3cf903462ca1799ef5f659a620dbdb92f349
@fmeum fmeum mentioned this pull request Nov 2, 2022
Merged
ShreeM01 pushed a commit that referenced this pull request Nov 2, 2022
@fmeum
Allow Java coverage collection for external targets (#16637)
acbb9e1 
* Removes a hardcoded filter that results in the LCOV merger filtering
  out coverage of external targets even if these are matched by
  `--instrumentation_filter`.
* Lets `LazyWritePathsFileAction` write out exec rather than
  root-relative paths, as advertised by its javadocs, so that the paths
  match those that the LCOV mergers filters by.

Work towards #16228
Work towards #16208

Closes #16268.

PiperOrigin-RevId: 485580267
Change-Id: I830d3cf903462ca1799ef5f659a620dbdb92f349
@ShreeM01
Copy link
Contributor

ShreeM01 commented Nov 2, 2022

merged in #16637

@meteorcloudy
Copy link
Member

@kshyanashree Unfortunately, this PR was rolled back in 4caae75 at HEAD, can you please also cherry pick the rollback of this change?

@meteorcloudy
Copy link
Member

@bazel-io fork 6.0.0

@bazel-io bazel-io mentioned this pull request Nov 14, 2022
Closed
@ShreeM01
Copy link
Contributor

@kshyanashree Unfortunately, this PR was rolled back in 4caae75 at HEAD, can you please also cherry pick the rollback of this change?

Sure @meteorcloudy! I will do that.

@fmeum
Copy link
Collaborator Author

fmeum commented Nov 14, 2022

@meteorcloudy @susinmotion Is there any more context on the nature of the breakage this causes your could provide me with?

@meteorcloudy
Copy link
Member

@c-mita @hvadehra Can you help provide a minimal reproducible case for the breakage?

@c-mita
Copy link
Member

c-mita commented Nov 15, 2022

Unfortunately not at the moment; we're not really sure why there's a failure.

It might just be a bad test, but unfortunately we won't not be in a position to investigate in detail until after BazelCon.

@fmeum
Copy link
Collaborator Author

fmeum commented Jan 24, 2023

@c-mita Friendly ping, did you get a chance to investigate the failure? If you can share details about the failing test, I can also look into it.

@c-mita
Copy link
Member

c-mita commented Jan 24, 2023

An internal rule also makes use of LazyWritePathsFileAction. I've only looked through the rule, but it seems to make use of this:
https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/analysis/actions/Substitution.java;l=230
The rule in question may be the only user of this).

I would guess this PR would also have to change this, but it's probably best I handle it on our side.

@fmeum
Copy link
Collaborator Author

fmeum commented Jan 24, 2023

Thanks for looking into it. Let me know if I can help with the roll-forward, but I agree its probably easier if you do it and fix the rule usage in the same CL. I can't reopen the current PR, but feel free to use it for that.

@c-mita
Copy link
Member

c-mita commented Jan 26, 2023

My guess was wrong; there's simply another lump of internal code that assumes these written to the file are root-relative.

Interestingly, when consuming this file, it then appears to stick the exec path in front, so I think it could be simplified and this PR could be accepted as is afterwards, but making the required change is not proving straightforward.

copybara-service bot pushed a commit that referenced this pull request Jan 30, 2023
@c-mita @copybara-github
Allow Java coverage collection for external targets.
00e9af1 
A roll-forward of #16268.

LazyWritePathsFileAction has been changed to accept a custom converter
parameter that can be used to specify exactly what path should be
written out to the file. This is required to support the use case of an
internal rule that still needs root-relative paths written out.

The default case (when no converter is specified) is to output the exec
path, as per the original PR.

PiperOrigin-RevId: 505678128
Change-Id: Ib1d8547dcb359a9e0fb7e462850424ec4218f7fb
@c-mita
Copy link
Member

c-mita commented Jan 30, 2023

It seems the internal rule really does require the old behaviour to preserved. As a compromise, I've adjusted LazyWritePathsFileAction to take an optional converter parameter. By default it writes out the exec path.

00e9af1 is the "roll-forward" with this change.

fmeum pushed a commit to fmeum/bazel that referenced this pull request Jan 30, 2023
@c-mita @fmeum
Allow Java coverage collection for external targets.
5f0e74b 
A roll-forward of bazelbuild#16268.

LazyWritePathsFileAction has been changed to accept a custom converter
parameter that can be used to specify exactly what path should be
written out to the file. This is required to support the use case of an
internal rule that still needs root-relative paths written out.

The default case (when no converter is specified) is to output the exec
path, as per the original PR.

PiperOrigin-RevId: 505678128
Change-Id: Ib1d8547dcb359a9e0fb7e462850424ec4218f7fb
@fmeum fmeum mentioned this pull request Jan 30, 2023
Merged
ShreeM01 pushed a commit that referenced this pull request Jan 30, 2023
@fmeum @c-mita
Allow Java coverage collection for external targets. (#17360)
40d83a7 
A roll-forward of #16268.

LazyWritePathsFileAction has been changed to accept a custom converter
parameter that can be used to specify exactly what path should be
written out to the file. This is required to support the use case of an
internal rule that still needs root-relative paths written out.

The default case (when no converter is specified) is to output the exec
path, as per the original PR.

PiperOrigin-RevId: 505678128
Change-Id: Ib1d8547dcb359a9e0fb7e462850424ec4218f7fb

Co-authored-by: Googler <[email protected]>
hvadehra pushed a commit that referenced this pull request Feb 14, 2023
@c-mita @hvadehra
Allow Java coverage collection for external targets.
2d9f925 
A roll-forward of #16268.

LazyWritePathsFileAction has been changed to accept a custom converter
parameter that can be used to specify exactly what path should be
written out to the file. This is required to support the use case of an
internal rule that still needs root-relative paths written out.

The default case (when no converter is specified) is to output the exec
path, as per the original PR.

PiperOrigin-RevId: 505678128
Change-Id: Ib1d8547dcb359a9e0fb7e462850424ec4218f7fb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@c-mita c-mita c-mita approved these changes

Assignees
No one assigned
Labels
team-Rules-Server Issues for serverside rules included with Bazel
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@fmeum @ShreeM01 @meteorcloudy @c-mita @bazel-io @sgowroji