-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Provide the default lcov_merger to all Starlark tests #13983
Conversation
5d3508b
to
737ad9d
Compare
Since 5b216b2, all Starlark-defined test rules either need to set the `_lcov_merger` attribute or fail if run with `bazel coverage`. This is fixed by attaching the default lcov merger as a late-bound default to all Starlark rules via a new attribute and falling back to that attribute in the TestActionBuilder. This commit ensures backwards compatibility is maintained with existing Starlark test rules that set the `_lcov_merger` attribute. This is also verified in the included integration test. Fixes bazelbuild#13978.
737ad9d
to
43191ee
Compare
@oquenchil Would you be able to take a look? The issue this resolves is labeled with team-Rules-CPP. |
@c-mita Can you help review this change? |
Unfortunately, I can't accept this PR as is. I think the best (only?) way to handle this at the moment is by changing Bazel's |
@c-mita I see the problem. While your proposed solution does resolve the linked issue, it would mean that there would still be no way for Starlark tests to access the merge as a late-bound attribute. How about the following path forward:
|
I'm ok with this approach; I've got a change ready to unblock the build. |
Since 5b216b2, all Starlark-defined test rules either need to set the
_lcov_merger
attribute or fail if run withbazel coverage
. This is fixed by attaching the default lcov merger as a late-bound default to all Starlark rules via a new attribute and falling back to that attribute in the TestActionBuilder.This commit ensures backwards compatibility is maintained with existing Starlark test rules that set the
_lcov_merger
attribute. This is also verified in the included integration test.Fixes #13978.