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

[bug] fix coverage linker flags when using --@rules_rust//rust/settin… #2836

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Ryang20718
Copy link

@Ryang20718 Ryang20718 commented Sep 5, 2024

When running bazel coverage with --@rules_rust//rust/settings:experimental_use_cc_common_link=true, we fail to generate coverage properly because of some missing linker flags (the c library as well as some missing symbols)

This pr fixes that
resolves #2729

@Ryang20718 Ryang20718 force-pushed the ryang/fix_coverage_cc_link branch 5 times, most recently from 0381262 to 8b53d91 Compare September 5, 2024 17:03
@Ryang20718 Ryang20718 marked this pull request as ready for review September 5, 2024 17:04
Copy link
Collaborator

@UebelAndre UebelAndre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I think there's one change left to get passing CI.

@@ -565,6 +572,8 @@ tasks:
- "//..."
test_targets:
- "//..."
coverage_targets:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you'll need to add coverage_flags here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😓 I thought test_flags are inherited as part of coverage?

on main, without my change, bazel coverage is broken for Build with no_std + alloc using cc_common.link infrastructure for linking

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is true of .bazelrc flags but I don't think bazel CI works that way. Perhaps I'm misinterpreting https://github.com/bazelbuild/continuous-integration

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are there coverage flags? https://github.com/bazelbuild/continuous-integration/blob/master/buildkite/README.md

I don't see any mention of that?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coverage is not very well documented in the Bazel ecosystem. I remember seeing this deep in the python code some time ago but don't remember exactly where.

@Ryang20718 Ryang20718 force-pushed the ryang/fix_coverage_cc_link branch 7 times, most recently from 302dc79 to 880da29 Compare September 9, 2024 19:45
.bazelci/presubmit.yml Show resolved Hide resolved
coverage_link_flags = []
if include_coverage:
# fixes missing symbols coverage fails due to gcc lacking a few linker args
coverage_link_flags = ["-u", "__llvm_profile_runtime"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if you're not using gcc?

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.

Bazel Coverage failing to generate lcov with --@rules_rust//rust/settings:experimental_use_cc_common_link=true
2 participants