Skip to content

Commit adc7077

Browse files
feat: 新增项目级构建 Hook (#530)
* feat: 新增项目级构建 Hook * review: one parser for mcpp.toml, and a cmd.exe line cmd.exe accepts Follow-up to the [hooks] feature. Five things the first shape got wrong, in descending order of how loudly they fail. 1. Windows: the hook never ran. `run_shell_deadline` built the command line with `windows_command_from_argv({"cmd.exe","/d","/s","/c", command})`, which quotes every token — so the switches stopped being switches and the command arrived carrying a quote pair cmd.exe does not consume: '"echo start>>hooks.log' is not recognized as an internal or external command That is #425 one layer up (`cmd.exe /c` does not use CreateProcess argument quoting). A shell command now has its own host-independent shaper, `windows_shell_command_line` — bare switches plus the single outer pair /s strips — and its own tests in test_windows_command_line.cpp, which compile on every platform. The Windows branch is unreachable on the machines this is developed on; a unit test is the only thing that can fail there first. 2. `[hooks]` is a section of mcpp.toml, so mcpp.manifest parses it. The feature came with a second, independent reader of the same file. It cost a redundant parse on every build, and it reported ITS syntax errors in ITS vocabulary: any manifest typo, anywhere in the file, came out as `error: invalid hook configuration: ...` for every project whether or not it used hooks. `Manifest::hooks` now carries the config, unknown keys warn (and error under --strict) like every other section, and mcpp.hooks is left with the part that is policy rather than grammar. 3. A hook no longer moves mcpp's working directory. The launchers already carry a per-child cwd (posix_spawn_file_actions_addchdir_np, lpCurrentDirectory); `run_shell_deadline` takes one, and the chdir-and-restore dance around the call is gone. 4. The fast-path veto moved into try_fast_build, next to the manifest read that answers it — cmd_build no longer loads the manifest a second time to ask whether it may call a function that loads it again. 5. Coverage the claims did not have: workspace members (each member's own hooks, in that member's root, root manifest silent), preparation failing fires nothing, an unknown key warns and the known ones still run, and the e2e compares hook logs by CONTENT — cmd.exe writes CRLF, so the previous `$'start\nfinished'` comparison could not have passed on Windows even with the command line fixed. Plus seven manifest tests for the grammar. Docs (en + zh) state what was implicit: which commands run hooks and which deliberately do not, what a virtual workspace root does, that an active hook opts the project out of the fast path, that preparation failure is silent, and that a `[hooks]` table is executable content in a repository you may have just cloned. * feat(hooks): a hook is a command owned for an interval, and `during_build` Adds the background command the feature was asked for — music that plays for the length of the build and stops when it ends, optionally restarting — by unifying it with the hooks that already existed rather than bolting a mode onto them. ## The model A hook is a command mcpp OWNS FOR AN INTERVAL. The event names the interval. build_start / build_finished / build_failed opens at the event, closes when the command exits during_build opens before the build, closes after it The first three are SELF-CLOSING, and "synchronous" stops being a separate mode: it is what an interval closed by its own command looks like. Everything that would otherwise be a special case for `during_build` falls out of that one difference instead of being declared: * `timeout_seconds` bounds one run, so it does not apply where the build already bounds it — and is rejected there rather than reinterpreted. * `loop` restarts a command that ended before its interval did, which a self-closing interval makes impossible — so it is rejected there too, with a message naming `during_build`. * `side_effect` is unchanged. For `during_build`, "failure" means could not start, or failed to stay up. Being stopped because the interval closed is not a failure. `during_build` closes BEFORE the terminal hook, so a "build finished" sound is not competing with the background music it replaces. Spelling: every event value is a string or a table (`{ cmd, timeout_seconds }` or `{ cmd, loop }`) — the string-or-table shape `[dependencies]` and `[resources].version-info` already use, so no new parsing semantics. ## What it actually cost The schema was the small half. 1. A PROCESS GROUP. `unix/bounded_process.cppm` killed the direct child, which is enough for `sh -c "sleep 5"` (the shell execs) and not enough for `sh -c 'player & wait'` — the shell dies and the player keeps the audio device. A background player that survives its build, from a process the user cannot name, is the worst failure this feature can have. POSIX now spawns with POSIX_SPAWN_SETPGROUP and stops with killpg; Windows already had the right shape in its job object. The same gap is why `mcpp test --timeout` and `[build] build_program_timeout` could leave grandchildren behind. The poller deliberately does NOT reap (waitid WNOWAIT): an unreaped leader is what keeps the group id from being recycled between the poll and the kill. 2. A SIGNAL HANDLER. Its own process group is what makes killpg possible AND what stops the terminal's SIGINT from reaching the child — so Ctrl-C would have killed mcpp and left the music playing. mcpp had no signal handling at all; there is now the minimum that is async-signal-safe (a `volatile sig_atomic_t` group id, killpg, re-raise). Windows gets a console handler, though its job object already covers process death. 3. A RESTART FLOOR. `loop` on a typo'd command is a fork bomb. 250 ms between runs, and five consecutive runs that end UNSUCCESSFULLY within a second stop the loop and report. Both halves matter: an early draft counted short runs regardless of exit code and killed the build after five restarts of a perfectly healthy `echo`. A supervisor thread exists only when `loop = true`. ## Criteria State, not log lines — "mcpp said it stopped the command" passes whether or not anything stopped. The e2e asserts the heartbeat file grew and then did not grow for a further second; that `loop` produces >= 2 runs where its absence produces exactly 1; that a command which cannot stay up stops and is reported; that `loop` on `build_start` is refused with a diagnostic naming `during_build`; and that an interrupted build leaves nothing running. The writer is a GRANDCHILD of the command mcpp starts, which is the case `kill(pid)` misses and `killpg` catches. The Ctrl-C case is POSIX-only and prints its skip, because a test that cannot fail on Windows would read as coverage. Design: .agents/docs/2026-08-30-project-build-hooks-owned-intervals.md * fix(hooks): Windows has no polite stop, and asking for one killed the console `background_stop` opened with what looked like the symmetric counterpart of the POSIX side's SIGTERM-then-grace: ::GenerateConsoleCtrlEvent(CTRL_BREAK_EVENT, ::GetProcessId(procH)); That call addresses a process GROUP attached to the CALLER's console, not a process. When the id does not name a live group of ours — and it does not, once the child has already exited, which `start /b`-style commands do immediately — the event reaches everything sharing that console instead. Measured on the Windows e2e runner: the entire suite died eleven seconds into the hooks test with exit code -1073741510 (0xC000013A, STATUS_CONTROL_C_EXIT) and printed no summary at all, because mcpp had sent Ctrl-Break to its own console. Nothing local can show this: the branch is #if'd out everywhere the code is developed, and the failure is not in the feature under test — it is the harness dying. The design document already said Windows has no graceful stop for a child with no console and no window of its own. The implementation did not believe it. The call is removed, `graceMs` is explicitly unspent on that platform, and the asymmetry with POSIX is stated in the declaration instead of faked with a call that reaches too far. The job object was always the mechanism. macOS ARM64 and Linux both ran the new e2e green on the previous commit (317_project_build_hooks.sh, 21s on macOS), so the POSIX half — process groups, the grandchild kill, and Ctrl-C cleanup — is confirmed on two OSes. * feat(hooks): ship it experimental — a hook gets no vote on the build `[hooks]` is marked experimental, and the marking is enforced rather than written down: `side_effect` now defaults to FALSE, and `side_effect = true` is REFUSED by the manifest parser. error: mcpp.toml: error: [hooks].side_effect = true is not available yet: [hooks] is experimental and cannot decide whether a build succeeded. … So every hook failure — cannot start, non-zero, timed out, failed to stay up — is a warning, and `mcpp build` keeps the result it earned on its own. Refused rather than quietly downgraded, because both silent behaviours are worse than an error. Honouring `true` would ship an experimental feature with a veto over every build. Ignoring it would leave a project believing its build is gated on a notifier when nothing is — which is the "accepted and does nothing" shape this design already rejects for `loop` on a self-closing event and for `timeout_seconds` on `during_build`. Making an exception for the one key whose wrong answer is invisible would be exactly backwards. The key stays in the schema, so a manifest written today does not have to change when the feature is promoted, and the mechanism under it already implements both values: the `sideEffect == true` branch in `mcpp.hooks` is unreachable today ON PURPOSE. Promotion is the deletion of one block in the parser, not a reconstruction. Two further limits are documented as permanent rather than provisional, so they are not read as part of the experiment: a dependency's `[hooks]` is skipped always (there is one Span construction and there are two invoke call sites, all in run_build_with_hooks, all fed from the context's own manifest), and only `mcpp build` runs hooks. Criteria: the e2e now asserts BOTH halves — the exit code says the hook had no vote, and the warning says the failure was not swallowed. Checking only the exit code would pass just as well if hooks had stopped running altogether. The timeout and give-up cases flipped from "build fails with error:" to "build succeeds with warning:", and a negative assertion was rewritten as `if grep` rather than `grep && { }`, whose exit status under `set -e` is an argument this file should not be having. Hook context (MCPP_PROFILE, MCPP_TARGET, …) is deferred; the experimental window is where that interface can still be named deliberately. --------- Co-authored-by: speak-agent <248744407+speak-agent@users.noreply.github.com>
1 parent f4cb9a1 commit adc7077

14 files changed

Lines changed: 2533 additions & 4 deletions
Lines changed: 239 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,239 @@
1+
# Project build hooks as owned intervals (#496)
2+
3+
## The question this design answers
4+
5+
The first shape of `[hooks]` gave `mcpp build` three commands — `build_start`,
6+
`build_finished`, `build_failed` — each run to completion with a timeout, each
7+
judged by its exit code.
8+
9+
The request that followed was a background command: music that plays *for the
10+
duration of the build* and stops when it ends, optionally restarting when the
11+
player exits. Written as a knob it looks like
12+
13+
```toml
14+
build_start = { cmd = "play bgm.mp3", loop = true }
15+
```
16+
17+
but `loop` is not the new thing. The new thing is that this command's life is
18+
**longer than the moment that started it**. Adding `background = true` to
19+
`build_start` would give that key two incompatible meanings and would silently
20+
change what `timeout_seconds` and `side_effect` mean for one of them — a table
21+
whose keys mean different things depending on a sibling key is the shape this
22+
design exists to avoid.
23+
24+
## The model
25+
26+
> A hook is a command mcpp **owns for an interval**. The event names the
27+
> interval. mcpp starts the command when the interval opens and ends it when
28+
> the interval closes.
29+
30+
| Event | The interval opens | The interval closes |
31+
|---|---|---|
32+
| `build_start` | after preparation, before the build | when the command exits |
33+
| `build_finished` | after a build that succeeded | when the command exits |
34+
| `build_failed` | after a build that failed | when the command exits |
35+
| `during_build` | after preparation, before the build | after the build, before the terminal hook |
36+
37+
The first three have **self-closing** intervals. "Synchronous" is not a
38+
separate mode in this model — it is what an interval closed by the command
39+
itself looks like. `during_build` is the one interval closed by something else,
40+
and everything that reads as a special case for it falls out of that single
41+
difference rather than being declared:
42+
43+
- **`timeout_seconds` bounds one run of the command.** For a self-closing
44+
interval that is the whole hook. For `during_build` the build already bounds
45+
it, so the key is *rejected* there rather than accepted and reinterpreted —
46+
a per-run cap would only be enforceable when `loop` is on (nothing is
47+
polling otherwise), and a key that works under one sibling setting and not
48+
another is the hole this design is trying not to dig.
49+
- **`loop` restarts a command that exits before its interval closes.** A
50+
self-closing interval ends *when the command exits*, so `loop` can never fire
51+
there. It is rejected on those events rather than accepted and ignored, with
52+
a message naming `during_build`.
53+
- **`side_effect` is unchanged**: does a hook failure fail the build. For
54+
`during_build`, "failure" means *failed to start*, or *failed to stay up*
55+
(below). Being stopped because the interval closed is not a failure.
56+
57+
Ordering inside the lifecycle:
58+
59+
```text
60+
during_build opens
61+
build_start
62+
├─ build succeeds → during_build closes → build_finished
63+
└─ build fails → during_build closes → build_failed
64+
```
65+
66+
`during_build` closes **before** the terminal hook, not after. A "build
67+
finished" sound playing over the background music it was supposed to replace is
68+
the whole reason the order is fixed rather than incidental.
69+
70+
## Schema
71+
72+
Every event value is a string or a table; the string is sugar.
73+
74+
```toml
75+
[hooks]
76+
build_start = "echo start" # = { cmd = "echo start" }
77+
build_finished = "notify-send 'build finished'"
78+
during_build = { cmd = "mcpp-hooks-audioplayer bgm", loop = true }
79+
80+
# Table-level, unchanged.
81+
timeout_seconds = 10
82+
enabled = true
83+
side_effect = true
84+
```
85+
86+
Per-event table keys: `cmd` (required, non-empty), `timeout_seconds`
87+
(overrides the table default for this event), `loop` (`during_build` only).
88+
89+
The string-or-table pattern is already how `[dependencies]` and
90+
`[resources].version-info` are spelled, so it introduces no new parsing
91+
semantics — Appendix A of docs/05 is satisfied for the same reason the first
92+
shape satisfied it: fixed keys, open values, and no key that duplicates an
93+
answer another section already gives.
94+
95+
## What the feature actually costs
96+
97+
The schema is the small half. A process whose life spans the build needs three
98+
things mcpp does not have.
99+
100+
### 1. A process group, because SIGKILL on a pid is not enough
101+
102+
`modules/platform/src/unix/bounded_process.cppm:216` kills the direct child:
103+
104+
```cpp
105+
::kill(pid, SIGKILL);
106+
```
107+
108+
For `sh -c "sleep 5"` that is sufficient — the shell execs the command. For
109+
`sh -c "while :; do play a.mp3; done"` it is not: the shell dies and `play`
110+
survives. **Music that cannot be stopped, from a process the user cannot name,
111+
is the worst failure this feature can have**, and it is the default outcome
112+
without process groups.
113+
114+
So the POSIX side gains `posix_spawnattr_setpgroup` + `killpg`. Windows already
115+
has the right shape: a Job object with `JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE`
116+
(`windows/bounded_process.cppm:205`), which takes the whole tree — and takes it
117+
even if mcpp dies, because the handle closes with the process.
118+
119+
This is worth doing on its own terms: the same gap is why
120+
`mcpp test --timeout` and `[build] build_program_timeout` can leave
121+
grandchildren behind today.
122+
123+
### 2. A signal handler, because a process group stops receiving Ctrl-C
124+
125+
The two requirements fight each other. A child in its own process group no
126+
longer receives the terminal's SIGINT — which is what makes `killpg` possible
127+
and *also* what makes Ctrl-C leave the music playing. Both halves are needed:
128+
its own group, plus a SIGINT/SIGTERM handler in mcpp that stops the group
129+
before re-raising.
130+
131+
mcpp installs no signal handlers today. The one added here is the minimum that
132+
is async-signal-safe: a `volatile sig_atomic_t` holding the process-group id, a
133+
handler that calls `killpg` (which is async-signal-safe) and re-raises the
134+
default action. It is installed only while a spanning hook is running.
135+
136+
Windows needs no equivalent for correctness — the job object already covers
137+
process death — but `SetConsoleCtrlHandler` is installed for a clean stop.
138+
139+
### 3. A restart floor, because `loop` on a typo is a fork bomb
140+
141+
`loop = true` with `play /nonexistant` restarts thousands of times per second
142+
for the length of the build. Two bounds, both fixed in v1 rather than
143+
configurable, because a knob whose wrong value is a spin is not a knob:
144+
145+
- **250 ms between runs.**
146+
- **Five consecutive runs that exited non-zero in under a second** stops the
147+
loop and reports a hook failure: *"during_build command failed to stay up".*
148+
Whether that fails the build is `side_effect`, as everywhere else.
149+
150+
A supervisor thread exists **only when `loop = true`**. Without it, a spanning
151+
hook is a spawn and a stop, and no thread is created.
152+
153+
## Stopping
154+
155+
POSIX: `SIGTERM` to the group, a 2 s grace period, then `SIGKILL`. A player
156+
asked to stop should get to close its audio device.
157+
158+
Windows: the job object is closed, which terminates the tree at once. There is
159+
no graceful equivalent that does not require enumerating the job's processes
160+
and posting window messages; the asymmetry is stated rather than hidden.
161+
162+
⚠️ The obvious symmetry — "ask with `GenerateConsoleCtrlEvent(CTRL_BREAK_EVENT,
163+
pid)` first" — was written, and it is wrong in a way no local run shows. That
164+
call addresses a process GROUP attached to the caller's console, not a process.
165+
When the id does not name a live group of ours (and it does not, once the child
166+
has exited — `start /b`-style commands exit immediately) the event reaches
167+
everything sharing the console. Measured on the Windows e2e runner: the entire
168+
suite died eleven seconds into the hooks test with exit code `-1073741510`
169+
(`0xC000013A`, `STATUS_CONTROL_C_EXIT`) and printed no summary, because mcpp had
170+
sent Ctrl-Break to its own console. The design said Windows has no graceful stop;
171+
the first implementation did not believe it. The code now matches the design.
172+
173+
## Output
174+
175+
A self-closing hook inherits stdio, which is safe because nothing else is
176+
writing at that moment. A spanning hook writes **concurrently with ninja** and
177+
would interleave into the middle of a compiler diagnostic. Its output is
178+
therefore discarded by default, and inherited under `--verbose` — no new schema
179+
key, and the answer to "why is there no music" is one flag away.
180+
181+
## Shipping it experimental
182+
183+
The feature ships with `side_effect` defaulting to **false** and `true`
184+
*refused* by the manifest parser. A hook therefore cannot change whether a
185+
build succeeded: every failure is a warning and `mcpp build` keeps the result
186+
it earned on its own.
187+
188+
Refused rather than downgraded, because both silent behaviours are worse than
189+
an error. Honouring `true` ships an experimental feature with a veto over every
190+
build. Ignoring it leaves a project believing its build is gated on a notifier
191+
when nothing is — the "accepted and does nothing" shape this design rejects
192+
everywhere else (`loop` on a self-closing event, `timeout_seconds` on
193+
`during_build`), so it would be inconsistent to make an exception for the one
194+
key whose wrong answer is invisible.
195+
196+
The key stays in the schema so manifests do not have to change when the feature
197+
is promoted, and the mechanism under it already implements both values — the
198+
`sideEffect == true` branch in `mcpp.hooks` is unreachable today ON PURPOSE.
199+
**Promotion is the deletion of one block in the parser**, not a
200+
reconstruction. That is the property to preserve when editing either side.
201+
202+
Two further limits are permanent rather than provisional and should not be read
203+
as part of the experiment:
204+
205+
- Only the ROOT project's hooks run. A dependency's `[hooks]` is inert by
206+
construction — there is exactly one `Span` construction and there are exactly
207+
two `invoke` call sites, all in `run_build_with_hooks`, all fed from the
208+
context's own manifest.
209+
- Only `mcpp build` runs hooks. `mcpp run`, `mcpp test` and
210+
`mcpp build --configure-only` build too, and deliberately do not.
211+
212+
## Criteria
213+
214+
The assertions this design has to earn are about *state*, not about log lines —
215+
"we called stop" is not evidence that anything stopped.
216+
217+
1. **It runs during the build.** The command appends a heartbeat line every
218+
200 ms; the file is non-empty when the build ends.
219+
2. **It is stopped.** Record the heartbeat file's size after `mcpp build`
220+
returns, wait one second, read it again: unchanged. A log line saying
221+
"stopped" would pass whether or not the process died.
222+
3. **`loop` restarts it.** A command that exits immediately produces a heartbeat
223+
count that grows across the build; without `loop` it produces exactly one.
224+
4. **The whole tree dies, not just the shell.** The command is
225+
`sh -c '... & wait'`, so the writer is a grandchild. This is the case
226+
`kill(pid)` misses and `killpg` catches, and it is the only assertion that
227+
distinguishes the fix from the bug.
228+
5. **The failure cap trips.** A command that fails instantly stops after five
229+
attempts and reports; the heartbeat count is bounded, not "large".
230+
6. **Ctrl-C leaves nothing behind.** POSIX only: `mcpp build` in the
231+
background, `kill -INT`, then criterion 2. Sending Ctrl-C to another process
232+
on Windows needs a helper this suite does not have; the gap is declared
233+
rather than papered over with a test that passes vacuously.
234+
7. **A self-closing event rejects `loop`.** Otherwise the key is accepted,
235+
does nothing, and the user concludes the feature does not work.
236+
8. **`side_effect = true` is refused, and every failure mode is a warning.**
237+
Two assertions, not one: the exit code says the hook had no vote, and the
238+
warning says the failure was not swallowed. A test that checked only the
239+
exit code would pass just as well if hooks had stopped running altogether.

0 commit comments

Comments
 (0)