Enable Dynamic Library and Header Package Creation ( experimental PR - Do not Submit)#61
Conversation
Make communication shared library with all necessary deps and header package for SDK deployment use-cases
![propel-code-bot[bot]](https://avatars.githubusercontent.com/in/1179004?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes suggested to fix macOS linking, header determinism, hermetic builds, and encapsulation.
Status: Changes Suggested | Risk: Medium | Readiness: 5/10 (Needs Polish)
Issues Identified & Suggestions
- Fix macOS link flags; narrow exports.
deploy/BUILD - Deterministic filtering; hermetic actions; drop debug flattening.
copy_headers.bzl,extract_headers_for_deps.bzl - Limit visibility and cross-package sources.
impl/BUILD,impl/tracing/BUILD,ipc_bridge/BUILD
Review Details
📁 9 files reviewed | 💬 13 comments
👍 / 👎 individual comments to help improve reviews for you
| "@platforms//os:macos": [ | ||
| "-Wl,-install_name,@rpath/libscore_communication.dylib", | ||
| "-Wl,-rpath,@loader_path", | ||
| "-static-libgcc", |
There was a problem hiding this comment.
[Logic] The macOS user_link_flags block unconditionally adds -static-libgcc and -static-libstdc++. Those options are GCC-specific and are not accepted by Apple’s clang toolchain – invoking clang++ with either flag fails with error: unknown argument '-static-libgcc' (likewise for -static-libstdc++). As soon as somebody builds this target on macOS the link step will abort before producing the .dylib. Please drop these flags (or wrap them in a Linux-only select) so that the mac configuration uses only install_name/rpath settings, e.g.
"@platforms//os:macos": [
"-Wl,-install_name,@rpath/libscore_communication.dylib",
"-Wl,-rpath,@loader_path",
],This keeps the mac build functional while still statically linking libgcc/libstdc++ on the Linux/QNX branches where those flags are supported.
Context for Agents
The macOS `user_link_flags` block unconditionally adds `-static-libgcc` and `-static-libstdc++`. Those options are GCC-specific and are not accepted by Apple’s clang toolchain – invoking `clang++` with either flag fails with `error: unknown argument '-static-libgcc'` (likewise for `-static-libstdc++`). As soon as somebody builds this target on macOS the link step will abort before producing the `.dylib`. Please drop these flags (or wrap them in a Linux-only select) so that the mac configuration uses only `install_name`/`rpath` settings, e.g.
```python
"@platforms//os:macos": [
"-Wl,-install_name,@rpath/libscore_communication.dylib",
"-Wl,-rpath,@loader_path",
],
```
This keeps the mac build functional while still statically linking libgcc/libstdc++ on the Linux/QNX branches where those flags are supported.
File: score/mw/com/deploy/BUILD
Line: 333Chain of Thought
- Validation Decision: accepted
- Confidence: 0.95
- Likelihood Score: 5/5
- Likelihood Reason: Building this target on macOS will inevitably fail because Apple clang does not accept -static-libgcc or -static-libstdc++. These are GCC-only driver flags with no equivalent in the Apple toolchain.
- Suggested Action: keep
- Tool Calls Used: 3
Reasoning
The comment is technically accurate and actionable:
-
New code: The BUILD file is entirely new (not in BASE), so this issue is 100% introduced by this PR.
-
Correct technical claim:
-static-libgccand-static-libstdc++are GCC driver options. Apple's clang toolchain on macOS:- Uses libc++ instead of libstdc++
- Uses compiler-rt instead of libgcc
- Does not recognize these flags, producing "unknown argument" errors
-
Affects two targets: The issue appears in both macOS select branches:
score_comtarget (lines 144-145)score_communicationtarget (lines 333-334)
-
Clear fix provided: Remove
-static-libgccand-static-libstdc++from the macOS blocks, keeping only theinstall_nameandrpathsettings that are appropriate for macOS. -
Inevitable failure: Any attempt to build either of these cc_shared_library targets on macOS will fail at the link step. The select statement specifically routes macOS builds through these flags, guaranteeing the error.
The comment is well-written, identifies a real bug introduced by this PR, provides a concrete actionable fix with example code, and correctly distinguishes which platforms need which flags.
Evidence Summary
1. File Snapshot (HEAD)
- File:
score/mw/com/deploy/BUILD - Lines:
140-146
140: ],
141: "@platforms//os:macos": [
142: "-Wl,-install_name,@rpath/libscore_com.dylib",
143: "-Wl,-rpath,@loader_path",
144: "-static-libgcc",
145: "-static-libstdc++",
146: ],
2. File Snapshot (HEAD)
- File:
score/mw/com/deploy/BUILD - Lines:
329-335
329: ],
330: "@platforms//os:macos": [
331: "-Wl,-install_name,@rpath/libscore_communication.dylib",
332: "-Wl,-rpath,@loader_path",
333: "-static-libgcc",
334: "-static-libstdc++",
335: ],
| # Track unique output paths to avoid duplicate declarations | ||
| seen_paths = {} | ||
|
|
||
| for hdr in input_headers: | ||
| # Get the path and strip external repository prefixes | ||
| short_path = hdr.short_path | ||
|
|
||
| # Remove external repository prefix (e.g., "../communication~/" or "external/boost.core~/") | ||
| if short_path.startswith("../"): | ||
| parts = short_path.split("/") | ||
| if len(parts) >= 3: |
There was a problem hiding this comment.
[Reliability] The deduplication logic in _copy_headers_impl drops any header whose normalized path was already seen:
if output_path in seen_paths:
continue
seen_paths[output_path] = hdrBecause input_headers comes from a depset, the traversal order is undefined. Whenever two artifacts collapse to the same output_path (e.g. the real header and its _virtual_includes variant, or two repositories both providing score/mw/com/types.h), whichever artifact happens to be yielded first wins in one build, and another one may win in the next build. This makes the exported header package non‑deterministic and can silently switch between incompatible header versions. Instead of relying on depset iteration order, enforce a deterministic selection (e.g. sort input_headers before processing and prefer _virtual_includes or the canonical repo explicitly) or fail fast when a collision occurs so consumers do not get a random header.
Context for Agents
The deduplication logic in `_copy_headers_impl` drops any header whose normalized path was already seen:
```python
if output_path in seen_paths:
continue
seen_paths[output_path] = hdr
```
Because `input_headers` comes from a depset, the traversal order is **undefined**. Whenever two artifacts collapse to the same `output_path` (e.g. the real header and its `_virtual_includes` variant, or two repositories both providing `score/mw/com/types.h`), whichever artifact happens to be yielded first wins in one build, and another one may win in the next build. This makes the exported header package non‑deterministic and can silently switch between incompatible header versions. Instead of relying on depset iteration order, enforce a deterministic selection (e.g. sort `input_headers` before processing and prefer `_virtual_includes` or the canonical repo explicitly) or fail fast when a collision occurs so consumers do not get a random header.
File: score/mw/com/deploy/copy_headers.bzl
Line: 42Chain of Thought
- Validation Decision: accepted
- Confidence: 0.82
- Likelihood Score: 3/5
- Likelihood Reason: Collisions are plausible because the code explicitly handles _virtual_includes paths alongside real headers — these can normalize to the same output path. While Bazel's default depset order is typically stable for a fixed build graph, it's explicitly undocumented and could change. The more concrete risk is that silent deduplication hides real conflicts, potentially copying the wrong header variant.
- Suggested Action: keep
- Tool Calls Used: 5
Reasoning
Evidence gathered:
-
New file:
copy_headers.bzldoes not exist in the base commit — this is entirely new code introduced by this PR, so the issue is squarely within diff responsibility. -
Depset order is undefined:
extract_headers_for_deps.bzlline 56 creates a depset withdepset(transitive = all_headers)without specifying an order. Bazel docs confirm: the default order "does not guarantee any particular iteration order." -
Collisions are plausible: The normalization logic strips
_virtual_includes/target_name/(lines 70-77),include/directories (lines 61-66), and external repository prefixes (lines 40-55). A header atscore/static_reflection/_virtual_includes/static_reflection/score/foo.hand a real header atscore/foo.hwould both normalize toscore/foo.h. This is a realistic scenario since the code was explicitly written to handle_virtual_includespaths. -
Silent deduplication is risky: Lines 95-96 silently skip collisions with no logging or error. Even if the depset order happened to be stable in practice, silently choosing one header over another without any deterministic preference is fragile and hides potential errors.
-
Actionable suggestions: The comment offers two concrete fixes — (a) sort and apply a deterministic preference, or (b) fail fast on collision. Both are specific and implementable.
Why likelihood is 3 (Plausible): While Bazel's default order tends to be stable for a fixed build graph, collisions ARE expected to occur because the code normalizes away _virtual_includes and include/ path components that distinguish otherwise-identical paths. The real risk is silent incorrect behavior rather than build-to-build flapping. The compilation_context.headers depset may well contain both the real header and the _virtual_includes symlink for the same logical header.
Evidence Summary
1. File Snapshot (HEAD)
- File:
score/mw/com/deploy/extract_headers_for_deps.bzl
1: # *******************************************************************************
2: # Copyright (c) 2025 Contributors to the Eclipse Foundation
3: #
4: # See the NOTICE file(s) distributed with this work for additional
5: # information regarding copyright ownership.
6: #
7: # This program and the accompanying materials are made available under the
8: # terms of the Apache License Version 2.0 which is available at
9: # https://www.apache.org/licenses/LICENSE-2.0
10: #
...
2. Library Docs
- Library:
bazel - Query:
depset to_list iteration order
Here is the relevant Bazel documentation on **depset `to_list` iteration order**:
---
## Depset Traversal / Iteration Orders
The `to_list()` operation performs a traversal over the depset's internal DAG (directed acyclic graph). The kind of traversal depends on the **order** that was specified at the time the depset was constructed.
Bazel supports multiple orders because different tools care about the order of their inputs. For example, a linker action may need to ensure that if `B` depends on `A`, then `A.o` comes before `B.o` on the linker's command line. Other tools might have the opposite requirement.
...
3. File Snapshot (HEAD)
- File:
score/mw/com/deploy/BUILD
1: # *******************************************************************************
2: # Copyright (c) 2025 Contributors to the Eclipse Foundation
3: #
4: # See the NOTICE file(s) distributed with this work for additional
5: # information regarding copyright ownership.
6: #
7: # This program and the accompanying materials are made available under the
8: # terms of the Apache License Version 2.0 which is available at
9: # https://www.apache.org/licenses/LICENSE-2.0
10: #
...
| # Export all public API symbols | ||
| # C++ symbols will be mangled, so we use wildcards | ||
| # Entire Score | ||
| *score*; |
There was a problem hiding this comment.
[Maintainability] The version script glob *score* will export every symbol whose mangled name contains the substring "score" — including symbols from third-party dependencies that happen to use this string, or internal implementation details that shouldn't be part of the public ABI. This creates an unstable ABI surface that can break silently across builds.
Use more specific patterns to export only the intended namespace:
SCORE_COM_1.0 {
global:
extern "C++" {
score::mw::com::*;
};
local:
*;
};
Or use _ZN5score* if demangled patterns aren't supported by the target linker.
Context for Agents
The version script glob `*score*` will export every symbol whose mangled name contains the substring "score" — including symbols from third-party dependencies that happen to use this string, or internal implementation details that shouldn't be part of the public ABI. This creates an unstable ABI surface that can break silently across builds.
Use more specific patterns to export only the intended namespace:
```
SCORE_COM_1.0 {
global:
extern "C++" {
score::mw::com::*;
};
local:
*;
};
```
Or use `_ZN5score*` if demangled patterns aren't supported by the target linker.
File: score/mw/com/deploy/BUILD
Line: 76Chain of Thought
- Validation Decision: accepted
- Confidence: 0.78
- Likelihood Score: 3/5
- Likelihood Reason: The broad
*score*pattern could theoretically match third-party symbols containing "score" in their mangled names. In practice, the risk is moderate since the main third-party deps (boost, nlohmann/json) are unlikely to contain "score" in symbol names, but it's still poor practice for ABI control. The library is statically linking libstdc++ and libgcc which reduces the blast radius. - Suggested Action: modify
- Tool Calls Used: 5
Reasoning
The comment correctly identifies a real issue: the version script pattern *score* matches any mangled symbol name containing the substring "score", which is overly broad for controlling a library's public ABI. This is entirely new code introduced in this PR (the file didn't exist in BASE).
However, the suggested fix of using only score::mw::com::* in the extern "C++" block is too narrow for this project's actual needs. The com_all_deps library explicitly includes dependencies from other namespaces:
@score_baselibs//score/language/futurecpp:futurecpp→ symbols inscore::language::futurecpp::*@score_logging//score/mw/log→ symbols inscore::mw::log::*
Using only score::mw::com::* would cause runtime symbol resolution failures for these needed symbols.
A better suggestion would be to use extern "C++" with a broader but still more precise pattern like score::* or to enumerate all needed namespaces:
extern "C++" {
score::mw::com::*;
score::language::*;
score::mw::log::*;
};
The core observation about *score* being too broad is valid and actionable, but the specific fix needs adjustment.
Evidence Summary
1. File Snapshot (HEAD)
- File:
score/mw/com/deploy/SYMBOL_INCLUSION_ANALYSIS.md
1: # Symbol Inclusion Analysis: Why `score_com` Fails and `score_communication` Works
2:
3: ## Document Purpose
4:
5: This document explains the critical difference between two shared library configurations in the SCORE Communication middleware:
6: - **`score_com`** - Does NOT work (missing symbols)
7: - **`score_communication`** - Works correctly (all symbols present)
8:
9: The root cause is how Bazel's `cc_shared_library` rule handles symbol inclusion from transitive dependencies.
10:
...
| output_headers.append(output_file) | ||
|
|
||
| # Copy the header | ||
| ctx.actions.run_shell( |
There was a problem hiding this comment.
[Reliability] This action shells out to host mkdir, rm, and cp. That breaks hermetic/remote builds and can fail on non-POSIX environments. Additionally, mkdir -p and rm -f are unnecessary since Bazel manages parent directories and output files for declared outputs. Replace the entire run_shell block with ctx.actions.symlink(output=output_file, target_file=hdr) which is fully hermetic, cross-platform, and requires no subprocess.
Context for Agents
This action shells out to host `mkdir`, `rm`, and `cp`. That breaks hermetic/remote builds and can fail on non-POSIX environments. Additionally, `mkdir -p` and `rm -f` are unnecessary since Bazel manages parent directories and output files for declared outputs. Replace the entire `run_shell` block with `ctx.actions.symlink(output=output_file, target_file=hdr)` which is fully hermetic, cross-platform, and requires no subprocess.
File: score/mw/com/deploy/copy_headers.bzl
Line: 105Chain of Thought
- Validation Decision: accepted
- Confidence: 0.90
- Likelihood Score: 4/5
- Likelihood Reason: Using POSIX shell commands (mkdir, rm, cp) via run_shell will definitely break on non-POSIX platforms (Windows) and breaks hermeticity for remote build execution. This is a real issue for any project using remote execution or cross-platform builds.
- Suggested Action: modify
- Tool Calls Used: 8
Reasoning
The comment correctly identifies that the new code at line 105-110 uses ctx.actions.run_shell with POSIX commands (mkdir -p, rm -f, cp) which breaks hermetic/remote builds and fails on non-POSIX environments.
Evidence:
- The file
copy_headers.bzlis entirely new (not found in base commit), so this is newly introduced by this PR. - The shell command
mkdir -p $(dirname {}) && rm -f {} && cp {} {}relies on POSIX utilities. - Bazel's built-in
ctx.actions.symlinkis a real API that handles this use case without spawning any subprocess, making it fully hermetic and cross-platform. - Furthermore, the
mkdir -pis unnecessary because Bazel automatically creates parent directories for declared outputs, andrm -fis unnecessary because Bazel manages output file lifecycle.
The suggestion to use ctx.actions.symlink is the correct, idiomatic Bazel approach. The mention of ctx.actions.copy_file is slightly imprecise (it's not a built-in Bazel action method - copy_file is a Skylib BUILD rule), but the comment hedges with "(if available)" and the overall advice is sound.
The comment is actionable: the developer can replace the entire run_shell block with a single line: ctx.actions.symlink(output=output_file, target_file=hdr).
Evidence Summary
1. File Snapshot (HEAD)
- File:
score/mw/com/deploy/extract_headers_for_deps.bzl
1: # *******************************************************************************
2: # Copyright (c) 2025 Contributors to the Eclipse Foundation
3: #
4: # See the NOTICE file(s) distributed with this work for additional
5: # information regarding copyright ownership.
6: #
7: # This program and the accompanying materials are made available under the
8: # terms of the Apache License Version 2.0 which is available at
9: # https://www.apache.org/licenses/LICENSE-2.0
10: #
...
2. Library Docs
- Library:
bazel - Query:
ctx.actions.symlink copy_file hermetic actions
I'll search for the relevant Bazel documentation on these topics.Now let me get the specific `ctx.actions.symlink` documentation from Bazel:Here is the relevant Bazel documentation on **`ctx.actions.symlink`**, **`copy_file`**, and **hermetic actions**:
---
## 1. `ctx.actions.symlink`
From the Bazel Rules API, `ctx.actions.symlink` creates a symlink action in a rule implementation. It is used to create a symbolic link from an output to an input file or directory:
```python
def _my_rule_impl(ctx):
...
| all_headers = [] | ||
|
|
||
| print("=== extract_headers: Starting header extraction ===") | ||
| print("Number of deps:", len(ctx.attr.deps)) | ||
|
|
||
| for dep in ctx.attr.deps: | ||
| if CcInfo not in dep: | ||
| print("Skipping dep (no CcInfo):", dep.label) | ||
| continue | ||
|
|
||
| print("Processing dep:", dep.label) | ||
| cc_info = dep[CcInfo] | ||
| compilation_context = cc_info.compilation_context | ||
|
|
||
| # headers: All headers needed for compilation (public + transitive) | ||
| headers_depset = compilation_context.headers | ||
| headers_list = headers_depset.to_list() | ||
| print(" Found", len(headers_list), "headers") | ||
|
|
||
| # Print first 10 headers as sample | ||
| for i, hdr in enumerate(headers_list[:10]): | ||
| print(" [", i, "]:", hdr.short_path) | ||
| if len(headers_list) > 10: | ||
| print(" ... and", len(headers_list) - 10, "more headers") | ||
|
|
||
| all_headers.append(headers_depset) | ||
|
|
||
| headers = depset(transitive = all_headers) | ||
| total_headers = len(headers.to_list()) | ||
| print("=== Total headers extracted:", total_headers, "===") | ||
|
|
||
| return [DefaultInfo(files = headers)] |
There was a problem hiding this comment.
[Performance] Debug print() statements and unnecessary depset.to_list() calls will execute during the Bazel analysis phase of every build that touches these targets. This spams build output with hundreds of lines and forces depset flattening (O(n) per dep, defeating Bazel's lazy depset merging), degrading analysis performance at scale.
Remove all print() calls and the .to_list() calls that only serve the debug output (lines 31-32, 36, 39, 45-52, 57-58).
| all_headers = [] | |
| print("=== extract_headers: Starting header extraction ===") | |
| print("Number of deps:", len(ctx.attr.deps)) | |
| for dep in ctx.attr.deps: | |
| if CcInfo not in dep: | |
| print("Skipping dep (no CcInfo):", dep.label) | |
| continue | |
| print("Processing dep:", dep.label) | |
| cc_info = dep[CcInfo] | |
| compilation_context = cc_info.compilation_context | |
| # headers: All headers needed for compilation (public + transitive) | |
| headers_depset = compilation_context.headers | |
| headers_list = headers_depset.to_list() | |
| print(" Found", len(headers_list), "headers") | |
| # Print first 10 headers as sample | |
| for i, hdr in enumerate(headers_list[:10]): | |
| print(" [", i, "]:", hdr.short_path) | |
| if len(headers_list) > 10: | |
| print(" ... and", len(headers_list) - 10, "more headers") | |
| all_headers.append(headers_depset) | |
| headers = depset(transitive = all_headers) | |
| total_headers = len(headers.to_list()) | |
| print("=== Total headers extracted:", total_headers, "===") | |
| return [DefaultInfo(files = headers)] | |
| all_headers = [] | |
| for dep in ctx.attr.deps: | |
| if CcInfo not in dep: | |
| continue | |
| cc_info = dep[CcInfo] | |
| compilation_context = cc_info.compilation_context | |
| all_headers.append(compilation_context.headers) | |
| headers = depset(transitive = all_headers) | |
| return [DefaultInfo(files = headers)] |
Context for Agents
Debug `print()` statements and unnecessary `depset.to_list()` calls will execute during the Bazel analysis phase of every build that touches these targets. This spams build output with hundreds of lines and forces depset flattening (O(n) per dep, defeating Bazel's lazy depset merging), degrading analysis performance at scale.
Remove all `print()` calls and the `.to_list()` calls that only serve the debug output (lines 31-32, 36, 39, 45-52, 57-58).
```suggestion
all_headers = []
for dep in ctx.attr.deps:
if CcInfo not in dep:
continue
cc_info = dep[CcInfo]
compilation_context = cc_info.compilation_context
all_headers.append(compilation_context.headers)
headers = depset(transitive = all_headers)
return [DefaultInfo(files = headers)]
```
File: score/mw/com/deploy/extract_headers_for_deps.bzl
Line: 60Chain of Thought
- Validation Decision: accepted
- Confidence: 0.97
- Likelihood Score: 5/5
- Likelihood Reason: The print() statements and to_list() calls are unconditional - they WILL execute on every build that touches these targets. This is inevitable, not theoretical.
- Suggested Action: keep
- Tool Calls Used: 2
Reasoning
The comment is accurate and well-supported by evidence:
-
New file, introduced by this PR: The file
extract_headers_for_deps.bzlis entirely new (all 79 lines are added). This is not pre-existing legacy debt. -
Technical claims are correct per Bazel documentation:
print()in Starlark runs during the analysis phase. The Bazel docs confirm: "print() in Starlark runs during the analysis phase. Using print() with to_list() for debugging is acceptable, but should not be left in production rule code."depset.to_list()defeats Bazel's lazy depset merging. The docs warn: "avoid any flattening of depsets except for debugging purposes" and note that callingto_list()in non-terminal targets leads to O(N²) behavior.
-
The code has clear debug artifacts:
- Lines 31-32: print statements for start/dep count
- Line 36: print for skipped deps
- Line 39: print for each processed dep
- Line 45:
headers_depset.to_list()used ONLY to support debug printing on lines 46, 49-52 - Line 57:
headers.to_list()used ONLY to print total count on line 58 - Both
to_list()calls serve no functional purpose; the actual logic only uses the depset (line 54 appendsheaders_depset, and line 60 returnsheaders)
-
Actionable: The comment provides a concrete code suggestion that removes all debug statements while preserving the correct functional logic. The suggested code properly collects depsets and merges them without flattening.
-
Performance impact is real: This is a non-terminal rule (used as a dependency target), so
to_list()calls here contribute to the documented O(N²) quadratic behavior when multiple targets with overlapping deps are built.
Evidence Summary
1. Library Docs
- Library:
bazel - Query:
depset to_list performance analysis phase print starlark
Here is the relevant Bazel documentation on **depset `to_list()` performance in the analysis phase (Starlark)**:
---
## Depset `to_list()` Performance
### ⚠️ Avoid Calling `depset.to_list()`
> *Source: [Bazel Rules Performance Guide](https://github.com/bazelbuild/bazel/blob/master/docs/rules/performance.mdx)*
...
|
|
||
| exports_files([ | ||
| "etc/mw_com_config.json", | ||
| "assert_handler.cpp", |
There was a problem hiding this comment.
[Maintainability] Exporting source files (.cpp, .h) via exports_files is generally an anti-pattern in Bazel. It encourages other packages to depend on implementation files directly, breaking encapsulation. The standard practice is to group sources into cc_library targets and have other packages depend on those libraries.
Since these files are being used by //score/mw/com/deploy:ipc_bridge_dynamic, a better approach would be to define a cc_library in this package that includes these sources, and then have the cc_binary in the deploy package depend on that library. This would also resolve the issue of cross-package source dependencies.
Context for Agents
Exporting source files (`.cpp`, `.h`) via `exports_files` is generally an anti-pattern in Bazel. It encourages other packages to depend on implementation files directly, breaking encapsulation. The standard practice is to group sources into `cc_library` targets and have other packages depend on those libraries.
Since these files are being used by `//score/mw/com/deploy:ipc_bridge_dynamic`, a better approach would be to define a `cc_library` in this package that includes these sources, and then have the `cc_binary` in the `deploy` package depend on that library. This would also resolve the issue of cross-package source dependencies.
File: score/mw/com/example/ipc_bridge/BUILD
Line: 105Chain of Thought
- Validation Decision: accepted
- Confidence: 0.90
- Likelihood Score: 4/5
- Likelihood Reason: The pattern of exporting source files for cross-package consumption is actively introduced in this PR. The deploy/BUILD already uses these exported files directly. If this pattern gets merged, it will encourage further anti-pattern usage and will create maintenance issues as these sources evolve. The existing cc_library targets in the same package demonstrate the proper approach.
- Suggested Action: keep
- Tool Calls Used: 4
Reasoning
The comment is accurate, actionable, and identifies a real issue introduced by this PR:
-
Accuracy: The claim that exporting
.cppand.hfiles viaexports_filesis an anti-pattern in Bazel is correct. Bazel's design encourages encapsulation throughcc_librarytargets. -
Introduced by this PR: In BASE, only
"etc/mw_com_config.json"was inexports_files. This PR adds 7 source files (assert_handler.cpp,assert_handler.h,datatype.cpp,datatype.h,sample_sender_receiver.cpp,sample_sender_receiver.h,main.cpp). -
Cross-package usage confirmed: The
deploy/BUILDfile'sipc_bridge_dynamiccc_binary (lines 421-439) directly references these exported files assrcs, confirming the cross-package source dependency. -
Existing cc_library targets already exist: The
ipc_bridge/BUILDalready hascc_librarytargets (sample_sender_receiverat line 35,datatypeat line 51) that properly wrap these source files. This makes the suggestion even more concrete - these targets could be reused or a new encompassing library could be created. -
Actionable: The comment suggests a concrete alternative - define a
cc_libraryin the ipc_bridge package that includes these sources, and have the deploy package'scc_binarydepend on that library instead.
The only nuance is that the deploy package may have intentionally compiled sources against a different dependency set (the dynamic score_com_dynamic instead of static deps). But even in that case, the proper Bazel approach would be to structure this differently rather than exporting raw source files.
Evidence Summary
1. File Snapshot (HEAD)
- File:
score/mw/com/deploy/BUILD
1: # *******************************************************************************
2: # Copyright (c) 2025 Contributors to the Eclipse Foundation
3: #
4: # See the NOTICE file(s) distributed with this work for additional
5: # information regarding copyright ownership.
6: #
7: # This program and the accompanying materials are made available under the
8: # terms of the Apache License Version 2.0 which is available at
9: # https://www.apache.org/licenses/LICENSE-2.0
10: #
...
2. File Snapshot (HEAD)
- File:
score/mw/com/example/ipc_bridge/BUILD
1: # *******************************************************************************
2: # Copyright (c) 2025 Contributors to the Eclipse Foundation
3: #
4: # See the NOTICE file(s) distributed with this work for additional
5: # information regarding copyright ownership.
6: #
7: # This program and the accompanying materials are made available under the
8: # terms of the Apache License Version 2.0 which is available at
9: # https://www.apache.org/licenses/LICENSE-2.0
10: #
...
3. File Snapshot (BASE)
- File:
score/mw/com/example/ipc_bridge/BUILD
1: # *******************************************************************************
2: # Copyright (c) 2025 Contributors to the Eclipse Foundation
3: #
4: # See the NOTICE file(s) distributed with this work for additional
5: # information regarding copyright ownership.
6: #
7: # This program and the accompanying materials are made available under the
8: # terms of the Apache License Version 2.0 which is available at
9: # https://www.apache.org/licenses/LICENSE-2.0
10: #
...
| features = COMPILER_WARNING_FEATURES, | ||
| tags = ["FFI"], | ||
| visibility = [ | ||
| "//score/mw/com:__subpackages__", |
There was a problem hiding this comment.
[Maintainability] Opening impl to //score/mw/com:__subpackages__ exposes all of the middleware implementation libraries (and, through similar changes elsewhere in this PR, tracing/plumbing/binding internals) to every package under score/mw/com, not just the deploy package that needs to aggregate symbols. This erodes the architectural boundary between the public API layer and the implementation layer: API code can now take hard dependencies on these internals, making them part of the contract and preventing you from refactoring or swapping the implementation without breaking consumers. Instead of widening visibility for the entire tree, keep these targets scoped to //score/mw/com/impl:__subpackages__ and grant access only to the packaging target (for example //score/mw/com/deploy:__pkg__ or a dedicated alias). That preserves layering while still allowing the shared-library build to pull the symbols it needs.
Context for Agents
Opening `impl` to `//score/mw/com:__subpackages__` exposes all of the middleware implementation libraries (and, through similar changes elsewhere in this PR, tracing/plumbing/binding internals) to every package under `score/mw/com`, not just the deploy package that needs to aggregate symbols. This erodes the architectural boundary between the public API layer and the implementation layer: API code can now take hard dependencies on these internals, making them part of the contract and preventing you from refactoring or swapping the implementation without breaking consumers. Instead of widening visibility for the entire tree, keep these targets scoped to `//score/mw/com/impl:__subpackages__` and grant access only to the packaging target (for example `//score/mw/com/deploy:__pkg__` or a dedicated alias). That preserves layering while still allowing the shared-library build to pull the symbols it needs.
File: score/mw/com/impl/BUILD
Line: 223Chain of Thought
- Validation Decision: accepted
- Confidence: 0.93
- Likelihood Score: 4/5
- Likelihood Reason: The overly-broad visibility is being actively introduced by this PR across many targets. Once merged, any package under score/mw/com (including example code, the public API layer, etc.) could take hard dependencies on implementation internals. This is a real architectural erosion that will be noticed/exploited as the codebase grows.
- Suggested Action: keep
- Tool Calls Used: 9
Reasoning
The comment accurately identifies a real issue introduced by this PR:
-
Before the PR: Visibility in
score/mw/com/impl/BUILDtargets was tightly scoped to specific packages (e.g.,//score/mw/com/impl/tracing:__subpackages__,//score/mw/com/impl/methods:__pkg__,//score/mw/com/impl/plumbing:__pkg__). Similarly, inbindings/lola/BUILDandplumbing/BUILD, visibility was narrow. -
After the PR:
//score/mw/com:__subpackages__is added across many targets. In some cases, the narrower//score/mw/com/impl:__subpackages__is replaced entirely with the broader scope. This grants access to ALL packages recursively underscore/mw/com/. -
Actual need: The new
score/mw/com/deploy/BUILDfile (added by this PR) is the primary consumer. Itscom_all_depstarget references impl targets directly. Only//score/mw/com/deploy:__pkg__needs access. -
The problem:
//score/mw/com:__subpackages__also includesscore/mw/com(public API),score/mw/com/example, and any future packages under that tree. This erodes the architectural boundary between public API and implementation. -
The suggestion: Using
//score/mw/com/deploy:__pkg__instead is a concrete, correct, and minimal-scope solution that preserves the existing layering.
The comment is accurate in its description of the issue, properly identifies this as a new concern introduced by the PR, and provides a specific actionable fix.
Evidence Summary
1. File Snapshot (HEAD)
- File:
score/mw/com/impl/tracing/BUILD - Lines:
1-50
1: # *******************************************************************************
2: # Copyright (c) 2025 Contributors to the Eclipse Foundation
3: #
4: # See the NOTICE file(s) distributed with this work for additional
5: # information regarding copyright ownership.
6: #
7: # This program and the accompanying materials are made available under the
8: # terms of the Apache License Version 2.0 which is available at
9: # https://www.apache.org/licenses/LICENSE-2.0
10: #
...
2. File Snapshot (HEAD)
- File:
score/mw/com/impl/bindings/lola/BUILD - Lines:
1-50
1: # *******************************************************************************
2: # Copyright (c) 2025 Contributors to the Eclipse Foundation
3: #
4: # See the NOTICE file(s) distributed with this work for additional
5: # information regarding copyright ownership.
6: #
7: # This program and the accompanying materials are made available under the
8: # terms of the Apache License Version 2.0 which is available at
9: # https://www.apache.org/licenses/LICENSE-2.0
10: #
...
3. File Snapshot (HEAD)
- File:
score/mw/com/deploy/BUILD
1: # *******************************************************************************
2: # Copyright (c) 2025 Contributors to the Eclipse Foundation
3: #
4: # See the NOTICE file(s) distributed with this work for additional
5: # information regarding copyright ownership.
6: #
7: # This program and the accompanying materials are made available under the
8: # terms of the Apache License Version 2.0 which is available at
9: # https://www.apache.org/licenses/LICENSE-2.0
10: #
...
4. File Snapshot (HEAD)
- File:
score/mw/com/impl/plumbing/BUILD - Lines:
1-50
1: # *******************************************************************************
2: # Copyright (c) 2025 Contributors to the Eclipse Foundation
3: #
4: # See the NOTICE file(s) distributed with this work for additional
5: # information regarding copyright ownership.
6: #
7: # This program and the accompanying materials are made available under the
8: # terms of the Apache License Version 2.0 which is available at
9: # https://www.apache.org/licenses/LICENSE-2.0
10: #
...
5. File Snapshot (BASE)
- File:
score/mw/com/impl/plumbing/BUILD - Lines:
18-34
18: cc_library(
19: name = "plumbing",
20: features = COMPILER_WARNING_FEATURES,
21: tags = ["FFI"],
22: visibility = [
23: "//score/mw/com/impl:__subpackages__",
24: ],
25: deps = [
26: ":binding_runtime_factory",
27: ":proxy_binding_factory",
...
6. File Snapshot (BASE)
- File:
score/mw/com/impl/bindings/lola/BUILD - Lines:
17-36
17: cc_library(
18: name = "lola",
19: features = COMPILER_WARNING_FEATURES,
20: tags = ["FFI"],
21: visibility = [
22: "//score/mw/com/impl/bindings/lola/test:__pkg__",
23: "//score/mw/com/impl/plumbing:__pkg__",
24: ],
25: deps = [
26: ":event",
...
7. File Snapshot (BASE)
- File:
score/mw/com/impl/tracing/BUILD - Lines:
17-50
17: package(
18: default_visibility = [
19: "//score/mw/com/impl/tracing:__subpackages__",
20: ],
21: )
22:
23: cc_library(
24: name = "type_erased_sample_ptr",
25: srcs = ["type_erased_sample_ptr.cpp"],
26: hdrs = ["type_erased_sample_ptr.h"],
...
| features = COMPILER_WARNING_FEATURES, | ||
| tags = ["FFI"], | ||
| visibility = ["//score/mw/com/impl:__subpackages__"], | ||
| visibility = ["//score/mw/com:__subpackages__", |
There was a problem hiding this comment.
[Maintainability] Expanding the visibility of tracing internals to the entire //score/mw/com:__subpackages__ namespace (the only change in this hunk) effectively makes these FFI helpers part of the public API for all COM subpackages. That breaks the previous encapsulation boundary and invites accidental coupling far beyond the new deployment target. If the only new consumer is //score/mw/com/deploy, scope the visibility to that package instead of the whole tree, e.g.:
visibility = [
"//score/mw/com/deploy:__pkg__",
"//score/mw/com/impl:__subpackages__",
]Please apply the same restrictive visibility to the other tracing/binding targets that were broadened in this PR to keep implementation details hidden from unrelated modules.
Context for Agents
Expanding the visibility of tracing internals to the entire `//score/mw/com:__subpackages__` namespace (the only change in this hunk) effectively makes these FFI helpers part of the public API for *all* COM subpackages. That breaks the previous encapsulation boundary and invites accidental coupling far beyond the new deployment target. If the only new consumer is `//score/mw/com/deploy`, scope the visibility to that package instead of the whole tree, e.g.:
```python
visibility = [
"//score/mw/com/deploy:__pkg__",
"//score/mw/com/impl:__subpackages__",
]
```
Please apply the same restrictive visibility to the other tracing/binding targets that were broadened in this PR to keep implementation details hidden from unrelated modules.
File: score/mw/com/impl/tracing/BUILD
Line: 137Chain of Thought
- Validation Decision: accepted
- Confidence: 0.95
- Likelihood Score: 4/5
- Likelihood Reason: The overly broad visibility is already in place in HEAD. Without correction, any package under score/mw/com/ can now depend on these internal tracing targets, creating coupling that will be hard to undo as the codebase grows. Given this is a framework/library codebase, accidental dependencies are likely to accrue.
- Suggested Action: keep
- Tool Calls Used: 9
Reasoning
The comment is accurate and actionable:
-
Accurate description of the change: The diff shows visibility was broadened from
["//score/mw/com/impl:__subpackages__"]to["//score/mw/com:__subpackages__", "//score/mw/com/impl:__subpackages__"]across three tracing targets. Thecom:__subpackages__entry is strictly a superset ofimpl:__subpackages__, so this is a significant broadening. -
Only consumer is deploy: Looking at
score/mw/com/deploy/BUILD, lines 272 and 276-277 show the deploy package depends on exactly the three broadened tracing targets (tracing_runtime,skeleton_event_tracing,proxy_event_tracing). No other new consumer outside ofimpl:__subpackages__is introduced. -
The suggestion is correct: Using
//score/mw/com/deploy:__pkg__instead of//score/mw/com:__subpackages__follows the principle of least privilege. The suggested visibility["//score/mw/com/deploy:__pkg__", "//score/mw/com/impl:__subpackages__"]correctly preserves existing access while adding only the specific new consumer. -
Cross-file consistency: The same broadening pattern appears in
bindings/lola/BUILDandplumbing/BUILD, making the request to apply restrictive visibility there as well consistent and valid. -
Introduced by this PR: This is a new change, not pre-existing debt. The PR explicitly widened these visibility rules.
Evidence Summary
1. File Snapshot (HEAD)
- File:
score/mw/com/deploy/BUILD
1: # *******************************************************************************
2: # Copyright (c) 2025 Contributors to the Eclipse Foundation
3: #
4: # See the NOTICE file(s) distributed with this work for additional
5: # information regarding copyright ownership.
6: #
7: # This program and the accompanying materials are made available under the
8: # terms of the Apache License Version 2.0 which is available at
9: # https://www.apache.org/licenses/LICENSE-2.0
10: #
...
2. File Snapshot (HEAD)
- File:
score/mw/com/impl/plumbing/BUILD - Lines:
1-50
1: # *******************************************************************************
2: # Copyright (c) 2025 Contributors to the Eclipse Foundation
3: #
4: # See the NOTICE file(s) distributed with this work for additional
5: # information regarding copyright ownership.
6: #
7: # This program and the accompanying materials are made available under the
8: # terms of the Apache License Version 2.0 which is available at
9: # https://www.apache.org/licenses/LICENSE-2.0
10: #
...
3. File Snapshot (HEAD)
- File:
score/mw/com/impl/BUILD - Lines:
1-50
1: # *******************************************************************************
2: # Copyright (c) 2025 Contributors to the Eclipse Foundation
3: #
4: # See the NOTICE file(s) distributed with this work for additional
5: # information regarding copyright ownership.
6: #
7: # This program and the accompanying materials are made available under the
8: # terms of the Apache License Version 2.0 which is available at
9: # https://www.apache.org/licenses/LICENSE-2.0
10: #
...
4. File Snapshot (HEAD)
- File:
score/mw/com/impl/bindings/lola/BUILD - Lines:
1-50
1: # *******************************************************************************
2: # Copyright (c) 2025 Contributors to the Eclipse Foundation
3: #
4: # See the NOTICE file(s) distributed with this work for additional
5: # information regarding copyright ownership.
6: #
7: # This program and the accompanying materials are made available under the
8: # terms of the Apache License Version 2.0 which is available at
9: # https://www.apache.org/licenses/LICENSE-2.0
10: #
...
5. File Snapshot (BASE)
- File:
score/mw/com/impl/bindings/lola/BUILD - Lines:
17-36
17: cc_library(
18: name = "lola",
19: features = COMPILER_WARNING_FEATURES,
20: tags = ["FFI"],
21: visibility = [
22: "//score/mw/com/impl/bindings/lola/test:__pkg__",
23: "//score/mw/com/impl/plumbing:__pkg__",
24: ],
25: deps = [
26: ":event",
...
6. File Snapshot (BASE)
- File:
score/mw/com/impl/plumbing/BUILD - Lines:
18-35
18: cc_library(
19: name = "plumbing",
20: features = COMPILER_WARNING_FEATURES,
21: tags = ["FFI"],
22: visibility = [
23: "//score/mw/com/impl:__subpackages__",
24: ],
25: deps = [
26: ":binding_runtime_factory",
27: ":proxy_binding_factory",
...
7. File Snapshot (BASE)
- File:
score/mw/com/impl/tracing/BUILD - Lines:
130-250
130:
131: cc_library(
132: name = "skeleton_event_tracing",
133: srcs = ["skeleton_event_tracing.cpp"],
134: hdrs = ["skeleton_event_tracing.h"],
135: features = COMPILER_WARNING_FEATURES,
136: tags = ["FFI"],
137: visibility = ["//score/mw/com/impl:__subpackages__"],
138: deps = [
139: ":common_event_tracing",
...
8. File Snapshot (BASE)
- File:
score/mw/com/impl/BUILD - Lines:
17-40
17: package(default_visibility = [
18: "//score/mw/com/impl:__subpackages__",
19: ])
20:
21: cc_library(
22: name = "impl",
23: features = COMPILER_WARNING_FEATURES,
24: tags = ["FFI"],
25: visibility = [
26: "//score/mw/com:__subpackages__",
...
| """Implementation function to extract PUBLIC headers from CcInfo provider. | ||
|
|
||
| This extracts: | ||
| 1. Direct public headers (from 'hdrs' attribute) of specified deps | ||
| 2. Transitive public headers from all dependencies | ||
|
|
||
| It does NOT include: | ||
| - Private headers (from 'srcs' attribute with .h extension) | ||
| - Implementation files | ||
| """ |
There was a problem hiding this comment.
[Documentation] The docstring (lines 17-25) claims this rule does NOT include private headers from srcs, but the implementation uses compilation_context.headers (line 44) which includes all headers (public and private) from the target and its transitive deps. The rule-level doc (line 76-77) even acknowledges this. The function docstring is misleading and will cause users to incorrectly trust that only public headers are extracted.
Update the docstring to accurately reflect the behavior:
| """Implementation function to extract PUBLIC headers from CcInfo provider. | |
| This extracts: | |
| 1. Direct public headers (from 'hdrs' attribute) of specified deps | |
| 2. Transitive public headers from all dependencies | |
| It does NOT include: | |
| - Private headers (from 'srcs' attribute with .h extension) | |
| - Implementation files | |
| """ | |
| """Implementation function to extract headers from CcInfo provider. | |
| This extracts all headers from compilation_context.headers, which includes: | |
| 1. Public headers (from 'hdrs' attribute) of specified deps | |
| 2. Private headers (from 'srcs' with .h extension) of specified deps | |
| 3. All transitive headers from dependencies | |
| Consumers should apply filtering (e.g., copy_headers) to narrow the set. | |
| """ |
Context for Agents
The docstring (lines 17-25) claims this rule does NOT include private headers from `srcs`, but the implementation uses `compilation_context.headers` (line 44) which includes **all** headers (public and private) from the target and its transitive deps. The rule-level doc (line 76-77) even acknowledges this. The function docstring is misleading and will cause users to incorrectly trust that only public headers are extracted.
Update the docstring to accurately reflect the behavior:
```suggestion
"""Implementation function to extract headers from CcInfo provider.
This extracts all headers from compilation_context.headers, which includes:
1. Public headers (from 'hdrs' attribute) of specified deps
2. Private headers (from 'srcs' with .h extension) of specified deps
3. All transitive headers from dependencies
Consumers should apply filtering (e.g., copy_headers) to narrow the set.
"""
```
File: score/mw/com/deploy/extract_headers_for_deps.bzl
Line: 26Chain of Thought
- Validation Decision: accepted
- Confidence: 0.95
- Likelihood Score: 4/5
- Likelihood Reason: The misleading docstring will likely cause users to believe only public headers are extracted, when in fact private headers from srcs are also included. Since this is a build rule for header packaging/deployment, this misunderstanding could lead to incorrect assumptions about what's being packaged.
- Suggested Action: keep
- Tool Calls Used: 3
Reasoning
The comment is accurate and well-supported by evidence:
-
Bazel documentation confirms that
compilation_context.headersincludes both public AND private headers from the target and its transitive dependencies. Bazel providesdirect_public_headersanddirect_private_headersas separate fields for distinguishing between them. -
The docstring is factually incorrect: Lines 23-24 explicitly state "It does NOT include: Private headers (from 'srcs' attribute with .h extension)" — but the implementation at line 44 uses
compilation_context.headerswhich DOES include private headers. -
Internal contradiction: The inline comment at line 43 says "All headers needed for compilation (public + transitive)" and the rule-level doc at lines 76-77 partially acknowledges this by saying "compilation_context.headers includes all headers needed for compilation."
-
This is newly introduced code — the entire file is added in this PR, so this is not pre-existing debt.
-
The suggestion is actionable: It provides a concrete replacement docstring that accurately describes the behavior and mentions that consumers should apply filtering.
-
The
copy_headers.bzlfile in the same PR does apply filtering (excluding certain directories), which confirms the suggested note about filtering is relevant and accurate.
Evidence Summary
1. File Snapshot (HEAD)
- File:
score/mw/com/deploy/extract_headers_for_deps.bzl
1: # *******************************************************************************
2: # Copyright (c) 2025 Contributors to the Eclipse Foundation
3: #
4: # See the NOTICE file(s) distributed with this work for additional
5: # information regarding copyright ownership.
6: #
7: # This program and the accompanying materials are made available under the
8: # terms of the Apache License Version 2.0 which is available at
9: # https://www.apache.org/licenses/LICENSE-2.0
10: #
...
2. Library Docs
- Library:
bazel - Query:
compilation_context.headers CcInfo public private headers
The initial results mention `CcInfo` and `CompilationContext` but don't detail the `headers` field specifically. Let me search more precisely.Here is the relevant documentation I found on **`CcInfo`**, **`CompilationContext`**, and **headers** in Bazel:
---
## `CcInfo` Provider
Source: [Bazel C++ Integration](https://github.com/bazelbuild/bazel/blob/master/site/en/configure/integrate-cpp.md)
Most C++ rules provide **`CcInfo`**, a provider containing:
...
3. File Snapshot (HEAD)
- File:
score/mw/com/deploy/copy_headers.bzl
1: # *******************************************************************************
2: # Copyright (c) 2025 Contributors to the Eclipse Foundation
3: #
4: # See the NOTICE file(s) distributed with this work for additional
5: # information regarding copyright ownership.
6: #
7: # This program and the accompanying materials are made available under the
8: # terms of the Apache License Version 2.0 which is available at
9: # https://www.apache.org/licenses/LICENSE-2.0
10: #
...
| """Copy headers from extracted headers into include/ directory structure. | ||
|
|
||
| Filters out headers from nlohmann, boost, and visitor top-level directories. | ||
| """ |
There was a problem hiding this comment.
[Documentation] The docstring states "Filters out headers from nlohmann, boost, and visitor top-level directories" but boost is commented out in the exclude list (line 25). This discrepancy will mislead anyone relying on this documentation.
Either update the docstring to match reality or uncomment the boost exclusion:
| """Copy headers from extracted headers into include/ directory structure. | |
| Filters out headers from nlohmann, boost, and visitor top-level directories. | |
| """ | |
| """Copy headers from extracted headers into include/ directory structure. | |
| Filters out headers from nlohmann and visitor top-level directories. | |
| Note: boost headers are NOT filtered due to linux dependency requirements. | |
| """ |
Context for Agents
The docstring states "Filters out headers from nlohmann, **boost**, and visitor top-level directories" but boost is commented out in the exclude list (line 25). This discrepancy will mislead anyone relying on this documentation.
Either update the docstring to match reality or uncomment the boost exclusion:
```suggestion
"""Copy headers from extracted headers into include/ directory structure.
Filters out headers from nlohmann and visitor top-level directories.
Note: boost headers are NOT filtered due to linux dependency requirements.
"""
```
File: score/mw/com/deploy/copy_headers.bzl
Line: 20Chain of Thought
- Validation Decision: accepted
- Confidence: 0.97
- Likelihood Score: 5/5
- Likelihood Reason: The discrepancy between the docstring (claims boost is filtered) and the code (boost exclusion is commented out) is definitively present and will inevitably mislead anyone reading the documentation.
- Suggested Action: keep
- Tool Calls Used: 2
Reasoning
The comment correctly identifies a clear documentation/code mismatch in a newly added file:
- Line 19 (docstring): States "Filters out headers from nlohmann, boost, and visitor top-level directories."
- Line 25 (code):
#"boost"is commented out with an explanatory note: "could not remove this due to linux dependency."
This means boost headers are NOT being filtered, directly contradicting the docstring. The file is brand new (doesn't exist in BASE), so this is entirely introduced by this PR.
The comment is accurate, actionable, and provides a concrete suggestion to fix the docstring to reflect the actual behavior. The suggestion correctly notes that boost headers are NOT filtered due to linux dependency requirements, which aligns with the inline code comment.
Evidence Summary
1. File Snapshot (HEAD)
- File:
score/mw/com/deploy/copy_headers.bzl - Lines:
16-27
16: def _copy_headers_impl(ctx):
17: """Copy headers from extracted headers into include/ directory structure.
18:
19: Filters out headers from nlohmann, boost, and visitor top-level directories.
20: """
21:
22: # Define top-level directory names to exclude
23: exclude_top_level_dirs = [
24: "nlohmann",
25: #"boost", # could not remove this due to linux dependency.
...
2 participants
Creating a Pull Request to demonstrate the Issues faced for creating a Dynamic Library.
This is temp Pull Request for Score team to review and comment. Provide suggestion on how to proceed.
Enable Dynamic Library and Header Package Creation for SCORE Communication Middleware
This experimental pull request demonstrates and enables the creation of a dynamic library (
libscore_communication.so) and its corresponding public header package for the SCORE Communication middleware using Bazel. The primary motivation is to address the issue where Bazel'scc_shared_libraryrule does not automatically include symbols from transitive dependencies, leading to runtime 'undefined symbol' errors. The solution involves explicitly listing all required transitive dependencies in an aggregationcc_librarytarget (com_all_deps) and introducing custom Bazel rules (extract_headers,copy_headers) to manage public header extraction and filtering for deployment.The PR includes extensive documentation in
SYMBOL_INCLUSION_ANALYSIS.mdexplaining the problem, the chosen two-phase solution (extraction then filtering), and best practices for Bazel shared library and header management. It also updates variousBUILDfiles to adjustvisibilitysettings, allowing broader access to internal components necessary for the comprehensive shared library build.Architecture Diagram:
flowchart TD subgraph Build_System BazelBuildSystem["Bazel Build System"] extract_headers_rule["extract_headers (+79 lines)"] copy_headers_rule["copy_headers (+123 lines)"] end subgraph Communication_Middleware_Build com_all_deps["cc_library: com_all_deps (+100s lines)"] libscore_communication_so["cc_shared_library: libscore_communication.so (+100s lines)"] end subgraph Artifacts RawHeaders["Raw Extracted Headers"] PublicHeaders["Filtered Public Headers"] AppBinary["cc_binary: ipc_bridge_dynamic"] end subgraph Documentation SymbolAnalysisDoc["SYMBOL_INCLUSION_ANALYSIS.md (+1047 lines)"] end BazelBuildSystem -->|defines & orchestrates| com_all_deps com_all_deps -->|provides all symbols to| libscore_communication_so libscore_communication_so -->|linked by| AppBinary BazelBuildSystem -->|uses| extract_headers_rule extract_headers_rule -->|extracts from| com_all_deps extract_headers_rule -->|outputs| RawHeaders BazelBuildSystem -->|uses| copy_headers_rule copy_headers_rule -->|filters & copies| RawHeaders copy_headers_rule -->|outputs| PublicHeaders PublicHeaders -->|used by| AppBinary SymbolAnalysisDoc -->|explains build logic & issues| BazelBuildSystem SymbolAnalysisDoc -->|explains shared library behavior| libscore_communication_soThis summary was automatically generated by @propel-code-bot