From 32437da69af31e3b56a05a794c9834aa4920f112 Mon Sep 17 00:00:00 2001 From: HikariTish Date: Sat, 29 Aug 2026 21:54:30 +0800 Subject: [PATCH 1/5] =?UTF-8?q?feat:=20=E6=96=B0=E5=A2=9E=E9=A1=B9?= =?UTF-8?q?=E7=9B=AE=E7=BA=A7=E6=9E=84=E5=BB=BA=20Hook?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/05-mcpp-toml.md | 58 ++++++++ docs/zh/05-mcpp-toml.md | 53 +++++++ modules/platform/src/process.cppm | 20 +++ src/cli/cmd_build.cppm | 39 +++++- src/hooks.cppm | 200 +++++++++++++++++++++++++++ tests/e2e/314_project_build_hooks.sh | 145 +++++++++++++++++++ 6 files changed, 511 insertions(+), 4 deletions(-) create mode 100644 src/hooks.cppm create mode 100644 tests/e2e/314_project_build_hooks.sh diff --git a/docs/05-mcpp-toml.md b/docs/05-mcpp-toml.md index 3dc5ce0c..06053868 100644 --- a/docs/05-mcpp-toml.md +++ b/docs/05-mcpp-toml.md @@ -2094,6 +2094,64 @@ See [07 — build.mcpp](07-build-mcpp.md). Naming such a file in command: nothing tracks it, and editing the file produces `ninja: no work to do`. +### 2.16 `[hooks]` — Project Build Lifecycle Commands + +`mcpp build` can run one host-shell command before the build and one command +for its terminal result: + +```toml +[hooks] +build_start = "echo build started" +build_failed = "notify-send 'build failed'" +build_finished = "notify-send 'build finished'" + +# Optional; these are the defaults. +timeout_seconds = 10 +enabled = true +side_effect = true +``` + +| Key | Type | Default | Meaning | +|---|---|---:|---| +| `build_start` | string | — | Runs after project preparation, immediately before the build | +| `build_failed` | string | — | Runs when the build exits unsuccessfully | +| `build_finished` | string | — | Runs when the build exits successfully | +| `timeout_seconds` | positive integer | `10` | Maximum time for each command | +| `enabled` | bool | `true` | Enables all commands in this table | +| `side_effect` | bool | `true` | Whether a hook failure makes the overall build fail | + +Commands run synchronously in the current project's root directory through the +host shell (`/bin/sh` or `cmd.exe`). Standard input/output/error keep their +ordinary terminal behaviour. Missing event commands are skipped. + +The lifecycle is: + +```text +build_start + ├─ build succeeds → build_finished + └─ build fails → build_failed +``` + +`build_failed` and `build_finished` are mutually exclusive. A hook command +that cannot start, returns non-zero, or exceeds its timeout is a hook failure. +With `side_effect = false`, mcpp reports a warning and preserves the build's +result; with `true`, it returns failure. A hook's own failure does not trigger +another hook. + +Hook programs can be installed as ordinary xlings dependencies. For example, +an audio notifier can keep its sound files inside its own executable rather +than adding media handling to mcpp: + +```toml +[hooks] +build_finished = "mcpp-hooks-audioplayer niulai-mm" +build_failed = "mcpp-hooks-audioplayer niulai-niulai" +side_effect = false + +[xlings] +deps = ["xim:mcpp-hooks-audioplayer@0.0.1"] +``` + ## Appendix A. Schema Ownership Principle (admission criteria for new fields) > **Closed syntax, open vocabulary**: whoever owns the parsing semantics defines the keys; whoever owns the domain knowledge defines the values. diff --git a/docs/zh/05-mcpp-toml.md b/docs/zh/05-mcpp-toml.md index 1ff98fc0..668bd64b 100644 --- a/docs/zh/05-mcpp-toml.md +++ b/docs/zh/05-mcpp-toml.md @@ -1791,6 +1791,59 @@ o.arg("./mkblob.sh").arg("blob.bin").arg("${mcpp.out_dir}/blob.o") 但 ldflags 是链接命令里的一串字符:没有任何东西跟踪它,改了它得到的是 `ninja: no work to do`。 +### 2.16 `[hooks]` —— 项目构建生命周期命令 + +`mcpp build` 可以在正式构建前执行一条宿主 Shell 命令,并根据最终结果再执行一条: + +```toml +[hooks] +build_start = "echo build started" +build_failed = "notify-send 'build failed'" +build_finished = "notify-send 'build finished'" + +# 可选;以下是默认值。 +timeout_seconds = 10 +enabled = true +side_effect = true +``` + +| 键 | 类型 | 默认值 | 含义 | +|---|---|---:|---| +| `build_start` | 字符串 | — | 项目准备完成、正式构建开始前执行 | +| `build_failed` | 字符串 | — | 构建以失败状态结束时执行 | +| `build_finished` | 字符串 | — | 构建成功结束时执行 | +| `timeout_seconds` | 正整数 | `10` | 每条命令的最长执行时间 | +| `enabled` | 布尔 | `true` | 是否启用本表中的全部命令 | +| `side_effect` | 布尔 | `true` | Hook 失败是否让本次构建失败 | + +命令在当前项目根目录中同步执行,使用宿主 Shell(`/bin/sh` 或 `cmd.exe`),标准输入、 +输出和错误沿用普通终端行为。没有配置的事件直接跳过。 + +生命周期为: + +```text +build_start + ├─ 构建成功 → build_finished + └─ 构建失败 → build_failed +``` + +`build_failed` 与 `build_finished` 互斥。命令无法启动、返回非零或超过时限均视为 +Hook 失败。`side_effect = false` 时 mcpp 报 warning 并保留原构建结果;设为 `true` +时返回失败。Hook 自身失败不会再触发另一个 Hook。 + +Hook 程序可以作为普通 xlings 依赖安装。例如,音频通知程序可以把音频内置进自己的 +可执行文件,无需让 mcpp 处理媒体资源: + +```toml +[hooks] +build_finished = "mcpp-hooks-audioplayer niulai-mm" +build_failed = "mcpp-hooks-audioplayer niulai-niulai" +side_effect = false + +[xlings] +deps = ["xim:mcpp-hooks-audioplayer@0.0.1"] +``` + ## 附录 A. Schema 所有权原则(新字段准入标准) > **语法封闭,词汇开放**:谁拥有解析语义谁定义键;谁拥有领域知识谁定义值。 diff --git a/modules/platform/src/process.cppm b/modules/platform/src/process.cppm index 3b13092f..9309d6c5 100644 --- a/modules/platform/src/process.cppm +++ b/modules/platform/src/process.cppm @@ -112,6 +112,14 @@ int run_exec_deadline(const std::vector& argv, std::chrono::milliseconds deadline, bool* timed_out); +// Run one host-shell command with inherited stdio and a real deadline. +// POSIX uses /bin/sh; Windows uses cmd.exe. This is for user-authored command +// strings such as project hooks — programmatic launches should keep using the +// argv-based run_exec_deadline API above. +int run_shell_deadline(std::string_view command, + std::chrono::milliseconds deadline, + bool* timed_out); + RunResult capture_exec_deadline( const std::vector& argv, const std::vector>& extraEnv, @@ -658,6 +666,18 @@ int run_exec_deadline(const std::vector& argv, return r.exit_code; } +int run_shell_deadline(std::string_view command, + std::chrono::milliseconds deadline, + bool* timed_out) +{ + std::vector argv; + if constexpr (mcpp::platform::is_windows) + argv = {"cmd.exe", "/d", "/s", "/c", std::string(command)}; + else + argv = {"/bin/sh", "-c", std::string(command)}; + return run_exec_deadline(argv, {}, deadline, timed_out); +} + RunResult capture_exec_deadline( const std::vector& argv, const std::vector>& extraEnv, diff --git a/src/cli/cmd_build.cppm b/src/cli/cmd_build.cppm index 13b53177..b05cd6b2 100644 --- a/src/cli/cmd_build.cppm +++ b/src/cli/cmd_build.cppm @@ -18,6 +18,7 @@ import mcpp.build.stage; import mcpp.build.schedule.detach_codegen; import mcpp.build.test_targets; import mcpp.dyndep; +import mcpp.hooks; import mcpp.log; import mcpp.project; import mcpp.manifest; @@ -42,6 +43,25 @@ workspace_fanout_members(bool wantAll, const std::string& package_filter) { return std::nullopt; } +int run_build_with_hooks(mcpp::build::BuildContext& ctx, bool verbose, + bool no_cache, std::string_view targetOverride) { + auto config = mcpp::hooks::load(ctx.projectRoot / "mcpp.toml"); + if (!config) { + mcpp::ui::error(std::format("invalid hook configuration: {}", config.error())); + return 2; + } + + if (!mcpp::hooks::invoke(*config, mcpp::hooks::Event::BuildStart, + ctx.projectRoot)) + return 1; + + int rc = mcpp::build::run_build_plan(ctx, verbose, no_cache, targetOverride); + auto terminalEvent = rc == 0 ? mcpp::hooks::Event::BuildFinished + : mcpp::hooks::Event::BuildFailed; + bool hookOk = mcpp::hooks::invoke(*config, terminalEvent, ctx.projectRoot); + return rc != 0 ? rc : (hookOk ? 0 : 1); +} + export int cmd_build(const mcpplibs::cmdline::ParsedArgs& parsed) { bool verbose = parsed.is_flag_set("verbose") || mcpp::log::is_verbose(); bool print_fp = parsed.is_flag_set("print-fingerprint"); @@ -118,7 +138,7 @@ export int cmd_build(const mcpplibs::cmdline::ParsedArgs& parsed) { auto ctx = mcpp::build::prepare_build(print_fp, /*includeDevDeps=*/false, /*extraTargets=*/{}, mo); if (!ctx) { std::println(stderr, "error: {}: {}", mp, ctx.error()); rc = 2; continue; } - int r = mcpp::build::run_build_plan(*ctx, verbose, no_cache, mo.target_triple); + int r = run_build_with_hooks(*ctx, verbose, no_cache, mo.target_triple); if (r != 0) rc = r; } return rc; @@ -137,8 +157,19 @@ export int cmd_build(const mcpplibs::cmdline::ParsedArgs& parsed) { && ov.cache_mode.empty()) { auto root = mcpp::project::find_manifest_root(std::filesystem::current_path()); if (root) { - if (auto rc = mcpp::build::try_fast_build(*root, verbose, no_cache)) { - return *rc; + auto config = mcpp::hooks::load(*root / "mcpp.toml"); + if (!config) { + mcpp::ui::error(std::format( + "invalid hook configuration: {}", config.error())); + return 2; + } + // Hooked builds must first prepare the project so xlings-provided + // hook programs are available before build_start. Projects with no + // active hooks keep the existing fast path unchanged. + if (!mcpp::hooks::active(*config)) { + if (auto rc = mcpp::build::try_fast_build(*root, verbose, no_cache)) { + return *rc; + } } } } @@ -147,7 +178,7 @@ export int cmd_build(const mcpplibs::cmdline::ParsedArgs& parsed) { /*extraTargets=*/{}, ov); if (!ctx) { std::println(stderr, "error: {}", ctx.error()); return 2; } - return mcpp::build::run_build_plan(*ctx, verbose, no_cache, ov.target_triple); + return run_build_with_hooks(*ctx, verbose, no_cache, ov.target_triple); } export int cmd_run(const mcpplibs::cmdline::ParsedArgs& parsed, diff --git a/src/hooks.cppm b/src/hooks.cppm new file mode 100644 index 00000000..7450ffb5 --- /dev/null +++ b/src/hooks.cppm @@ -0,0 +1,200 @@ +// mcpp.hooks — project-level build lifecycle commands from mcpp.toml. + +export module mcpp.hooks; + +import std; +import mcpp.libs.toml; +import mcpp.platform.process; +import mcpp.ui; + +export namespace mcpp::hooks { + +enum class Event { BuildStart, BuildFailed, BuildFinished }; + +struct Config { + std::string buildStart; + std::string buildFailed; + std::string buildFinished; + int timeoutSeconds = 10; + bool enabled = true; + bool sideEffect = true; +}; + +std::expected +load(const std::filesystem::path& manifestPath); + +bool active(const Config& config); + +// True means the build may continue/keep its original result. False means a +// hook failed with side_effect=true. +bool invoke(const Config& config, Event event, + const std::filesystem::path& projectRoot); + +} // namespace mcpp::hooks + +namespace mcpp::hooks { + +namespace { + +std::string_view event_name(Event event) { + switch (event) { + case Event::BuildStart: return "build_start"; + case Event::BuildFailed: return "build_failed"; + case Event::BuildFinished: return "build_finished"; + } + return "unknown"; +} + +std::string_view event_command(const Config& config, Event event) { + switch (event) { + case Event::BuildStart: return config.buildStart; + case Event::BuildFailed: return config.buildFailed; + case Event::BuildFinished: return config.buildFinished; + } + return {}; +} + +bool report_failure(const Config& config, std::string message) { + if (config.sideEffect) { + mcpp::ui::error(message); + return false; + } + mcpp::ui::warning(message); + return true; +} + +} // namespace + +std::expected +load(const std::filesystem::path& manifestPath) { + namespace toml = mcpp::libs::toml; + + auto doc = toml::parse_file(manifestPath); + if (!doc) { + auto const& e = doc.error(); + if (e.where.line != 0) + return std::unexpected(std::format( + "mcpp.toml:{}:{}: {}", e.where.line, e.where.column, e.message)); + return std::unexpected(e.message); + } + + Config config; + auto* hooksValue = doc->get("hooks"); + if (!hooksValue) return config; + if (!hooksValue->is_table()) + return std::unexpected("[hooks] must be a table"); + + auto const& table = hooksValue->as_table(); + static constexpr std::string_view known[] = { + "build_start", "build_failed", "build_finished", + "timeout_seconds", "enabled", "side_effect", + }; + for (auto const& [key, _] : table) { + if (std::ranges::find(known, key) == std::end(known)) + return std::unexpected(std::format("[hooks] has unknown key '{}'", key)); + } + + std::string error; + auto read_command = [&](std::string_view key, std::string& out) { + auto it = table.find(key); + if (it == table.end()) return true; + if (!it->second.is_string() || it->second.as_string().empty()) { + error = std::format("[hooks].{} must be a non-empty string", key); + return false; + } + out = it->second.as_string(); + return true; + }; + if (!read_command("build_start", config.buildStart) + || !read_command("build_failed", config.buildFailed) + || !read_command("build_finished", config.buildFinished)) + return std::unexpected(std::move(error)); + + if (auto it = table.find("timeout_seconds"); it != table.end()) { + if (!it->second.is_int() || it->second.as_int() <= 0 + || it->second.as_int() > std::numeric_limits::max()) + return std::unexpected( + "[hooks].timeout_seconds must be a positive integer"); + config.timeoutSeconds = static_cast(it->second.as_int()); + } + + auto read_bool = [&](std::string_view key, bool& out) { + auto it = table.find(key); + if (it == table.end()) return true; + if (!it->second.is_bool()) { + error = std::format("[hooks].{} must be a boolean", key); + return false; + } + out = it->second.as_bool(); + return true; + }; + if (!read_bool("enabled", config.enabled) + || !read_bool("side_effect", config.sideEffect)) + return std::unexpected(std::move(error)); + + return config; +} + +bool active(const Config& config) { + return config.enabled + && (!config.buildStart.empty() || !config.buildFailed.empty() + || !config.buildFinished.empty()); +} + +bool invoke(const Config& config, Event event, + const std::filesystem::path& projectRoot) { + if (!config.enabled) return true; + auto command = event_command(config, event); + if (command.empty()) return true; + + std::error_code ec; + auto previous = std::filesystem::current_path(ec); + if (ec) + return report_failure(config, std::format( + "hook '{}' could not read the current directory: {}", + event_name(event), ec.message())); + + // ponytail: hooks are synchronous; a scoped cwd is the smallest portable + // way to honour the project-root contract. Pass cwd through the process + // API if hook execution ever becomes concurrent. + std::filesystem::current_path(projectRoot, ec); + if (ec) + return report_failure(config, std::format( + "hook '{}' could not enter the project root: {}", + event_name(event), ec.message())); + + struct RestorePath { + std::filesystem::path path; + bool active = true; + ~RestorePath() { + if (!active) return; + std::error_code ignored; + std::filesystem::current_path(path, ignored); + } + } restore{previous}; + + bool timedOut = false; + int rc = mcpp::platform::process::run_shell_deadline( + command, std::chrono::seconds(config.timeoutSeconds), &timedOut); + + std::error_code restoreError; + std::filesystem::current_path(previous, restoreError); + restore.active = false; + if (restoreError) { + mcpp::ui::error(std::format( + "hook '{}' could not restore the working directory: {}", + event_name(event), restoreError.message())); + return false; + } + + if (timedOut) + return report_failure(config, std::format( + "hook '{}' timed out after {}s", + event_name(event), config.timeoutSeconds)); + if (rc != 0) + return report_failure(config, std::format( + "hook '{}' exited with status {}", event_name(event), rc)); + return true; +} + +} // namespace mcpp::hooks diff --git a/tests/e2e/314_project_build_hooks.sh b/tests/e2e/314_project_build_hooks.sh new file mode 100644 index 00000000..596887a6 --- /dev/null +++ b/tests/e2e/314_project_build_hooks.sh @@ -0,0 +1,145 @@ +#!/usr/bin/env bash +# requires: +# Project-level build hooks: lifecycle order, policy, timeout and project cwd. +set -e + +TMP=$(mktemp -d) +trap 'rm -rf "$TMP"' EXIT +mkdir -p "$TMP/app/src" "$TMP/app/nested" + +cat > "$TMP/app/src/main.cpp" <<'EOF' +int main() { return 0; } +EOF + +write_manifest() { + cat > "$TMP/app/mcpp.toml" +} + +write_manifest <<'EOF' +[package] +name = "hookprobe" +version = "0.1.0" + +[hooks] +build_start = "echo start>>hooks.log" +build_failed = "echo failed>>hooks.log" +build_finished = "echo finished>>hooks.log" +timeout_seconds = 10 +enabled = true +side_effect = true +EOF + +# Invoke below the project root: hook-relative paths still belong to the root. +cd "$TMP/app/nested" +"$MCPP" build > success-1.log 2>&1 || { + cat success-1.log; echo "FAIL: hooked build failed"; exit 1; } +"$MCPP" build > success-2.log 2>&1 || { + cat success-2.log; echo "FAIL: second hooked build failed"; exit 1; } +cd "$TMP/app" +[[ "$(cat hooks.log)" == $'start\nfinished\nstart\nfinished' ]] || { + cat hooks.log; echo "FAIL: success lifecycle order/count is wrong"; exit 1; } + +# A compiler failure chooses build_failed, never build_finished. +cat > src/main.cpp <<'EOF' +#error "intentional hook failure path" +int main() { return 0; } +EOF +: > hooks.log +rc=0 +"$MCPP" build > build-failed.log 2>&1 || rc=$? +[[ $rc -ne 0 ]] || { cat build-failed.log; echo "FAIL: broken source built"; exit 1; } +[[ "$(cat hooks.log)" == $'start\nfailed' ]] || { + cat hooks.log; echo "FAIL: failure lifecycle is wrong"; exit 1; } + +cat > src/main.cpp <<'EOF' +int main() { return 0; } +EOF + +# A non-side-effecting hook reports its failure but preserves build success. +write_manifest <<'EOF' +[package] +name = "hookprobe" +version = "0.1.0" + +[hooks] +build_finished = "mcpp-hook-command-that-does-not-exist" +side_effect = false +EOF +"$MCPP" build > ignored-hook-failure.log 2>&1 || { + cat ignored-hook-failure.log + echo "FAIL: side_effect=false changed build success"; exit 1; } +grep -q "warning: hook 'build_finished' exited with status" ignored-hook-failure.log || { + cat ignored-hook-failure.log; echo "FAIL: ignored hook failure was not reported"; exit 1; } + +# The same hook failure is fatal with the default side_effect=true. +write_manifest <<'EOF' +[package] +name = "hookprobe" +version = "0.1.0" + +[hooks] +build_finished = "mcpp-hook-command-that-does-not-exist" +EOF +rc=0 +"$MCPP" build > fatal-hook-failure.log 2>&1 || rc=$? +[[ $rc -ne 0 ]] || { + cat fatal-hook-failure.log; echo "FAIL: fatal hook failure returned success"; exit 1; } +grep -q "error: hook 'build_finished' exited with status" fatal-hook-failure.log || { + cat fatal-hook-failure.log; echo "FAIL: fatal hook failure was not reported"; exit 1; } + +# Disabled hooks do nothing, including not forcing a command to run. +write_manifest <<'EOF' +[package] +name = "hookprobe" +version = "0.1.0" + +[hooks] +build_start = "echo disabled>>disabled.log" +build_finished = "echo disabled>>disabled.log" +enabled = false +EOF +rm -f disabled.log +"$MCPP" build > disabled.log.out 2>&1 || { + cat disabled.log.out; echo "FAIL: disabled hook build failed"; exit 1; } +[[ ! -e disabled.log ]] || { + cat disabled.log; echo "FAIL: enabled=false still ran hooks"; exit 1; } + +# A positive timeout is enforced by the platform process runner. +case "$(uname -s)" in + MINGW*|MSYS*|CYGWIN*) slow_command="ping -n 6 127.0.0.1 >NUL" ;; + *) slow_command="sleep 5" ;; +esac +write_manifest < timeout.log 2>&1 || rc=$? +[[ $rc -ne 0 ]] || { cat timeout.log; echo "FAIL: timed-out hook returned success"; exit 1; } +grep -q "hook 'build_finished' timed out after 1s" timeout.log || { + cat timeout.log; echo "FAIL: hook timeout was not reported"; exit 1; } + +# Invalid hook values fail before invoking any command. +write_manifest <<'EOF' +[package] +name = "hookprobe" +version = "0.1.0" + +[hooks] +build_start = "echo should-not-run>>invalid.log" +timeout_seconds = 0 +EOF +rm -f invalid.log +rc=0 +"$MCPP" build > invalid-config.log 2>&1 || rc=$? +[[ $rc -ne 0 ]] || { cat invalid-config.log; echo "FAIL: invalid hooks were accepted"; exit 1; } +[[ ! -e invalid.log ]] || { cat invalid.log; echo "FAIL: invalid hook ran"; exit 1; } +grep -q "timeout_seconds must be a positive integer" invalid-config.log || { + cat invalid-config.log; echo "FAIL: invalid hook diagnostic is missing"; exit 1; } + +echo "OK" From 9b9f0261565d5940bbe3f840d84b068a05e6bac8 Mon Sep 17 00:00:00 2001 From: speak-agent <248744407+speak-agent@users.noreply.github.com> Date: Sun, 30 Aug 2026 17:22:54 +0800 Subject: [PATCH 2/5] review: one parser for mcpp.toml, and a cmd.exe line cmd.exe accepts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- docs/05-mcpp-toml.md | 52 +++- docs/zh/05-mcpp-toml.md | 39 ++- modules/manifest/src/toml.cppm | 75 ++++++ modules/manifest/src/types.cppm | 32 +++ modules/platform/src/process.cppm | 75 +++++- src/build/execute.cppm | 13 + src/cli/cmd_build.cppm | 40 +-- src/hooks.cppm | 180 +++---------- tests/e2e/314_project_build_hooks.sh | 145 ----------- tests/e2e/317_project_build_hooks.sh | 316 +++++++++++++++++++++++ tests/unit/test_manifest.cpp | 146 +++++++++++ tests/unit/test_windows_command_line.cpp | 45 ++++ 12 files changed, 821 insertions(+), 337 deletions(-) delete mode 100644 tests/e2e/314_project_build_hooks.sh create mode 100644 tests/e2e/317_project_build_hooks.sh diff --git a/docs/05-mcpp-toml.md b/docs/05-mcpp-toml.md index 06053868..3d6fbc2f 100644 --- a/docs/05-mcpp-toml.md +++ b/docs/05-mcpp-toml.md @@ -2116,13 +2116,15 @@ side_effect = true | `build_start` | string | — | Runs after project preparation, immediately before the build | | `build_failed` | string | — | Runs when the build exits unsuccessfully | | `build_finished` | string | — | Runs when the build exits successfully | -| `timeout_seconds` | positive integer | `10` | Maximum time for each command | +| `timeout_seconds` | integer, 1–86400 | `10` | Maximum time for each command | | `enabled` | bool | `true` | Enables all commands in this table | | `side_effect` | bool | `true` | Whether a hook failure makes the overall build fail | -Commands run synchronously in the current project's root directory through the -host shell (`/bin/sh` or `cmd.exe`). Standard input/output/error keep their -ordinary terminal behaviour. Missing event commands are skipped. +Commands run synchronously through the host shell (`/bin/sh` or `cmd.exe`), +with the **project root** as their working directory — not the directory +`mcpp build` was typed in, so a relative path in a hook means the same thing +wherever the build was started. Standard input/output/error keep their ordinary +terminal behaviour. Missing event commands are skipped. The lifecycle is: @@ -2132,11 +2134,43 @@ build_start └─ build fails → build_failed ``` -`build_failed` and `build_finished` are mutually exclusive. A hook command -that cannot start, returns non-zero, or exceeds its timeout is a hook failure. -With `side_effect = false`, mcpp reports a warning and preserves the build's -result; with `true`, it returns failure. A hook's own failure does not trigger -another hook. +`build_failed` and `build_finished` are mutually exclusive, and both are +reachable only after `build_start` has run. A project that cannot be *prepared* +— an invalid manifest, an unresolvable dependency, no usable toolchain — fires +nothing: it has not started building, and its hook program may be exactly what +preparation would have installed. + +A hook command that cannot start, returns non-zero, or exceeds its timeout is a +hook failure. With `side_effect = false`, mcpp reports a warning and preserves +the build's result; with `true`, it returns failure — but a build that failed on +its own keeps its own exit code, so `mcpp build` never reports a compile error +as a notifier problem. A hook's own failure does not trigger another hook. + +Scope, precisely: + +- Only `mcpp build` runs hooks. `mcpp run`, `mcpp test` and + `mcpp build --configure-only` build too, and deliberately do not. +- Hooks belong to the **package being built**. In a workspace fan-out that is + each member in turn — its own `[hooks]`, around its own build, in its own + root. A *virtual* workspace root (`[workspace]` with no `[package]`) builds + nothing, so a `[hooks]` table there never fires. +- A dependency's `[hooks]` is never run. Only the root project's — installing a + package cannot make its author's shell command part of the consuming + project's build. +- Declaring an active hook opts the project out of the no-op fast path, because + `build_start` is specified to run after preparation. Expect `mcpp build` on an + already-current hooked project to cost a preparation pass rather than + milliseconds. + +An unrecognised key in `[hooks]` is a warning (an error under `--strict`), so a +manifest written for a newer mcpp still loads. An unrecognised *value* — a +non-string command, a `timeout_seconds` outside 1–86400 — is a manifest error. + +> **A hook is code, and `mcpp.toml` is part of the repository.** Building a +> freshly cloned project runs whatever its `[hooks]` say, with the privileges +> of whoever invoked `mcpp build`. This is the same trust `build.mcpp` already +> asks for ([07 — build.mcpp](07-build-mcpp.md)); `[hooks]` widens its reach +> rather than introducing it. Hook programs can be installed as ordinary xlings dependencies. For example, an audio notifier can keep its sound files inside its own executable rather diff --git a/docs/zh/05-mcpp-toml.md b/docs/zh/05-mcpp-toml.md index 668bd64b..41b6e3be 100644 --- a/docs/zh/05-mcpp-toml.md +++ b/docs/zh/05-mcpp-toml.md @@ -1812,12 +1812,13 @@ side_effect = true | `build_start` | 字符串 | — | 项目准备完成、正式构建开始前执行 | | `build_failed` | 字符串 | — | 构建以失败状态结束时执行 | | `build_finished` | 字符串 | — | 构建成功结束时执行 | -| `timeout_seconds` | 正整数 | `10` | 每条命令的最长执行时间 | +| `timeout_seconds` | 整数,1–86400 | `10` | 每条命令的最长执行时间 | | `enabled` | 布尔 | `true` | 是否启用本表中的全部命令 | | `side_effect` | 布尔 | `true` | Hook 失败是否让本次构建失败 | -命令在当前项目根目录中同步执行,使用宿主 Shell(`/bin/sh` 或 `cmd.exe`),标准输入、 -输出和错误沿用普通终端行为。没有配置的事件直接跳过。 +命令通过宿主 Shell(`/bin/sh` 或 `cmd.exe`)同步执行,工作目录是**项目根目录**—— +不是敲 `mcpp build` 的那个目录,所以 Hook 里的相对路径在哪儿发起构建都指同一处。 +标准输入、输出和错误沿用普通终端行为。没有配置的事件直接跳过。 生命周期为: @@ -1827,9 +1828,35 @@ build_start └─ 构建失败 → build_failed ``` -`build_failed` 与 `build_finished` 互斥。命令无法启动、返回非零或超过时限均视为 -Hook 失败。`side_effect = false` 时 mcpp 报 warning 并保留原构建结果;设为 `true` -时返回失败。Hook 自身失败不会再触发另一个 Hook。 +`build_failed` 与 `build_finished` 互斥,而且两者都只在 `build_start` 已经执行之后 +才可达。项目**准备**阶段就失败的情况——manifest 非法、依赖无法解析、没有可用工具链 +——一个 Hook 都不触发:此时构建尚未开始,而 Hook 程序本身可能正是准备阶段要装的东西。 + +命令无法启动、返回非零或超过时限均视为 Hook 失败。`side_effect = false` 时 mcpp 报 +warning 并保留原构建结果;设为 `true` 时返回失败——但构建自身失败时保留它自己的退出码, +所以 `mcpp build` 不会把一次编译错误报成通知程序的问题。Hook 自身失败不会再触发另一个 +Hook。 + +作用范围: + +- 只有 `mcpp build` 执行 Hook。`mcpp run`、`mcpp test` 和 + `mcpp build --configure-only` 同样会构建,但有意不执行。 +- Hook 属于**被构建的那个包**。workspace 展开时就是逐个成员:各自的 `[hooks]`、 + 各自的构建、各自的根目录。**虚拟** workspace 根(只有 `[workspace]` 没有 + `[package]`)不构建任何东西,写在那里的 `[hooks]` 永不触发。 +- 依赖的 `[hooks]` 永不执行,只有根项目的会——装一个包不会把包作者的 Shell 命令 + 变成消费方构建的一部分。 +- 声明了生效的 Hook 就等于让项目放弃空转快路径,因为 `build_start` 规定在准备阶段之后 + 执行。对已经是最新状态的带 Hook 项目,`mcpp build` 的代价是一次准备,而不是毫秒级。 + +`[hooks]` 里不认识的**键**是 warning(`--strict` 下为错误),所以为更新版 mcpp 写的 +manifest 在这一版仍能加载;不认识的**值**——命令不是字符串、`timeout_seconds` 不在 +1–86400 之间——是 manifest 错误。 + +> **Hook 是代码,而 `mcpp.toml` 是仓库的一部分。** 构建一个刚克隆下来的项目,会以 +> 执行 `mcpp build` 的那个账户的权限,运行它 `[hooks]` 里写的任何东西。这与 +> `build.mcpp`([07 — build.mcpp](07-build-mcpp.md))已经要求的信任是同一份; +> `[hooks]` 扩大的是它的范围,而不是引入了一份新的信任。 Hook 程序可以作为普通 xlings 依赖安装。例如,音频通知程序可以把音频内置进自己的 可执行文件,无需让 mcpp 处理媒体资源: diff --git a/modules/manifest/src/toml.cppm b/modules/manifest/src/toml.cppm index c20773fa..66a22161 100644 --- a/modules/manifest/src/toml.cppm +++ b/modules/manifest/src/toml.cppm @@ -1520,6 +1520,81 @@ std::expected parse_string(std::string_view content, } } + // [hooks] — project build lifecycle commands (#496). Parsed HERE rather + // than by the module that runs them, for the reason Appendix A of + // docs/05-mcpp-toml.md states: mcpp.toml has one grammar and one parser. + // A second reader of the same file would report ITS syntax errors in its + // own vocabulary — a typo in [package] arriving as "invalid hook + // configuration" — and would sit outside the warning/--strict policy every + // other section is subject to. + if (auto* hooksValue = doc->get("hooks"); + hooksValue && !hooksValue->is_table()) { + return std::unexpected(error(origin, + "[hooks] must be a table of lifecycle commands")); + } + if (auto* ht = doc->get_table("hooks")) { + // Values are the author's own and visible in front of them: a wrong + // type is an error, not a silent default. An unrecognised KEY is a + // warning (--strict makes it an error), same split as [build] — so a + // manifest written for a later mcpp still loads on this one. + auto read_command = [&](std::string_view key, std::string& out) + -> std::optional { + auto it = ht->find(key); + if (it == ht->end()) return std::nullopt; + if (!it->second.is_string() || it->second.as_string().empty()) + return error(origin, std::format( + "[hooks].{} must be a non-empty command string", key)); + out = it->second.as_string(); + return std::nullopt; + }; + for (auto [key, out] : std::initializer_list< + std::pair>{ + {"build_start", &m.hooks.buildStart}, + {"build_failed", &m.hooks.buildFailed}, + {"build_finished", &m.hooks.buildFinished}}) { + if (auto e = read_command(key, *out)) return std::unexpected(*e); + } + + if (auto it = ht->find("timeout_seconds"); it != ht->end()) { + // Bounded above as well as below: the value becomes a + // std::chrono::seconds deadline, and "a timeout so large it is not + // one" is a mistake worth naming rather than honouring. + constexpr std::int64_t kMaxTimeout = 24 * 60 * 60; + if (!it->second.is_int() || it->second.as_int() <= 0 + || it->second.as_int() > kMaxTimeout) + return std::unexpected(error(origin, std::format( + "[hooks].timeout_seconds must be a positive integer " + "(seconds, at most {})", kMaxTimeout))); + m.hooks.timeoutSeconds = static_cast(it->second.as_int()); + } + + for (auto [key, out] : std::initializer_list< + std::pair>{ + {"enabled", &m.hooks.enabled}, + {"side_effect", &m.hooks.sideEffect}}) { + auto it = ht->find(key); + if (it == ht->end()) continue; + if (!it->second.is_bool()) + return std::unexpected(error(origin, std::format( + "[hooks].{} must be a boolean", key))); + *out = it->second.as_bool(); + } + + static constexpr std::string_view kKnownHookKeys[] = { + "build_start", "build_failed", "build_finished", + "timeout_seconds", "enabled", "side_effect", + }; + for (auto& [k, _] : *ht) { + bool known = false; + for (auto kk : kKnownHookKeys) if (k == kk) { known = true; break; } + if (!known) + m.schemaWarnings.push_back(std::format( + "[hooks] has unsupported key '{}' (ignored). Keys: " + "build_start, build_failed, build_finished, " + "timeout_seconds, enabled, side_effect.", k)); + } + } + // [lib] — library root convention (cargo-style). if (auto v = doc->get_string("lib.path")) { m.lib.path = *v; diff --git a/modules/manifest/src/types.cppm b/modules/manifest/src/types.cppm index 9d3ec6a0..ace3921f 100644 --- a/modules/manifest/src/types.cppm +++ b/modules/manifest/src/types.cppm @@ -919,6 +919,37 @@ struct WorkspaceConfig { bool present = false; }; +// `[hooks]` — project build lifecycle commands (#496). +// +// The commands are host-shell strings written by the project author, run by +// `mcpp build` around the build it performs. See docs/05-mcpp-toml.md §2.16. +// +// ⚠️ ONLY THE ROOT PROJECT'S HOOKS ARE EVER RUN. Every manifest mcpp parses +// carries this field, including a DEPENDENCY's — and `mcpp build` reaches the +// invoker (mcpp.hooks) with the root project's manifest alone. A dependency +// that declares hooks is inert by construction, which is the only reason +// `mcpp add` of a third-party package does not become "run their shell +// command on my next build". Anything that adds a second call site inherits +// that responsibility. +struct Hooks { + std::string buildStart; + std::string buildFailed; + std::string buildFinished; + int timeoutSeconds = 10; // per command + bool enabled = true; // whole table + // Whether a hook failure fails the build. False downgrades it to a + // warning and preserves whatever the build itself returned. + bool sideEffect = true; + + // "This project has work for `mcpp build` to do." Distinct from `enabled`: + // a table that only sets policy keys declares no command, and must leave + // the build path it would otherwise divert (the fast path) untouched. + bool active() const { + return enabled && !(buildStart.empty() && buildFailed.empty() + && buildFinished.empty()); + } +}; + // [profile.] — bundled build settings (opt level, debug, lto, strip). struct Profile { std::string optLevel = "2"; @@ -985,6 +1016,7 @@ struct Manifest { Resources resources; // [resources] (mcpp#365) RuntimeConfig runtimeConfig; XlingsConfig xlings; // [xlings] build environment (L-1) + Hooks hooks; // [hooks] lifecycle commands (#496) std::vector conditionalConfigs; // [target.'cfg(...)'.build], deferred std::map profiles; // [profile.] // [features] — feature name → implied features ("default" = default set). diff --git a/modules/platform/src/process.cppm b/modules/platform/src/process.cppm index 9309d6c5..78fccfdc 100644 --- a/modules/platform/src/process.cppm +++ b/modules/platform/src/process.cppm @@ -112,11 +112,22 @@ int run_exec_deadline(const std::vector& argv, std::chrono::milliseconds deadline, bool* timed_out); -// Run one host-shell command with inherited stdio and a real deadline. -// POSIX uses /bin/sh; Windows uses cmd.exe. This is for user-authored command -// strings such as project hooks — programmatic launches should keep using the -// argv-based run_exec_deadline API above. +// Run one host-shell command with inherited stdio, a working directory and a +// real deadline. POSIX uses /bin/sh; Windows uses cmd.exe. This is for +// user-authored command strings such as project hooks — programmatic launches +// keep using the argv-based run_exec_deadline API above. +// +// `cwd` is where the command runs; empty means "inherit ours". It is a +// PARAMETER rather than something the caller arranges with a chdir: the +// process-wide working directory is shared state, and the launchers underneath +// already carry a per-child cwd (posix_spawn_file_actions_addchdir_np / +// CreateProcess's lpCurrentDirectory). +// +// Returns 127 when the shell itself could not be started — the same code a +// shell uses for a command it cannot find, and never confusable with a +// command that ran. int run_shell_deadline(std::string_view command, + std::string_view cwd, std::chrono::milliseconds deadline, bool* timed_out); @@ -168,6 +179,23 @@ int extract_exit_code(int raw_status); std::string windows_command_from_argv(const std::vector& argv); std::string windows_wrap_for_cmd_c(std::string_view cmd); +// The command line that runs a USER-AUTHORED shell command through cmd.exe. +// +// ⚠️ NOT `windows_command_from_argv({"cmd.exe", "/d", "/s", "/c", command})`. +// That shape is for a program plus its argv, where CreateProcess's parsing is +// what has to be satisfied. cmd.exe is not parsed that way: its switches must +// arrive BARE (quoted, they are no longer switches), and the command tail is +// governed by the /C quote rule above rather than by argv quoting — so an +// argv-quoted command arrives carrying a pair cmd does not consume. That is +// #425 one layer up, and it is why this is its own shape: +// +// cmd.exe /d /s /c "" +// +// /s makes the rule unconditional (strip exactly the outer pair), so the +// command reaches cmd verbatim no matter how many quotes it contains; /d skips +// AutoRun so a user's registry-installed shell hook cannot alter it. +std::string windows_shell_command_line(std::string_view command); + } // namespace mcpp::platform::process // ─── Implementation ────────────────────────────────────────────────────── @@ -189,6 +217,12 @@ std::string windows_wrap_for_cmd_c(std::string_view cmd) { return "\"" + std::string(cmd) + "\""; } +std::string windows_shell_command_line(std::string_view command) { + // One derivation for the outer pair: the same wrap the /c rule above is + // written against. + return "cmd.exe /d /s /c " + windows_wrap_for_cmd_c(command); +} + namespace { // Append a non-interactive stdin redirect to prevent child processes from @@ -593,12 +627,17 @@ struct BoundedOutcome { // `capture == false` runs the child on the caller's stdio: live output, and a // real terminal for anything that checks. `run_exec_deadline` needs that; the // capturing variants need the pipe. +// `windowsCommandLine` overrides what the Windows branch launches. Empty (the +// normal case) means "derive it from argv". A shell command is the one caller +// that must NOT be derived that way — see windows_shell_command_line — and the +// POSIX branch is unaffected either way, because it never flattens argv. BoundedOutcome dispatch_bounded( const std::vector& argv, const std::vector>& extraEnv, std::string_view cwd, std::chrono::milliseconds deadline, - bool capture) + bool capture, + std::string_view windowsCommandLine = {}) { BoundedOutcome outcome; @@ -625,7 +664,9 @@ BoundedOutcome dispatch_bounded( : nullptr; if constexpr (mcpp::platform::is_windows) { - const auto cmd = windows_command_from_argv(argv); + const auto cmd = windowsCommandLine.empty() + ? windows_command_from_argv(argv) + : std::string(windowsCommandLine); auto r = mcpp::platform::winproc::capture_with_deadline( cmd.c_str(), envArg, envCount, cwdArg, ms, sink, &outcome.output); outcome.supported = r.supported; @@ -667,15 +708,25 @@ int run_exec_deadline(const std::vector& argv, } int run_shell_deadline(std::string_view command, + std::string_view cwd, std::chrono::milliseconds deadline, bool* timed_out) { - std::vector argv; - if constexpr (mcpp::platform::is_windows) - argv = {"cmd.exe", "/d", "/s", "/c", std::string(command)}; - else - argv = {"/bin/sh", "-c", std::string(command)}; - return run_exec_deadline(argv, {}, deadline, timed_out); + if (timed_out) *timed_out = false; + if (command.empty() || deadline.count() <= 0) return 127; + + // argv is what the POSIX branch launches; the Windows branch takes the + // shaped command line instead. Both are built here so neither platform's + // spelling can drift into a launcher that does not use it. + const std::vector argv{"/bin/sh", "-c", std::string(command)}; + auto r = dispatch_bounded(argv, {}, cwd, deadline, /*capture=*/false, + windows_shell_command_line(command)); + // No fallback to the unbounded launcher here, unlike run_exec_deadline: a + // hook's deadline and its working directory are both part of what the + // caller asked for, and the unbounded path can honour neither. + if (!r.supported) return 127; + if (timed_out) *timed_out = r.timed_out; + return r.exit_code; } RunResult capture_exec_deadline( diff --git a/src/build/execute.cppm b/src/build/execute.cppm index 8c5d25bd..1799ac68 100644 --- a/src/build/execute.cppm +++ b/src/build/execute.cppm @@ -808,6 +808,10 @@ struct FastPathIdentity { // `[build] target` — the project's DEFAULT cross target, and the reason // try_fast_run cannot assume the artifact runs here. See its use. std::string defaultTarget; + // `[hooks]` with at least one command (#496). Third field down riding on + // the same single manifest read, and the only one that can VETO the fast + // path rather than describe it — see try_fast_build. + bool hooksActive = false; }; std::optional @@ -822,6 +826,7 @@ fast_path_identity(const std::filesystem::path& projectRoot, m->resources.files, mcpp::extension_table_for(m->buildConfig.moduleExtensions), m->buildConfig.target, + m->hooks.active(), }; } @@ -835,6 +840,14 @@ export std::optional try_fast_build(const std::filesystem::path& projectRoo auto want = fast_path_identity(projectRoot); if (!want) return std::nullopt; + // #496. A project with build hooks always takes the full path. The fast + // path is defined as "skip preparation", and `build_start` is specified to + // run AFTER it — a hook program installed as an `[xlings] deps` entry does + // not exist until preparation has run. Declining here rather than in + // cmd_build keeps the decision next to the manifest that answers it; the + // full path then runs the hooks around run_build_plan. + if (want->hooksActive) return std::nullopt; + // P3: read multi-entry cache and find the entry matching this // (target, profile, cache mode) triple. Matching on the target alone served // the wrong profile's artifacts, and ignoring the cache mode replayed a diff --git a/src/cli/cmd_build.cppm b/src/cli/cmd_build.cppm index b05cd6b2..5a56f801 100644 --- a/src/cli/cmd_build.cppm +++ b/src/cli/cmd_build.cppm @@ -43,22 +43,30 @@ workspace_fanout_members(bool wantAll, const std::string& package_filter) { return std::nullopt; } +// run_build_plan, wrapped in the project's `[hooks]` lifecycle (#496). +// +// The hooks come off the context's own manifest, so they are the ones belonging +// to the package being built — which in a workspace fan-out is the MEMBER, once +// per member. The lifecycle is deliberately paired: build_finished/build_failed +// are only ever reached after build_start has run, so a project that could not +// be prepared at all (bad manifest, unresolvable dependency, no toolchain) +// fires nothing — its hook programs may be exactly what preparation failed to +// install. int run_build_with_hooks(mcpp::build::BuildContext& ctx, bool verbose, bool no_cache, std::string_view targetOverride) { - auto config = mcpp::hooks::load(ctx.projectRoot / "mcpp.toml"); - if (!config) { - mcpp::ui::error(std::format("invalid hook configuration: {}", config.error())); - return 2; - } + auto const& hooks = ctx.manifest.hooks; - if (!mcpp::hooks::invoke(*config, mcpp::hooks::Event::BuildStart, + if (!mcpp::hooks::invoke(hooks, mcpp::hooks::Event::BuildStart, ctx.projectRoot)) return 1; int rc = mcpp::build::run_build_plan(ctx, verbose, no_cache, targetOverride); auto terminalEvent = rc == 0 ? mcpp::hooks::Event::BuildFinished : mcpp::hooks::Event::BuildFailed; - bool hookOk = mcpp::hooks::invoke(*config, terminalEvent, ctx.projectRoot); + // The build's own exit code outranks the hook's: `mcpp build` returning + // "the notifier failed" for a compile error would answer a question nobody + // asked. A hook failure only decides the exit code of a build that worked. + bool hookOk = mcpp::hooks::invoke(hooks, terminalEvent, ctx.projectRoot); return rc != 0 ? rc : (hookOk ? 0 : 1); } @@ -157,19 +165,11 @@ export int cmd_build(const mcpplibs::cmdline::ParsedArgs& parsed) { && ov.cache_mode.empty()) { auto root = mcpp::project::find_manifest_root(std::filesystem::current_path()); if (root) { - auto config = mcpp::hooks::load(*root / "mcpp.toml"); - if (!config) { - mcpp::ui::error(std::format( - "invalid hook configuration: {}", config.error())); - return 2; - } - // Hooked builds must first prepare the project so xlings-provided - // hook programs are available before build_start. Projects with no - // active hooks keep the existing fast path unchanged. - if (!mcpp::hooks::active(*config)) { - if (auto rc = mcpp::build::try_fast_build(*root, verbose, no_cache)) { - return *rc; - } + // A project with active `[hooks]` declines the fast path from + // inside try_fast_build, where the manifest that says so is + // already loaded. + if (auto rc = mcpp::build::try_fast_build(*root, verbose, no_cache)) { + return *rc; } } } diff --git a/src/hooks.cppm b/src/hooks.cppm index 7450ffb5..16f581ce 100644 --- a/src/hooks.cppm +++ b/src/hooks.cppm @@ -1,9 +1,14 @@ -// mcpp.hooks — project-level build lifecycle commands from mcpp.toml. +// mcpp.hooks — running the project's `[hooks]` lifecycle commands. +// +// The CONFIGURATION is not parsed here: `[hooks]` is a section of mcpp.toml +// and mcpp.toml has one parser (mcpp.manifest). What lives here is the part +// that is policy rather than grammar — when a command runs, what a failure +// means, and what the user is told. export module mcpp.hooks; import std; -import mcpp.libs.toml; +import mcpp.manifest; import mcpp.platform.process; import mcpp.ui; @@ -11,23 +16,13 @@ export namespace mcpp::hooks { enum class Event { BuildStart, BuildFailed, BuildFinished }; -struct Config { - std::string buildStart; - std::string buildFailed; - std::string buildFinished; - int timeoutSeconds = 10; - bool enabled = true; - bool sideEffect = true; -}; - -std::expected -load(const std::filesystem::path& manifestPath); - -bool active(const Config& config); - -// True means the build may continue/keep its original result. False means a -// hook failed with side_effect=true. -bool invoke(const Config& config, Event event, +// Run the command this event names, if the project declared one. +// +// Returns whether the BUILD may keep its own result. False means a hook failed +// while `side_effect` was on, i.e. the failure is the build's now. A missing +// command, a disabled table, and a hook that failed under `side_effect = false` +// all return true — from the build's point of view nothing happened. +bool invoke(const mcpp::manifest::Hooks& hooks, Event event, const std::filesystem::path& projectRoot); } // namespace mcpp::hooks @@ -45,17 +40,20 @@ std::string_view event_name(Event event) { return "unknown"; } -std::string_view event_command(const Config& config, Event event) { +std::string_view event_command(const mcpp::manifest::Hooks& hooks, Event event) { switch (event) { - case Event::BuildStart: return config.buildStart; - case Event::BuildFailed: return config.buildFailed; - case Event::BuildFinished: return config.buildFinished; + case Event::BuildStart: return hooks.buildStart; + case Event::BuildFailed: return hooks.buildFailed; + case Event::BuildFinished: return hooks.buildFinished; } return {}; } -bool report_failure(const Config& config, std::string message) { - if (config.sideEffect) { +// One place decides what a hook failure costs, so `side_effect` cannot be +// honoured on one failure mode and forgotten on another. The message is +// identical either way — only its severity and the build's fate differ. +bool report_failure(const mcpp::manifest::Hooks& hooks, std::string message) { + if (hooks.sideEffect) { mcpp::ui::error(message); return false; } @@ -65,134 +63,26 @@ bool report_failure(const Config& config, std::string message) { } // namespace -std::expected -load(const std::filesystem::path& manifestPath) { - namespace toml = mcpp::libs::toml; - - auto doc = toml::parse_file(manifestPath); - if (!doc) { - auto const& e = doc.error(); - if (e.where.line != 0) - return std::unexpected(std::format( - "mcpp.toml:{}:{}: {}", e.where.line, e.where.column, e.message)); - return std::unexpected(e.message); - } - - Config config; - auto* hooksValue = doc->get("hooks"); - if (!hooksValue) return config; - if (!hooksValue->is_table()) - return std::unexpected("[hooks] must be a table"); - - auto const& table = hooksValue->as_table(); - static constexpr std::string_view known[] = { - "build_start", "build_failed", "build_finished", - "timeout_seconds", "enabled", "side_effect", - }; - for (auto const& [key, _] : table) { - if (std::ranges::find(known, key) == std::end(known)) - return std::unexpected(std::format("[hooks] has unknown key '{}'", key)); - } - - std::string error; - auto read_command = [&](std::string_view key, std::string& out) { - auto it = table.find(key); - if (it == table.end()) return true; - if (!it->second.is_string() || it->second.as_string().empty()) { - error = std::format("[hooks].{} must be a non-empty string", key); - return false; - } - out = it->second.as_string(); - return true; - }; - if (!read_command("build_start", config.buildStart) - || !read_command("build_failed", config.buildFailed) - || !read_command("build_finished", config.buildFinished)) - return std::unexpected(std::move(error)); - - if (auto it = table.find("timeout_seconds"); it != table.end()) { - if (!it->second.is_int() || it->second.as_int() <= 0 - || it->second.as_int() > std::numeric_limits::max()) - return std::unexpected( - "[hooks].timeout_seconds must be a positive integer"); - config.timeoutSeconds = static_cast(it->second.as_int()); - } - - auto read_bool = [&](std::string_view key, bool& out) { - auto it = table.find(key); - if (it == table.end()) return true; - if (!it->second.is_bool()) { - error = std::format("[hooks].{} must be a boolean", key); - return false; - } - out = it->second.as_bool(); - return true; - }; - if (!read_bool("enabled", config.enabled) - || !read_bool("side_effect", config.sideEffect)) - return std::unexpected(std::move(error)); - - return config; -} - -bool active(const Config& config) { - return config.enabled - && (!config.buildStart.empty() || !config.buildFailed.empty() - || !config.buildFinished.empty()); -} - -bool invoke(const Config& config, Event event, +bool invoke(const mcpp::manifest::Hooks& hooks, Event event, const std::filesystem::path& projectRoot) { - if (!config.enabled) return true; - auto command = event_command(config, event); + if (!hooks.enabled) return true; + auto command = event_command(hooks, event); if (command.empty()) return true; - std::error_code ec; - auto previous = std::filesystem::current_path(ec); - if (ec) - return report_failure(config, std::format( - "hook '{}' could not read the current directory: {}", - event_name(event), ec.message())); - - // ponytail: hooks are synchronous; a scoped cwd is the smallest portable - // way to honour the project-root contract. Pass cwd through the process - // API if hook execution ever becomes concurrent. - std::filesystem::current_path(projectRoot, ec); - if (ec) - return report_failure(config, std::format( - "hook '{}' could not enter the project root: {}", - event_name(event), ec.message())); - - struct RestorePath { - std::filesystem::path path; - bool active = true; - ~RestorePath() { - if (!active) return; - std::error_code ignored; - std::filesystem::current_path(path, ignored); - } - } restore{previous}; - bool timedOut = false; + // The project root is passed to the launcher, not arranged with a chdir: + // mcpp's working directory is process-wide state, and a hook is not + // entitled to move it even briefly. int rc = mcpp::platform::process::run_shell_deadline( - command, std::chrono::seconds(config.timeoutSeconds), &timedOut); - - std::error_code restoreError; - std::filesystem::current_path(previous, restoreError); - restore.active = false; - if (restoreError) { - mcpp::ui::error(std::format( - "hook '{}' could not restore the working directory: {}", - event_name(event), restoreError.message())); - return false; - } + command, projectRoot.string(), + std::chrono::seconds(hooks.timeoutSeconds), &timedOut); if (timedOut) - return report_failure(config, std::format( - "hook '{}' timed out after {}s", - event_name(event), config.timeoutSeconds)); + return report_failure(hooks, std::format( + "hook '{}' timed out after {}s", event_name(event), + hooks.timeoutSeconds)); if (rc != 0) - return report_failure(config, std::format( + return report_failure(hooks, std::format( "hook '{}' exited with status {}", event_name(event), rc)); return true; } diff --git a/tests/e2e/314_project_build_hooks.sh b/tests/e2e/314_project_build_hooks.sh deleted file mode 100644 index 596887a6..00000000 --- a/tests/e2e/314_project_build_hooks.sh +++ /dev/null @@ -1,145 +0,0 @@ -#!/usr/bin/env bash -# requires: -# Project-level build hooks: lifecycle order, policy, timeout and project cwd. -set -e - -TMP=$(mktemp -d) -trap 'rm -rf "$TMP"' EXIT -mkdir -p "$TMP/app/src" "$TMP/app/nested" - -cat > "$TMP/app/src/main.cpp" <<'EOF' -int main() { return 0; } -EOF - -write_manifest() { - cat > "$TMP/app/mcpp.toml" -} - -write_manifest <<'EOF' -[package] -name = "hookprobe" -version = "0.1.0" - -[hooks] -build_start = "echo start>>hooks.log" -build_failed = "echo failed>>hooks.log" -build_finished = "echo finished>>hooks.log" -timeout_seconds = 10 -enabled = true -side_effect = true -EOF - -# Invoke below the project root: hook-relative paths still belong to the root. -cd "$TMP/app/nested" -"$MCPP" build > success-1.log 2>&1 || { - cat success-1.log; echo "FAIL: hooked build failed"; exit 1; } -"$MCPP" build > success-2.log 2>&1 || { - cat success-2.log; echo "FAIL: second hooked build failed"; exit 1; } -cd "$TMP/app" -[[ "$(cat hooks.log)" == $'start\nfinished\nstart\nfinished' ]] || { - cat hooks.log; echo "FAIL: success lifecycle order/count is wrong"; exit 1; } - -# A compiler failure chooses build_failed, never build_finished. -cat > src/main.cpp <<'EOF' -#error "intentional hook failure path" -int main() { return 0; } -EOF -: > hooks.log -rc=0 -"$MCPP" build > build-failed.log 2>&1 || rc=$? -[[ $rc -ne 0 ]] || { cat build-failed.log; echo "FAIL: broken source built"; exit 1; } -[[ "$(cat hooks.log)" == $'start\nfailed' ]] || { - cat hooks.log; echo "FAIL: failure lifecycle is wrong"; exit 1; } - -cat > src/main.cpp <<'EOF' -int main() { return 0; } -EOF - -# A non-side-effecting hook reports its failure but preserves build success. -write_manifest <<'EOF' -[package] -name = "hookprobe" -version = "0.1.0" - -[hooks] -build_finished = "mcpp-hook-command-that-does-not-exist" -side_effect = false -EOF -"$MCPP" build > ignored-hook-failure.log 2>&1 || { - cat ignored-hook-failure.log - echo "FAIL: side_effect=false changed build success"; exit 1; } -grep -q "warning: hook 'build_finished' exited with status" ignored-hook-failure.log || { - cat ignored-hook-failure.log; echo "FAIL: ignored hook failure was not reported"; exit 1; } - -# The same hook failure is fatal with the default side_effect=true. -write_manifest <<'EOF' -[package] -name = "hookprobe" -version = "0.1.0" - -[hooks] -build_finished = "mcpp-hook-command-that-does-not-exist" -EOF -rc=0 -"$MCPP" build > fatal-hook-failure.log 2>&1 || rc=$? -[[ $rc -ne 0 ]] || { - cat fatal-hook-failure.log; echo "FAIL: fatal hook failure returned success"; exit 1; } -grep -q "error: hook 'build_finished' exited with status" fatal-hook-failure.log || { - cat fatal-hook-failure.log; echo "FAIL: fatal hook failure was not reported"; exit 1; } - -# Disabled hooks do nothing, including not forcing a command to run. -write_manifest <<'EOF' -[package] -name = "hookprobe" -version = "0.1.0" - -[hooks] -build_start = "echo disabled>>disabled.log" -build_finished = "echo disabled>>disabled.log" -enabled = false -EOF -rm -f disabled.log -"$MCPP" build > disabled.log.out 2>&1 || { - cat disabled.log.out; echo "FAIL: disabled hook build failed"; exit 1; } -[[ ! -e disabled.log ]] || { - cat disabled.log; echo "FAIL: enabled=false still ran hooks"; exit 1; } - -# A positive timeout is enforced by the platform process runner. -case "$(uname -s)" in - MINGW*|MSYS*|CYGWIN*) slow_command="ping -n 6 127.0.0.1 >NUL" ;; - *) slow_command="sleep 5" ;; -esac -write_manifest < timeout.log 2>&1 || rc=$? -[[ $rc -ne 0 ]] || { cat timeout.log; echo "FAIL: timed-out hook returned success"; exit 1; } -grep -q "hook 'build_finished' timed out after 1s" timeout.log || { - cat timeout.log; echo "FAIL: hook timeout was not reported"; exit 1; } - -# Invalid hook values fail before invoking any command. -write_manifest <<'EOF' -[package] -name = "hookprobe" -version = "0.1.0" - -[hooks] -build_start = "echo should-not-run>>invalid.log" -timeout_seconds = 0 -EOF -rm -f invalid.log -rc=0 -"$MCPP" build > invalid-config.log 2>&1 || rc=$? -[[ $rc -ne 0 ]] || { cat invalid-config.log; echo "FAIL: invalid hooks were accepted"; exit 1; } -[[ ! -e invalid.log ]] || { cat invalid.log; echo "FAIL: invalid hook ran"; exit 1; } -grep -q "timeout_seconds must be a positive integer" invalid-config.log || { - cat invalid-config.log; echo "FAIL: invalid hook diagnostic is missing"; exit 1; } - -echo "OK" diff --git a/tests/e2e/317_project_build_hooks.sh b/tests/e2e/317_project_build_hooks.sh new file mode 100644 index 00000000..4ad37a21 --- /dev/null +++ b/tests/e2e/317_project_build_hooks.sh @@ -0,0 +1,316 @@ +#!/usr/bin/env bash +# requires: +# Project-level build hooks (#496): lifecycle order, policy, timeout, the +# directory a hook runs in, and the two places nothing may fire. +set -e +source "$(dirname "$0")/_host_path.sh" + +TMP=$(mktemp -d) +trap 'rm -rf "$TMP"' EXIT +mkdir -p "$TMP/app/src" "$TMP/app/nested" + +cat > "$TMP/app/src/main.cpp" <<'EOF' +int main() { return 0; } +EOF + +write_manifest() { + cat > "$TMP/app/mcpp.toml" +} + +# cmd.exe writes CRLF and /bin/sh writes LF, so the line ENDINGS are the host's +# and the line CONTENT is the hook's. Compare the content. +# expect_log ... +expect_log() { + local file=$1; shift + local want got + want=$(printf '%s\n' "$@") + got=$(tr -d '\r' < "$file" 2>/dev/null || true) + if [[ "$got" != "$want" ]]; then + echo "FAIL: $file" + echo "--- expected ---"; printf '%s\n' "$want" + echo "--- actual ---"; printf '%s\n' "$got" + exit 1 + fi +} + +# A WHOLE line, matched as one: a substring would still pass if the message +# were reworded around it. No pipe into grep — a `grep -q` that exits on the +# first match SIGPIPEs its producer, which reads as failure the moment anyone +# adds `set -o pipefail` to this file. +expect_line() { # expect_line + local file=$1 line=$2 + local body + body=$(tr -d '\r' < "$file" 2>/dev/null || true) + if [[ $'\n'"$body"$'\n' != *$'\n'"$line"$'\n'* ]]; then + cat "$file" + echo "FAIL: '$line' is not a line of $file" + exit 1 + fi +} + +# ── The success lifecycle, twice ───────────────────────────────────────── +# +# Twice on purpose: the second `mcpp build` is the one that would otherwise +# take the no-op fast path, which skips preparation and would skip the hooks +# with it. A single run cannot tell the two paths apart. +write_manifest <<'EOF' +[package] +name = "hookprobe" +version = "0.1.0" + +[hooks] +build_start = "echo start>>hooks.log" +build_failed = "echo failed>>hooks.log" +build_finished = "echo finished>>hooks.log" +timeout_seconds = 10 +enabled = true +side_effect = true +EOF + +# Invoked BELOW the project root: a hook's relative paths belong to the root, +# not to wherever the user happened to type `mcpp build`. +cd "$TMP/app/nested" +"$MCPP" build > success-1.log 2>&1 || { + cat success-1.log; echo "FAIL: hooked build failed"; exit 1; } +"$MCPP" build > success-2.log 2>&1 || { + cat success-2.log; echo "FAIL: second hooked build failed"; exit 1; } +cd "$TMP/app" +[[ ! -e nested/hooks.log ]] || { + echo "FAIL: hook ran in the invocation directory, not the project root"; exit 1; } +expect_log hooks.log start finished start finished + +# ── A compiler failure chooses build_failed, never build_finished ──────── +cat > src/main.cpp <<'EOF' +#error "intentional hook failure path" +int main() { return 0; } +EOF +: > hooks.log +rc=0 +"$MCPP" build > build-failed.log 2>&1 || rc=$? +[[ $rc -ne 0 ]] || { cat build-failed.log; echo "FAIL: broken source built"; exit 1; } +expect_log hooks.log start failed + +cat > src/main.cpp <<'EOF' +int main() { return 0; } +EOF + +# ── side_effect = false: reported, but the build keeps its own result ──── +write_manifest <<'EOF' +[package] +name = "hookprobe" +version = "0.1.0" + +[hooks] +build_finished = "mcpp-hook-command-that-does-not-exist" +side_effect = false +EOF +"$MCPP" build > ignored-hook-failure.log 2>&1 || { + cat ignored-hook-failure.log + echo "FAIL: side_effect=false changed build success"; exit 1; } +grep -q "warning: hook 'build_finished' exited with status" ignored-hook-failure.log || { + cat ignored-hook-failure.log; echo "FAIL: ignored hook failure was not reported"; exit 1; } + +# ── The same failure is fatal under the default side_effect = true ────── +write_manifest <<'EOF' +[package] +name = "hookprobe" +version = "0.1.0" + +[hooks] +build_finished = "mcpp-hook-command-that-does-not-exist" +EOF +rc=0 +"$MCPP" build > fatal-hook-failure.log 2>&1 || rc=$? +[[ $rc -ne 0 ]] || { + cat fatal-hook-failure.log; echo "FAIL: fatal hook failure returned success"; exit 1; } +grep -q "error: hook 'build_finished' exited with status" fatal-hook-failure.log || { + cat fatal-hook-failure.log; echo "FAIL: fatal hook failure was not reported"; exit 1; } + +# ── enabled = false switches off a table that still names commands ────── +write_manifest <<'EOF' +[package] +name = "hookprobe" +version = "0.1.0" + +[hooks] +build_start = "echo disabled>>disabled.log" +build_finished = "echo disabled>>disabled.log" +enabled = false +EOF +rm -f disabled.log +"$MCPP" build > disabled.log.out 2>&1 || { + cat disabled.log.out; echo "FAIL: disabled hook build failed"; exit 1; } +[[ ! -e disabled.log ]] || { + cat disabled.log; echo "FAIL: enabled=false still ran hooks"; exit 1; } + +# ── The timeout is a real bound, on both host shells ──────────────────── +case "$(uname -s)" in + MINGW*|MSYS*|CYGWIN*) slow_command="ping -n 6 127.0.0.1 >NUL" ;; + *) slow_command="sleep 5" ;; +esac +write_manifest < timeout.log 2>&1 || rc=$? +[[ $rc -ne 0 ]] || { cat timeout.log; echo "FAIL: timed-out hook returned success"; exit 1; } +expect_line timeout.log "error: hook 'build_finished' timed out after 1s" + +# ── An invalid value fails the manifest, before any command runs ──────── +# +# And it fails as a MANIFEST error naming the key — `[hooks]` is a section of +# mcpp.toml, so a bad value there is reported by the same parser and in the +# same words as a bad value anywhere else in the file. +write_manifest <<'EOF' +[package] +name = "hookprobe" +version = "0.1.0" + +[hooks] +build_start = "echo should-not-run>>invalid.log" +timeout_seconds = 0 +EOF +rm -f invalid.log +rc=0 +"$MCPP" build > invalid-config.log 2>&1 || rc=$? +[[ $rc -ne 0 ]] || { cat invalid-config.log; echo "FAIL: invalid hooks were accepted"; exit 1; } +[[ ! -e invalid.log ]] || { cat invalid.log; echo "FAIL: invalid hook ran"; exit 1; } +grep -q "\[hooks\].timeout_seconds must be a positive integer" invalid-config.log || { + cat invalid-config.log; echo "FAIL: invalid hook diagnostic is missing"; exit 1; } + +# ── An unknown key is a warning, and the known ones still run ─────────── +: > hooks.log +write_manifest <<'EOF' +[package] +name = "hookprobe" +version = "0.1.0" + +[hooks] +build_start = "echo start>>hooks.log" +build_finsihed = "echo typo>>hooks.log" +EOF +"$MCPP" build > unknown-key.log 2>&1 || { + cat unknown-key.log; echo "FAIL: unknown hook key was fatal"; exit 1; } +grep -q "\[hooks\] has unsupported key 'build_finsihed'" unknown-key.log || { + cat unknown-key.log; echo "FAIL: unknown hook key was not reported"; exit 1; } +expect_log hooks.log start + +# ── Preparation failing fires NOTHING ─────────────────────────────────── +# +# The lifecycle is paired: build_finished/build_failed are only reachable after +# build_start has run. A project that cannot be prepared has not started +# building — and its hook program may be exactly what preparation would have +# installed, so "report the failure" is not available here. +: > hooks.log +write_manifest <<'EOF' +[package] +name = "hookprobe" +version = "0.1.0" + +[hooks] +build_start = "echo start>>hooks.log" +build_failed = "echo failed>>hooks.log" + +[dependencies] +absent = { path = "no/such/dependency" } +EOF +rc=0 +"$MCPP" build > prepare-failed.log 2>&1 || rc=$? +[[ $rc -ne 0 ]] || { + cat prepare-failed.log; echo "FAIL: a missing path dependency built"; exit 1; } +expect_log hooks.log + +# ── A DEPENDENCY's hooks never run ────────────────────────────────────── +# +# Every manifest mcpp parses carries a `[hooks]` field, a dependency's +# included. Only the root project's is ever invoked, and that is the whole +# reason `mcpp add` of a third-party package is not "run their shell command +# on my next build". A comment cannot hold that; this can. +DEP="$TMP/dep" +DEP_HOST="$(host_path "$DEP")" +mkdir -p "$DEP/src" +cat > "$DEP/src/dep.cppm" <<'EOF' +export module dep; +export int dep_answer() { return 0; } +EOF +cat > "$DEP/mcpp.toml" <<'EOF' +[package] +name = "dep" +version = "0.1.0" + +[targets.dep] +kind = "lib" + +[hooks] +build_start = "echo dependency>>dependency-hook.log" +build_finished = "echo dependency>>dependency-hook.log" +EOF +: > hooks.log +rm -f "$DEP/dependency-hook.log" dependency-hook.log +write_manifest < src/main.cpp <<'EOF' +import dep; +int main() { return dep_answer(); } +EOF +"$MCPP" build > dependency-hooks.log 2>&1 || { + cat dependency-hooks.log; echo "FAIL: build with a hooked dependency failed"; exit 1; } +expect_log hooks.log start finished +[[ ! -e "$DEP/dependency-hook.log" && ! -e dependency-hook.log ]] || { + echo "FAIL: a dependency's hooks ran"; exit 1; } + +# ── A workspace runs each MEMBER's hooks, in that member's root ───────── +# +# Members are separate packages, so their hooks are separate too — and the +# workspace root's own `[hooks]` belongs to a node that builds nothing. +WS="$TMP/ws" +mkdir -p "$WS/alpha/src" "$WS/beta/src" +cat > "$WS/mcpp.toml" <<'EOF' +[workspace] +members = ["alpha", "beta"] + +[hooks] +build_start = "echo root>>root.log" +build_finished = "echo root>>root.log" +EOF +for member in alpha beta; do + cat > "$WS/$member/src/main.cpp" <<'EOF' +int main() { return 0; } +EOF + cat > "$WS/$member/mcpp.toml" < ws-build.log 2>&1 || { + cat ws-build.log; echo "FAIL: workspace build with hooks failed"; exit 1; } +expect_log alpha/hooks.log alpha-start alpha-finished +expect_log beta/hooks.log beta-start beta-finished +[[ ! -e root.log ]] || { + cat root.log; echo "FAIL: the workspace root's own hooks fired"; exit 1; } +[[ ! -e hooks.log ]] || { + cat hooks.log; echo "FAIL: a member hook ran in the workspace root"; exit 1; } + +echo "OK" diff --git a/tests/unit/test_manifest.cpp b/tests/unit/test_manifest.cpp index ffe3418a..dae08e89 100644 --- a/tests/unit/test_manifest.cpp +++ b/tests/unit/test_manifest.cpp @@ -3965,3 +3965,149 @@ cmdline = { version = "0.0.x", features = ["a"] } if (w.find("not a requirement") != std::string::npos) found = true; EXPECT_TRUE(found); } + +// ─── [hooks] — project build lifecycle commands (#496) ─────────────────── +// +// The section is parsed here rather than by the module that runs the commands, +// so this is where its grammar is pinned. The invocation policy (what a +// failure costs, when each event fires) is e2e 314's subject. + +namespace { +constexpr std::string_view kHookPreamble = R"( +[package] +name = "x" +version = "0.1.0" +)"; + +std::expected +parse_with_hooks(std::string_view hooksSection) { + return mcpp::manifest::parse_string( + std::string(kHookPreamble) + std::string(hooksSection)); +} +} // namespace + +TEST(ManifestHooks, AbsentSectionIsInertButNotDisabled) { + auto m = mcpp::manifest::parse_string(kHookPreamble); + ASSERT_TRUE(m.has_value()) << (m ? "" : m.error().format()); + // `enabled` defaults to true and says nothing about whether there is work + // to do; `active()` is the predicate the fast path is allowed to consult. + EXPECT_TRUE(m->hooks.enabled); + EXPECT_FALSE(m->hooks.active()); + EXPECT_EQ(m->hooks.timeoutSeconds, 10); + EXPECT_TRUE(m->hooks.sideEffect); +} + +TEST(ManifestHooks, CommandsAndPolicyAreRead) { + auto m = parse_with_hooks(R"( +[hooks] +build_start = "echo start" +build_failed = "echo failed" +build_finished = "echo finished" +timeout_seconds = 30 +side_effect = false +)"); + ASSERT_TRUE(m.has_value()) << (m ? "" : m.error().format()); + EXPECT_EQ(m->hooks.buildStart, "echo start"); + EXPECT_EQ(m->hooks.buildFailed, "echo failed"); + EXPECT_EQ(m->hooks.buildFinished, "echo finished"); + EXPECT_EQ(m->hooks.timeoutSeconds, 30); + EXPECT_FALSE(m->hooks.sideEffect); + EXPECT_TRUE(m->hooks.active()); +} + +// `enabled = false` has to switch off a table that still names commands — +// otherwise the only way to silence hooks is to delete them. +TEST(ManifestHooks, DisabledTableIsNotActive) { + auto m = parse_with_hooks(R"( +[hooks] +build_start = "echo start" +enabled = false +)"); + ASSERT_TRUE(m.has_value()) << (m ? "" : m.error().format()); + EXPECT_EQ(m->hooks.buildStart, "echo start"); + EXPECT_FALSE(m->hooks.active()); +} + +// Policy without a command is not work: a table that only raises the timeout +// must leave the fast path alone. +TEST(ManifestHooks, PolicyOnlyTableIsNotActive) { + auto m = parse_with_hooks(R"( +[hooks] +timeout_seconds = 60 +)"); + ASSERT_TRUE(m.has_value()) << (m ? "" : m.error().format()); + EXPECT_FALSE(m->hooks.active()); + EXPECT_EQ(m->hooks.timeoutSeconds, 60); +} + +// Both ends of the documented range are the range: the largest accepted value +// is asserted next to the smallest rejected one, so a change to either is a +// change to a test. +TEST(ManifestHooks, TheDocumentedTimeoutCeilingIsAccepted) { + auto m = parse_with_hooks(R"( +[hooks] +build_start = "echo start" +timeout_seconds = 86400 +)"); + ASSERT_TRUE(m.has_value()) << (m ? "" : m.error().format()); + EXPECT_EQ(m->hooks.timeoutSeconds, 86400); +} + +TEST(ManifestHooks, ValueErrorsAreRejectedWithTheKeyNamed) { + struct Case { std::string_view section; std::string_view needle; }; + for (auto const& c : { + Case{R"([hooks] +timeout_seconds = 0)", "timeout_seconds"}, + Case{R"([hooks] +timeout_seconds = "10")", "timeout_seconds"}, + Case{R"([hooks] +build_start = "")", "build_start"}, + Case{R"([hooks] +build_finished = 7)", "build_finished"}, + Case{R"([hooks] +enabled = "yes")", "enabled"}, + Case{R"([hooks] +side_effect = 1)", "side_effect"}, + // The upper bound is documented (1–86400), so it is asserted. An + // undocumented ceiling and no ceiling look the same from outside. + Case{R"([hooks] +timeout_seconds = 86401)", "timeout_seconds"}, + }) { + auto m = parse_with_hooks(c.section); + ASSERT_FALSE(m.has_value()) << c.section; + EXPECT_NE(m.error().message.find(c.needle), std::string::npos) + << c.section << " -> " << m.error().message; + } +} + +// `hooks = "…"` is the shape a user reaches for when they want one command +// and no table. Saying so beats parsing nothing and running nothing. +TEST(ManifestHooks, AScalarHooksKeyIsRejected) { + auto m = mcpp::manifest::parse_string(R"( +hooks = "echo hi" + +[package] +name = "x" +version = "0.1.0" +)"); + ASSERT_FALSE(m.has_value()); + EXPECT_NE(m.error().message.find("[hooks]"), std::string::npos) + << m.error().message; +} + +// An unknown KEY is a warning, not an error — the same split [build] uses. +// A manifest written for a later mcpp (say, a `build_cancelled` event) still +// loads on this one, while a typo is still reported. +TEST(ManifestHooks, UnknownKeyWarnsInsteadOfFailing) { + auto m = parse_with_hooks(R"( +[hooks] +build_start = "echo start" +build_stared = "echo typo" +)"); + ASSERT_TRUE(m.has_value()) << (m ? "" : m.error().format()); + bool found = false; + for (auto const& w : m->schemaWarnings) + if (w.find("build_stared") != std::string::npos) found = true; + EXPECT_TRUE(found); + EXPECT_TRUE(m->hooks.active()); +} diff --git a/tests/unit/test_windows_command_line.cpp b/tests/unit/test_windows_command_line.cpp index 191acf95..a9c3656f 100644 --- a/tests/unit/test_windows_command_line.cpp +++ b/tests/unit/test_windows_command_line.cpp @@ -66,6 +66,51 @@ TEST(WindowsCommandLine, EmbeddedQuotesAreEscaped) { EXPECT_EQ(q, "\"a\\\"b\""); } +// ─── A user-authored shell command (project `[hooks]`, #496) ───────────── +// +// What cmd.exe does with `/c ` when /S is given: strip the first +// character and the last quote character, run the rest. Modelling it here is +// the whole point — the assertion is "the author's command arrives verbatim", +// not "the string looks plausible". +static std::string cmd_c_tail_under_slash_s(std::string_view line) { + constexpr std::string_view kPrefix = "cmd.exe /d /s /c "; + EXPECT_TRUE(line.starts_with(kPrefix)) << line; + std::string tail(line.substr(kPrefix.size())); + if (tail.empty() || tail.front() != '"') return tail; // rule does not fire + tail.erase(0, 1); + tail.erase(tail.rfind('"'), 1); + return tail; +} + +// The switches must arrive BARE. Quoted (`"cmd.exe" "/d" "/s" "/c" "..."`, +// which is what windows_command_from_argv produces for the same argv) they +// are no longer switches, and the command tail keeps a quote pair cmd never +// consumes — the CI failure this shape exists to prevent. +TEST(WindowsCommandLine, ShellCommandKeepsCmdSwitchesBare) { + auto line = proc::windows_shell_command_line("echo hi"); + EXPECT_TRUE(line.starts_with("cmd.exe /d /s /c ")) << line; + EXPECT_EQ(line.find("\"/c\""), std::string::npos) << line; +} + +// A redirect is the ordinary case for a hook that appends to a log, and it is +// also the case argv quoting destroys: `>` inside a quoted argument is a +// literal, not a redirect. +TEST(WindowsCommandLine, ShellCommandDeliversARedirectVerbatim) { + constexpr std::string_view command = "echo start>>hooks.log"; + EXPECT_EQ(cmd_c_tail_under_slash_s(proc::windows_shell_command_line(command)), + command); +} + +// More than one interior quote pair is exactly where the /C rule bites, and +// where the "wrap once" shape earns its keep: whatever the author wrote comes +// back byte for byte. +TEST(WindowsCommandLine, ShellCommandDeliversQuotedPathsVerbatim) { + constexpr std::string_view command = + R"("C:\Program Files\notify\notify.exe" --title "build done")"; + EXPECT_EQ(cmd_c_tail_under_slash_s(proc::windows_shell_command_line(command)), + command); +} + // The POSIX half must keep its own convention — a shared helper that // silently applied Windows quoting on Linux would break every sh command. TEST(WindowsCommandLine, PosixQuotingIsUnaffected) { From dd6a5f6faadea4da1eaac2044f3a90182e26563c Mon Sep 17 00:00:00 2001 From: speak-agent <248744407+speak-agent@users.noreply.github.com> Date: Sun, 30 Aug 2026 18:28:54 +0800 Subject: [PATCH 3/5] feat(hooks): a hook is a command owned for an interval, and `during_build` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- ...-30-project-build-hooks-owned-intervals.md | 193 ++++++++++++++++ docs/05-mcpp-toml.md | 91 ++++++-- docs/zh/05-mcpp-toml.md | 74 ++++-- modules/manifest/src/toml.cppm | 109 +++++++-- modules/manifest/src/types.cppm | 50 +++- modules/platform/src/process.cppm | 108 +++++++++ .../platform/src/unix/bounded_process.cppm | 188 +++++++++++++++ .../platform/src/windows/bounded_process.cppm | 190 +++++++++++++++ src/cli/cmd_build.cppm | 13 +- src/hooks.cppm | 216 ++++++++++++++++-- tests/e2e/317_project_build_hooks.sh | 166 +++++++++++++- tests/unit/test_manifest.cpp | 98 +++++++- 12 files changed, 1409 insertions(+), 87 deletions(-) create mode 100644 .agents/docs/2026-08-30-project-build-hooks-owned-intervals.md diff --git a/.agents/docs/2026-08-30-project-build-hooks-owned-intervals.md b/.agents/docs/2026-08-30-project-build-hooks-owned-intervals.md new file mode 100644 index 00000000..dee1aba8 --- /dev/null +++ b/.agents/docs/2026-08-30-project-build-hooks-owned-intervals.md @@ -0,0 +1,193 @@ +# Project build hooks as owned intervals (#496) + +## The question this design answers + +The first shape of `[hooks]` gave `mcpp build` three commands — `build_start`, +`build_finished`, `build_failed` — each run to completion with a timeout, each +judged by its exit code. + +The request that followed was a background command: music that plays *for the +duration of the build* and stops when it ends, optionally restarting when the +player exits. Written as a knob it looks like + +```toml +build_start = { cmd = "play bgm.mp3", loop = true } +``` + +but `loop` is not the new thing. The new thing is that this command's life is +**longer than the moment that started it**. Adding `background = true` to +`build_start` would give that key two incompatible meanings and would silently +change what `timeout_seconds` and `side_effect` mean for one of them — a table +whose keys mean different things depending on a sibling key is the shape this +design exists to avoid. + +## The model + +> A hook is a command mcpp **owns for an interval**. The event names the +> interval. mcpp starts the command when the interval opens and ends it when +> the interval closes. + +| Event | The interval opens | The interval closes | +|---|---|---| +| `build_start` | after preparation, before the build | when the command exits | +| `build_finished` | after a build that succeeded | when the command exits | +| `build_failed` | after a build that failed | when the command exits | +| `during_build` | after preparation, before the build | after the build, before the terminal hook | + +The first three have **self-closing** intervals. "Synchronous" is not a +separate mode in this model — it is what an interval closed by the command +itself looks like. `during_build` is the one interval closed by something else, +and everything that reads as a special case for it falls out of that single +difference rather than being declared: + +- **`timeout_seconds` bounds one run of the command.** For a self-closing + interval that is the whole hook. For `during_build` the build already bounds + it, so the key is *rejected* there rather than accepted and reinterpreted — + a per-run cap would only be enforceable when `loop` is on (nothing is + polling otherwise), and a key that works under one sibling setting and not + another is the hole this design is trying not to dig. +- **`loop` restarts a command that exits before its interval closes.** A + self-closing interval ends *when the command exits*, so `loop` can never fire + there. It is rejected on those events rather than accepted and ignored, with + a message naming `during_build`. +- **`side_effect` is unchanged**: does a hook failure fail the build. For + `during_build`, "failure" means *failed to start*, or *failed to stay up* + (below). Being stopped because the interval closed is not a failure. + +Ordering inside the lifecycle: + +```text +during_build opens +build_start + ├─ build succeeds → during_build closes → build_finished + └─ build fails → during_build closes → build_failed +``` + +`during_build` closes **before** the terminal hook, not after. A "build +finished" sound playing over the background music it was supposed to replace is +the whole reason the order is fixed rather than incidental. + +## Schema + +Every event value is a string or a table; the string is sugar. + +```toml +[hooks] +build_start = "echo start" # = { cmd = "echo start" } +build_finished = "notify-send 'build finished'" +during_build = { cmd = "mcpp-hooks-audioplayer bgm", loop = true } + +# Table-level, unchanged. +timeout_seconds = 10 +enabled = true +side_effect = true +``` + +Per-event table keys: `cmd` (required, non-empty), `timeout_seconds` +(overrides the table default for this event), `loop` (`during_build` only). + +The string-or-table pattern is already how `[dependencies]` and +`[resources].version-info` are spelled, so it introduces no new parsing +semantics — Appendix A of docs/05 is satisfied for the same reason the first +shape satisfied it: fixed keys, open values, and no key that duplicates an +answer another section already gives. + +## What the feature actually costs + +The schema is the small half. A process whose life spans the build needs three +things mcpp does not have. + +### 1. A process group, because SIGKILL on a pid is not enough + +`modules/platform/src/unix/bounded_process.cppm:216` kills the direct child: + +```cpp +::kill(pid, SIGKILL); +``` + +For `sh -c "sleep 5"` that is sufficient — the shell execs the command. For +`sh -c "while :; do play a.mp3; done"` it is not: the shell dies and `play` +survives. **Music that cannot be stopped, from a process the user cannot name, +is the worst failure this feature can have**, and it is the default outcome +without process groups. + +So the POSIX side gains `posix_spawnattr_setpgroup` + `killpg`. Windows already +has the right shape: a Job object with `JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE` +(`windows/bounded_process.cppm:205`), which takes the whole tree — and takes it +even if mcpp dies, because the handle closes with the process. + +This is worth doing on its own terms: the same gap is why +`mcpp test --timeout` and `[build] build_program_timeout` can leave +grandchildren behind today. + +### 2. A signal handler, because a process group stops receiving Ctrl-C + +The two requirements fight each other. A child in its own process group no +longer receives the terminal's SIGINT — which is what makes `killpg` possible +and *also* what makes Ctrl-C leave the music playing. Both halves are needed: +its own group, plus a SIGINT/SIGTERM handler in mcpp that stops the group +before re-raising. + +mcpp installs no signal handlers today. The one added here is the minimum that +is async-signal-safe: a `volatile sig_atomic_t` holding the process-group id, a +handler that calls `killpg` (which is async-signal-safe) and re-raises the +default action. It is installed only while a spanning hook is running. + +Windows needs no equivalent for correctness — the job object already covers +process death — but `SetConsoleCtrlHandler` is installed for a clean stop. + +### 3. A restart floor, because `loop` on a typo is a fork bomb + +`loop = true` with `play /nonexistant` restarts thousands of times per second +for the length of the build. Two bounds, both fixed in v1 rather than +configurable, because a knob whose wrong value is a spin is not a knob: + +- **250 ms between runs.** +- **Five consecutive runs that exited non-zero in under a second** stops the + loop and reports a hook failure: *"during_build command failed to stay up".* + Whether that fails the build is `side_effect`, as everywhere else. + +A supervisor thread exists **only when `loop = true`**. Without it, a spanning +hook is a spawn and a stop, and no thread is created. + +## Stopping + +POSIX: `SIGTERM` to the group, a 2 s grace period, then `SIGKILL`. A player +asked to stop should get to close its audio device. + +Windows: the job object is closed, which terminates the tree at once. There is +no graceful equivalent that does not require enumerating the job's processes +and posting window messages; the asymmetry is stated rather than hidden. + +## Output + +A self-closing hook inherits stdio, which is safe because nothing else is +writing at that moment. A spanning hook writes **concurrently with ninja** and +would interleave into the middle of a compiler diagnostic. Its output is +therefore discarded by default, and inherited under `--verbose` — no new schema +key, and the answer to "why is there no music" is one flag away. + +## Criteria + +The assertions this design has to earn are about *state*, not about log lines — +"we called stop" is not evidence that anything stopped. + +1. **It runs during the build.** The command appends a heartbeat line every + 200 ms; the file is non-empty when the build ends. +2. **It is stopped.** Record the heartbeat file's size after `mcpp build` + returns, wait one second, read it again: unchanged. A log line saying + "stopped" would pass whether or not the process died. +3. **`loop` restarts it.** A command that exits immediately produces a heartbeat + count that grows across the build; without `loop` it produces exactly one. +4. **The whole tree dies, not just the shell.** The command is + `sh -c '... & wait'`, so the writer is a grandchild. This is the case + `kill(pid)` misses and `killpg` catches, and it is the only assertion that + distinguishes the fix from the bug. +5. **The failure cap trips.** A command that fails instantly stops after five + attempts and reports; the heartbeat count is bounded, not "large". +6. **Ctrl-C leaves nothing behind.** POSIX only: `mcpp build` in the + background, `kill -INT`, then criterion 2. Sending Ctrl-C to another process + on Windows needs a helper this suite does not have; the gap is declared + rather than papered over with a test that passes vacuously. +7. **A self-closing event rejects `loop`.** Otherwise the key is accepted, + does nothing, and the user concludes the feature does not work. diff --git a/docs/05-mcpp-toml.md b/docs/05-mcpp-toml.md index 3d6fbc2f..66a6341b 100644 --- a/docs/05-mcpp-toml.md +++ b/docs/05-mcpp-toml.md @@ -2096,8 +2096,8 @@ do`. ### 2.16 `[hooks]` — Project Build Lifecycle Commands -`mcpp build` can run one host-shell command before the build and one command -for its terminal result: +A hook is a command `mcpp build` **owns for an interval**, and the event names +the interval: ```toml [hooks] @@ -2105,35 +2105,62 @@ build_start = "echo build started" build_failed = "notify-send 'build failed'" build_finished = "notify-send 'build finished'" +# Runs alongside the build and is stopped when it ends. +during_build = { cmd = "mcpp-hooks-audioplayer bgm", loop = true } + # Optional; these are the defaults. timeout_seconds = 10 enabled = true side_effect = true ``` -| Key | Type | Default | Meaning | +| Key | Type | Default | The interval it names | |---|---|---:|---| -| `build_start` | string | — | Runs after project preparation, immediately before the build | -| `build_failed` | string | — | Runs when the build exits unsuccessfully | -| `build_finished` | string | — | Runs when the build exits successfully | -| `timeout_seconds` | integer, 1–86400 | `10` | Maximum time for each command | +| `build_start` | command | — | Opens after project preparation, closes when the command exits | +| `build_finished` | command | — | Opens after a build that succeeded, closes when the command exits | +| `build_failed` | command | — | Opens after a build that failed, closes when the command exits | +| `during_build` | command | — | Opens before the build, closes after it | +| `timeout_seconds` | integer, 1–86400 | `10` | Bounds one run of a command | | `enabled` | bool | `true` | Enables all commands in this table | | `side_effect` | bool | `true` | Whether a hook failure makes the overall build fail | -Commands run synchronously through the host shell (`/bin/sh` or `cmd.exe`), -with the **project root** as their working directory — not the directory -`mcpp build` was typed in, so a relative path in a hook means the same thing -wherever the build was started. Standard input/output/error keep their ordinary -terminal behaviour. Missing event commands are skipped. +The first three intervals are **self-closing** — they end when the command +does. "Synchronous" is not a separate mode here; it is what a self-closing +interval looks like. `during_build` is the one interval closed by something +else, and the two keys that only make sense for one shape follow from that +rather than being exceptions. + +A command is a string, or a table when it needs options: + +| Table key | Applies to | Meaning | +|---|---|---| +| `cmd` | every event | The command. Required. | +| `timeout_seconds` | self-closing events | Overrides the table default for this event | +| `loop` | `during_build` | Restart the command if it exits before the build ends | + +`loop` on a self-closing event and `timeout_seconds` on `during_build` are both +**errors**, not ignored keys: a self-closing interval ends when its command +exits, so there is nothing to restart, and `during_build` is already bounded by +the build. A key that is accepted and does nothing reads as a broken feature. + +Commands run through the host shell (`/bin/sh` or `cmd.exe`), with the +**project root** as their working directory — not the directory `mcpp build` +was typed in, so a relative path in a hook means the same thing wherever the +build was started. A self-closing command keeps ordinary terminal +input/output. Missing event commands are skipped. The lifecycle is: ```text +during_build opens build_start - ├─ build succeeds → build_finished - └─ build fails → build_failed + ├─ build succeeds → during_build closes → build_finished + └─ build fails → during_build closes → build_failed ``` +`during_build` closes **before** the terminal hook, so a "build finished" sound +is not competing with the background music it replaces. + `build_failed` and `build_finished` are mutually exclusive, and both are reachable only after `build_start` has run. A project that cannot be *prepared* — an invalid manifest, an unresolvable dependency, no usable toolchain — fires @@ -2141,10 +2168,26 @@ nothing: it has not started building, and its hook program may be exactly what preparation would have installed. A hook command that cannot start, returns non-zero, or exceeds its timeout is a -hook failure. With `side_effect = false`, mcpp reports a warning and preserves -the build's result; with `true`, it returns failure — but a build that failed on -its own keeps its own exit code, so `mcpp build` never reports a compile error -as a notifier problem. A hook's own failure does not trigger another hook. +hook failure. For `during_build` there is one more: a looped command that +**fails to stay up** — five consecutive runs ending unsuccessfully within a +second — stops being restarted and is reported. (A command that finishes +quickly and *successfully* is doing exactly what `loop` was asked to repeat, +and is not a failure.) With `side_effect = false`, mcpp reports a warning and +preserves the build's result; with `true`, it returns failure — but a build that +failed on its own keeps its own exit code, so `mcpp build` never reports a +compile error as a notifier problem. A hook's own failure does not trigger +another hook. + +Two things are worth knowing about a `during_build` command specifically: + +- **Its output is discarded**, because it writes concurrently with the build + and would otherwise land in the middle of a compiler diagnostic. Run + `mcpp build --verbose` to see it. +- **It is stopped as a process tree**, not as a process. `player & wait` makes + the player a grandchild of the command mcpp started, and stopping only the + latter would leave the audio device held after the build. mcpp puts the + command in its own process group (a job object on Windows) and stops that, + including when the build is interrupted with Ctrl-C. Scope, precisely: @@ -2162,9 +2205,10 @@ Scope, precisely: already-current hooked project to cost a preparation pass rather than milliseconds. -An unrecognised key in `[hooks]` is a warning (an error under `--strict`), so a -manifest written for a newer mcpp still loads. An unrecognised *value* — a -non-string command, a `timeout_seconds` outside 1–86400 — is a manifest error. +An unrecognised key in `[hooks]`, or inside one event's table, is a warning (an +error under `--strict`), so a manifest written for a newer mcpp still loads. An +unrecognised *value* — a missing or non-string `cmd`, a `timeout_seconds` +outside 1–86400, a key offered to the wrong interval — is a manifest error. > **A hook is code, and `mcpp.toml` is part of the repository.** Building a > freshly cloned project runs whatever its `[hooks]` say, with the privileges @@ -2178,6 +2222,7 @@ than adding media handling to mcpp: ```toml [hooks] +during_build = { cmd = "mcpp-hooks-audioplayer bgm", loop = true } build_finished = "mcpp-hooks-audioplayer niulai-mm" build_failed = "mcpp-hooks-audioplayer niulai-niulai" side_effect = false @@ -2186,6 +2231,10 @@ side_effect = false deps = ["xim:mcpp-hooks-audioplayer@0.0.1"] ``` +Background music for the length of the build, a different sound for how it +ended, and `side_effect = false` so that a missing audio device is a warning +rather than a failed build. + ## Appendix A. Schema Ownership Principle (admission criteria for new fields) > **Closed syntax, open vocabulary**: whoever owns the parsing semantics defines the keys; whoever owns the domain knowledge defines the values. diff --git a/docs/zh/05-mcpp-toml.md b/docs/zh/05-mcpp-toml.md index 41b6e3be..debbe968 100644 --- a/docs/zh/05-mcpp-toml.md +++ b/docs/zh/05-mcpp-toml.md @@ -1793,7 +1793,7 @@ o.arg("./mkblob.sh").arg("blob.bin").arg("${mcpp.out_dir}/blob.o") ### 2.16 `[hooks]` —— 项目构建生命周期命令 -`mcpp build` 可以在正式构建前执行一条宿主 Shell 命令,并根据最终结果再执行一条: +Hook 是 `mcpp build` **在一段区间内持有**的命令,事件名就是那段区间: ```toml [hooks] @@ -1801,41 +1801,75 @@ build_start = "echo build started" build_failed = "notify-send 'build failed'" build_finished = "notify-send 'build finished'" +# 与构建并行,构建结束时被停止。 +during_build = { cmd = "mcpp-hooks-audioplayer bgm", loop = true } + # 可选;以下是默认值。 timeout_seconds = 10 enabled = true side_effect = true ``` -| 键 | 类型 | 默认值 | 含义 | +| 键 | 类型 | 默认值 | 它命名的区间 | |---|---|---:|---| -| `build_start` | 字符串 | — | 项目准备完成、正式构建开始前执行 | -| `build_failed` | 字符串 | — | 构建以失败状态结束时执行 | -| `build_finished` | 字符串 | — | 构建成功结束时执行 | -| `timeout_seconds` | 整数,1–86400 | `10` | 每条命令的最长执行时间 | +| `build_start` | 命令 | — | 项目准备完成后开启,命令退出时闭合 | +| `build_finished` | 命令 | — | 构建成功后开启,命令退出时闭合 | +| `build_failed` | 命令 | — | 构建失败后开启,命令退出时闭合 | +| `during_build` | 命令 | — | 构建开始前开启,构建结束后闭合 | +| `timeout_seconds` | 整数,1–86400 | `10` | 单次运行的时限 | | `enabled` | 布尔 | `true` | 是否启用本表中的全部命令 | | `side_effect` | 布尔 | `true` | Hook 失败是否让本次构建失败 | -命令通过宿主 Shell(`/bin/sh` 或 `cmd.exe`)同步执行,工作目录是**项目根目录**—— -不是敲 `mcpp build` 的那个目录,所以 Hook 里的相对路径在哪儿发起构建都指同一处。 -标准输入、输出和错误沿用普通终端行为。没有配置的事件直接跳过。 +前三个区间是**自闭合**的——命令退出,区间就结束。"同步"在这里不是一种单独的模式, +它就是自闭合区间的样子。`during_build` 是唯一由别的东西闭合的区间,而那两个只对其中 +一种形状有意义的键,是从这一点推出来的,不是额外规定的例外。 + +命令写成字符串;需要选项时写成表: + +| 表内键 | 适用于 | 含义 | +|---|---|---| +| `cmd` | 所有事件 | 命令本身,必填 | +| `timeout_seconds` | 自闭合事件 | 覆盖本表默认值 | +| `loop` | `during_build` | 命令在区间闭合前退出时重新启动 | + +`loop` 写在自闭合事件上、`timeout_seconds` 写在 `during_build` 上,都是**错误**而不是 +被忽略的键:自闭合区间随命令退出而结束,没有东西可重启;而 `during_build` 已经由构建 +定界。一个被接受却什么都不做的键,读起来就是"这功能坏了"。 + +命令通过宿主 Shell(`/bin/sh` 或 `cmd.exe`)执行,工作目录是**项目根目录**——不是敲 +`mcpp build` 的那个目录,所以 Hook 里的相对路径在哪儿发起构建都指同一处。自闭合命令 +的标准输入、输出和错误沿用普通终端行为。没有配置的事件直接跳过。 生命周期为: ```text +during_build 开启 build_start - ├─ 构建成功 → build_finished - └─ 构建失败 → build_failed + ├─ 构建成功 → during_build 闭合 → build_finished + └─ 构建失败 → during_build 闭合 → build_failed ``` +`during_build` 在终止 Hook **之前**闭合,这样"构建完成"的提示音不会和它要替换掉的 +背景音乐撞在一起。 + `build_failed` 与 `build_finished` 互斥,而且两者都只在 `build_start` 已经执行之后 才可达。项目**准备**阶段就失败的情况——manifest 非法、依赖无法解析、没有可用工具链 ——一个 Hook 都不触发:此时构建尚未开始,而 Hook 程序本身可能正是准备阶段要装的东西。 -命令无法启动、返回非零或超过时限均视为 Hook 失败。`side_effect = false` 时 mcpp 报 -warning 并保留原构建结果;设为 `true` 时返回失败——但构建自身失败时保留它自己的退出码, -所以 `mcpp build` 不会把一次编译错误报成通知程序的问题。Hook 自身失败不会再触发另一个 -Hook。 +命令无法启动、返回非零或超过时限均视为 Hook 失败。`during_build` 还多一种:开了 `loop` +的命令**起不来**——连续五次在一秒内以非零状态结束——就不再重启,并被报出来。(很快就 +成功结束的命令,正是 `loop` 被要求重复的那件事,不算失败。)`side_effect = false` 时 +mcpp 报 warning 并保留原构建结果;设为 `true` 时返回失败——但构建自身失败时保留它自己 +的退出码,所以 `mcpp build` 不会把一次编译错误报成通知程序的问题。Hook 自身失败不会再 +触发另一个 Hook。 + +关于 `during_build` 有两件事值得单独知道: + +- **它的输出被丢弃**,因为它与构建并发写出,否则会插进某条编译诊断的中间。要看它的 + 输出就跑 `mcpp build --verbose`。 +- **停止的单位是进程树,不是进程。** `player & wait` 让播放器成为 mcpp 所启动那条命令 + 的孙子进程,只停掉后者会让音频设备在构建结束后仍被占着。mcpp 把命令放进它自己的 + 进程组(Windows 上是 job object)并停止整组,构建被 Ctrl-C 打断时也一样。 作用范围: @@ -1849,9 +1883,9 @@ Hook。 - 声明了生效的 Hook 就等于让项目放弃空转快路径,因为 `build_start` 规定在准备阶段之后 执行。对已经是最新状态的带 Hook 项目,`mcpp build` 的代价是一次准备,而不是毫秒级。 -`[hooks]` 里不认识的**键**是 warning(`--strict` 下为错误),所以为更新版 mcpp 写的 -manifest 在这一版仍能加载;不认识的**值**——命令不是字符串、`timeout_seconds` 不在 -1–86400 之间——是 manifest 错误。 +`[hooks]` 里、以及某个事件表里不认识的**键**都是 warning(`--strict` 下为错误),所以为更新版 mcpp 写的 +manifest 在这一版仍能加载;不认识的**值**——`cmd` 缺失或不是字符串、`timeout_seconds` 不在 +1–86400 之间、键写给了错误的区间——是 manifest 错误。 > **Hook 是代码,而 `mcpp.toml` 是仓库的一部分。** 构建一个刚克隆下来的项目,会以 > 执行 `mcpp build` 的那个账户的权限,运行它 `[hooks]` 里写的任何东西。这与 @@ -1863,6 +1897,7 @@ Hook 程序可以作为普通 xlings 依赖安装。例如,音频通知程序可 ```toml [hooks] +during_build = { cmd = "mcpp-hooks-audioplayer bgm", loop = true } build_finished = "mcpp-hooks-audioplayer niulai-mm" build_failed = "mcpp-hooks-audioplayer niulai-niulai" side_effect = false @@ -1871,6 +1906,9 @@ side_effect = false deps = ["xim:mcpp-hooks-audioplayer@0.0.1"] ``` +构建全程的背景音乐,加上一段区分结果的提示音;`side_effect = false` 让缺少音频设备 +只是一条 warning,而不是一次失败的构建。 + ## 附录 A. Schema 所有权原则(新字段准入标准) > **语法封闭,词汇开放**:谁拥有解析语义谁定义键;谁拥有领域知识谁定义值。 diff --git a/modules/manifest/src/toml.cppm b/modules/manifest/src/toml.cppm index 66a22161..d692e4bb 100644 --- a/modules/manifest/src/toml.cppm +++ b/modules/manifest/src/toml.cppm @@ -1533,38 +1533,109 @@ std::expected parse_string(std::string_view content, "[hooks] must be a table of lifecycle commands")); } if (auto* ht = doc->get_table("hooks")) { + // Bounded above as well as below: the value becomes a + // std::chrono::seconds deadline, and "a timeout so large it is not + // one" is a mistake worth naming rather than honouring. One constant, + // used by both the table-level default and the per-event override, so + // the two cannot disagree about what they accept. + constexpr std::int64_t kMaxHookTimeout = 24 * 60 * 60; + // Values are the author's own and visible in front of them: a wrong // type is an error, not a silent default. An unrecognised KEY is a // warning (--strict makes it an error), same split as [build] — so a // manifest written for a later mcpp still loads on this one. - auto read_command = [&](std::string_view key, std::string& out) - -> std::optional { + // + // A command is a string or a table. `spanning` says which INTERVAL the + // event names, and that decides which table keys exist: a self-closing + // interval can be bounded (`timeout_seconds`) but can never be + // restarted (`loop`), and a spanning one is the reverse. A key offered + // to the wrong event is an error naming the right one — accepted and + // ignored, it would read as "the feature does not work". + auto read_command = [&](std::string_view key, HookCommand& out, + bool spanning) -> std::optional { auto it = ht->find(key); if (it == ht->end()) return std::nullopt; - if (!it->second.is_string() || it->second.as_string().empty()) + + auto const& value = it->second; + if (value.is_string()) { + if (value.as_string().empty()) + return error(origin, std::format( + "[hooks].{} must be a non-empty command string", key)); + out.cmd = value.as_string(); + return std::nullopt; + } + if (!value.is_table()) + return error(origin, std::format( + "[hooks].{} must be a command string or a table with `cmd`", + key)); + + auto const& t = value.as_table(); + auto ci = t.find("cmd"); + if (ci == t.end() || !ci->second.is_string() + || ci->second.as_string().empty()) return error(origin, std::format( - "[hooks].{} must be a non-empty command string", key)); - out = it->second.as_string(); + "[hooks].{}.cmd must be a non-empty command string", key)); + out.cmd = ci->second.as_string(); + + if (auto ti = t.find("timeout_seconds"); ti != t.end()) { + if (spanning) + return error(origin, std::format( + "[hooks].{}.timeout_seconds does not apply: this " + "command runs for as long as the build, which bounds " + "it", key)); + if (!ti->second.is_int() || ti->second.as_int() <= 0 + || ti->second.as_int() > kMaxHookTimeout) + return error(origin, std::format( + "[hooks].{}.timeout_seconds must be a positive integer " + "(seconds, at most {})", key, kMaxHookTimeout)); + out.timeoutSeconds = static_cast(ti->second.as_int()); + } + if (auto li = t.find("loop"); li != t.end()) { + if (!spanning) + return error(origin, std::format( + "[hooks].{}.loop does not apply: this command's " + "interval ends when it exits, so there is nothing to " + "restart. `during_build` is the event that spans the " + "build", key)); + if (!li->second.is_bool()) + return error(origin, + std::format("[hooks].{}.loop must be a boolean", key)); + out.loop = li->second.as_bool(); + } + + static constexpr std::string_view kSelfClosingKeys[] = { + "cmd", "timeout_seconds" }; + static constexpr std::string_view kSpanningKeys[] = { "cmd", "loop" }; + for (auto& [k, _] : t) { + bool known = false; + if (spanning) { + for (auto kk : kSpanningKeys) if (k == kk) known = true; + } else { + for (auto kk : kSelfClosingKeys) if (k == kk) known = true; + } + if (!known) + m.schemaWarnings.push_back(std::format( + "[hooks].{} has unsupported key '{}' (ignored). Keys: {}.", + key, k, spanning ? "cmd, loop" : "cmd, timeout_seconds")); + } return std::nullopt; }; - for (auto [key, out] : std::initializer_list< - std::pair>{ - {"build_start", &m.hooks.buildStart}, - {"build_failed", &m.hooks.buildFailed}, - {"build_finished", &m.hooks.buildFinished}}) { - if (auto e = read_command(key, *out)) return std::unexpected(*e); + for (auto [key, out, spanning] : std::initializer_list< + std::tuple>{ + {"build_start", &m.hooks.buildStart, false}, + {"build_failed", &m.hooks.buildFailed, false}, + {"build_finished", &m.hooks.buildFinished, false}, + {"during_build", &m.hooks.duringBuild, true}}) { + if (auto e = read_command(key, *out, spanning)) + return std::unexpected(*e); } if (auto it = ht->find("timeout_seconds"); it != ht->end()) { - // Bounded above as well as below: the value becomes a - // std::chrono::seconds deadline, and "a timeout so large it is not - // one" is a mistake worth naming rather than honouring. - constexpr std::int64_t kMaxTimeout = 24 * 60 * 60; if (!it->second.is_int() || it->second.as_int() <= 0 - || it->second.as_int() > kMaxTimeout) + || it->second.as_int() > kMaxHookTimeout) return std::unexpected(error(origin, std::format( "[hooks].timeout_seconds must be a positive integer " - "(seconds, at most {})", kMaxTimeout))); + "(seconds, at most {})", kMaxHookTimeout))); m.hooks.timeoutSeconds = static_cast(it->second.as_int()); } @@ -1581,7 +1652,7 @@ std::expected parse_string(std::string_view content, } static constexpr std::string_view kKnownHookKeys[] = { - "build_start", "build_failed", "build_finished", + "build_start", "build_failed", "build_finished", "during_build", "timeout_seconds", "enabled", "side_effect", }; for (auto& [k, _] : *ht) { @@ -1590,7 +1661,7 @@ std::expected parse_string(std::string_view content, if (!known) m.schemaWarnings.push_back(std::format( "[hooks] has unsupported key '{}' (ignored). Keys: " - "build_start, build_failed, build_finished, " + "build_start, build_failed, build_finished, during_build, " "timeout_seconds, enabled, side_effect.", k)); } } diff --git a/modules/manifest/src/types.cppm b/modules/manifest/src/types.cppm index ace3921f..e53890cf 100644 --- a/modules/manifest/src/types.cppm +++ b/modules/manifest/src/types.cppm @@ -931,11 +931,45 @@ struct WorkspaceConfig { // `mcpp add` of a third-party package does not become "run their shell // command on my next build". Anything that adds a second call site inherits // that responsibility. +// One event's command. Spelled either as a bare string or as a table — the +// same string-or-table shape `[dependencies]` and `[resources].version-info` +// already use, so it adds no parsing semantics. +struct HookCommand { + std::string cmd; + // Per-event override of the table's `timeout_seconds`. 0 = inherit. Only + // meaningful for a self-closing interval; see below. + int timeoutSeconds = 0; + // Restart the command if it exits before its interval closes. Only + // `during_build` has an interval that can outlast a run, so this is + // REJECTED on the other events rather than accepted and ignored. + bool loop = false; + + bool empty() const { return cmd.empty(); } +}; + +// 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" is not a separate mode — +// it is what an interval closed by the command itself looks like. Everything +// that reads as a special case for `during_build` falls out of that one +// difference instead of being declared: `timeout_seconds` bounds one run and +// so does not apply where the build already bounds it; `loop` restarts a +// command that ended before its interval did, which a self-closing interval +// makes impossible. +// +// See .agents/docs/2026-08-30-project-build-hooks-owned-intervals.md. struct Hooks { - std::string buildStart; - std::string buildFailed; - std::string buildFinished; - int timeoutSeconds = 10; // per command + HookCommand buildStart; + HookCommand buildFailed; + HookCommand buildFinished; + HookCommand duringBuild; + + int timeoutSeconds = 10; // default for one run of a hook command bool enabled = true; // whole table // Whether a hook failure fails the build. False downgrades it to a // warning and preserves whatever the build itself returned. @@ -946,7 +980,13 @@ struct Hooks { // the build path it would otherwise divert (the fast path) untouched. bool active() const { return enabled && !(buildStart.empty() && buildFailed.empty() - && buildFinished.empty()); + && buildFinished.empty() && duringBuild.empty()); + } + + // The bound on one run of `c`. The per-event value wins; 0 means it was + // not given, which is what makes "inherit" expressible at all. + int timeout_for(const HookCommand& c) const { + return c.timeoutSeconds > 0 ? c.timeoutSeconds : timeoutSeconds; } }; diff --git a/modules/platform/src/process.cppm b/modules/platform/src/process.cppm index 78fccfdc..657e49fa 100644 --- a/modules/platform/src/process.cppm +++ b/modules/platform/src/process.cppm @@ -131,6 +131,47 @@ int run_shell_deadline(std::string_view command, std::chrono::milliseconds deadline, bool* timed_out); +// ─── A shell command mcpp owns for longer than one call (#496) ─────────── +// +// `run_shell_deadline` owns its child for the length of the call. A project +// `[hooks] during_build` command is owned for the length of the BUILD, so it +// is started here, polled while the build runs, and stopped afterwards. +// +// The handle is opaque and carries whichever platform's identity is real: a +// process GROUP on POSIX, a job object on Windows. Neither is a pid, and that +// is deliberate — the command is user-authored shell, so the thing that must +// die is a tree, not a process. +struct BackgroundCommand { + bool ok = false; + long long group = 0; // POSIX: process-group id + unsigned long long job = 0; // Windows: job object + unsigned long long process = 0; // Windows: the child +}; + +// `inheritStdio == false` discards the child's output. That is the right +// default for anything running alongside the build: its writes would otherwise +// interleave into the middle of a compiler diagnostic. +BackgroundCommand start_shell_background(std::string_view command, + std::string_view cwd, + bool inheritStdio); + +// True while it is still up. When it has exited and `exitCode` is given, the +// code is written there — "the player finished the track" and "the command +// does not exist" are the same event to a poller that only answers yes/no, and +// the `loop` supervisor has to tell them apart. +bool background_running(const BackgroundCommand& child, int* exitCode = nullptr); + +// Asks, waits `grace`, then takes the tree. +void stop_background(const BackgroundCommand& child, + std::chrono::milliseconds grace); + +// Ctrl-C. Registering the child means an interrupted build does not leave it +// running — which, for the case this exists for, is a background player the +// user can no longer name. Only one command is guarded at a time; a build owns +// at most one. +void guard_background_on_signal(const BackgroundCommand& child); +void clear_background_guard(); + RunResult capture_exec_deadline( const std::vector& argv, const std::vector>& extraEnv, @@ -729,6 +770,73 @@ int run_shell_deadline(std::string_view command, return r.exit_code; } +// The same split as dispatch_bounded, for the same reason: POSIX names a +// program with an argv array, Windows with a single command line. Both +// spellings are built here so neither can drift into a launcher that does not +// use it. +BackgroundCommand start_shell_background(std::string_view command, + std::string_view cwd, + bool inheritStdio) +{ + BackgroundCommand out; + if (command.empty()) return out; + const std::string cwdStore(cwd); + const char* cwdArg = cwdStore.empty() ? nullptr : cwdStore.c_str(); + + if constexpr (mcpp::platform::is_windows) { + const auto line = windows_shell_command_line(command); + auto r = mcpp::platform::winproc::spawn_background( + line.c_str(), cwdArg, inheritStdio ? 1 : 0); + out.ok = r.ok; + out.job = r.job; + out.process = r.process; + } else { + const std::string cmdStore(command); + const char* argv[] = {"/bin/sh", "-c", cmdStore.c_str()}; + auto r = mcpp::platform::unixproc::spawn_background( + argv, 3, cwdArg, inheritStdio ? 1 : 0); + out.ok = r.ok; + out.group = r.group; + } + return out; +} + +bool background_running(const BackgroundCommand& child, int* exitCode) { + if (!child.ok) return false; + if constexpr (mcpp::platform::is_windows) + return mcpp::platform::winproc::background_running(child.process, + exitCode) == 1; + else + return mcpp::platform::unixproc::background_running(child.group, + exitCode) == 1; +} + +void stop_background(const BackgroundCommand& child, + std::chrono::milliseconds grace) +{ + if (!child.ok) return; + if constexpr (mcpp::platform::is_windows) + mcpp::platform::winproc::background_stop(child.job, child.process, + grace.count()); + else + mcpp::platform::unixproc::background_stop(child.group, grace.count()); +} + +void guard_background_on_signal(const BackgroundCommand& child) { + if (!child.ok) return; + if constexpr (mcpp::platform::is_windows) + mcpp::platform::winproc::guard_job_on_signal(child.job); + else + mcpp::platform::unixproc::guard_group_on_signal(child.group); +} + +void clear_background_guard() { + if constexpr (mcpp::platform::is_windows) + mcpp::platform::winproc::clear_job_guard(); + else + mcpp::platform::unixproc::clear_group_guard(); +} + RunResult capture_exec_deadline( const std::vector& argv, const std::vector>& extraEnv, diff --git a/modules/platform/src/unix/bounded_process.cppm b/modules/platform/src/unix/bounded_process.cppm index 75b49091..5aa0e50c 100644 --- a/modules/platform/src/unix/bounded_process.cppm +++ b/modules/platform/src/unix/bounded_process.cppm @@ -87,6 +87,56 @@ DeadlineRun capture_with_deadline(const char* const* argvEntries, OutputSink sink, void* ctx); +// ─── A child that outlives the call that started it (#496) ─────────────── +// +// `capture_with_deadline` owns its child for the length of one call. A project +// `[hooks] during_build` command is owned for the length of the BUILD, so the +// caller needs a handle it can poll and stop later. Every member is a builtin, +// same constraint as DeadlineRun. +// +// ⚠️ `group`, NOT a pid. The child is placed in a process group of its own +// (posix_spawnattr_setpgroup) and stopped with killpg, because the thing being +// started is a user-authored SHELL command: `sh -c 'player & wait'` makes the +// writer a grandchild, and `kill(pid)` reaches only the shell. A background +// player that survives its build, from a process the user cannot name, is the +// worst outcome this API has — so the group is the unit throughout. +struct BackgroundChild { + bool ok = false; + long long group = 0; // the child's process-group id (== its pid) +}; + +// `inheritStdio == 0` sends the child's output to /dev/null. A spanning hook +// writes CONCURRENTLY with ninja and would otherwise interleave into the middle +// of a compiler diagnostic. +BackgroundChild spawn_background(const char* const* argvEntries, + unsigned long argvCount, + const char* cwd, + int inheritStdio); + +// 1 = still running, 0 = exited, -1 = unknown. When it returns 0, `exitCode` +// (if given) receives the shell convention: the status, or 128+signal. +// +// The code is part of the answer rather than a second call, because the only +// caller that needs it — the `loop` supervisor — has to distinguish "the +// player finished the track" from "the command does not exist". Restarting the +// first forever is the feature; restarting the second forever is a spin. +int background_running(long long group, int* exitCode); + +// SIGTERM, `graceMs`, then SIGKILL — to the GROUP. Reaps the direct child. +void background_stop(long long group, long long graceMs); + +// ─── Ctrl-C ────────────────────────────────────────────────────────────── +// +// Its own process group is what makes killpg possible AND what stops the +// terminal's SIGINT from reaching the child: Ctrl-C would kill mcpp and leave +// the player running. Both halves are required, so the group that must not +// outlive us is registered here for the duration. +// +// The handler does the minimum that is async-signal-safe: killpg (which is), +// then the default action. +void guard_group_on_signal(long long group); +void clear_group_guard(); + } // namespace mcpp::platform::unixproc namespace mcpp::platform::unixproc { @@ -228,6 +278,135 @@ DeadlineRun capture_with_deadline(const char* const* argvEntries, return out; } +// ─── Background children ───────────────────────────────────────────────── + +namespace { + +// Read by a signal handler, so `volatile sig_atomic_t` and nothing else: the +// handler may run between any two instructions and may not lock, allocate, or +// call anything that is not async-signal-safe. 0 means "nothing to clean up". +volatile sig_atomic_t g_guardedGroup = 0; + +extern "C" void background_signal_handler(int sig) { + const auto group = g_guardedGroup; + // killpg is async-signal-safe. SIGKILL rather than SIGTERM: this is the + // path where mcpp is about to stop existing, and there is nobody left to + // escalate if the group ignores the polite request. + if (group > 0) ::killpg(static_cast(group), SIGKILL); + // Die of the signal we were sent, so the exit status is the one the shell + // and any outer script expect from a Ctrl-C. + ::signal(sig, SIG_DFL); + ::raise(sig); +} + +} // namespace + +BackgroundChild spawn_background(const char* const* argvEntries, + unsigned long argvCount, + const char* cwd, + int inheritStdio) +{ + BackgroundChild out; + if (argvCount == 0 || !argvEntries) return out; + + std::vector cargv; + cargv.reserve(argvCount + 1); + for (unsigned long i = 0; i < argvCount; ++i) + cargv.push_back(const_cast(argvEntries[i])); + cargv.push_back(nullptr); + + posix_spawn_file_actions_t fa; + ::posix_spawn_file_actions_init(&fa); + if (cwd && *cwd) + ::posix_spawn_file_actions_addchdir_np(&fa, cwd); + if (!inheritStdio) { + // /dev/null on all three: a spanning hook must not write into ninja's + // output, and must not be able to block on a terminal read either. + ::posix_spawn_file_actions_addopen(&fa, 0, "/dev/null", O_RDONLY, 0); + ::posix_spawn_file_actions_addopen(&fa, 1, "/dev/null", O_WRONLY, 0); + ::posix_spawn_file_actions_adddup2(&fa, 1, 2); + } + + // POSIX_SPAWN_SETPGROUP with pgroup 0: the child becomes the leader of a + // new group whose id is its pid. Standard POSIX, unlike SETSID. + posix_spawnattr_t attr; + ::posix_spawnattr_init(&attr); + ::posix_spawnattr_setpgroup(&attr, 0); + ::posix_spawnattr_setflags(&attr, POSIX_SPAWN_SETPGROUP); + + pid_t pid = 0; + const int sp = ::posix_spawnp(&pid, cargv[0], &fa, &attr, + cargv.data(), current_environ()); + ::posix_spawn_file_actions_destroy(&fa); + ::posix_spawnattr_destroy(&attr); + if (sp != 0) return out; + + out.ok = true; + out.group = pid; + return out; +} + +// ⚠️ DOES NOT REAP, and that is the whole point. +// +// A zombie still holds its pid, so an unreaped leader is what keeps the GROUP +// id from being recycled — and `background_stop` signals that group. Reaping +// here would hand the id back to the kernel between the poll and the kill, +// which on a busy machine is how a stop lands on somebody else's process. +// waitid(WNOWAIT) answers "has it exited?" without giving the id back. +int background_running(long long group, int* exitCode) { + if (group <= 0) return -1; + siginfo_t info{}; + info.si_pid = 0; + if (::waitid(P_PID, static_cast(group), &info, + WEXITED | WNOHANG | WNOWAIT) != 0) + return -1; + if (info.si_pid == 0) return 1; + if (exitCode) + *exitCode = (info.si_code == CLD_EXITED) + ? info.si_status + : 128 + info.si_status; // shell convention for a signal + return 0; +} + +void background_stop(long long group, long long graceMs) { + if (group <= 0) return; + const pid_t pgid = static_cast(group); + + // The group, not the pid. This is the line the design is about: `kill(pid)` + // reaches only the shell mcpp started, and `sh -c 'player & wait'` makes + // the player a grandchild — still holding the audio device afterwards. + ::killpg(pgid, SIGTERM); + + const auto until = std::chrono::steady_clock::now() + + std::chrono::milliseconds(graceMs); + while (background_running(group, nullptr) == 1 + && std::chrono::steady_clock::now() < until) { + struct timespec ts{0, 10'000'000}; // 10ms + ::nanosleep(&ts, nullptr); + } + + // Unconditional, and BEFORE the reap: the leader may have gone politely + // while something it forked has not, and the group is still addressable + // only for as long as the unreaped leader holds the id. + ::killpg(pgid, SIGKILL); + int status = 0; + ::waitpid(pgid, &status, 0); +} + +void guard_group_on_signal(long long group) { + g_guardedGroup = static_cast(group); + ::signal(SIGINT, background_signal_handler); + ::signal(SIGTERM, background_signal_handler); + ::signal(SIGHUP, background_signal_handler); +} + +void clear_group_guard() { + g_guardedGroup = 0; + ::signal(SIGINT, SIG_DFL); + ::signal(SIGTERM, SIG_DFL); + ::signal(SIGHUP, SIG_DFL); +} + #else DeadlineRun capture_with_deadline(const char* const*, unsigned long, @@ -237,6 +416,15 @@ DeadlineRun capture_with_deadline(const char* const*, unsigned long, return {}; } +BackgroundChild spawn_background(const char* const*, unsigned long, + const char*, int) { + return {}; +} +int background_running(long long, int*) { return -1; } +void background_stop(long long, long long) {} +void guard_group_on_signal(long long) {} +void clear_group_guard() {} + #endif } // namespace mcpp::platform::unixproc diff --git a/modules/platform/src/windows/bounded_process.cppm b/modules/platform/src/windows/bounded_process.cppm index 0b30e914..980245fe 100644 --- a/modules/platform/src/windows/bounded_process.cppm +++ b/modules/platform/src/windows/bounded_process.cppm @@ -97,6 +97,52 @@ DeadlineRun capture_with_deadline(const char* commandLine, OutputSink sink, void* ctx); +// ─── A child that outlives the call that started it (#496) ─────────────── +// +// The peer of unixproc::spawn_background, and the same contract: a project +// `[hooks] during_build` command is owned for the length of the BUILD, so the +// caller gets a handle it can poll and stop later. Builtins only, like +// DeadlineRun — HANDLEs travel as integers rather than as a std type. +// +// The job object does here what a process group does on POSIX, and does it +// better in one respect: JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE means the tree +// dies when the last handle to the job closes, which includes mcpp exiting for +// ANY reason — Ctrl-C, an unhandled exception, or being killed outright. POSIX +// needs an explicit signal handler for the same guarantee and still cannot +// cover `kill -9`. +struct BackgroundChild { + bool ok = false; + unsigned long long job = 0; // HANDLE to the job object + unsigned long long process = 0; // HANDLE to the child +}; + +// `inheritStdio == 0` sends the child's output to NUL. A spanning hook writes +// CONCURRENTLY with ninja and would otherwise interleave into the middle of a +// compiler diagnostic. +BackgroundChild spawn_background(const char* commandLine, + const char* cwd, + int inheritStdio); + +// 1 = still running, 0 = exited, -1 = unknown. When it returns 0, `exitCode` +// (if given) receives the process's exit code. Same contract as the POSIX +// peer, and for the same caller: the `loop` supervisor has to tell "the +// player finished the track" from "the command does not exist". +int background_running(unsigned long long process, int* exitCode); + +// Closes the job, which terminates the whole tree at once. `graceMs` is +// accepted for signature parity with the POSIX side and is spent waiting for +// the child to leave on its own after the console-control event; Windows has +// no portable graceful stop for a process with no window and no console of its +// own, and pretending otherwise would be a promise this cannot keep. +void background_stop(unsigned long long job, unsigned long long process, + long long graceMs); + +// Ctrl-C. The job already covers process death, so this exists only so that a +// deliberate interrupt stops the tree BEFORE mcpp unwinds, rather than as a +// side effect of it exiting. +void guard_job_on_signal(unsigned long long job); +void clear_job_guard(); + } // namespace mcpp::platform::winproc namespace mcpp::platform::winproc { @@ -315,6 +361,144 @@ DeadlineRun capture_with_deadline(const char* commandLine, return out; } +// ─── Background children ───────────────────────────────────────────────── + +namespace { + +// Read by a console control handler, which runs on a thread of the OS's +// choosing. Only the handle is shared, and closing a job handle is atomic from +// the caller's point of view. +volatile unsigned long long g_guardedJob = 0; + +BOOL WINAPI background_console_handler(DWORD) { + const auto job = g_guardedJob; + // TerminateJobObject, not CloseHandle: this handler races `background_stop` + // on the normal path, and terminating is idempotent while closing the same + // handle twice is not. The handle stays valid for whoever closes it. + if (job) + ::TerminateJobObject( + reinterpret_cast(static_cast(job)), 1); + return FALSE; // FALSE = also run the default handler, i.e. still exit +} + +} // namespace + +BackgroundChild spawn_background(const char* commandLine, + const char* cwd, + int inheritStdio) +{ + BackgroundChild out; + if (!commandLine || !*commandLine) return out; + + HANDLE job = ::CreateJobObjectA(nullptr, nullptr); + if (job) { + JOBOBJECT_EXTENDED_LIMIT_INFORMATION jeli{}; + jeli.BasicLimitInformation.LimitFlags = + JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE; + ::SetInformationJobObject(job, JobObjectExtendedLimitInformation, + &jeli, sizeof(jeli)); + } + + SECURITY_ATTRIBUTES sa{}; + sa.nLength = sizeof(sa); + sa.bInheritHandle = TRUE; + + Handle nulIo; + STARTUPINFOA si{}; + si.cb = sizeof(si); + if (!inheritStdio) { + nulIo.h = ::CreateFileA("NUL", GENERIC_READ | GENERIC_WRITE, + FILE_SHARE_READ | FILE_SHARE_WRITE, &sa, + OPEN_EXISTING, 0, nullptr); + if (nulIo.h && nulIo.h != INVALID_HANDLE_VALUE) { + si.dwFlags = STARTF_USESTDHANDLES; + si.hStdInput = nulIo.h; + si.hStdOutput = nulIo.h; + si.hStdError = nulIo.h; + } + } + + PROCESS_INFORMATION pi{}; + std::string cmdBuf(commandLine); // CreateProcessA may modify it + + // CREATE_SUSPENDED so the child joins the job BEFORE it can spawn anything + // — a grandchild created in that gap would escape the kill, which for a + // background player is the difference between "stops" and "plays forever". + // + // CREATE_NEW_PROCESS_GROUP is the peer of POSIX_SPAWN_SETPGROUP: the child + // stops receiving the console's Ctrl-C, which is what makes the guard + // below necessary and what stops a stray Ctrl-C from half-killing the tree. + const BOOL ok = ::CreateProcessA( + nullptr, cmdBuf.data(), nullptr, nullptr, + /*bInheritHandles=*/TRUE, + CREATE_SUSPENDED | CREATE_NEW_PROCESS_GROUP + | (inheritStdio ? 0u : CREATE_NO_WINDOW), + nullptr, (cwd && *cwd) ? cwd : nullptr, &si, &pi); + if (!ok) { + if (job) ::CloseHandle(job); + return out; + } + if (job) ::AssignProcessToJobObject(job, pi.hProcess); + ::ResumeThread(pi.hThread); + ::CloseHandle(pi.hThread); + + out.ok = true; + out.job = static_cast( + reinterpret_cast(job)); + out.process = static_cast( + reinterpret_cast(pi.hProcess)); + return out; +} + +int background_running(unsigned long long process, int* exitCode) { + if (!process) return -1; + auto h = reinterpret_cast(static_cast(process)); + const DWORD r = ::WaitForSingleObject(h, 0); + if (r == WAIT_TIMEOUT) return 1; + if (r != WAIT_OBJECT_0) return -1; + if (exitCode) { + DWORD code = 0; + ::GetExitCodeProcess(h, &code); + *exitCode = static_cast(code); + } + return 0; +} + +void background_stop(unsigned long long job, unsigned long long process, + long long graceMs) +{ + auto procH = process ? reinterpret_cast( + static_cast(process)) + : nullptr; + auto jobH = job ? reinterpret_cast( + static_cast(job)) + : nullptr; + + // Ask first. A child created with CREATE_NEW_PROCESS_GROUP is its own + // group, so this reaches it and nothing else — including not reaching + // mcpp, which is the reason the group exists. + if (procH && graceMs > 0) { + ::GenerateConsoleCtrlEvent(CTRL_BREAK_EVENT, ::GetProcessId(procH)); + ::WaitForSingleObject(procH, static_cast(graceMs)); + } + + // Then take the tree. Closing a KILL_ON_JOB_CLOSE job is the whole + // mechanism: TerminateProcess on the child alone would leave whatever it + // started behind. + if (jobH) ::CloseHandle(jobH); + if (procH) ::CloseHandle(procH); +} + +void guard_job_on_signal(unsigned long long job) { + g_guardedJob = job; + ::SetConsoleCtrlHandler(background_console_handler, TRUE); +} + +void clear_job_guard() { + g_guardedJob = 0; + ::SetConsoleCtrlHandler(background_console_handler, FALSE); +} + #else DeadlineRun capture_with_deadline(const char*, const char* const*, unsigned long, @@ -323,6 +507,12 @@ DeadlineRun capture_with_deadline(const char*, const char* const*, unsigned long return {}; } +BackgroundChild spawn_background(const char*, const char*, int) { return {}; } +int background_running(unsigned long long, int*) { return -1; } +void background_stop(unsigned long long, unsigned long long, long long) {} +void guard_job_on_signal(unsigned long long) {} +void clear_job_guard() {} + #endif } // namespace mcpp::platform::winproc diff --git a/src/cli/cmd_build.cppm b/src/cli/cmd_build.cppm index 5a56f801..650f9df8 100644 --- a/src/cli/cmd_build.cppm +++ b/src/cli/cmd_build.cppm @@ -56,18 +56,29 @@ int run_build_with_hooks(mcpp::build::BuildContext& ctx, bool verbose, bool no_cache, std::string_view targetOverride) { auto const& hooks = ctx.manifest.hooks; + // `during_build` opens first and closes last: its interval is the one that + // spans everything below. Its output is discarded unless --verbose, which + // is the only way it could interleave into a compiler diagnostic. + mcpp::hooks::Span span(hooks, ctx.projectRoot, /*inheritOutput=*/verbose); + if (!span.ok()) return 1; + if (!mcpp::hooks::invoke(hooks, mcpp::hooks::Event::BuildStart, ctx.projectRoot)) return 1; int rc = mcpp::build::run_build_plan(ctx, verbose, no_cache, targetOverride); + + // Closed BEFORE the terminal hook. A "build finished" sound competing with + // the background music it replaces is the ordering this line settles. + bool spanOk = span.finish(); + auto terminalEvent = rc == 0 ? mcpp::hooks::Event::BuildFinished : mcpp::hooks::Event::BuildFailed; // The build's own exit code outranks the hook's: `mcpp build` returning // "the notifier failed" for a compile error would answer a question nobody // asked. A hook failure only decides the exit code of a build that worked. bool hookOk = mcpp::hooks::invoke(hooks, terminalEvent, ctx.projectRoot); - return rc != 0 ? rc : (hookOk ? 0 : 1); + return rc != 0 ? rc : ((spanOk && hookOk) ? 0 : 1); } export int cmd_build(const mcpplibs::cmdline::ParsedArgs& parsed) { diff --git a/src/hooks.cppm b/src/hooks.cppm index 16f581ce..a7baa9cb 100644 --- a/src/hooks.cppm +++ b/src/hooks.cppm @@ -4,6 +4,12 @@ // and mcpp.toml has one parser (mcpp.manifest). What lives here is the part // that is policy rather than grammar — when a command runs, what a failure // means, and what the user is told. +// +// The model is one idea: a hook is a command mcpp OWNS FOR AN INTERVAL, and +// the event names the interval. `invoke` runs a SELF-CLOSING one (the interval +// ends when the command exits). `Span` owns a SPANNING one (`during_build`), +// whose interval is closed by the build. See +// .agents/docs/2026-08-30-project-build-hooks-owned-intervals.md. export module mcpp.hooks; @@ -16,7 +22,8 @@ export namespace mcpp::hooks { enum class Event { BuildStart, BuildFailed, BuildFinished }; -// Run the command this event names, if the project declared one. +// Run the command this event names, if the project declared one, and wait for +// it. The interval is self-closing: it ends when the command exits. // // Returns whether the BUILD may keep its own result. False means a hook failed // while `side_effect` was on, i.e. the failure is the build's now. A missing @@ -25,12 +32,73 @@ enum class Event { BuildStart, BuildFailed, BuildFinished }; bool invoke(const mcpp::manifest::Hooks& hooks, Event event, const std::filesystem::path& projectRoot); +// `during_build`: a command started before the build and stopped after it. +// +// RAII because the interval must close on every path out of the build, +// including the ones nobody writes down — an early return, an exception, a +// diagnostic that gives up. A background player that outlives its build is the +// failure this whole shape exists to prevent, so the destructor is the only +// place the stop can be promised from. +class Span { +public: + Span() = default; + Span(const mcpp::manifest::Hooks& hooks, + const std::filesystem::path& projectRoot, + bool inheritOutput); + ~Span(); + Span(const Span&) = delete; + Span& operator=(const Span&) = delete; + + // Whether the BUILD may proceed. False means the command could not be + // started and `side_effect` was on — the same verdict, in the same words, + // that a failed `build_start` produces. Already reported. + bool ok() const { return ok_; } + + // Close the interval and report what happened while it was open. Returns + // whether the build may keep its result. + // + // Called explicitly BEFORE the terminal hook rather than left to the + // destructor: a "build finished" sound playing over the background music + // it was supposed to replace is the ordering this exists for. + bool finish(); + +private: + void close(); + + mcpp::platform::process::BackgroundCommand child_; + std::string command_; + std::filesystem::path root_; + bool inheritOutput_ = false; + bool declared_ = false; + bool started_ = false; + bool closed_ = false; + bool sideEffect_ = true; + bool ok_ = true; + std::atomic stop_{false}; + std::atomic gaveUp_{false}; + std::thread supervisor_; +}; + } // namespace mcpp::hooks namespace mcpp::hooks { namespace { +namespace proc = mcpp::platform::process; + +// `loop` restarts a command that ended before its interval did. Two bounds, +// fixed rather than configurable, because a knob whose wrong value is a spin +// is not a knob: a typo in the command (`play /nonexistant`) would otherwise +// restart thousands of times per second for the length of the build. +constexpr auto kRestartDelay = std::chrono::milliseconds(250); +constexpr auto kTooShort = std::chrono::milliseconds(1000); +constexpr int kShortRunsBeforeGivingUp = 5; + +// How long a stopped command is given to leave on its own before it is taken. +// A player asked to stop should get to close its audio device. +constexpr auto kStopGrace = std::chrono::milliseconds(2000); + std::string_view event_name(Event event) { switch (event) { case Event::BuildStart: return "build_start"; @@ -40,20 +108,25 @@ std::string_view event_name(Event event) { return "unknown"; } -std::string_view event_command(const mcpp::manifest::Hooks& hooks, Event event) { +const mcpp::manifest::HookCommand& +event_command(const mcpp::manifest::Hooks& hooks, Event event) { switch (event) { - case Event::BuildStart: return hooks.buildStart; case Event::BuildFailed: return hooks.buildFailed; case Event::BuildFinished: return hooks.buildFinished; + case Event::BuildStart: break; } - return {}; + return hooks.buildStart; } +} // namespace + // One place decides what a hook failure costs, so `side_effect` cannot be -// honoured on one failure mode and forgotten on another. The message is -// identical either way — only its severity and the build's fate differ. -bool report_failure(const mcpp::manifest::Hooks& hooks, std::string message) { - if (hooks.sideEffect) { +// honoured on one failure mode and forgotten on another — and there are now +// four of them (cannot start, non-zero, timed out, failed to stay up). The +// message is identical either way; only its severity and the build's fate +// differ. +bool report_hook_failure_flag(bool sideEffect, const std::string& message) { + if (sideEffect) { mcpp::ui::error(message); return false; } @@ -61,30 +134,139 @@ bool report_failure(const mcpp::manifest::Hooks& hooks, std::string message) { return true; } -} // namespace +bool report_hook_failure(const mcpp::manifest::Hooks& hooks, + const std::string& message) { + return report_hook_failure_flag(hooks.sideEffect, message); +} bool invoke(const mcpp::manifest::Hooks& hooks, Event event, const std::filesystem::path& projectRoot) { if (!hooks.enabled) return true; - auto command = event_command(hooks, event); - if (command.empty()) return true; + auto const& hook = event_command(hooks, event); + if (hook.empty()) return true; bool timedOut = false; // The project root is passed to the launcher, not arranged with a chdir: // mcpp's working directory is process-wide state, and a hook is not // entitled to move it even briefly. - int rc = mcpp::platform::process::run_shell_deadline( - command, projectRoot.string(), - std::chrono::seconds(hooks.timeoutSeconds), &timedOut); + int rc = proc::run_shell_deadline( + hook.cmd, projectRoot.string(), + std::chrono::seconds(hooks.timeout_for(hook)), &timedOut); if (timedOut) - return report_failure(hooks, std::format( + return report_hook_failure(hooks, std::format( "hook '{}' timed out after {}s", event_name(event), - hooks.timeoutSeconds)); + hooks.timeout_for(hook))); if (rc != 0) - return report_failure(hooks, std::format( + return report_hook_failure(hooks, std::format( "hook '{}' exited with status {}", event_name(event), rc)); return true; } +Span::Span(const mcpp::manifest::Hooks& hooks, + const std::filesystem::path& projectRoot, + bool inheritOutput) + : command_(hooks.duringBuild.cmd) + , root_(projectRoot) + , inheritOutput_(inheritOutput) + , declared_(hooks.enabled && !hooks.duringBuild.empty()) + , sideEffect_(hooks.sideEffect) +{ + if (!declared_) return; + + child_ = proc::start_shell_background(command_, root_.string(), + inheritOutput_); + if (!child_.ok) { + ok_ = report_hook_failure(hooks, + "hook 'during_build' could not be started"); + return; + } + started_ = true; + + // Its own process group is what makes a whole-tree stop possible and what + // stops the terminal's Ctrl-C from reaching it. Both halves are needed, so + // the group is registered for the duration. + proc::guard_background_on_signal(child_); + + if (!hooks.duringBuild.loop) return; + + // A supervisor exists ONLY for `loop`. Without it, a spanning hook is a + // spawn and a stop, and no thread is created. + supervisor_ = std::thread([this, runStarted = std::chrono::steady_clock::now()] + () mutable { + int shortFailures = 0; + while (!stop_.load()) { + int code = 0; + if (proc::background_running(child_, &code)) { + std::this_thread::sleep_for(std::chrono::milliseconds(50)); + continue; + } + // How long the run that JUST ENDED lasted. Measuring from the + // restart instead would compare a spawn against itself and read + // every run as instantaneous — which is a give-up on the fifth + // restart of a perfectly healthy command. + const auto ranFor = std::chrono::steady_clock::now() - runStarted; + + // Stop before restarting, not after: the run is over but whatever + // it forked may not be, and the group is addressable only while + // the unreaped leader still holds its id. + proc::stop_background(child_, std::chrono::milliseconds(0)); + proc::clear_background_guard(); + if (stop_.load()) return; + + // "Failed to stay up" is BOTH halves: short AND unsuccessful. A + // command that finishes quickly and cleanly — one `ding.mp3`, an + // `echo` — is doing exactly what `loop` was asked to repeat, and + // counting it here would turn the feature into its own kill switch. + if (code != 0 && ranFor < kTooShort) { + if (++shortFailures >= kShortRunsBeforeGivingUp) { + gaveUp_.store(true); + return; + } + } else { + shortFailures = 0; + } + + std::this_thread::sleep_for(kRestartDelay); + if (stop_.load()) return; + + runStarted = std::chrono::steady_clock::now(); + child_ = proc::start_shell_background(command_, root_.string(), + inheritOutput_); + if (!child_.ok) { + gaveUp_.store(true); + return; + } + proc::guard_background_on_signal(child_); + } + }); +} + +void Span::close() { + if (!declared_ || closed_) return; + closed_ = true; + stop_.store(true); + if (supervisor_.joinable()) supervisor_.join(); + if (started_) { + proc::clear_background_guard(); + proc::stop_background(child_, kStopGrace); + } +} + +bool Span::finish() { + const bool alreadyClosed = closed_; + close(); + if (alreadyClosed || !started_) return ok_; + if (!gaveUp_.load()) return ok_; + // Reported once, here, rather than from the supervisor thread: ui writes + // are not synchronised, and a thread printing into the middle of ninja's + // output is the interleaving `during_build` discards its child's stdout to + // avoid in the first place. + return report_hook_failure_flag(sideEffect_, std::format( + "hook 'during_build' failed to stay up: {} consecutive runs ended " + "within {}ms", kShortRunsBeforeGivingUp, kTooShort.count())); +} + +Span::~Span() { close(); } + } // namespace mcpp::hooks diff --git a/tests/e2e/317_project_build_hooks.sh b/tests/e2e/317_project_build_hooks.sh index 4ad37a21..8df04ae2 100644 --- a/tests/e2e/317_project_build_hooks.sh +++ b/tests/e2e/317_project_build_hooks.sh @@ -145,9 +145,16 @@ rm -f disabled.log # ── The timeout is a real bound, on both host shells ──────────────────── case "$(uname -s)" in - MINGW*|MSYS*|CYGWIN*) slow_command="ping -n 6 127.0.0.1 >NUL" ;; - *) slow_command="sleep 5" ;; + MINGW*|MSYS*|CYGWIN*) HOST_WINDOWS=1 ;; + *) HOST_WINDOWS=0 ;; esac +if [[ $HOST_WINDOWS -eq 1 ]]; then + slow_command="ping -n 6 127.0.0.1 >NUL" + pause_2s="ping -n 3 127.0.0.1 >NUL" +else + slow_command="sleep 5" + pause_2s="sleep 2" +fi write_manifest < writer.cmd <<'EOF' +@echo off +:loop +echo tick>>beat.log +ping -n 2 127.0.0.1 >NUL +goto loop +EOF + # `start /b` returns immediately, so the command mcpp starts EXITS and the + # writer is left behind it — the job object is the only thing that can + # still reach it. + writer_with_grandchild="start /b cmd /c writer.cmd" +else + cat > writer.sh <<'EOF' +while :; do echo tick>>beat.log; sleep 0.2; done +EOF + # The shell mcpp starts forks the writer and waits, so the writer is a + # GRANDCHILD. This is the case `kill(pid)` misses and `killpg` catches, and + # it is the only assertion that tells the fix from the bug. + writer_with_grandchild="sh writer.sh & wait" +fi + +beat_is_frozen() { # the file grew, and then stopped growing + local before after + before=$(wc -c < beat.log 2>/dev/null || echo 0) + sleep 1 + after=$(wc -c < beat.log 2>/dev/null || echo 0) + [[ "$before" != "0" && "$before" == "$after" ]] +} + +: > hooks.log +rm -f beat.log +write_manifest < during-build.log 2>&1 || { + cat during-build.log; echo "FAIL: build with during_build failed"; exit 1; } +beat_is_frozen || { + echo "FAIL: during_build kept running after the build (or never ran)" + wc -c beat.log 2>/dev/null; exit 1; } +# The interval closed BEFORE the terminal hook, which is the ordering that +# keeps a "build finished" sound from playing over the music it replaces. +expect_log hooks.log finished + +# ── `loop` restarts a command that ends before its interval does ──────── +rm -f runs.log +write_manifest < loop-on.log 2>&1 || { + cat loop-on.log; echo "FAIL: looped during_build failed the build"; exit 1; } +looped=$(wc -l < runs.log | tr -d ' ') +[[ "$looped" -ge 2 ]] || { + cat loop-on.log; echo "FAIL: loop=true ran $looped time(s), expected >= 2"; exit 1; } + +# Without `loop`, the same command runs exactly once — the denominator that +# makes the count above mean something. +rm -f runs.log +write_manifest < loop-off.log 2>&1 || { + cat loop-off.log; echo "FAIL: unlooped during_build failed the build"; exit 1; } +once=$(wc -l < runs.log | tr -d ' ') +[[ "$once" -eq 1 ]] || { + cat loop-off.log; echo "FAIL: loop=false ran $once time(s), expected 1"; exit 1; } + +# ── A command that cannot stay up stops, rather than spinning ─────────── +# +# `loop` on a typo is a fork bomb otherwise. The bound is five consecutive +# runs that end unsuccessfully within a second. +write_manifest < loop-giveup.log 2>&1 || rc=$? +[[ $rc -ne 0 ]] || { + cat loop-giveup.log; echo "FAIL: a during_build that never ran was not fatal"; exit 1; } +grep -q "error: hook 'during_build' failed to stay up" loop-giveup.log || { + cat loop-giveup.log; echo "FAIL: giving up was not reported"; exit 1; } + +# ── `loop` on a self-closing event is refused, not ignored ────────────── +write_manifest <<'EOF' +[package] +name = "hookprobe" +version = "0.1.0" + +[hooks] +build_start = { cmd = "echo start", loop = true } +EOF +rc=0 +"$MCPP" build > loop-misplaced.log 2>&1 || rc=$? +[[ $rc -ne 0 ]] || { + cat loop-misplaced.log; echo "FAIL: loop on build_start was accepted"; exit 1; } +grep -q "during_build" loop-misplaced.log || { + cat loop-misplaced.log + echo "FAIL: the diagnostic does not name the event that supports loop"; exit 1; } + +# ── An interrupted build leaves nothing running ───────────────────────── +# +# POSIX only, and the skip is printed rather than silent: sending Ctrl-C to +# another process on Windows needs a helper this suite does not have, and a +# test that cannot fail there would read as coverage. +if [[ $HOST_WINDOWS -eq 1 ]]; then + echo "SKIP(within test): Ctrl-C cleanup — no way to signal another process here" +else + rm -f beat.log + write_manifest < interrupted.log 2>&1 & + build_pid=$! + sleep 3 + kill -INT "$build_pid" 2>/dev/null || true + wait "$build_pid" 2>/dev/null || true + beat_is_frozen || { + echo "FAIL: Ctrl-C left during_build running"; exit 1; } +fi + # ── A DEPENDENCY's hooks never run ────────────────────────────────────── # # Every manifest mcpp parses carries a `[hooks]` field, a dependency's diff --git a/tests/unit/test_manifest.cpp b/tests/unit/test_manifest.cpp index dae08e89..0625a3c9 100644 --- a/tests/unit/test_manifest.cpp +++ b/tests/unit/test_manifest.cpp @@ -4007,9 +4007,9 @@ timeout_seconds = 30 side_effect = false )"); ASSERT_TRUE(m.has_value()) << (m ? "" : m.error().format()); - EXPECT_EQ(m->hooks.buildStart, "echo start"); - EXPECT_EQ(m->hooks.buildFailed, "echo failed"); - EXPECT_EQ(m->hooks.buildFinished, "echo finished"); + EXPECT_EQ(m->hooks.buildStart.cmd, "echo start"); + EXPECT_EQ(m->hooks.buildFailed.cmd, "echo failed"); + EXPECT_EQ(m->hooks.buildFinished.cmd, "echo finished"); EXPECT_EQ(m->hooks.timeoutSeconds, 30); EXPECT_FALSE(m->hooks.sideEffect); EXPECT_TRUE(m->hooks.active()); @@ -4024,7 +4024,7 @@ build_start = "echo start" enabled = false )"); ASSERT_TRUE(m.has_value()) << (m ? "" : m.error().format()); - EXPECT_EQ(m->hooks.buildStart, "echo start"); + EXPECT_EQ(m->hooks.buildStart.cmd, "echo start"); EXPECT_FALSE(m->hooks.active()); } @@ -4111,3 +4111,93 @@ build_stared = "echo typo" EXPECT_TRUE(found); EXPECT_TRUE(m->hooks.active()); } + +// ─── The table form, and the interval each event names ─────────────────── + +TEST(ManifestHooks, ABareStringIsSugarForATable) { + auto m = parse_with_hooks(R"( +[hooks] +build_start = "echo start" +)"); + ASSERT_TRUE(m.has_value()) << (m ? "" : m.error().format()); + EXPECT_EQ(m->hooks.buildStart.cmd, "echo start"); + EXPECT_EQ(m->hooks.buildStart.timeoutSeconds, 0); // 0 = inherit + EXPECT_FALSE(m->hooks.buildStart.loop); +} + +// The per-event value wins over the table's, and 0 is what makes "not given" +// expressible — without it there is no way to tell an inherited 10 from a +// declared one. +TEST(ManifestHooks, APerEventTimeoutOverridesTheTableDefault) { + auto m = parse_with_hooks(R"( +[hooks] +timeout_seconds = 10 +build_finished = { cmd = "slow-notifier", timeout_seconds = 45 } +build_start = "echo start" +)"); + ASSERT_TRUE(m.has_value()) << (m ? "" : m.error().format()); + EXPECT_EQ(m->hooks.timeout_for(m->hooks.buildFinished), 45); + EXPECT_EQ(m->hooks.timeout_for(m->hooks.buildStart), 10); +} + +TEST(ManifestHooks, DuringBuildIsReadAndMakesTheTableActive) { + auto m = parse_with_hooks(R"( +[hooks] +during_build = { cmd = "play bgm.mp3", loop = true } +)"); + ASSERT_TRUE(m.has_value()) << (m ? "" : m.error().format()); + EXPECT_EQ(m->hooks.duringBuild.cmd, "play bgm.mp3"); + EXPECT_TRUE(m->hooks.duringBuild.loop); + EXPECT_TRUE(m->hooks.active()); +} + +// ⭐ The two keys that only exist for one interval. Accepted-and-ignored is +// the failure mode this rejects: the user writes `loop` on `build_start`, +// nothing repeats, and the feature looks broken rather than misused. Each +// message names the event that does support the key. +TEST(ManifestHooks, AKeyOfferedToTheWrongIntervalIsRejected) { + auto loopOnSelfClosing = parse_with_hooks(R"( +[hooks] +build_start = { cmd = "play bgm.mp3", loop = true } +)"); + ASSERT_FALSE(loopOnSelfClosing.has_value()); + EXPECT_NE(loopOnSelfClosing.error().message.find("during_build"), + std::string::npos) << loopOnSelfClosing.error().message; + + auto timeoutOnSpanning = parse_with_hooks(R"( +[hooks] +during_build = { cmd = "play bgm.mp3", timeout_seconds = 30 } +)"); + ASSERT_FALSE(timeoutOnSpanning.has_value()); + EXPECT_NE(timeoutOnSpanning.error().message.find("timeout_seconds"), + std::string::npos) << timeoutOnSpanning.error().message; +} + +TEST(ManifestHooks, ATableWithoutCmdIsRejected) { + for (std::string_view section : { + R"([hooks] +during_build = { loop = true })", + R"([hooks] +build_start = { cmd = "" })", + R"([hooks] +build_start = 42)", + }) { + auto m = parse_with_hooks(section); + ASSERT_FALSE(m.has_value()) << section; + EXPECT_NE(m.error().message.find("cmd"), std::string::npos) + << section << " -> " << m.error().message; + } +} + +TEST(ManifestHooks, AnUnknownKeyInsideAnEventTableWarns) { + auto m = parse_with_hooks(R"( +[hooks] +during_build = { cmd = "play bgm.mp3", lopo = true } +)"); + ASSERT_TRUE(m.has_value()) << (m ? "" : m.error().format()); + bool found = false; + for (auto const& w : m->schemaWarnings) + if (w.find("lopo") != std::string::npos) found = true; + EXPECT_TRUE(found); + EXPECT_TRUE(m->hooks.active()); +} From fa14e6d6db6c4e604ab3ffe3e338bee03a6bf9dc Mon Sep 17 00:00:00 2001 From: speak-agent <248744407+speak-agent@users.noreply.github.com> Date: Sun, 30 Aug 2026 18:51:32 +0800 Subject: [PATCH 4/5] fix(hooks): Windows has no polite stop, and asking for one killed the console MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- ...-30-project-build-hooks-owned-intervals.md | 11 ++++ .../platform/src/windows/bounded_process.cppm | 50 +++++++++++++------ 2 files changed, 45 insertions(+), 16 deletions(-) diff --git a/.agents/docs/2026-08-30-project-build-hooks-owned-intervals.md b/.agents/docs/2026-08-30-project-build-hooks-owned-intervals.md index dee1aba8..f484228c 100644 --- a/.agents/docs/2026-08-30-project-build-hooks-owned-intervals.md +++ b/.agents/docs/2026-08-30-project-build-hooks-owned-intervals.md @@ -159,6 +159,17 @@ Windows: the job object is closed, which terminates the tree at once. There is no graceful equivalent that does not require enumerating the job's processes and posting window messages; the asymmetry is stated rather than hidden. +⚠️ The obvious symmetry — "ask with `GenerateConsoleCtrlEvent(CTRL_BREAK_EVENT, +pid)` first" — was written, and it is wrong in a way no local run shows. 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 exited — `start /b`-style commands exit immediately) the event reaches +everything sharing the console. 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, because mcpp had +sent Ctrl-Break to its own console. The design said Windows has no graceful stop; +the first implementation did not believe it. The code now matches the design. + ## Output A self-closing hook inherits stdio, which is safe because nothing else is diff --git a/modules/platform/src/windows/bounded_process.cppm b/modules/platform/src/windows/bounded_process.cppm index 980245fe..ce930b1a 100644 --- a/modules/platform/src/windows/bounded_process.cppm +++ b/modules/platform/src/windows/bounded_process.cppm @@ -129,11 +129,15 @@ BackgroundChild spawn_background(const char* commandLine, // player finished the track" from "the command does not exist". int background_running(unsigned long long process, int* exitCode); -// Closes the job, which terminates the whole tree at once. `graceMs` is -// accepted for signature parity with the POSIX side and is spent waiting for -// the child to leave on its own after the console-control event; Windows has -// no portable graceful stop for a process with no window and no console of its -// own, and pretending otherwise would be a promise this cannot keep. +// Closes the job, which terminates the whole tree at once. +// +// `graceMs` is accepted for signature parity with the POSIX peer and is NOT +// spent: Windows has no portable graceful stop for a child with no console and +// no window of its own, and the call that looks like one +// (GenerateConsoleCtrlEvent) addresses a process group attached to THIS +// console — see the implementation for what that cost. The asymmetry with the +// POSIX side's SIGTERM-then-grace is real and is declared here rather than +// papered over. void background_stop(unsigned long long job, unsigned long long process, long long graceMs); @@ -474,17 +478,31 @@ void background_stop(unsigned long long job, unsigned long long process, static_cast(job)) : nullptr; - // Ask first. A child created with CREATE_NEW_PROCESS_GROUP is its own - // group, so this reaches it and nothing else — including not reaching - // mcpp, which is the reason the group exists. - if (procH && graceMs > 0) { - ::GenerateConsoleCtrlEvent(CTRL_BREAK_EVENT, ::GetProcessId(procH)); - ::WaitForSingleObject(procH, static_cast(graceMs)); - } - - // Then take the tree. Closing a KILL_ON_JOB_CLOSE job is the whole - // mechanism: TerminateProcess on the child alone would leave whatever it - // started behind. + // ⚠️ NO POLITE ASK HERE, AND `graceMs` IS DELIBERATELY UNSPENT. + // + // The obvious "ask first" is + // + // ::GenerateConsoleCtrlEvent(CTRL_BREAK_EVENT, ::GetProcessId(procH)); + // + // and it is wrong in a way that does not show up locally. That call + // addresses a process GROUP attached to this console, not a process; when + // the id does not name a live group of ours — which it does not, once the + // child has already exited, and `start /b`-style commands exit at once — + // the event reaches everything sharing the console instead. Measured on + // the Windows e2e runner: the whole 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 + // Ctrl-Break'd its own console. + // + // Windows has no portable graceful stop for a child with no console and no + // window of its own. The job IS the mechanism; the POSIX peer's + // SIGTERM-then-grace has no equivalent here, and the asymmetry is stated + // in the declaration rather than faked with a call that reaches too far. + (void)graceMs; + + // Closing a KILL_ON_JOB_CLOSE job takes the whole tree. TerminateProcess + // on the child alone would leave whatever it started behind — which for + // `start /b cmd /c player` is the player. if (jobH) ::CloseHandle(jobH); if (procH) ::CloseHandle(procH); } From 0d8db931c38041121677d7cb763a8ae6c4c76ce3 Mon Sep 17 00:00:00 2001 From: speak-agent <248744407+speak-agent@users.noreply.github.com> Date: Sun, 30 Aug 2026 19:41:36 +0800 Subject: [PATCH 5/5] =?UTF-8?q?feat(hooks):=20ship=20it=20experimental=20?= =?UTF-8?q?=E2=80=94=20a=20hook=20gets=20no=20vote=20on=20the=20build?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `[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. --- ...-30-project-build-hooks-owned-intervals.md | 35 +++++++++ docs/05-mcpp-toml.md | 53 ++++++++++---- docs/zh/05-mcpp-toml.md | 41 ++++++++--- modules/manifest/src/toml.cppm | 20 ++++++ modules/manifest/src/types.cppm | 17 ++++- src/hooks.cppm | 14 +++- tests/e2e/317_project_build_hooks.sh | 71 ++++++++++++++----- tests/unit/test_manifest.cpp | 34 ++++++++- 8 files changed, 236 insertions(+), 49 deletions(-) diff --git a/.agents/docs/2026-08-30-project-build-hooks-owned-intervals.md b/.agents/docs/2026-08-30-project-build-hooks-owned-intervals.md index f484228c..8a9a855b 100644 --- a/.agents/docs/2026-08-30-project-build-hooks-owned-intervals.md +++ b/.agents/docs/2026-08-30-project-build-hooks-owned-intervals.md @@ -178,6 +178,37 @@ would interleave into the middle of a compiler diagnostic. Its output is therefore discarded by default, and inherited under `--verbose` — no new schema key, and the answer to "why is there no music" is one flag away. +## Shipping it experimental + +The feature ships with `side_effect` defaulting to **false** and `true` +*refused* by the manifest parser. A hook therefore cannot change whether a +build succeeded: every failure is a warning and `mcpp build` keeps the result +it earned on its own. + +Refused rather than downgraded, because both silent behaviours are worse than +an error. Honouring `true` ships an experimental feature with a veto over every +build. Ignoring it leaves a project believing its build is gated on a notifier +when nothing is — the "accepted and does nothing" shape this design rejects +everywhere else (`loop` on a self-closing event, `timeout_seconds` on +`during_build`), so it would be inconsistent to make an exception for the one +key whose wrong answer is invisible. + +The key stays in the schema so manifests do 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. That is the property to preserve when editing either side. + +Two further limits are permanent rather than provisional and should not be read +as part of the experiment: + +- Only the ROOT project's hooks run. A dependency's `[hooks]` is inert by + construction — there is exactly one `Span` construction and there are exactly + two `invoke` call sites, all in `run_build_with_hooks`, all fed from the + context's own manifest. +- Only `mcpp build` runs hooks. `mcpp run`, `mcpp test` and + `mcpp build --configure-only` build too, and deliberately do not. + ## Criteria The assertions this design has to earn are about *state*, not about log lines — @@ -202,3 +233,7 @@ The assertions this design has to earn are about *state*, not about log lines rather than papered over with a test that passes vacuously. 7. **A self-closing event rejects `loop`.** Otherwise the key is accepted, does nothing, and the user concludes the feature does not work. +8. **`side_effect = true` is refused, and every failure mode is a warning.** + Two assertions, not one: the exit code says the hook had no vote, and the + warning says the failure was not swallowed. A test that checked only the + exit code would pass just as well if hooks had stopped running altogether. diff --git a/docs/05-mcpp-toml.md b/docs/05-mcpp-toml.md index 66a6341b..4b2810f8 100644 --- a/docs/05-mcpp-toml.md +++ b/docs/05-mcpp-toml.md @@ -2094,7 +2094,15 @@ See [07 — build.mcpp](07-build-mcpp.md). Naming such a file in command: nothing tracks it, and editing the file produces `ninja: no work to do`. -### 2.16 `[hooks]` — Project Build Lifecycle Commands +### 2.16 `[hooks]` — Project Build Lifecycle Commands (experimental) + +> **Experimental.** A hook cannot currently decide whether a build succeeded. +> Every hook failure is reported as a **warning** and `mcpp build` keeps the +> result it earned on its own; `side_effect = true` is refused with an error +> rather than honoured. The key stays in the schema so that manifests written +> today do not have to change when the feature is promoted. Two further limits +> are permanent rather than provisional: only the root project's hooks run, and +> only `mcpp build` runs them. A hook is a command `mcpp build` **owns for an interval**, and the event names the interval: @@ -2111,7 +2119,7 @@ during_build = { cmd = "mcpp-hooks-audioplayer bgm", loop = true } # Optional; these are the defaults. timeout_seconds = 10 enabled = true -side_effect = true +side_effect = false # `true` is refused while this is experimental ``` | Key | Type | Default | The interval it names | @@ -2122,7 +2130,7 @@ side_effect = true | `during_build` | command | — | Opens before the build, closes after it | | `timeout_seconds` | integer, 1–86400 | `10` | Bounds one run of a command | | `enabled` | bool | `true` | Enables all commands in this table | -| `side_effect` | bool | `true` | Whether a hook failure makes the overall build fail | +| `side_effect` | bool | `false` | Whether a hook failure makes the build fail. **Reserved** — only `false` is accepted while this is experimental | The first three intervals are **self-closing** — they end when the command does. "Synchronous" is not a separate mode here; it is what a self-closing @@ -2172,11 +2180,24 @@ hook failure. For `during_build` there is one more: a looped command that **fails to stay up** — five consecutive runs ending unsuccessfully within a second — stops being restarted and is reported. (A command that finishes quickly and *successfully* is doing exactly what `loop` was asked to repeat, -and is not a failure.) With `side_effect = false`, mcpp reports a warning and -preserves the build's result; with `true`, it returns failure — but a build that -failed on its own keeps its own exit code, so `mcpp build` never reports a -compile error as a notifier problem. A hook's own failure does not trigger -another hook. +and is not a failure.) Every one of those is reported as a **warning**, and the +build keeps the result it earned on its own — while `[hooks]` is experimental +it does not get a vote. A hook's own failure does not trigger another hook. + +`side_effect = true` is what will change that, and asking for it today is an +error: + +```text +error: mcpp.toml: error: [hooks].side_effect = true is not available yet: +[hooks] is experimental and cannot decide whether a build succeeded. … +``` + +Refused rather than quietly downgraded, because both silent options are worse: +honouring it would give an experimental feature a veto over every build, and +ignoring it would leave a project believing its build is gated on a notifier +when nothing is. When the feature is promoted, `true` will mean "a hook failure +fails the build" — and a build that failed on its own will still keep its own +exit code, so `mcpp build` never reports a compile error as a notifier problem. Two things are worth knowing about a `during_build` command specifically: @@ -2197,9 +2218,11 @@ Scope, precisely: each member in turn — its own `[hooks]`, around its own build, in its own root. A *virtual* workspace root (`[workspace]` with no `[package]`) builds nothing, so a `[hooks]` table there never fires. -- A dependency's `[hooks]` is never run. Only the root project's — installing a - package cannot make its author's shell command part of the consuming - project's build. +- A dependency's `[hooks]` is **skipped**, always. Only the root project's run. + Every manifest mcpp parses carries the section, a dependency's included, and + nothing reads it — which is what keeps `mcpp add` from meaning "run this + author's shell command on my next build". This is a property of the design, + not a default awaiting a switch. - Declaring an active hook opts the project out of the no-op fast path, because `build_start` is specified to run after preparation. Expect `mcpp build` on an already-current hooked project to cost a preparation pass rather than @@ -2231,9 +2254,11 @@ side_effect = false deps = ["xim:mcpp-hooks-audioplayer@0.0.1"] ``` -Background music for the length of the build, a different sound for how it -ended, and `side_effect = false` so that a missing audio device is a warning -rather than a failed build. +Background music for the length of the build, and a different sound for how it +ended. `side_effect = false` is written out rather than left to the default: +it is the value this manifest wants on its own terms — a missing audio device +should never fail a build — so it will still say so once the key has more than +one accepted value. ## Appendix A. Schema Ownership Principle (admission criteria for new fields) diff --git a/docs/zh/05-mcpp-toml.md b/docs/zh/05-mcpp-toml.md index debbe968..2cbde30e 100644 --- a/docs/zh/05-mcpp-toml.md +++ b/docs/zh/05-mcpp-toml.md @@ -1791,7 +1791,13 @@ o.arg("./mkblob.sh").arg("blob.bin").arg("${mcpp.out_dir}/blob.o") 但 ldflags 是链接命令里的一串字符:没有任何东西跟踪它,改了它得到的是 `ninja: no work to do`。 -### 2.16 `[hooks]` —— 项目构建生命周期命令 +### 2.16 `[hooks]` —— 项目构建生命周期命令(实验性) + +> **实验性。** Hook 目前**不能**决定一次构建成功与否。所有 Hook 失败都以 +> **warning** 报出,`mcpp build` 保留它自己挣来的结果;`side_effect = true` +> 会被报错拒绝,而不是被采纳。这个键保留在 schema 里,这样今天写下的 manifest +> 在该功能转正时无需改动。另有两条限制是永久的、不是临时的:**只有根项目的 Hook +> 会执行**,而且**只有 `mcpp build` 会执行它们**。 Hook 是 `mcpp build` **在一段区间内持有**的命令,事件名就是那段区间: @@ -1807,7 +1813,7 @@ during_build = { cmd = "mcpp-hooks-audioplayer bgm", loop = true } # 可选;以下是默认值。 timeout_seconds = 10 enabled = true -side_effect = true +side_effect = false # 实验期内 `true` 会被拒绝 ``` | 键 | 类型 | 默认值 | 它命名的区间 | @@ -1818,7 +1824,7 @@ side_effect = true | `during_build` | 命令 | — | 构建开始前开启,构建结束后闭合 | | `timeout_seconds` | 整数,1–86400 | `10` | 单次运行的时限 | | `enabled` | 布尔 | `true` | 是否启用本表中的全部命令 | -| `side_effect` | 布尔 | `true` | Hook 失败是否让本次构建失败 | +| `side_effect` | 布尔 | `false` | Hook 失败是否让本次构建失败。**保留键**——实验期内只接受 `false` | 前三个区间是**自闭合**的——命令退出,区间就结束。"同步"在这里不是一种单独的模式, 它就是自闭合区间的样子。`during_build` 是唯一由别的东西闭合的区间,而那两个只对其中 @@ -1858,10 +1864,21 @@ build_start 命令无法启动、返回非零或超过时限均视为 Hook 失败。`during_build` 还多一种:开了 `loop` 的命令**起不来**——连续五次在一秒内以非零状态结束——就不再重启,并被报出来。(很快就 -成功结束的命令,正是 `loop` 被要求重复的那件事,不算失败。)`side_effect = false` 时 -mcpp 报 warning 并保留原构建结果;设为 `true` 时返回失败——但构建自身失败时保留它自己 -的退出码,所以 `mcpp build` 不会把一次编译错误报成通知程序的问题。Hook 自身失败不会再 -触发另一个 Hook。 +成功结束的命令,正是 `loop` 被要求重复的那件事,不算失败。)以上每一种都以 **warning** +报出,构建保留它自己挣来的结果——`[hooks]` 还在实验期,它没有投票权。Hook 自身失败不会 +再触发另一个 Hook。 + +改变这一点的正是 `side_effect = true`,而今天写它是一个错误: + +```text +error: mcpp.toml: error: [hooks].side_effect = true is not available yet: +[hooks] is experimental and cannot decide whether a build succeeded. … +``` + +是拒绝而不是悄悄降级,因为两种沉默的做法都更糟:采纳它等于让一个实验性功能对每一次 +构建都有否决权;忽略它则让项目以为自己的构建被通知程序把着关,而实际上没有。功能转正 +后,`true` 的含义是"Hook 失败让构建失败"——而构建自身失败时仍保留它自己的退出码,所以 +`mcpp build` 不会把一次编译错误报成通知程序的问题。 关于 `during_build` 有两件事值得单独知道: @@ -1878,8 +1895,9 @@ mcpp 报 warning 并保留原构建结果;设为 `true` 时返回失败——但 - Hook 属于**被构建的那个包**。workspace 展开时就是逐个成员:各自的 `[hooks]`、 各自的构建、各自的根目录。**虚拟** workspace 根(只有 `[workspace]` 没有 `[package]`)不构建任何东西,写在那里的 `[hooks]` 永不触发。 -- 依赖的 `[hooks]` 永不执行,只有根项目的会——装一个包不会把包作者的 Shell 命令 - 变成消费方构建的一部分。 +- 依赖的 `[hooks]` **一律跳过**,只有根项目的会执行。mcpp 解析的每一份 manifest 都 + 带着这一节,依赖的也带,而没有任何东西去读它——这正是"装一个包"不会变成"在我下次 + 构建时跑包作者的 Shell 命令"的原因。这是设计的性质,不是一个等着被打开的默认值。 - 声明了生效的 Hook 就等于让项目放弃空转快路径,因为 `build_start` 规定在准备阶段之后 执行。对已经是最新状态的带 Hook 项目,`mcpp build` 的代价是一次准备,而不是毫秒级。 @@ -1906,8 +1924,9 @@ side_effect = false deps = ["xim:mcpp-hooks-audioplayer@0.0.1"] ``` -构建全程的背景音乐,加上一段区分结果的提示音;`side_effect = false` 让缺少音频设备 -只是一条 warning,而不是一次失败的构建。 +构建全程的背景音乐,加上一段区分结果的提示音。`side_effect = false` 写出来而不是靠 +默认值:它是这份 manifest 自己就想要的值——缺个音频设备不该让构建失败——所以等这个键 +有了不止一个可接受的值之后,它仍然会这么写。 ## 附录 A. Schema 所有权原则(新字段准入标准) diff --git a/modules/manifest/src/toml.cppm b/modules/manifest/src/toml.cppm index d692e4bb..1c61e222 100644 --- a/modules/manifest/src/toml.cppm +++ b/modules/manifest/src/toml.cppm @@ -1651,6 +1651,26 @@ std::expected parse_string(std::string_view content, *out = it->second.as_bool(); } + // ⚠️ THE EXPERIMENTAL GATE, AND THE WHOLE OF IT. + // + // `[hooks]` is experimental, so it may not decide whether a build + // succeeded. Asking for `side_effect = true` is refused rather than + // downgraded, because the two possible silent behaviours are both + // worse than an error: honouring it ships an experimental feature with + // a veto over every build, and ignoring it leaves a project believing + // its build is gated on a notifier when nothing is. + // + // Everything under this line already implements both values. Deleting + // this block is what promoting the feature consists of. + if (m.hooks.sideEffect) + return std::unexpected(error(origin, + "[hooks].side_effect = true is not available yet: [hooks] is " + "experimental and cannot decide whether a build succeeded. A " + "failing hook is reported as a warning and the build keeps its " + "own result. Remove the key (the default is false) — it is " + "reserved so that manifests do not have to change when the " + "feature is promoted.")); + static constexpr std::string_view kKnownHookKeys[] = { "build_start", "build_failed", "build_finished", "during_build", "timeout_seconds", "enabled", "side_effect", diff --git a/modules/manifest/src/types.cppm b/modules/manifest/src/types.cppm index e53890cf..675957a4 100644 --- a/modules/manifest/src/types.cppm +++ b/modules/manifest/src/types.cppm @@ -971,9 +971,20 @@ struct Hooks { int timeoutSeconds = 10; // default for one run of a hook command bool enabled = true; // whole table - // Whether a hook failure fails the build. False downgrades it to a - // warning and preserves whatever the build itself returned. - bool sideEffect = true; + + // ⚠️ EXPERIMENTAL: FALSE, AND CURRENTLY THE ONLY VALUE. + // + // The key means "a hook failure fails the build". While `[hooks]` is + // experimental it does not get to decide that: a hook that fails is + // reported as a warning and the build keeps whatever result it earned on + // its own. `side_effect = true` is REJECTED by the parser rather than + // accepted and ignored — a project that believes its build is gated on a + // notifier, and is not, has been told something false. + // + // The mechanism below it is intact and is what the key will switch on when + // the feature graduates; the parser check is the whole of the gate, so + // removing it is the whole of the change. + bool sideEffect = false; // "This project has work for `mcpp build` to do." Distinct from `enabled`: // a table that only sets policy keys declares no command, and must leave diff --git a/src/hooks.cppm b/src/hooks.cppm index a7baa9cb..bd84c393 100644 --- a/src/hooks.cppm +++ b/src/hooks.cppm @@ -1,5 +1,10 @@ // mcpp.hooks — running the project's `[hooks]` lifecycle commands. // +// ⚠️ EXPERIMENTAL. A hook cannot currently change whether a build succeeded: +// every failure is a warning, and `side_effect = true` is refused by the +// manifest parser. Only the ROOT project's hooks are ever run — a dependency's +// `[hooks]` is inert. See docs/05-mcpp-toml.md §2.16. +// // The CONFIGURATION is not parsed here: `[hooks]` is a section of mcpp.toml // and mcpp.toml has one parser (mcpp.manifest). What lives here is the part // that is policy rather than grammar — when a command runs, what a failure @@ -72,7 +77,7 @@ private: bool declared_ = false; bool started_ = false; bool closed_ = false; - bool sideEffect_ = true; + bool sideEffect_ = false; bool ok_ = true; std::atomic stop_{false}; std::atomic gaveUp_{false}; @@ -125,6 +130,13 @@ event_command(const mcpp::manifest::Hooks& hooks, Event event) { // four of them (cannot start, non-zero, timed out, failed to stay up). The // message is identical either way; only its severity and the build's fate // differ. +// +// ⚠️ WHILE `[hooks]` IS EXPERIMENTAL, `sideEffect` IS ALWAYS FALSE — the +// manifest parser refuses `side_effect = true` (see +// modules/manifest/src/toml.cppm). The `true` branch below is therefore +// unreachable today ON PURPOSE: it is the behaviour the key will select when +// the feature is promoted, and keeping it here means promotion is a deletion +// in the parser rather than a reconstruction here. bool report_hook_failure_flag(bool sideEffect, const std::string& message) { if (sideEffect) { mcpp::ui::error(message); diff --git a/tests/e2e/317_project_build_hooks.sh b/tests/e2e/317_project_build_hooks.sh index 8df04ae2..0b4e6613 100644 --- a/tests/e2e/317_project_build_hooks.sh +++ b/tests/e2e/317_project_build_hooks.sh @@ -64,7 +64,7 @@ build_failed = "echo failed>>hooks.log" build_finished = "echo finished>>hooks.log" timeout_seconds = 10 enabled = true -side_effect = true +side_effect = false EOF # Invoked BELOW the project root: a hook's relative paths belong to the root, @@ -94,7 +94,12 @@ cat > src/main.cpp <<'EOF' int main() { return 0; } EOF -# ── side_effect = false: reported, but the build keeps its own result ──── +# ── A failing hook is REPORTED and the build keeps its own result ─────── +# +# While `[hooks]` is experimental it may not decide whether a build succeeded. +# The build below succeeds with a hook that cannot run at all, and the two +# assertions are both needed: the exit code says the hook had no vote, and the +# warning says it was not silently swallowed. write_manifest <<'EOF' [package] name = "hookprobe" @@ -102,15 +107,22 @@ version = "0.1.0" [hooks] build_finished = "mcpp-hook-command-that-does-not-exist" -side_effect = false EOF "$MCPP" build > ignored-hook-failure.log 2>&1 || { cat ignored-hook-failure.log - echo "FAIL: side_effect=false changed build success"; exit 1; } + echo "FAIL: a failing hook changed the build result"; exit 1; } grep -q "warning: hook 'build_finished' exited with status" ignored-hook-failure.log || { - cat ignored-hook-failure.log; echo "FAIL: ignored hook failure was not reported"; exit 1; } + cat ignored-hook-failure.log; echo "FAIL: the hook failure was not reported"; exit 1; } +# `if grep` rather than `grep && { }`: a NEGATIVE assertion written with `&&` +# leaves the list's exit status as grep's, and reading that under `set -e` is +# an argument this file should not be having. +if grep -q "error: hook 'build_finished'" ignored-hook-failure.log; then + cat ignored-hook-failure.log + echo "FAIL: an experimental hook failure was reported as an error"; exit 1 +fi -# ── The same failure is fatal under the default side_effect = true ────── +# The explicit spelling of the current behaviour keeps working, so a manifest +# does not have to change when the feature is promoted. write_manifest <<'EOF' [package] name = "hookprobe" @@ -118,13 +130,32 @@ version = "0.1.0" [hooks] build_finished = "mcpp-hook-command-that-does-not-exist" +side_effect = false +EOF +"$MCPP" build > explicit-false.log 2>&1 || { + cat explicit-false.log; echo "FAIL: side_effect=false was not accepted"; exit 1; } + +# ── `side_effect = true` is REFUSED, not quietly downgraded ───────────── +# +# Honouring it would give an experimental feature a veto over every build; +# ignoring it would leave the project believing its build is gated on a +# notifier when nothing is. Both silent options are worse than an error. +write_manifest <<'EOF' +[package] +name = "hookprobe" +version = "0.1.0" + +[hooks] +build_finished = "echo done" +side_effect = true EOF rc=0 -"$MCPP" build > fatal-hook-failure.log 2>&1 || rc=$? +"$MCPP" build > side-effect-true.log 2>&1 || rc=$? [[ $rc -ne 0 ]] || { - cat fatal-hook-failure.log; echo "FAIL: fatal hook failure returned success"; exit 1; } -grep -q "error: hook 'build_finished' exited with status" fatal-hook-failure.log || { - cat fatal-hook-failure.log; echo "FAIL: fatal hook failure was not reported"; exit 1; } + cat side-effect-true.log; echo "FAIL: side_effect=true was accepted"; exit 1; } +grep -q "experimental" side-effect-true.log || { + cat side-effect-true.log + echo "FAIL: the refusal does not say why"; exit 1; } # ── enabled = false switches off a table that still names commands ────── write_manifest <<'EOF' @@ -164,10 +195,11 @@ version = "0.1.0" build_finished = "$slow_command" timeout_seconds = 1 EOF -rc=0 -"$MCPP" build > timeout.log 2>&1 || rc=$? -[[ $rc -ne 0 ]] || { cat timeout.log; echo "FAIL: timed-out hook returned success"; exit 1; } -expect_line timeout.log "error: hook 'build_finished' timed out after 1s" +"$MCPP" build > timeout.log 2>&1 || { + cat timeout.log; echo "FAIL: a timed-out hook changed the build result"; exit 1; } +# The bound is real even though the build survives it: without the timeout the +# command would have run for five seconds, and the line below would be absent. +expect_line timeout.log "warning: hook 'build_finished' timed out after 1s" # ── An invalid value fails the manifest, before any command runs ──────── # @@ -337,11 +369,12 @@ version = "0.1.0" build_start = "$pause_2s" during_build = { cmd = "mcpp-hook-command-that-does-not-exist", loop = true } EOF -rc=0 -"$MCPP" build > loop-giveup.log 2>&1 || rc=$? -[[ $rc -ne 0 ]] || { - cat loop-giveup.log; echo "FAIL: a during_build that never ran was not fatal"; exit 1; } -grep -q "error: hook 'during_build' failed to stay up" loop-giveup.log || { +# The build still succeeds — an experimental hook has no vote — but giving up +# has to be VISIBLE, or a silent spin and a silent stop look identical. +"$MCPP" build > loop-giveup.log 2>&1 || { + cat loop-giveup.log + echo "FAIL: a during_build that never ran changed the build result"; exit 1; } +grep -q "warning: hook 'during_build' failed to stay up" loop-giveup.log || { cat loop-giveup.log; echo "FAIL: giving up was not reported"; exit 1; } # ── `loop` on a self-closing event is refused, not ignored ────────────── diff --git a/tests/unit/test_manifest.cpp b/tests/unit/test_manifest.cpp index 0625a3c9..5b0b1f9a 100644 --- a/tests/unit/test_manifest.cpp +++ b/tests/unit/test_manifest.cpp @@ -3994,7 +3994,39 @@ TEST(ManifestHooks, AbsentSectionIsInertButNotDisabled) { EXPECT_TRUE(m->hooks.enabled); EXPECT_FALSE(m->hooks.active()); EXPECT_EQ(m->hooks.timeoutSeconds, 10); - EXPECT_TRUE(m->hooks.sideEffect); + // Experimental: a hook may not decide whether a build succeeded, so this + // defaults to false and is the only value the parser accepts. + EXPECT_FALSE(m->hooks.sideEffect); +} + +// ⚠️ The experimental gate. Refused rather than downgraded: honouring it would +// give an experimental feature a veto over every build, and ignoring it would +// leave the project believing its build is gated on a notifier when nothing +// is. Both silent options are worse than the error. +TEST(ManifestHooks, SideEffectTrueIsRefusedWhileTheFeatureIsExperimental) { + auto m = parse_with_hooks(R"( +[hooks] +build_finished = "echo done" +side_effect = true +)"); + ASSERT_FALSE(m.has_value()); + EXPECT_NE(m.error().message.find("experimental"), std::string::npos) + << m.error().message; + EXPECT_NE(m.error().message.find("side_effect"), std::string::npos) + << m.error().message; +} + +// The key stays in the vocabulary: writing the value that IS the current +// behaviour must keep working, so a manifest does not have to change when the +// feature is promoted. +TEST(ManifestHooks, SideEffectFalseIsAcceptedSoTheKeyStaysReserved) { + auto m = parse_with_hooks(R"( +[hooks] +build_finished = "echo done" +side_effect = false +)"); + ASSERT_TRUE(m.has_value()) << (m ? "" : m.error().format()); + EXPECT_FALSE(m->hooks.sideEffect); } TEST(ManifestHooks, CommandsAndPolicyAreRead) {