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

Allow C/C++ coverage collection for external targets #16261

Closed
wants to merge 2 commits into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Sep 13, 2022

Removes hardcoded filters that result in no coverage being reported for cc_* targets in external repositories even when matched by the --instrumentation_filter.

Work towards #16228
Work towards #16208

@fmeum fmeum changed the title WIP: Allow coverage collection for external targets WIP: Allow C/C++ coverage collection for external targets Sep 13, 2022
@fmeum fmeum changed the title WIP: Allow C/C++ coverage collection for external targets Allow C/C++ coverage collection for external targets Sep 13, 2022
@fmeum
Copy link
Collaborator Author

fmeum commented Sep 13, 2022

@c-mita Would you be available to review this?

The test against coverage tools seems to fail because the coverage output differs from that with the coverage tools at HEAD (the latter has branch coverage). Please let me know if you have an idea how to bridge that gap.

@fmeum fmeum marked this pull request as ready for review September 13, 2022 18:30
@fmeum fmeum requested a review from lberki as a code owner September 13, 2022 18:30
@ShreeM01 ShreeM01 added coverage team-Rules-CPP Issues for C++ rules awaiting-user-response Awaiting a response from the author labels Sep 13, 2022
@fmeum
Copy link
Collaborator Author

fmeum commented Sep 14, 2022

@kshyanashree I see you assigned awaiting-user-response rather than awaiting-review, is there anything I should clarify?

@sgowroji
Copy link
Member

Hello @fmeum, Above buildkite checks are getting failed. Could you please fix them, so that we can change it back to awaiting-review. Even after retrying the checks, they are still getting failed. Thanks!

@fmeum
Copy link
Collaborator Author

fmeum commented Sep 14, 2022

@sgowroji Yes, there are conceptual problems with the test that I have asked @c-mita about in #16261 (comment). I think that these are best solved as part of the review.

@sgowroji
Copy link
Member

@fmeum Thank you so much for responding.

@sgowroji sgowroji added awaiting-review PR is awaiting review from an assigned reviewer and removed awaiting-user-response Awaiting a response from the author labels Sep 14, 2022
@lberki
Copy link
Contributor

lberki commented Sep 14, 2022

@c-mita can you take a look? This is coverage collection about which you know more than I do.

@lberki lberki requested a review from c-mita September 14, 2022 07:42
@c-mita
Copy link
Member

c-mita commented Sep 14, 2022

@c-mita Would you be available to review this?

The test against coverage tools seems to fail because the coverage output differs from that with the coverage tools at HEAD (the latter has branch coverage). Please let me know if you have an idea how to bridge that gap.

There seems to be more to the failure than that; the results indicate that no output is being generated at all for the llvm tests in 18.04.

I'm not familiar with the llvm side of coverage; I don't fully understand how the tools come together here and what flags are available to control behaviour. With gcov, branch coverage has to be specifically enabled but it doesn't seem like that's an option here. Perhaps there was a change in behaviour across versions where branch coverage is only emitted for later versions?

If it's controllable with a flag then I would suggest disabling it for now purely for the sake of consistency with gcov.

@celkas
Copy link

celkas commented Sep 29, 2022

@fmeum @c-mita Thanks for Your efforts so far. What can be done to make this happen?

@fmeum
Copy link
Collaborator Author

fmeum commented Sep 30, 2022

@c-mita I fixed the tests by copying the gcov/llvm-cov setup from other test cases. I'm a bit worried that the tests are skipped instead of running, but if so, that would be the case for basically all other coverage tests.

@c-mita
Copy link
Member

c-mita commented Oct 4, 2022

I don't really know how to avoid that; different versions of the tools often have their little differences and it's a lot of work to keep track of and support them all.

@fmeum fmeum force-pushed the 16228-external-coverage branch 3 times, most recently from 97971cb to 21d0f13 Compare October 7, 2022 20:43
Removes hardcoded filters that result in no coverage being reported
for `cc_*` targets in external repositories even when matched by the
`--instrumentation_filter`.
@fmeum
Copy link
Collaborator Author

fmeum commented Oct 25, 2022

@c-mita Friendly ping

@c-mita c-mita self-assigned this Oct 25, 2022
@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
@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 Oct 25, 2022
@sgowroji sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Oct 26, 2022
@fmeum fmeum deleted the 16228-external-coverage branch October 26, 2022 16:16
fmeum added a commit to fmeum/bazel that referenced this pull request Oct 26, 2022
Removes hardcoded filters that result in no coverage being reported for `cc_*` targets in external repositories even when matched by the `--instrumentation_filter`.

Work towards bazelbuild#16228
Work towards bazelbuild#16208

Closes bazelbuild#16261.

PiperOrigin-RevId: 483911568
Change-Id: Ibca6fb39a8e946e06beeb03414ad8ccbc42f53d6
ShreeM01 pushed a commit that referenced this pull request Oct 26, 2022
Removes hardcoded filters that result in no coverage being reported for `cc_*` targets in external repositories even when matched by the `--instrumentation_filter`.

Work towards #16228
Work towards #16208

Closes #16261.

PiperOrigin-RevId: 483911568
Change-Id: Ibca6fb39a8e946e06beeb03414ad8ccbc42f53d6
@celkas
Copy link

celkas commented Oct 26, 2022

Thank You, @fmeum !!!

@meteorcloudy meteorcloudy removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Dec 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
coverage team-Rules-CPP Issues for C++ rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants