add design decision include path by RolandJentschETAS · Pull Request #3196 · eclipse-score/score

RolandJentschETAS · 2026-08-19T14:22:28Z

add design decision include path

github-actions · 2026-08-19T14:27:32Z

Documentation preview for this pull request is available at:
pr-3196: https://eclipse-score.github.io/score/pr-3196/

anmittag · 2026-08-20T11:43:43Z

TLC 2026-08-20: @qor-lb , @arsibo please review

aschemmel-tech

Option B conflicts with DR-003-proc (as in #3194).

LittleHuba

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.

LittleHuba · 2026-08-20T13:06:48Z

+  to learn which headers are public.
+- **Accidental API exposure:** A private header may be listed in `hdrs` by mistake,
+  or a consumer may reach for an internal header.
+- **Poor scalability:** As the library grows the folder becomes cluttered.


Disagree on this. You can create subdirectories.

Agreed and changed, A flat package can be organized with subdirectories and per-package targets, so "clutter" was overstated. This disadvantage has been removed; Option A is now the recommended layout and its write-up no longer claims poor scalability.

LittleHuba · 2026-08-20T13:12:25Z

+- **Zero Bazel path overhead:** No `strip_include_prefix` / `includes` juggling. The
+  header's on-disk path *is* its include path, so there is one obvious way to write
+  an `#include` and no divergence between what the file tree shows and what the
+  compiler sees.


This is actually quite a crucial advantage. Using strip_include_prefix messes with external tools. Relying on this option would make packaging tremendously hard since you would have to rewrite source to actually achieve a compilation with any tool that is not aware of bazel.

Agreed. It is now called out as an advantage and in the consumer-view table ("works with non-Bazel tools: yes").

LittleHuba · 2026-08-20T13:13:11Z

+- **Easier local navigation:** Declarations sit next to their implementations; no
+  jumping between distant folders in the IDE. Refactoring that splits or renames a
+  unit touches a single directory, and "go to file" / fuzzy-open lands on the pair
+  immediately.


This is not just about navigation. This is also about maintainability. By vicinity of code in the source tree you also know likely affected code.

Agreed. Option A now lists the maintainability/vicinity benefit ("the source tree makes likely-affected code visible by vicinity — a maintainability property, not just a navigation convenience"), and it's a rationale point.

LittleHuba · 2026-08-20T13:17:41Z

+- **High encapsulation:** The public contract is physically isolated. It is
+  immediately clear to a consumer what may be used.


Not really true. E.g. we include private headers in public headers for templated APIs. There we also leak private symbols which could be used by users. This is a false promise.
See https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1204r0.html#src-dir

Also consider that Bazel prohibits you refering to files in other packages. So your Bazel rules would become very awkward if you would not put all headers into includes that are referenced in at least one public header.

Also Agreed. This is now treated as the decisive argument. Option A solves it with impl/ + restricted visibility, which holds even for templated public headers.

LittleHuba · 2026-08-20T13:18:53Z

+- **Clean BUILD files:** `hdrs = glob(["include/**/*.h"])` and
+  `srcs = glob(["src/**"])` without risk of accidentally exposing private files.


How can you raise globbing here as a advantage when you state it in option A as an advantage that it is avoided?

Sorry that was contradictory. Both glob-related claims were removed.

LittleHuba · 2026-08-20T14:54:04Z

+Public headers go into `hdrs` and are exposed through `strip_include_prefix` so that
+the on-disk `include/` prefix is removed from the include path consumers see:


Doing this, completely breaks other tools because headers are not where source files expect them.

Agreed — this is now explicitly documented as an Option B/C disadvantage and an Option A advantage (include path == on-disk location), including in the consumer-view table.

LittleHuba · 2026-08-20T14:55:39Z

+
+| Option | Producer `BUILD.bazel` (public headers) | Consumer `#include` |
+|--------|------------------------------------------|---------------------|
+| **A: Flat** | `hdrs = glob(["*.h"])` — no prefix handling | `#include "libs/my_lib/my_lib.h"` — leaks repo layout, not namespaced |


Where does the libs come from? We do not have this today.

Fixed — all examples now use real S-CORE paths (score/mw/my_component/…) instead of the invented libs/my_lib/.

LittleHuba · 2026-08-20T14:57:09Z

+- **General open-source convention (dedicated `include/`):** The
+  [Pitchfork Layout (PFL)](https://github.com/vector-of-bool/pitchfork) and the
+  [WG21 Canonical Project Structure (P1204R0)](https://open-std.org/JTC1/SC22/WG21/docs/papers/2018/p1204r0.html)
+  both place public headers under a project-named `include/<lib>/` root — matching
+  **Options B/C**.


This is just wrong.

There is no general open-source convention. There are many project examples for both.

Canonical Project Structure explicitly mentions pitch forking as a bad practice that should not be followed - see https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1204r0.html#src-dir

The "general open-source convention (dedicated include/)" claim is removed; the guidance now notes both styles exist.

LittleHuba · 2026-08-20T15:02:01Z

+- **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.


This is why we are strict about full include paths in our source in score_communication.
The same logic applies for option B. It is just enforced because without providing the full path you simply don't find the header. The result in your source files is the same.

Agreed. Option A adopts exactly that rule — full project-prefixed repo-root include paths — and the DR notes that the resulting #include in source is effectively the same discipline communication already enforces, without needing strip_include_prefix.

LittleHuba · 2026-08-20T15:02:53Z

+## 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.


As stated previously, this discussion is not related to the main topic of this DR. It should be handled separately to avoid confusion.

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 ?

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.

aschemmel-tech

Thanks for the rework, option A selected matches with DR-003-proc

add design decision include path

e0e2cc0

RolandJentschETAS requested review from arsibo and qor-lb as code owners August 19, 2026 14:22

github-project-automation Bot added this to TLC - Technical Lead Circle and S-CORE Roadmap Aug 19, 2026

github-project-automation Bot moved this to Backlog in S-CORE Roadmap Aug 19, 2026

add file name collision

8985ad9

anmittag assigned RolandJentschETAS Aug 20, 2026

anmittag removed this from S-CORE Roadmap Aug 20, 2026

anmittag removed this from TLC - Technical Lead Circle Aug 20, 2026

anmittag added this to ARC- Architecture Community Aug 20, 2026

aschemmel-tech requested changes Aug 20, 2026

View reviewed changes

LittleHuba requested changes Aug 20, 2026

View reviewed changes

fix review findings

3a1c0da

aschemmel-tech approved these changes Aug 21, 2026

View reviewed changes

		- High encapsulation: The public contract is physically isolated. It is
		immediately clear to a consumer what may be used.

		- Clean BUILD files: `hdrs = glob(["include/*/.h"])` and
		`srcs = glob(["src/**"])` without risk of accidentally exposing private files.

		Public headers go into `hdrs` and are exposed through `strip_include_prefix` so that
		the on-disk `include/` prefix is removed from the include path consumers see:

Uh oh!

Conversation

RolandJentschETAS commented Aug 19, 2026

Uh oh!

github-actions Bot commented Aug 19, 2026

Uh oh!

anmittag commented Aug 20, 2026

Uh oh!

aschemmel-tech left a comment

Choose a reason for hiding this comment

Uh oh!

LittleHuba left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LittleHuba Aug 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aschemmel-tech left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

LittleHuba Aug 20, 2026 •

edited

Loading