add design decision include path - #3196
Conversation
github-project-automation
Bot
added this to
TLC - Technical Lead Circle and S-CORE Roadmap
|
Documentation preview for this pull request is available at: |
aschemmel-tech
left a comment
aschemmel-tech
left a comment
There was a problem hiding this comment.
Option B conflicts with DR-003-proc (as in #3194).
LittleHuba
left a comment
LittleHuba
left a comment
There was a problem hiding this comment.
I find this proposal heavily skewed towards a pitchfork layout (option B) (even clearly stating pitchfork layout in the evaluation criteria. Based on that, I find it hard to base a discussion on this, since it feels like the arguments were written based on the decision instead of the other way round.
From my comments you can probably guess that I'm clearly leaning towards option A. And this review felt like a uphill battle from the start. Looking just at the advantages/disadvantages of options A and B, the evaluation should not be leaning so heavily towards option B.
Two sources for reference why I heavily prefer option A:
- https://bazel.build/tutorials/cpp-use-cases where the pitch fork layout is clearly only stated as a legacy adoption use case. This means that Bazel is not built for this project structure and that you will feel lots of friction trying to use it.
- https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1204r0.html#src-dir has a full section just talking about why pitch fork layout is bad and should not be used
Side note: I appreciate the accepted alternative that will safe communication and baselibs from having to rewrite 90% of their code. I still do not feel comfortable agreeing with the final decision because of the consequences for the overall project.
| ## Header Name Collisions Across Modules | ||
|
|
||
| A common worry is what happens when several modules expose an identically named | ||
| header — for example `error.h`. The important point is that collisions are decided by | ||
| the **include-path string**, not the file name. Two `error.h` files coexist without | ||
| issue as long as their include paths differ: | ||
|
|
||
| ```cpp | ||
| #include "score/filesystem/error.h" // baselibs | ||
| #include "score/concurrency/future/error.h" // baselibs | ||
| ``` | ||
|
|
||
| Both exist side by side in `baselibs` today with no conflict, because the package | ||
| prefix makes them unique. A problem only arises when the path is shortened to the bare | ||
| file name and two dependencies provide it: | ||
|
|
||
| ```cpp | ||
| #include "error.h" // provided by module A AND module B → ambiguous | ||
| ``` | ||
|
|
||
| If a target depends on both libraries, `-I`/`-isystem` ordering decides which file | ||
| wins — the wrong header may be included, silently violating the One Definition Rule. | ||
| The **Bazel module name does not protect against this**: `@module_a` / `@module_b` do | ||
| not appear in the C++ include path by default; the path is determined solely by the | ||
| package location and by `strip_include_prefix` / `include_prefix` / `includes`. | ||
|
|
||
| Consequences per option: | ||
|
|
||
| - **Option A (flat, repo-root includes):** collision-safe *as long as* headers are | ||
| included by their full repo-root path. The danger is resetting the prefix | ||
| (`strip_include_prefix = "."` as in `utils/base64`, or `includes = ["."]`), which | ||
| collapses the path to `#include "error.h"` and re-introduces the ambiguity globally. | ||
| - **Option B (`include/<component>/`):** collision-safe, but the protection comes from | ||
| the **component-name nesting**, not from the `include/` directory itself. A flat | ||
| `include/error.h` (without the `<component>` subdirectory) still collides. | ||
| - **Option C (Hybrid):** same as B — public headers stay unique via | ||
| `include/<component>/`. | ||
| - **Flat + `impl/` + visibility:** same as A — the repo-root path is preserved, and | ||
| the restricted `impl/` visibility additionally shrinks the set of externally | ||
| reachable headers. | ||
|
|
||
| **Rule:** Uniqueness must be guaranteed by the include-path **prefix** (the project or | ||
| component name) — via the repo package path under Option A, or via the | ||
| `include/<component>/` nesting under Options B/C. A bare `include/` without a | ||
| component-named subdirectory does **not** solve the problem. | ||
|
|
||
| --- | ||
|
|
||
| ## Consequences | ||
|
|
||
| ### Positive | ||
|
|
||
| - The public API of every C++ component is visible, isolated, and self-documenting. | ||
| - Bazel `hdrs`/`srcs` boundaries align with the physical layout, reducing accidental | ||
| API leakage. | ||
| - Consumers get stable, collision-free include paths regardless of how the dependency | ||
| is resolved (in-repo, override, or registry module). | ||
| - Public SDK packaging for non-Bazel consumers is a folder copy. | ||
| - The layout is consistent with widely used community conventions (PFL, P1204). | ||
|
|
||
| ### Negative / Costs | ||
|
|
||
| - A modest, one-time increase in Bazel boilerplate (`strip_include_prefix`) and the | ||
| `include/<component>/` nesting. | ||
| - Minor day-to-day navigation overhead from the `include/` ↔ `src/` split. | ||
| - Existing flat components must be migrated to gain the benefits (can be incremental). | ||
|
|
||
| ### Follow-Up Actions | ||
|
|
||
| - Provide a component template / scaffolding (directory skeleton + `BUILD.bazel`) that | ||
| encodes the `include/<component>/` + `src/` layout and `strip_include_prefix`. | ||
| - Document the convention in the S-CORE contribution guidelines and C++ coding | ||
| guidelines, including the private-vs-public include-path rules. | ||
| - Define a migration path for existing flat components (opportunistic, per module). | ||
| - Consider a lightweight CI/lint check that flags private headers appearing in `hdrs` | ||
| or public headers being included via non-canonical paths. |
There was a problem hiding this comment.
As stated previously, this discussion is not related to the main topic of this DR. It should be handled separately to avoid confusion.
There was a problem hiding this comment.
Agreed. Header-name collisions are orthogonal to the public-header layout decision — they're governed by the include-path prefix, which every option can get right or wrong independently. Keeping a full analysis here blurs the actual decision. It's not part of the discussion of the option discussion anymore. But an general discussion. Shall I remove that from the PR or move it somewhere else ?
There was a problem hiding this comment.
IMO we need a separate DR for the topic of include path collision and symbol collision. AFAIK there were already some initial talks in some meetings in S-CORE.
There was a problem hiding this comment.
I have remove that chapter. Can be introduced like recommended into an own design decision.
@LittleHuba : Can you please check now again ?
aschemmel-tech
left a comment
aschemmel-tech
left a comment
There was a problem hiding this comment.
Thanks for the rework, option A selected matches with DR-003-proc
| - The public surface is discovered via the `impl/` convention and Bazel visibility | ||
| rather than a single `include/` folder, so the convention must be applied | ||
| consistently. | ||
| - Contributors must consistently use full project-prefixed include paths and must not |
There was a problem hiding this comment.
We should also encounter how we check new sources and monitor the structure
There was a problem hiding this comment.
There was some work in clang-tidy that went in this direction but was never finished:
llvm/llvm-project#63226
| Bazel `hdrs`/`srcs` split. This is the current de-facto S-CORE convention. | ||
| Headers and sources live **next to each other** in the component's package, and the | ||
| public/private boundary is drawn by Bazel: public headers go in `hdrs`, everything | ||
| non-public is isolated in an `impl/` subpackage whose `visibility` is restricted to |
There was a problem hiding this comment.
Not quite sure how prescribing this impl package makes things better/worse.
While we have this pattern in mw::com, it is honestly a lie to some extent (see https://github.com/eclipse-score/communication/blob/main/score/mw/com/types.h).
For mw::com this is something we will probably never change, but other libraries might not want that pattern.
Before we make users deduce public API from directory structures we should actually provide clear documentation with the public interface. In communication we work with breathe, Sphinx and these tags to mark public API: https://github.com/eclipse-score/communication/blob/main/score/mw/com/types.h#L37
TL;DR: I vote for removing the impl package guideline in favor of describing the public API via tagging and a generated documentation, while leaving projects to come up with a good source structure based on their architecture.
5 participants
add design decision include path