Include virtual-includes and vendored headers in the coverage scope - #3
Merged
Conversation
eclipse-score/baselibs#558: a workspace target whose hdrs are vendored from an external repository behind strip_include_prefix (//score/flatbuffers:flatbufferscpp) lost all its headers from the report. Two causes: the scope aspect dropped every file under external/, and the compiler does not even record that path. With strip_include_prefix / include_prefix Bazel compiles against a generated _virtual_includes/ symlink tree and the coverage mapping names that file, under a configuration-specific prefix (bazel-out/k8-fastbuild/bin/... in the test binaries, bazel-out/k8-opt-exec-*/bin/... in the scope's baseline archive). Aspect: for workspace targets the post-processing identity of the public headers (CcInfo.compilation_context.direct_public_headers) is listed as well: the _virtual_includes/ path, and headers a workspace target vendors from an external repository (external/<repo>/...). Other generated headers stay out; external targets stay out. Reporter: get_covered_files returns raw -> normalized names and strip_config_prefix drops bazel-out/<config>/bin/ so covmap paths compare equal to the allowlist's short_path form; the same normalization applies to LCOV SF records and HTML titles. redundant_baseline_variants excludes the baseline archive's variant of a header that a test binary already covers, so such a header appears once instead of twice (once at 0 %). Verification: new analysis test (strip_include_prefix fixture), 5 unit tests for the normalization and the variant exclusion, and a header-only library behind strip_include_prefix in the integration workspace whose hand-derived record (3 of 6 lines, one call) joins expected_lcov.dat; all 15 integration checks pass. Validated on baselibs //score/flatbuffers/...: the 13 vendored headers appear in LCOV and HTML, each exactly once. The approach follows the patch candidates of baselibs#577 but needs no extra_deps attribute: the scope aspect handles it for every workspace target. Signed-off-by: Dan Calavrezo <195309321+dcalavrezo-qorix@users.noreply.github.com> (cherry picked from commit 40b1746)
dcalavrezo-qorix
requested review from
AlexanderLanin,
MaximilianSoerenPollak and
nradakovic
as code owners
September 11, 2026 08:49
License Check Results🚀 The license check job ran with the Bazel command: bazel run //:license-checkStatus: Click to expand output |
MaximilianSoerenPollak
previously approved these changes
Sep 11, 2026
MaximilianSoerenPollak
left a comment
MaximilianSoerenPollak
left a comment
Contributor
There was a problem hiding this comment.
That looks good as far as i can tell.
Explanation makes sense and this for sure should be something that should work.
There was a problem hiding this comment.
🟡 Changes recommended
The requirements conflict and the external-repository regression path lacks automated coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Extends coverage reporting to include vendored and virtual-include headers while normalizing configuration-specific Bazel paths.
Changes:
- Includes public virtual and externally vendored headers in coverage scope.
- Normalizes generated-header paths and removes duplicate baseline variants.
- Adds unit, analysis, integration, and documentation updates.
File summaries
| File | Description |
|---|---|
score_coverage/coverage_scope.bzl |
Expands header scope collection. |
score_coverage/reporter.py |
Normalizes paths and suppresses duplicates. |
score_coverage/tests/reporter_test.py |
Tests reporter normalization behavior. |
score_coverage/tests/starlark/coverage_scope_tests.bzl |
Tests virtual-header scope collection. |
score_coverage/tests/starlark/fixtures/BUILD |
Defines the analysis fixture. |
score_coverage/tests/starlark/fixtures/vendor/include/vendored/api.h |
Adds the fixture header. |
integration_tests/tools/coverage/BUILD |
Adds the fixture to coverage scope. |
integration_tests/src/BUILD |
Defines the integration library. |
integration_tests/src/coverable_test.cpp |
Exercises the virtual header. |
integration_tests/src/vendored/include/vendored/inline_math.h |
Adds covered and uncovered inline functions. |
integration_tests/expected_lcov.dat |
Adds expected virtual-header coverage. |
docs/requirements/tool_requirements.rst |
Updates scope and path requirements. |
docs/verification/verification_report.rst |
Updates verification inventory. |
docs/release/release_notes.rst |
Documents the feature. |
docs/manual/known_problems.rst |
Documents reported virtual paths. |
Review details
- Files reviewed: 13/15 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+123
to
+125
| def _test_virtual_include_headers_are_in_scope(name): | ||
| coverage_scope(name = name + "_subject", testonly = True, deps = [_FIX + ":vendored"]) | ||
| analysis_test(name = name, impl = _test_virtual_include_headers_are_in_scope_impl, target = name + "_subject") |
Contributor
Author
There was a problem hiding this comment.
Done: a small local module (score_coverage/tests/starlark/external_fixture, local_path_override) now provides a header that two fixture targets vendor, once behind strip_include_prefix and once directly. The new tests assert both identities in the allowlist, external/coverage_external_fixture+/include/ext/ext.h and the _virtual_includes/ path, so the ../ -> external/ conversion is exercised.
Comment on lines
+45
to
+49
| For workspace targets it shall additionally list the post-processing | ||
| identity of the public headers: the generated ``_virtual_includes/`` path a | ||
| header gets through ``strip_include_prefix`` or ``include_prefix`` (the path | ||
| the coverage mapping records), and headers a workspace target vendors from | ||
| an external repository. |
Contributor
Author
There was a problem hiding this comment.
Agreed, reworded: tool_req__coverage_scope_excludes now says external targets are not traversed and generated files are not listed, with the two exceptions named in tool_req__coverage_scope_transitive (headers a workspace target declares from an external repository, and the _virtual_includes/ identities of its public headers).
- Analysis tests: a local module (score_coverage/tests/starlark/ external_fixture, wired via local_path_override) provides a header that two workspace fixture targets vendor, once behind strip_include_prefix and once directly. The tests assert both identities in the allowlist: external/coverage_external_fixture+/include/ext/ext.h and the _virtual_includes/ path. This exercises the ../<repo> -> external/<repo> conversion that baselibs#558 depends on. - tool_req__coverage_scope_excludes no longer contradicts tool_req__coverage_scope_transitive: external targets are not traversed and generated files are not listed, except the two header identities the transitive requirement names. - //:license-check: alias to a dash_license_checker in //tools over the Python requirements lock, so the cicd-workflows license check has its target. The root package still loads only runtime rules. Signed-off-by: Dan Calavrezo <195309321+dcalavrezo-qorix@users.noreply.github.com>
dcalavrezo-qorix
force-pushed
the
dcalavrezo_558
branch
from
September 11, 2026 11:00
f5fcd29 to
5d426f0
Compare
paulquiring
approved these changes
Sep 11, 2026
paulquiring
left a comment
paulquiring
left a comment
There was a problem hiding this comment.
Tested, solves eclipse-score/baselibs#558
MaximilianSoerenPollak
approved these changes
Sep 11, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What
Fixes the header loss reported in eclipse-score/baselibs#558: a workspace
target whose
hdrsare vendored from an external repository behindstrip_include_prefix(//score/flatbuffers:flatbufferscpp) had all itsheaders filtered out of the coverage report.
Two causes, both addressed:
external/.strip_include_prefix/
include_prefix, Bazel compiles against a generated_virtual_includes/symlink tree and the coverage mapping names that file, under a
configuration-specific prefix (
bazel-out/k8-fastbuild/bin/...in thetest binaries,
bazel-out/k8-opt-exec-*/bin/...in the scope's baselinearchive).
How
coverage_scope.bzl): for workspace targets, the post-processingidentity of the public headers (
CcInfo.compilation_context.direct_public_headers)is listed as well: the
_virtual_includes/path and headers vendored from anexternal repository. Other generated headers and external targets stay out.
reporter.py):get_covered_filesreturns raw → normalizednames and
strip_config_prefixdropsbazel-out/<config>/bin/, so covmappaths compare equal to the allowlist's
short_pathform; the samenormalization applies to LCOV
SF:records and HTML titles.redundant_baseline_variantsexcludes the baseline archive's variant of aheader a test binary already covers, so such a header appears once instead
of twice (the second at 0 %).
Follows the patch candidates of eclipse-score/baselibs#577, but needs no
extra_depsattribute: the aspect handles every workspace target.Verification
strip_include_prefixfixture): allowlist lists both the declared header and the_virtual_includes/pathstrip_include_prefix, hand-derived record (3 of 6 lines, one call) added toexpected_lcov.dat//score/flatbuffers/...with this module overridden locallybase.h: 14 of 21 lines)Notes
(
score/flatbuffers/_virtual_includes/flatbufferscpp/flatbuffers/base.h),the identity the compiler records; documented in the known problems and the
tool requirements.
tool_req__coverage_scope_transitive,tool_req__coverage_report_relative_paths, release notes, known problems,verification report inventory.