Skip to content

Commit 35d2006

Browse files
SunrisepeakSunrisepeak
authored
Implement dependency usage requirements (#101)
* Implement dependency usage requirements * Fix ninja include flag test on Windows
1 parent 75cf6c0 commit 35d2006

14 files changed

Lines changed: 1006 additions & 121 deletions

‎.agents/docs/2026-06-02-imgui-mcpp-dependency-fixes.md‎

Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,121 @@
1+
# 2026-06-02 imgui mcpp dependency fixes
2+
3+
## Scope
4+
5+
Branch: `codex/imgui-mcpp-dependency-fixes`
6+
7+
This document tracks the mcpp-side changes discovered while validating the
8+
`imgui-private` package and its `compat.imgui` / `compat.glfw` /
9+
`compat.opengl` dependency chain. The imgui package work stays outside this
10+
branch; this branch is only for mcpp fixes.
11+
12+
## Root Cause Chain
13+
14+
The imgui validation exposed two separate mcpp-side problems.
15+
16+
1. C++ build flags dropped package include dirs.
17+
18+
`src/build/flags.cppm` built `CompileFlags::cxx` with fewer `{}` format
19+
placeholders than arguments. The final `include_flags` argument was not
20+
emitted into the C++ baseline flags, while `CompileFlags::cc` did include it.
21+
22+
Impact: module scanning and C++ compilation for the current package could
23+
miss include dirs propagated from direct dependencies. In imgui, this showed
24+
up as scanner failures like `imgui.h` or backend `.cpp` files not found.
25+
26+
Why gtest did not prove this path: gtest's existing success path did not
27+
exercise the same current-package C++ baseline include propagation shape. It
28+
could compile through its own package metadata while imgui needed direct
29+
dependency include dirs during current-package module/backend scans.
30+
31+
2. Version dependency lookup reused stale unmarked xpkg directories.
32+
33+
`loadVersionDep()` accepted an installed path if `install_path()` found a
34+
directory. That bypassed the stricter `.mcpp_ok` install marker semantics
35+
already used in `resolve_xpkg_path()`.
36+
37+
Impact: an old `compat.glfw@3.4` sandbox directory with layout
38+
`3.4/glfw-3.4/include` and `3.4/glfw-3.4/src` was reused even though the
39+
current index metadata expects `include` and `src` at the version root after
40+
the install hook. mcpp then skipped GLFW's own source files, linked only
41+
imgui and GLFW consumers, and failed with undefined `glfwInit`,
42+
`glfwCreateWindow`, etc.
43+
44+
## Changes In This Branch
45+
46+
- `src/build/flags.cppm`
47+
- Fixed the `std::format` placeholder count for `CompileFlags::cxx`, so
48+
`include_flags` is emitted for C++ scan/compile rules.
49+
50+
- `tests/unit/test_ninja_backend.cpp`
51+
- Added `NinjaBackend.CxxFlagsIncludeBuildIncludeDirs` to lock the C++ include
52+
propagation behavior.
53+
54+
- `src/cli.cppm`
55+
- In version dependency loading, require either `.mcpp_ok` or a layout that
56+
matches the current index entry's `mcpp` metadata before reusing an existing
57+
xpkg directory.
58+
- Adopt and mark compatible pre-marker installs; clean stale unmarked
59+
directories whose layout no longer matches the index before reinstall.
60+
- Mark the install path complete after a successful version dependency
61+
install.
62+
63+
- `tests/e2e/60_stale_xpkg_cache_reinstall.sh`
64+
- Added a stale-cache regression that creates an old unmarked xpkg layout and
65+
verifies mcpp reinstalls it instead of linking against the stale contents.
66+
67+
## Evidence So Far
68+
69+
- Red test for the include bug:
70+
- `NinjaBackend.CxxFlagsIncludeBuildIncludeDirs` failed before the flags fix
71+
because `flags.cxx` was `-std=c++23 -fmodules -O2` without `-I...`.
72+
73+
- Green unit suite after the flags fix and current resolver change:
74+
- Command: `mcpp test -- --gtest_filter=NinjaBackend.CxxFlagsIncludeBuildIncludeDirs`
75+
- Result: full mcpp test run completed with `18 passed; 0 failed`.
76+
77+
- imgui-private validation after clearing stale `compat.glfw@3.4` cache:
78+
- `backend_test ... ok`
79+
- `imgui_test ... ok`
80+
- `test result ok. 2 passed; 0 failed`
81+
82+
- Red e2e for stale-cache behavior before the resolver fix:
83+
- `tests/e2e/60_stale_xpkg_cache_reinstall.sh` failed with
84+
`undefined reference to stale_value`, proving the stale dependency sources
85+
were not linked.
86+
87+
- Green e2e after rebuilding the mcpp CLI with the resolver fix:
88+
- Command:
89+
`MCPP=target/x86_64-linux-gnu/85da010ca4e7d6e2/bin/mcpp bash tests/e2e/60_stale_xpkg_cache_reinstall.sh`
90+
- Result: `OK`
91+
92+
- Boundary regression caught and fixed:
93+
- `tests/e2e/52_local_path_namespaced_index.sh` initially failed after a
94+
marker-only implementation because it intentionally pre-populates a
95+
compatible project-local xpkg layout without `.mcpp_ok`.
96+
- The resolver now adopts an unmarked directory only when the current index's
97+
`mcpp` metadata can resolve the package source/manifest from that root.
98+
99+
- Latest focused verification with rebuilt CLI:
100+
- `mcpp test`: `18 passed; 0 failed`
101+
- `tests/e2e/52_local_path_namespaced_index.sh`: `OK`
102+
- `tests/e2e/54_package_owned_ldflags.sh`: `OK`
103+
- `tests/e2e/55_dependency_shared_artifact.sh`: `OK`
104+
- `tests/e2e/56_transitive_shared_artifact.sh`: `OK`
105+
- `tests/e2e/57_static_dep_shared_artifact.sh`: `OK`
106+
- `tests/e2e/60_stale_xpkg_cache_reinstall.sh`: `OK`
107+
108+
## Pending Verification
109+
110+
- Re-run imgui-private with the rebuilt mcpp binary without manually deleting
111+
package directories.
112+
- Before opening a PR, decide whether to also run the full e2e suite or leave
113+
that to CI after the focused dependency set above.
114+
115+
## Notes
116+
117+
- The imgui package itself is currently not the blocker after a fresh GLFW
118+
install. The observed backend tests pass once the dependency cache layout is
119+
corrected.
120+
- The mcpp fixes should be submitted as a separate PR from imgui package/index
121+
updates.

‎.agents/docs/2026-06-02-usage-requirements-architecture.md‎

Lines changed: 235 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,235 @@
1+
# Module-First Usage Requirements Architecture
2+
3+
## Context
4+
5+
The ImGui module experiment exposed two separate issues in mcpp:
6+
7+
1. C++ compile flags computed `include_flags` but did not place them in the
8+
final `cxxflags` string. That is a correctness bug in flag assembly.
9+
2. The current dependency include model is too coarse for a module-first
10+
package ecosystem. `build.include_dirs` is used both to build a package and
11+
as the package's consumer-visible include surface.
12+
13+
For traditional header packages the symmetric behavior is convenient. For
14+
module packages it is too broad: a package can include upstream headers
15+
internally while exposing only `import package.module;` to consumers.
16+
17+
This document is the live target for the current PR. The architecture should
18+
not be a thin wrapper around `BuildConfig.includeDirs`; it should make package
19+
usage requirements explicit in the resolved package graph and move consumers
20+
onto that graph incrementally inside this PR.
21+
22+
## Design Goals
23+
24+
- Keep existing package descriptors working during migration.
25+
- Treat modules as the primary public surface for module packages.
26+
- Distinguish package-private build requirements from consumer-visible usage
27+
requirements.
28+
- Make scanner, P1689 scan, compile commands, and real compile rules consume
29+
the same resolved usage model.
30+
- Keep link/runtime propagation separate from header/include propagation.
31+
- Avoid forcing users of module packages to see or declare implementation
32+
headers.
33+
34+
## Terms
35+
36+
- Private build requirements:
37+
- Requirements needed to compile a package's own sources.
38+
- Example: `mcpplibs.imgui` wrappers need Dear ImGui backend headers
39+
internally.
40+
- Public usage requirements:
41+
- Requirements that consumers inherit when depending on the package.
42+
- Example: `compat.glfw` exports `GLFW/glfw3.h` as a traditional header
43+
surface.
44+
- Link requirements:
45+
- Libraries and linker flags that follow objects into final link units.
46+
- These are independent from whether headers are public.
47+
48+
## Target Data Model
49+
50+
The resolved build graph should own usage requirements as first-class data
51+
rather than mutating manifests during dependency resolution:
52+
53+
```cpp
54+
enum class Visibility {
55+
Private,
56+
Public,
57+
Interface,
58+
};
59+
60+
struct UsageRequirements {
61+
std::vector<std::filesystem::path> includeDirs;
62+
std::vector<std::string> cflags;
63+
std::vector<std::string> cxxflags;
64+
std::vector<std::string> ldflags;
65+
std::vector<std::string> modules;
66+
};
67+
68+
struct PackageNode {
69+
Manifest manifest;
70+
UsageRequirements privateBuild;
71+
UsageRequirements publicUsage;
72+
UsageRequirements linkUsage;
73+
};
74+
75+
struct DependencyEdge {
76+
PackageId from;
77+
PackageId to;
78+
Visibility visibility;
79+
};
80+
```
81+
82+
The important property is that `Manifest` remains the parsed package
83+
description. Resolved requirements live in the package graph and build plan,
84+
not by appending dependency state back into the manifest.
85+
86+
## Dependency Visibility Semantics
87+
88+
Use CMake-like visibility:
89+
90+
- `private`
91+
- The dependency is needed to compile/link the package itself.
92+
- Its usage requirements are not propagated to consumers.
93+
- `public`
94+
- The dependency is needed to compile/link the package itself.
95+
- Its public usage requirements are propagated to consumers.
96+
- `interface`
97+
- The package itself does not compile/link against the dependency.
98+
- Consumers inherit the dependency's public usage requirements.
99+
100+
Future manifest surface:
101+
102+
```toml
103+
[dependencies.compat]
104+
imgui = { version = "1.92.8", visibility = "private" }
105+
glfw = { version = "3.4", visibility = "private" }
106+
```
107+
108+
Compatibility rule: old dependency declarations default to `public`, matching
109+
current mcpp behavior.
110+
111+
## Build Graph Behavior
112+
113+
Resolution should build a package graph:
114+
115+
1. Parse manifests without rewriting them.
116+
2. Create `PackageNode` entries for root and dependencies.
117+
3. Create `DependencyEdge` entries with resolved visibility.
118+
4. Compute each node's effective private build requirements from:
119+
- the package's own build requirements,
120+
- private/public dependency requirements,
121+
- platform/toolchain requirements.
122+
5. Compute each node's public usage requirements from:
123+
- the package's declared public/interface requirements,
124+
- public/interface dependency requirements.
125+
6. Compute link closure independently from include/header closure.
126+
127+
Scanner, P1689 scan, compile commands, and Ninja compile rules should all derive
128+
from each compile unit's resolved private build requirements. A module import
129+
edge should require BMI/object/link availability, not implementation header
130+
visibility.
131+
132+
## ImGui Implications
133+
134+
`imgui-private` should model upstream headers as private implementation
135+
requirements:
136+
137+
- `compat.imgui`: private build/link requirement for wrappers and backend
138+
implementation files.
139+
- `compat.glfw`: private build/link requirement for GLFW backend modules.
140+
- `compat.opengl`: private build requirement for OpenGL backend headers.
141+
142+
Consumers of the module package should only need:
143+
144+
```cpp
145+
import imgui.core;
146+
import imgui.backend.glfw_opengl3;
147+
```
148+
149+
Consumers that want to write direct GLFW/OpenGL header code should declare
150+
`compat.glfw` or `compat.opengl` themselves.
151+
152+
## Current PR Plan
153+
154+
1. Parse dependency visibility:
155+
- Add `visibility = "private" | "public" | "interface"` to dependency
156+
specs.
157+
- Default omitted visibility to `public`, preserving existing packages.
158+
2. Introduce resolved usage graph data:
159+
- Add `UsageRequirements` and dependency edge visibility to the package
160+
graph surface currently represented by `PackageRoot`.
161+
- Keep parsed `Manifest` immutable during dependency resolution for include
162+
propagation.
163+
3. Compute usage requirements during dependency resolution:
164+
- Map a package's legacy `build.include_dirs` to its own private build
165+
requirements and public usage requirements.
166+
- For `private` and `public` edges, make dependency public usage visible to
167+
the dependent package's private build.
168+
- For `public` and `interface` edges, propagate dependency public usage to
169+
the dependent package's public usage.
170+
4. Make build consumers use resolved usage:
171+
- Regex scanner, P1689 scanner, compile commands, and Ninja compile rules
172+
should read the same per-package resolved private build include dirs.
173+
- Keep link flag propagation in the existing path for this PR; link usage is
174+
a separate axis in the model and should be migrated after include usage is
175+
proven by tests.
176+
- Keep `build.cflags` and `build.cxxflags` package-private in this PR;
177+
public compile definitions/options need a dedicated manifest surface
178+
before they should propagate to consumers.
179+
5. Preserve the already discovered fixes:
180+
- Keep the C++ `include_flags` assembly regression fix.
181+
- Keep stale unmarked xpkg install recovery.
182+
6. Release path:
183+
- Open PR, wait for CI, and admin squash merge after approval.
184+
- Do not trigger the `0.0.43` release from this PR.
185+
- Defer version bump / release / package-index / xlings-res rollout until
186+
the next requested scheme is implemented and released together.
187+
188+
## Current PR Scope
189+
190+
Implement:
191+
192+
- The C++ `include_flags` assembly fix.
193+
- Focused tests for the C++ include flag behavior.
194+
- The stale unmarked xpkg install recovery already discovered during ImGui
195+
validation.
196+
- Dependency visibility parsing and validation.
197+
- Resolved include usage requirements for `PackageRoot`.
198+
- Dependency-edge include propagation without mutating parsed manifests.
199+
- Scanner/build-plan consumption of resolved include usage.
200+
- PR CI and admin squash merge.
201+
202+
Leave for follow-up after this PR:
203+
204+
- Full `linkUsage` migration away from `Manifest::buildConfig.ldflags`
205+
mutation.
206+
- `0.0.43` version bump and release trigger.
207+
- package-index / xlings-res rollout for the released mcpp version.
208+
- Generated module wrapper surface expansion for ImGui.
209+
- mcpp-index publication of ImGui after the module package has a public source
210+
repository and the required mcpp release is available.
211+
212+
## Live Status
213+
214+
- Done: dependency specs carry a `visibility` field with parser validation for
215+
TOML descriptors.
216+
- Done: C++ `include_flags` assembly fix and unit coverage.
217+
- Done: stale unmarked xpkg cache recovery path and e2e coverage.
218+
- Done: resolved include usage graph and scanner/build-plan consumers.
219+
- Done: targeted unit/e2e coverage for dependency visibility propagation.
220+
- Done: compatibility checks for existing public transitive include behavior.
221+
- Done: local verification with `mcpp test` and full e2e suite.
222+
- Pending: PR, CI, and admin squash merge.
223+
- Deferred by user decision: `0.0.43` version bump, release trigger,
224+
xim-pkgindex update, and xlings-res rollout.
225+
226+
## Local Verification
227+
228+
- `mcpp build`: passed.
229+
- `mcpp test`: passed (`18 passed; 0 failed`).
230+
- `tests/e2e/run_all.sh`: passed (`65 passed, 0 failed, 0 skipped`).
231+
- Targeted checks:
232+
- `tests/e2e/31_transitive_deps.sh`: passed.
233+
- `tests/e2e/50_package_owned_build_flags.sh`: passed.
234+
- `tests/e2e/60_stale_xpkg_cache_reinstall.sh`: passed.
235+
- `tests/e2e/61_dependency_visibility.sh`: passed.

‎src/build/flags.cppm‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -243,7 +243,7 @@ CompileFlags compute_flags(const BuildPlan& plan) {
243243
}
244244
std::string cxx_std_flag =
245245
plan.cppStandardFlag.empty() ? std::string("-std=c++23") : plan.cppStandardFlag;
246-
f.cxx = std::format("{}{}{}{}{}{}{}{}{}", cxx_std_flag, module_flag, std_module_flag,
246+
f.cxx = std::format("{}{}{}{}{}{}{}{}{}{}", cxx_std_flag, module_flag, std_module_flag,
247247
std_compat_module_flag, prebuilt_module_flag,
248248
opt_flag, pic_flag, compile_toolchain_flags, b_flag, include_flags);
249249
f.cc = std::format("-std={}{}{}{}{}{}", c_std, opt_flag, pic_flag, compile_toolchain_flags,

‎src/build/plan.cppm‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -408,9 +408,15 @@ BuildPlan make_plan(const mcpp::manifest::Manifest& manifest,
408408
main_cu.source = *lu.entryMain;
409409
main_cu.object = std::filesystem::path("obj") / object_filename_for(*lu.entryMain);
410410
main_cu.packageName = qualified_package_name(manifest);
411-
main_cu.localIncludeDirs = local_include_dirs_for_manifest(projectRoot, manifest);
412-
main_cu.packageCflags = manifest.buildConfig.cflags;
413-
main_cu.packageCxxflags = manifest.buildConfig.cxxflags;
411+
if (!packages.empty() && packages[0].usageResolved) {
412+
main_cu.localIncludeDirs = packages[0].privateBuild.includeDirs;
413+
main_cu.packageCflags = packages[0].privateBuild.cflags;
414+
main_cu.packageCxxflags = packages[0].privateBuild.cxxflags;
415+
} else {
416+
main_cu.localIncludeDirs = local_include_dirs_for_manifest(projectRoot, manifest);
417+
main_cu.packageCflags = manifest.buildConfig.cflags;
418+
main_cu.packageCxxflags = manifest.buildConfig.cxxflags;
419+
}
414420

415421
// We didn't scan main.cpp earlier (it's not in scanner output unless globbed in).
416422
// Best-effort: scan its imports here.

0 commit comments

Comments
 (0)