Charles Mita

Results 50 comments of Charles Mita

> The reason is that at line 206 of CcModule.java, this method is called where the ruleContext could be null, the change here is just to protect against that and...

This breaks coverage integration tests for some of our rules internally; I'm not entirely sure why and unfortunately I'm not able to do much debugging at this time.

I understand the desire, but I'm uncomfortable continuing further in this direction (adding more flags to allow overriding parts of coverage collection). I really think coverage collection would benefit by...

The error in question is a link-time error, where coverage symbols referenced by a C++ source are not available. e.g. ``` ld: error: undefined symbol: llvm_gcda_emit_function >>> referenced by foo.cc...

I'm not familiar with the Python rules or how coverage works for them, but with that caveat, LGTM. ;)

> I've been looking into this a bit, and it's definitely possible today by using the C++ related coverage script attribute. It feels a bit like a hack but afaict...

cc @fmeum As was mentioned in the discussion for PR #15634. Simply changing `CcCommon::configureFeaturesOrThrowEvalException` to check `CcCompilationHelper::isCoverageEnabled(RuleContext)` isn't sufficient. ((https://cs.opensource.google/bazel/bazel/+/fe1c1768:src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java;l=982) The problem is that test rules, which are often filtered...

Because more work is required to solve this, I want to move this discussion off this PR and onto the issue. I'm closing this PR because it isn't sufficient on...

I'm wary of merging, at least right now. Internally, we have tools that expect to be able to open files referenced in coverage reports which would not be possible if...

> @c-mita Can this be closed? Not quite; I want to check what happens with proto_library in the new year before it's dismissed. There was interest expressed in this PR...