Exit collect_coverage.sh early if LCOV_MERGER is not set. #14352
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lberki
reviewed
Dec 1, 2021
Contributor
lberki
left a comment
lberki
left a comment
There was a problem hiding this comment.
Remind me again, why isn't it feasible to add the :lcov_merger attribute to all Starlark test rules?
Rules that provide InstrumentedFilesInfo but do not define the _lcov_merger attribute will result in this script being called without the required LCOV_MERGER environment variable being set. Since _all_ rules now provide an InstrumentedFilesInfo provider by default, but are not required to provide the attribute, this scenario will be encountered far more frequently and should not be an error. Because of the way coverage collection within Google differs to Bazel, it isn't safe to change TestActionBuilder to simply not prepare the collect_coverage.sh script the way it would if no coverage was to be collected, so we have to change the script itself.
c-mita
force-pushed
the
no_lcov_merger
branch
from
eb316ac to
2da7ab8
Compare
Member
Author
Other little minor issues arise, but the second point is the big one. |
Contributor
|
On second thought, there isn't any downside for this change, so LGTM. It would be nice to have an overarching design for adding coverage collection for any language, but that shouldn't be a blocker for this change. |
lberki
approved these changes
Dec 1, 2021
brentleyjones
pushed a commit
to brentleyjones/bazel
that referenced
this pull request
Dec 1, 2021
Rules that provide InstrumentedFilesInfo but do not define the _lcov_merger attribute will result in this script being called without the required LCOV_MERGER environment variable being set. Since _all_ rules now provide an InstrumentedFilesInfo provider by default, but are not required to provide the attribute, this scenario will be encountered far more frequently and should not be an error. Because of the way coverage collection within Google differs to Bazel, it isn't safe to change TestActionBuilder to simply not prepare the collect_coverage.sh script the way it would if no coverage was to be collected, so we have to change the script itself. Fixes bazelbuild#13978 Closes bazelbuild#14352. PiperOrigin-RevId: 413369871 (cherry picked from commit aa52f2d)
meteorcloudy
pushed a commit
that referenced
this pull request
Dec 1, 2021
Rules that provide InstrumentedFilesInfo but do not define the _lcov_merger attribute will result in this script being called without the required LCOV_MERGER environment variable being set. Since _all_ rules now provide an InstrumentedFilesInfo provider by default, but are not required to provide the attribute, this scenario will be encountered far more frequently and should not be an error. Because of the way coverage collection within Google differs to Bazel, it isn't safe to change TestActionBuilder to simply not prepare the collect_coverage.sh script the way it would if no coverage was to be collected, so we have to change the script itself. Fixes #13978 Closes #14352. PiperOrigin-RevId: 413369871 (cherry picked from commit aa52f2d) Co-authored-by: Charles Mita <[email protected]>
Bencodes
pushed a commit
to Bencodes/bazel
that referenced
this pull request
Jan 10, 2022
Rules that provide InstrumentedFilesInfo but do not define the _lcov_merger attribute will result in this script being called without the required LCOV_MERGER environment variable being set. Since _all_ rules now provide an InstrumentedFilesInfo provider by default, but are not required to provide the attribute, this scenario will be encountered far more frequently and should not be an error. Because of the way coverage collection within Google differs to Bazel, it isn't safe to change TestActionBuilder to simply not prepare the collect_coverage.sh script the way it would if no coverage was to be collected, so we have to change the script itself. Fixes bazelbuild#13978 Closes bazelbuild#14352. PiperOrigin-RevId: 413369871
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rules that provide InstrumentedFilesInfo but do not define the
_lcov_merger attribute will result in this script being called without
the required LCOV_MERGER environment variable being set.
Since all rules now provide an InstrumentedFilesInfo provider by
default, but are not required to provide the attribute, this scenario
will be encountered far more frequently and should not be an error.
Because of the way coverage collection within Google differs to Bazel,
it isn't safe to change TestActionBuilder to simply not prepare the
collect_coverage.sh script the way it would if no coverage was to be
collected, so we have to change the script itself.
Fixes #13978