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

Expose CoverageOutputGenerator on a Fragment #14724

Closed
wants to merge 1 commit into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Feb 4, 2022

This allows Starlark rules to define _lcov_merger as a late-bound Starlark configuration_field in order not to pay the cost of building it when coverage isn't enabled.

Fixes #8736
Fixes #10642

@fmeum fmeum requested a review from lberki as a code owner February 4, 2022 18:00
@fmeum
Copy link
Collaborator Author

fmeum commented Feb 4, 2022

@c-mita

@keith
Copy link
Member

keith commented Feb 28, 2022

@c-mita can you review?

Copy link
Member

@c-mita c-mita left a comment

Choose a reason for hiding this comment

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

LGTM, maybe add a comment on the output_generator doc indicating it may be None, since I don't think that's obvious behaviour.

allowReturnNones = true,
structField = true,
doc =
"Returns label pointed to by "
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a note saying that this will be None if coverage collection isn't enabled?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done, I also slightly reworded the comment.

@c-mita
Copy link
Member

c-mita commented Mar 2, 2022

Ideally, I'd prefer this was guarded by an experimental flag, but I don't think that's feasible without making the feature clunky to use, so I'll let that pass.

This allows Starlark rules to define _lcov_merger as a
late-bound Starlark configuration_field in order not to pay the cost of
building it when coverage isn't enabled.
Copy link
Member

@c-mita c-mita left a comment

Choose a reason for hiding this comment

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

LGTM.
Thanks.

@bazel-io bazel-io closed this in 14d8be0 Mar 8, 2022
fmeum added a commit to fmeum/bazel that referenced this pull request Mar 8, 2022
This allows Starlark rules to define _lcov_merger as a late-bound Starlark configuration_field in order not to pay the cost of building it when coverage isn't enabled.

Fixes bazelbuild#8736
Fixes bazelbuild#10642

Closes bazelbuild#14724.

RELNOTES: Add coverage configuration fragment, used to expose output_generator label.
PiperOrigin-RevId: 433156089
Wyverald pushed a commit that referenced this pull request Mar 8, 2022
This allows Starlark rules to define _lcov_merger as a late-bound Starlark configuration_field in order not to pay the cost of building it when coverage isn't enabled.

Fixes #8736
Fixes #10642

Closes #14724.

RELNOTES: Add coverage configuration fragment, used to expose output_generator label.
PiperOrigin-RevId: 433156089
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.

_lcov_merger late-bound test attribute Expose CoverageOutputGenerator's label in a configuration
3 participants