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

Add support for split_coverage_post_processing #2000

Merged
merged 1 commit into from
Jul 28, 2023

Conversation

UebelAndre
Copy link
Collaborator

@UebelAndre UebelAndre commented Jun 8, 2023

This change introduces experimental_use_coverage_metadata_files (#2082) which is required to support --experimental_split_coverage_postprocessing'

Changes:

  • Implemented coverage collection logic in Rust.
  • Added a flag --@rules_rust//rust/settings:experimental_use_coverage_metadata_files to toggle the changes necessary for supporting --experimental_split_coverage_postprocessing.
  • Added regression testing in CI to test --experimental_split_coverage_postprocessing.

@UebelAndre
Copy link
Collaborator Author

bazelbuild/bazel#18634 is required as rules_rust currently only supports llvm based coverage and utilizes the same collect_cc_coverage attr hook.

@UebelAndre
Copy link
Collaborator Author

Seeing some odd timeouts on some tests: https://buildkite.com/bazel/rules-rust-rustlang/builds/8718#0188ad81-d635-4fc5-be1a-cb4c1222f476

@illicitonion wondering if you have any ideas as to what might be causing this?

@illicitonion
Copy link
Collaborator

I'm not sure, sorry! Hopefully if you can fetch some logs from the timing out tests there's something useful in there...

@UebelAndre UebelAndre force-pushed the coverage branch 6 times, most recently from 9ca5833 to e943c30 Compare June 14, 2023 03:40
@UebelAndre
Copy link
Collaborator Author

I figured out the issue and am now waiting for bazelbuild/bazel#18634

@UebelAndre UebelAndre force-pushed the coverage branch 2 times, most recently from e686fbf to 440ab13 Compare June 21, 2023 13:27
@UebelAndre
Copy link
Collaborator Author

@illicitonion or @krasimirgg friendly ping 😅

@krasimirgg
Copy link
Collaborator

@UebelAndre I'm looking into this, also reading up and going over the dependencies.
From reading #2000 (comment), I gather this is necessary to support experimental_split_coverage_postprocessing, but from reading
bazelbuild/bazel#18634 (comment), I gather that this isn't necessary?

So is this just a "nice simplification" towards experimental_split_coverage_postprocessing support?

@UebelAndre
Copy link
Collaborator Author

UebelAndre commented Jul 28, 2023

@UebelAndre I'm looking into this, also reading up and going over the dependencies. From reading #2000 (comment), I gather this is necessary to support experimental_split_coverage_postprocessing, but from reading bazelbuild/bazel#18634 (comment), I gather that this isn't necessary?

So is this just a "nice simplification" towards experimental_split_coverage_postprocessing support?

@krasimirgg It turned out it was necessary. If you look at https://github.com/bazelbuild/bazel/blob/6.3.0/src/main/starlark/builtins_bzl/common/cc/cc_binary.bzl#L204 the fix is using internal functionality from cc_common to add additional files to the coverage action. My assumption was that whatever rules_cc did to fix this issue would be usable by rules_rust but once I had time to get back to this problem I found that to be false. So, it's a great coincidence that coverage_common.instrumented_files_info.metadata_files was made public so pure starlark rules can access the test binary in coverage actions. If there's a better way to implement this though I'm happy to change directions. I only care that coverage works with this flag.

@krasimirgg
Copy link
Collaborator

Thanks for clarifying! Looks good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants