bazel
bazel copied to clipboard
bazelbuild
Enable coverage for generated source files
Currently generated source files can't be included in the coverage report. Remove the check to enable coverage for the generated files.
Please create an issue for this first so more people can review the relative merits of the feature and if/how it should be done. I strongly suspect that this change would be contentious as is.
+1 I think this would be interesting for our use case, but I'm not sure we'd want it by default, and ideally there would be some way to disable it.
Also reported on Slack: https://bazelbuild.slack.com/archives/CA31HN1T3/p1588710579121400
+1 This is essential for coverage to work; this should absolutely be enabled by default. Maybe it is a bug in isSourceArtifact, but I think that check is not needed.
Currently this blocks us using the Bazel release as-is
@meteorcloudy @c-mita @meisterT is it possible to merge this? It has passed 13 months for a one-line review.
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 they don't exist in the repo.
Either we would need to guard this feature behind a yet-another-flag or any such tool has to be able to handle files not existing in the repo.
I get it.. but this sounds like a report generator issue, not that the coverage data should avoid source files. That seems dangerous to me.
We do extra filtering on the coverage.dat file to e.g. exclude parts of debug frameworks, but that is an intermediate step, and not something we would consider on the raw output.
@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 to me at Bazelcon.
I can imagine supporting this feature under an experimental flag for the moment; the cost to doing so doesn't seem high to me.
@c-mita are you waiting for a user response, if so can you change the label from awaiting-review to awaiting-user-response to reflect that?
@c-mita Not entirely sure whether it's relevant for this particular flag, but this may lack a line in CoreOptions#getExec to carry it over to the exec configuration.
@c-mita Not entirely sure whether it's relevant for this particular flag, but this may lack a line in
CoreOptions#getExecto carry it over to the exec configuration.
I don't that matters since, AFAIU, this shouldn't be carried over to the exec configuration. The other coverage flags aren't carried over either.
LGTM.
I was going to ask for a test, but in investigating how best to do it, I ended up writing it so I'll just add it in myself when I import.
Actually, i couldn't even ask you to write tests; the tests for InstrumentedFilesCollector haven't been open-sourced for some reason...
Hi @c-mita, Since I can see that this PR has been approved, please let me know whether I should proceed with importing it. Thanks!
@c-mita could you please provide an update on this? Is it still under internal review? Thanks
There were some discussions internally and a little bit of push-back regarding the addition of a new flag. Then it sort of just fell through the cracks; sorry about that.
If there are no issues, then we may want to push for enabling this all the time and removing the flag.
Thank you! Is there any work needed to get this in the upcoming Bazel 6.3 release?
Thank you! Is there any work needed to get this in the upcoming Bazel 6.3 release?
@UebelAndre Here is the PR to push this change to the 6.3.0 just for your reference: https://github.com/bazelbuild/bazel/pull/18664
Also, just for your reference, here is the list of the issues/PR's that shall be resolved for the 6.3.0 release: https://github.com/bazelbuild/bazel/milestone/53
Thanks
