Make CC_CODE_COVERAGE_SCRIPT failures fail tests

[keith]

If you're editing, or overriding, this script and you introduce and
introduce an error, previously this wouldn't fail the test invocation.

Fixes #18650
Fixes #26541

[comius]

there are CI failures

[keertk]

Hi there! Thank you for contributing to the Bazel repository. We appreciate your time and effort. We're doing a clean up of old PRs and will be closing this one since it seems to have stalled. Please feel free to reopen/let us know if you’re still interested in pursuing this or if you'd like to discuss anything further. We’ll respond as soon as we have the bandwidth/resources to do so.

[keith]

@keertk can you reopen please

[comius]

Closing as another 2 months passed by. Feel free to reopen.

[keith]

we should really do this but fixing the existing tests is definitely annoying

[keith]

reopening with a opt-in env var to continue ignoring this failure meant only to avoid all the issues with bazel tests + CI setup (users should avoid this flag)

[keith]

@comius what do you think about this solution to avoid having to fix all the tests + CI runner setup

[keith]

@comius can you take a look?

[keith]

@c-mita @comius ptal

[keith]

@c-mita @comius

[keith]

@c-mita @comius ptal 🙏🏻

[keith]

@pzembrod can you checkout this one too? 🙏🏻

[pzembrod]

Will double-check with @c-mita tomorrow.

[c-mita]

How many tests fail without the flag set? I wouldn't think it's many?

[keith]

last I checked it was many and i only added the opt outs as needed, i will test again.

[keith]

The core issue is the CI machines don't have the tools installed GCov does not exist at the given path: ''

[keith]

see the full logs here https://buildkite.com/bazel/bazel-bazel-github-presubmit/builds/27750#019783f9-ea1d-4151-b1b9-de5834c2a046

[c-mita]

Ok, so before this can be accepted, I think one of two things needs to be changed:

• collect_cc_coverage.sh is called if and only if C++ coverage is actually required.
• The default is to ignore failures in the script.

The former is more complex but is I think the better solution. And we should get around to resolving the coverage linking issue (#15634 (comment)), although I don't think that's a hard requirement for this.

[fmeum]

collect_cc_coverage.sh is called if and only if C++ coverage is actually required.

I agree that this is the best approach, but I'm not sure how to determine whether this is the case. For example, what about a java_binary that has a JNI .so in data (not linked into the Java runtime)?

[c-mita]

collect_cc_coverage.sh is called if and only if C++ coverage is actually required.

I agree that this is the best approach, but I'm not sure how to determine whether this is the case. For example, what about a java_binary that has a JNI .so in data (not linked into the Java runtime)?

I'm not a fan of it, but coverage_common.instrumented_files_info accepts a coverage_environment parameter, which the C++ rules are allowed to use (and they already do).

We could just add another environment variable saying "hey, C++ coverage is needed now"?
Maybe even just testing for the variables it already sets is enough?

[fmeum]

Checking for the variables sounds good to me

[keith]

isn't this virtually already done by the fact that CC_CODE_COVERAGE_SCRIPT is only set if it has the relevant attr https://github.com/keith/bazel/blob/fbec90fb287618e3c46e71cbc961daf1587d88d4/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java#L272-L276 which is only added to cc_test? https://github.com/keith/bazel/blob/fbec90fb287618e3c46e71cbc961daf1587d88d4/src/main/starlark/builtins_bzl/common/cc/cc_test.bzl#L101 (ignoring the fact that users could set this env var themselves)

[fmeum]

This attribute isn't specific to cc_test, every test rule that wants to support coverage should add it. But checking for COVERAGE_GCOV_PATH would work (maybe the script already does that).

[keith]

This attribute isn't specific to cc_test, every test rule that wants to support coverage should add it.

should, will, or does? since right now it really is only cc_test AFAICT.

I don't think the gcov one is entirely safe since there are codepaths that shouldn't need that tool today, but it looks like GENERATE_LLVM_LCOV is safe since it should always be present. Updated to use that. I will re-test CI to see if we can avoid some of the disables now

[fmeum]

Should and sometimes does, see https://github.com/bazel-contrib/rules_go/blob/1172e60b3fdb3c1b1f6efc781eda9819533375ac/go/private/rules/test.bzl#L466 and https://github.com/bazel-contrib/rules_python/blob/175a33610e853388c83730d9e2b5b2ac3626649d/python/private/attributes.bzl#L391.

https://bazel.build/extending/rules#test_rules documents this.

That variable is a good fit as well, it's unconditionally set to some value in the built-in Starlark logic.

[keith]

green now, still had to disable in 1 place where it fails on macOS CI

[keith]

@c-mita I think this is ready for another pass 🙏🏻