Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 82 additions & 0 deletions .agents/docs/2026-09-25-issue-690-implementation-plan.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
---
subject: plan
status: active
---

# #690: implementation plan

- Design: [2026-09-25-issue-690-workspace-build-inheritance-consistency.md](2026-09-25-issue-690-workspace-build-inheritance-consistency.md)
- Base: `origin/main` b4824697 (mcpp 2026.9.24.1). Release target: **mcpp 2026.9.25.1**.
- Delivery: one pull request in `mcpp-community/mcpp` that carries every workstream, followed by the release chain in the repositories listed in section 5. The design record's per-workstream pull requests (its section 7) are merged into one, as requested in review. Each workstream keeps its own criterion inside that pull request.

---

## 1. Tasks

| Id | Workstream | Content | Files (owner) |
|---|---|---|---|
| T1 | W1 | Build inheritance moves to the dependency load site. `makePackageRoot` stops inheriting. Snapshot post-condition. | `src/build/prepare.cppm` (lead) |
| T2 | W2 | One `[workspace.build]` key table. `ios_deployment_target` accepted. | `modules/manifest/src/toml.cppm` (lead) |
| T3 | W3 | A git-hosted member inherits its repository's `[workspace.build]`. | `src/build/prepare.cppm` (lead) |
| T4 | W4 | `load_effective_manifest`. `publish`, `pack`, `emit xpkg` and `toolchain list` read it. | `src/project.cppm`, `src/publish/pipeline.cppm`, `src/pack/route.cppm`, `src/toolchain/lifecycle.cppm` (agent B) |
| T5 | W5 | Normalised published manifest, sibling-edge rewrite, reproducible archive from git objects, descriptor `deps` from the normalised manifest. | `src/pm/publisher.cppm`, `modules/libs/src/toml.cppm` (serialiser), new `src/publish/normalize.cppm` (agent B) |
| T6 | W6 | Root include broadcast removed (C, C++, NASM). `kCacheEpoch` 3 to 4. Consumer-include advice on a dependency's missing header. Command-identity unit test. | `src/build/flags.cppm`, `src/build/cache_key.cppm`, `src/build/ninja_backend.cppm`, `src/build/execute.cppm` (agent A) |
| T7 | W7 | `[build] defines` is a keyed set. `!NAME` removes an inherited entry. | `src/build/prepare.cppm` (`fold_build_defines_into_flags`, lead) |
| T8 | harness | `_inherit_toolchain.sh` links payloads per version, so a version installed by a test lands in the test's home and not in the developer's registry (#293 recurrence, measured 2026-09-25). | `tests/e2e/_inherit_toolchain.sh` (lead) |
| T9 | docs | `docs/07`, `docs/04`, `docs/11` (English and 简体中文), SPEC-004 §8 amendment and §9, CHANGELOG, version 2026.9.25.1. | lead, after T1 to T8 |

## 2. Dependencies

```
T2 ----------------------------+
T1 --> T3 --> T7 --------------+--> T9 --> PR --> CI --> review --> merge --> release chain
T4 --> T5 ---------------------+
T6 ----------------------------+
T8 ----------------------------+
```

- T3 edits the same load site as T1 and follows it.
- T7 edits the fold that T1's post-condition names, and follows T1.
- T5 needs T4's loader.
- T6 and T8 are independent of every other task.
- Agents A (T6) and B (T4, T5) work in their own worktrees from the plan commit. The lead integrates by cherry-pick. File ownership in section 1 is exclusive. A task that needs a line in another owner's file reports it instead of editing it.

## 3. Review angles

Each angle names the property it requires and the evidence that shows it.

| Angle | Requirement | Evidence |
|---|---|---|
| Architecture | One pipeline per member. One effective-manifest loader. Private requirements stay inside their package. | Removal of the `makePackageRoot` inheritance block and of the `flags.cppm` broadcast. Every raw `manifest::load` of a project manifest is reviewed and recorded. |
| Stability | No silent state. Internal invariants fail loudly. | Snapshot post-condition. Cache epoch increment. Harness fix T8. |
| Simplicity | No new manifest keys. One new value form (`!NAME`). | Diff of the parser's key table. |
| User experience | Every refusal names the file, the key and the fix. The dependency-header advice names the consumer directory. | e2e output assertions. |
| Compatibility | Older engines read every published normalised manifest. No previously valid manifest is refused by the build. | e2e with the published 2026.9.24.1 binary as a consumer of a normalised archive. |
| Cross-platform | Windows, macOS and Linux CI. Path relativisation and archive creation use git and `std::filesystem`, not host tools with platform-specific flags. | CI matrix. |
| Consistency | `-p lib` and `-p app` compile `lib` identically. A git consumer compiles the member as its repository does. | e2e 321 counts. Git-member e2e. |
| Seamless upgrade | No user action is required. The cost is one cold dependency-cache rebuild (epoch 4) and one rebuild of fingerprints whose duplicated flags disappear. | CHANGELOG. |
| Test coverage | Every workstream has a criterion that fails with its fix removed. | Section 4. |

## 4. Criteria per task

| Task | Criterion | Kind |
|---|---|---|
| T1 | e2e 321: each workspace word exactly once, in both positions, in `.cpp` and `.c` entries, workspace before member. | e2e |
| T1 | Snapshot with unfolded `defines` yields the internal error. | unit |
| T2 | A workspace declaring every table row parses, and every value is inherited. | unit |
| T3 | `file://` repository member: the consumer build passes an `#error` guard, and the flag occurs once. | e2e |
| T4 | In a member that omits `version`, `emit xpkg` and `publish --dry-run` succeed. In a member without `[toolchain]`, `toolchain list` marks the workspace's toolchain. | e2e |
| T5 | The archive's `mcpp.toml` carries the inherited values and a version edge. `mcpp.toml.orig` is present. The descriptor lists the sibling. A consumer of the unpacked archive builds, under the new engine and under 2026.9.24.1. An edge without `version` is refused with the sibling's version in the message. Two runs produce byte-identical archives. The serialiser round-trips. | e2e, unit |
| T6 | A root private `limits.h` containing `#error` does not reach a path dependency or an index dependency. The dependency's compile command is identical under two roots that differ in include settings. The root's units carry each root directory once. The advice names the root directory. | e2e, unit |
| T7 | `defines = ["X=1"]` inherited and `defines = ["X=2"]` in the member emit only `-DX=2`. `!X` emits no `-DX`. | e2e, unit |
| T8 | After a test installs a version that the developer registry lacks, the developer registry still lacks it. | e2e harness check |

## 5. Release chain

1. PR in `mcpp-community/mcpp` with the version 2026.9.25.1 in `mcpp.toml` and `modules/versioning/src/version.cppm`, and the internal xlings pin at the latest xlings release (2026.9.20.1). All required checks green. Squash merge.
2. `origin/main` HEAD run green.
3. `release.yml` dispatched. Each archive is uploaded to GitCode with the local tools the moment it appears (`tools/mirror_res.sh` from xlings, GitCode leg only). GET 200 and byte comparison on both mirrors.
4. `openxlings/xim-pkgindex` bump pull request merged. The published index artifact is read to confirm `latest`.
5. `.xlings.json` (the mcpp that builds mcpp) is not moved: review of 2026-09-25 asked for no separate bootstrap pull request unless a build needs it.
6. `mcpp-community/mcpp-index`: one pull request that moves its CI pins to 2026.9.25.1, which runs every member on every platform against the release. Before the release, 16 members were run locally with the candidate binary (all passed), and no member declares `include_dirs`, so the W6 change has no consumer-side reliance to break there.
7. Sandbox verification: `.agents/docs/2026-09-25-issue-690-verify.sh` in `xlings subos use <n> --sandbox --cmd ...`, with the CN mirror set for both xlings and mcpp inside the sandbox, and the same script against 2026.9.24.1 as the control.
50 changes: 50 additions & 0 deletions .agents/docs/2026-09-25-issue-690-self-review.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
---
subject: review
status: active
---

# #690: self-review before release, engine and ecosystem

- Pull request: mcpp-community/mcpp#691 (2026.9.25.1)
- Design: [2026-09-25-issue-690-workspace-build-inheritance-consistency.md](2026-09-25-issue-690-workspace-build-inheritance-consistency.md); plan: [2026-09-25-issue-690-implementation-plan.md](2026-09-25-issue-690-implementation-plan.md)
- Method: the full `src/` and `modules/` diff read against the principles of the design record (P1 to P8), each finding checked by a measurement or a code citation, and the ecosystem consumers enumerated.

---

## 1. Findings of the review, and what was done

| # | Finding | Evidence | Resolution |
|---|---|---|---|
| R1 | The archive commit made by `mcpp publish` honoured `commit.gpgSign`. On a host that signs commits, the object would carry a signature timestamp, which breaks reproducibility, or it would fail where no key is available. | Code: `git commit-tree` without `--no-gpg-sign`. After the fix, measured with `commit.gpgsign = true` and `gpg.program = /bin/false` on the repository: `publish --dry-run` succeeds twice with the same sha256. | `--no-gpg-sign` added. |
| R2 | The manifest blob was hashed with the repository's filters, and the scratch file lies inside the repository (`target/dist`). | Code: `git hash-object -w` without `--no-filters`. | `--no-filters` added. |
| R3 | The publisher carried a second copy of the inheritable key set, written in parallel with `kWorkspaceBuildKeys` because the two tasks started from the same base. | Code: `kStringVectors`, `kPathVectors`, `kScalars` in `normalize.cppm`. | Replaced by `kWorkspaceBuildKeys`. |
| R4 | A host-tool sub-build receives its dependency's manifest preloaded. After W1 that manifest is already inherited, and the sub-build's member branch would have inherited it a second time. | Code: `prepare_build` member branch before W4. | The preloaded manifest is treated as effective. Only its workspace is recorded, for the membership test of its own dependencies. |
| R5 | Two e2e scripts from the parallel tasks used the number 770, the same as the lead's. | Directory listing. | Renumbered to 772 and 773. |
| R7 | A host-tool sub-build merged its dependency's conditional sections a second time (design record F12). Found as an open item of this review and then measured: `-include once.h` from a matching section reached the tool twice, on 2026.9.24.1 as well. | e2e 775 fails on 2026.9.24.1 and passes on the candidate; the package builds on its own (control). | The sub-build receives `Manifest::beforeConditionalMerge`. |
| R6 | A member inside an index archive (a Form A descriptor pointing at `*/<dir>/mcpp.toml`) did not inherit its archive's workspace, which is the same position independence gap as F4 for a third route. | Measured, design record F11 and e2e 774. | Applied through `inherit_as_workspace_member`, bounded by the install root. |

## 2. Principles, checked

| Principle | Holds because | Residual |
|---|---|---|
| P1 position independence | One function (`inherit_as_workspace_member`) serves the sibling, git and index-archive routes. The root inherits at load through the effective loader. e2e 770 and 774 count the words in each position. | None known. |
| P2 merge, normalise, snapshot | `makePackageRoot` performs no merge and refuses unfolded `defines`. | The layer-conditional second pass folds again by design, and the keyed fold removes superseded words across passes (unit test `SecondPassRemovesAWordTheFirstPassFolded`). |
| P3 one source of truth | One key table, one loader, one inheritance function. | `inherit_workspace_build` in `project.cppm` still lists its fields explicitly. The unit test `EveryTableRowIsParsedAndInherited` fails if the table and that function disagree. |
| P4 scope | No include directory is broadcast; the std module, the scanner and every rule read per-unit includes. | A consumer-supplied configuration header has no channel. None is needed today (section 3). |
| P5 cache soundness | `kCacheEpoch` 4; a dependency's command is shown identical under two roots (e2e 765 (c)). | None known. |
| P6 published form | Normalised manifest, reproducible archive, `.orig` kept, released 2026.9.24.1 client builds it (e2e 772 with `MCPP_BOOT`). | `[indices]` inherited from the workspace is not written into the published manifest; a member whose dependencies resolve through a workspace-declared index publishes a manifest that names no index for them. This matches a non-member package, which also publishes no `[indices]`. |
| P7 loud invariants | Internal error text follows `plan.cppm`'s form. | None. |
| P8 measured blast radius | Section 3. | The full mcpp-index matrix runs after release, on the pin-moving pull request. |

## 3. Ecosystem review

- **mcpp-index members.** 166 test members. None declares `include_dirs` (the two matches are comments), so W6 removes nothing a member relied on. The root workspace declares no `[workspace.build]`, so W1 changes no member's flags. 16 members with C sources, `defines` and include directories (`cjson`, `zlib`, `brotli`, `c-ares`, `expat`, `libpng`, `sqlite3`, `pcre2`, `spdlog-compiled`, `fmtlib.fmt`, `yaml-cpp`, `xxhash`, `md4c`, `libffi`, `mimalloc`, `yyjson`) pass `mcpp test -p` with the candidate.
- **Index packages with a workspace in their archive.** 22 are installed on the measuring machine. None declares `[workspace.package]`, `[workspace.build]` or `[workspace.dependencies]`, so F11's inheritance changes none of them.
- **Descriptor comments.** `compat.godot-cpp.lua:163` states that "a consumer-side header shadow never reaches" the package. That statement was false for an uncached compile before this release and is true after it; no descriptor change is needed.
- **Published clients.** The normalised manifest uses only keys 2026.9.24.1 accepts. `!NAME` in `defines` needs 2026.9.25.1, and docs/04 states the floor.
- **Caches.** Epoch 4 orphans every dependency-cache entry once. mcpp-index's CI caches already key on `MCPP_VERSION`, so the pin move costs the same cold run it always does.
- **xlings.** The internal pin moves to 2026.9.20.1 (openxlings/xlings#610). `check_version_pins.sh` passes. The bootstrap `.xlings.json` does not move (review decision 2026-09-25).

## 4. Open items outside #690

- **The e2e harness still shares existing payload versions by link.** An in-place rewrite of an existing payload by a test still reaches the developer's registry (#293, first shape).
Loading
Loading