Fix split post-processing of LLVM-based coverage #18634
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fmeum
force-pushed
the
fix-sandboxed-llvm-cov
branch
from
000b054
to
dc7504a
Compare
fmeum
changed the title
fmeum
force-pushed
the
fix-sandboxed-llvm-cov
branch
8 times, most recently
from
166de10
to
bb6706f
Compare
fmeum
force-pushed
the
fix-sandboxed-llvm-cov
branch
4 times, most recently
from
d166f8c
to
54bd646
Compare
`--experimental_split_coverage_postprocessing` now works in combination with `--experimental_generate_llvm_lcov`, which required adding the runtime objects as inputs to the separate coverage post-processing spawn. Previously, they were only added to the runfiles of the test action, which aren't (and shouldn't) be added to the coverage spawn. As a side effect, this provides a more natural fix for bazelbuild#15121 than bb6f1a7, which added an additional check to only add those runtime objects to the `llvm-cov` command line that were present in runfiles. Now, since all runtime objects are staged as inputs, they are known to be available. This improves coverage accuracy when e.g. runtime objects are packaged into jars and loaded at runtime.
fmeum
force-pushed
the
fix-sandboxed-llvm-cov
branch
from
54bd646
to
68a467d
Compare
github-actions
bot
added
awaiting-review
|
@oquenchil Could you review this PR? @UebelAndre noticed the problem when trying to implement support for |
|
@UebelAndre With this fix, would |
I don’t think it would be necessary but we are currently adding |
|
@oquenchil friendly ping. It'd be great if this could get in for the 6.3 release. |
|
@c-mita another friendly ping. I'm really hoping to have this in for 6.3 to fix both llvm based Cc coverage and to support Rust coverage (which uses llvm). |
|
@c-mita Awesome! Anything else blocking the merge? |
|
Hi @c-mita, Since I can see that this PR has been approved, please let me know whether I should proceed with importing it. |
|
@bazel-io flag |
bazel-io
added
the
potential release blocker
c-mita
added
awaiting-PR-merge
Yes please; forgot to set the tags again... |
|
@bazel-io fork 6.3.0 |
bazel-io
removed
the
potential release blocker
github-actions
bot
removed
the
awaiting-PR-merge
iancha1992
pushed a commit
to iancha1992/bazel
that referenced
this pull request
Jun 21, 2023
`--experimental_split_coverage_postprocessing` now works in combination with `--experimental_generate_llvm_lcov`, which required adding the runtime objects as inputs to the separate coverage post-processing spawn. Previously, they were only added to the runfiles of the test action, which aren't (and shouldn't) be added to the coverage spawn. As a side effect, this provides a more natural fix for bazelbuild#15121 than bb6f1a7, which added an additional check to only add those runtime objects to the `llvm-cov` command line that were present in runfiles. Now, since all runtime objects are staged as inputs, they are known to be available. This improves coverage accuracy when e.g. runtime objects are packaged into jars and loaded at runtime. Along the way the common setup for LLVM-based coverage tests has been extracted into a helper function and the tests have been updated to work with a broader range of clang versions. Previously, tests weren't run at all due to the clang version on the runners not falling into the restrictive range. Now, the tests are executed on the Ubuntu 20.04 runner (but not on any other runners, including macOS). Closes bazelbuild#18634. PiperOrigin-RevId: 542055078 Change-Id: Id8851f8685bb7724891acca395bb91556ee2b29a
iancha1992
added a commit
that referenced
this pull request
Jun 21, 2023
`--experimental_split_coverage_postprocessing` now works in combination with `--experimental_generate_llvm_lcov`, which required adding the runtime objects as inputs to the separate coverage post-processing spawn. Previously, they were only added to the runfiles of the test action, which aren't (and shouldn't) be added to the coverage spawn. As a side effect, this provides a more natural fix for #15121 than bb6f1a7, which added an additional check to only add those runtime objects to the `llvm-cov` command line that were present in runfiles. Now, since all runtime objects are staged as inputs, they are known to be available. This improves coverage accuracy when e.g. runtime objects are packaged into jars and loaded at runtime. Along the way the common setup for LLVM-based coverage tests has been extracted into a helper function and the tests have been updated to work with a broader range of clang versions. Previously, tests weren't run at all due to the clang version on the runners not falling into the restrictive range. Now, the tests are executed on the Ubuntu 20.04 runner (but not on any other runners, including macOS). Closes #18634. PiperOrigin-RevId: 542055078 Change-Id: Id8851f8685bb7724891acca395bb91556ee2b29a Co-authored-by: Fabian Meumertzheim <fabian@meumertzhe.im>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
--experimental_split_coverage_postprocessingnow works in combination with--experimental_generate_llvm_lcov, which required adding the runtime objects as inputs to the separate coverage post-processing spawn. Previously, they were only added to the runfiles of the test action, which aren't (and shouldn't) be added to the coverage spawn.As a side effect, this provides a more natural fix for #15121 than bb6f1a7, which added an additional check to only add those runtime objects to the
llvm-covcommand line that were present in runfiles. Now, since all runtime objects are staged as inputs, they are known to be available. This improves coverage accuracy when e.g. runtime objects are packaged into jars and loaded at runtime.Along the way the common setup for LLVM-based coverage tests has been extracted into a helper function and the tests have been updated to work with a broader range of clang versions. Previously, tests weren't run at all due to the clang version on the runners not falling into the restrictive range. Now, the tests are executed on the Ubuntu 20.04 runner (but not on any other runners, including macOS).