Skip to content

Commit 1244c1c

Browse files
committed
review: one parser for mcpp.toml, and a cmd.exe line cmd.exe accepts
Follow-up to the [hooks] feature. Five things the first shape got wrong, in descending order of how loudly they fail. 1. Windows: the hook never ran. `run_shell_deadline` built the command line with `windows_command_from_argv({"cmd.exe","/d","/s","/c", command})`, which quotes every token — so the switches stopped being switches and the command arrived carrying a quote pair cmd.exe does not consume: '"echo start>>hooks.log' is not recognized as an internal or external command That is #425 one layer up (`cmd.exe /c` does not use CreateProcess argument quoting). A shell command now has its own host-independent shaper, `windows_shell_command_line` — bare switches plus the single outer pair /s strips — and its own tests in test_windows_command_line.cpp, which compile on every platform. The Windows branch is unreachable on the machines this is developed on; a unit test is the only thing that can fail there first. 2. `[hooks]` is a section of mcpp.toml, so mcpp.manifest parses it. The feature came with a second, independent reader of the same file. It cost a redundant parse on every build, and it reported ITS syntax errors in ITS vocabulary: any manifest typo, anywhere in the file, came out as `error: invalid hook configuration: ...` for every project whether or not it used hooks. `Manifest::hooks` now carries the config, unknown keys warn (and error under --strict) like every other section, and mcpp.hooks is left with the part that is policy rather than grammar. 3. A hook no longer moves mcpp's working directory. The launchers already carry a per-child cwd (posix_spawn_file_actions_addchdir_np, lpCurrentDirectory); `run_shell_deadline` takes one, and the chdir-and-restore dance around the call is gone. 4. The fast-path veto moved into try_fast_build, next to the manifest read that answers it — cmd_build no longer loads the manifest a second time to ask whether it may call a function that loads it again. 5. Coverage the claims did not have: workspace members (each member's own hooks, in that member's root, root manifest silent), preparation failing fires nothing, an unknown key warns and the known ones still run, and the e2e compares hook logs by CONTENT — cmd.exe writes CRLF, so the previous `$'start\nfinished'` comparison could not have passed on Windows even with the command line fixed. Plus seven manifest tests for the grammar. Docs (en + zh) state what was implicit: which commands run hooks and which deliberately do not, what a virtual workspace root does, that an active hook opts the project out of the fast path, that preparation failure is silent, and that a `[hooks]` table is executable content in a repository you may have just cloned.
1 parent 32437da commit 1244c1c

11 files changed

Lines changed: 620 additions & 205 deletions

File tree

‎docs/05-mcpp-toml.md‎

Lines changed: 41 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2120,9 +2120,11 @@ side_effect = true
21202120
| `enabled` | bool | `true` | Enables all commands in this table |
21212121
| `side_effect` | bool | `true` | Whether a hook failure makes the overall build fail |
21222122

2123-
Commands run synchronously in the current project's root directory through the
2124-
host shell (`/bin/sh` or `cmd.exe`). Standard input/output/error keep their
2125-
ordinary terminal behaviour. Missing event commands are skipped.
2123+
Commands run synchronously through the host shell (`/bin/sh` or `cmd.exe`),
2124+
with the **project root** as their working directory — not the directory
2125+
`mcpp build` was typed in, so a relative path in a hook means the same thing
2126+
wherever the build was started. Standard input/output/error keep their ordinary
2127+
terminal behaviour. Missing event commands are skipped.
21262128

21272129
The lifecycle is:
21282130

@@ -2132,11 +2134,42 @@ build_start
21322134
└─ build fails → build_failed
21332135
```
21342136

2135-
`build_failed` and `build_finished` are mutually exclusive. A hook command
2136-
that cannot start, returns non-zero, or exceeds its timeout is a hook failure.
2137-
With `side_effect = false`, mcpp reports a warning and preserves the build's
2138-
result; with `true`, it returns failure. A hook's own failure does not trigger
2139-
another hook.
2137+
`build_failed` and `build_finished` are mutually exclusive, and both are
2138+
reachable only after `build_start` has run. A project that cannot be *prepared*
2139+
— an invalid manifest, an unresolvable dependency, no usable toolchain — fires
2140+
nothing: it has not started building, and its hook program may be exactly what
2141+
preparation would have installed.
2142+
2143+
A hook command that cannot start, returns non-zero, or exceeds its timeout is a
2144+
hook failure. With `side_effect = false`, mcpp reports a warning and preserves
2145+
the build's result; with `true`, it returns failure — but a build that failed on
2146+
its own keeps its own exit code, so `mcpp build` never reports a compile error
2147+
as a notifier problem. A hook's own failure does not trigger another hook.
2148+
2149+
Scope, precisely:
2150+
2151+
- Only `mcpp build` runs hooks. `mcpp run`, `mcpp test` and
2152+
`mcpp build --configure-only` build too, and deliberately do not.
2153+
- Hooks belong to the **package being built**. In a workspace fan-out that is
2154+
each member in turn — its own `[hooks]`, around its own build, in its own
2155+
root. A *virtual* workspace root (`[workspace]` with no `[package]`) builds
2156+
nothing, so a `[hooks]` table there never fires.
2157+
- A dependency's `[hooks]` is never run. Only the root project's — installing a
2158+
package cannot make its author's shell command part of your build.
2159+
- Declaring an active hook opts the project out of the no-op fast path, because
2160+
`build_start` is specified to run after preparation. Expect `mcpp build` on an
2161+
already-current hooked project to cost a preparation pass rather than
2162+
milliseconds.
2163+
2164+
An unrecognised key in `[hooks]` is a warning (an error under `--strict`), so a
2165+
manifest written for a newer mcpp still loads. An unrecognised *value* — a
2166+
non-string command, a non-positive `timeout_seconds` — is a manifest error.
2167+
2168+
> **A hook is code, and `mcpp.toml` is part of the repository.** Building a
2169+
> project you have just cloned runs whatever its `[hooks]` say, with your
2170+
> privileges. This is the same trust `build.mcpp` already asks for
2171+
> ([07 — build.mcpp](07-build-mcpp.md)), and it is worth knowing you are
2172+
> extending it.
21402173
21412174
Hook programs can be installed as ordinary xlings dependencies. For example,
21422175
an audio notifier can keep its sound files inside its own executable rather

‎docs/zh/05-mcpp-toml.md‎

Lines changed: 31 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1816,8 +1816,9 @@ side_effect = true
18161816
| `enabled` | 布尔 | `true` | 是否启用本表中的全部命令 |
18171817
| `side_effect` | 布尔 | `true` | Hook 失败是否让本次构建失败 |
18181818

1819-
命令在当前项目根目录中同步执行,使用宿主 Shell(`/bin/sh` 或 `cmd.exe`),标准输入、
1820-
输出和错误沿用普通终端行为。没有配置的事件直接跳过。
1819+
命令通过宿主 Shell(`/bin/sh` 或 `cmd.exe`)同步执行,工作目录是**项目根目录**——
1820+
不是敲 `mcpp build` 的那个目录,所以 Hook 里的相对路径在哪儿发起构建都指同一处。
1821+
标准输入、输出和错误沿用普通终端行为。没有配置的事件直接跳过。
18211822

18221823
生命周期为:
18231824

@@ -1827,9 +1828,34 @@ build_start
18271828
└─ 构建失败 → build_failed
18281829
```
18291830

1830-
`build_failed` 与 `build_finished` 互斥。命令无法启动、返回非零或超过时限均视为
1831-
Hook 失败。`side_effect = false` 时 mcpp 报 warning 并保留原构建结果;设为 `true`
1832-
时返回失败。Hook 自身失败不会再触发另一个 Hook。
1831+
`build_failed` 与 `build_finished` 互斥,而且两者都只在 `build_start` 已经执行之后
1832+
才可达。项目**准备**阶段就失败的情况——manifest 非法、依赖无法解析、没有可用工具链
1833+
——一个 Hook 都不触发:此时构建尚未开始,而 Hook 程序本身可能正是准备阶段要装的东西。
1834+
1835+
命令无法启动、返回非零或超过时限均视为 Hook 失败。`side_effect = false` 时 mcpp 报
1836+
warning 并保留原构建结果;设为 `true` 时返回失败——但构建自身失败时保留它自己的退出码,
1837+
所以 `mcpp build` 不会把一次编译错误报成通知程序的问题。Hook 自身失败不会再触发另一个
1838+
Hook。
1839+
1840+
作用范围:
1841+
1842+
- 只有 `mcpp build` 执行 Hook。`mcpp run`、`mcpp test` 和
1843+
`mcpp build --configure-only` 同样会构建,但有意不执行。
1844+
- Hook 属于**被构建的那个包**。workspace 展开时就是逐个成员:各自的 `[hooks]`、
1845+
各自的构建、各自的根目录。**虚拟** workspace 根(只有 `[workspace]` 没有
1846+
`[package]`)不构建任何东西,写在那里的 `[hooks]` 永不触发。
1847+
- 依赖的 `[hooks]` 永不执行,只有根项目的会——装一个包不会把包作者的 Shell 命令
1848+
变成你构建的一部分。
1849+
- 声明了生效的 Hook 就等于让项目放弃空转快路径,因为 `build_start` 规定在准备阶段之后
1850+
执行。对已经是最新状态的带 Hook 项目,`mcpp build` 的代价是一次准备,而不是毫秒级。
1851+
1852+
`[hooks]` 里不认识的**键**是 warning(`--strict` 下为错误),所以为更新版 mcpp 写的
1853+
manifest 在这一版仍能加载;不认识的**值**——命令不是字符串、`timeout_seconds` 非正数
1854+
——是 manifest 错误。
1855+
1856+
> **Hook 是代码,而 `mcpp.toml` 是仓库的一部分。** 构建一个刚克隆下来的项目,会以你的
1857+
> 权限执行它 `[hooks]` 里写的任何东西。这与 `build.mcpp`
1858+
> ([07 — build.mcpp](07-build-mcpp.md))已经要求的信任是同一份,值得知道你正在把它扩大。
18331859
18341860
Hook 程序可以作为普通 xlings 依赖安装。例如,音频通知程序可以把音频内置进自己的
18351861
可执行文件,无需让 mcpp 处理媒体资源:

‎modules/manifest/src/toml.cppm‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1520,6 +1520,81 @@ std::expected<Manifest, ManifestError> parse_string(std::string_view content,
15201520
}
15211521
}
15221522

1523+
// [hooks] — project build lifecycle commands (#496). Parsed HERE rather
1524+
// than by the module that runs them, for the reason Appendix A of
1525+
// docs/05-mcpp-toml.md states: mcpp.toml has one grammar and one parser.
1526+
// A second reader of the same file would report ITS syntax errors in its
1527+
// own vocabulary — a typo in [package] arriving as "invalid hook
1528+
// configuration" — and would sit outside the warning/--strict policy every
1529+
// other section is subject to.
1530+
if (auto* hooksValue = doc->get("hooks");
1531+
hooksValue && !hooksValue->is_table()) {
1532+
return std::unexpected(error(origin,
1533+
"[hooks] must be a table of lifecycle commands"));
1534+
}
1535+
if (auto* ht = doc->get_table("hooks")) {
1536+
// Values are the author's own and visible in front of them: a wrong
1537+
// type is an error, not a silent default. An unrecognised KEY is a
1538+
// warning (--strict makes it an error), same split as [build] — so a
1539+
// manifest written for a later mcpp still loads on this one.
1540+
auto read_command = [&](std::string_view key, std::string& out)
1541+
-> std::optional<ManifestError> {
1542+
auto it = ht->find(key);
1543+
if (it == ht->end()) return std::nullopt;
1544+
if (!it->second.is_string() || it->second.as_string().empty())
1545+
return error(origin, std::format(
1546+
"[hooks].{} must be a non-empty command string", key));
1547+
out = it->second.as_string();
1548+
return std::nullopt;
1549+
};
1550+
for (auto [key, out] : std::initializer_list<
1551+
std::pair<std::string_view, std::string*>>{
1552+
{"build_start", &m.hooks.buildStart},
1553+
{"build_failed", &m.hooks.buildFailed},
1554+
{"build_finished", &m.hooks.buildFinished}}) {
1555+
if (auto e = read_command(key, *out)) return std::unexpected(*e);
1556+
}
1557+
1558+
if (auto it = ht->find("timeout_seconds"); it != ht->end()) {
1559+
// Bounded above as well as below: the value becomes a
1560+
// std::chrono::seconds deadline, and "a timeout so large it is not
1561+
// one" is a mistake worth naming rather than honouring.
1562+
constexpr std::int64_t kMaxTimeout = 24 * 60 * 60;
1563+
if (!it->second.is_int() || it->second.as_int() <= 0
1564+
|| it->second.as_int() > kMaxTimeout)
1565+
return std::unexpected(error(origin, std::format(
1566+
"[hooks].timeout_seconds must be a positive integer "
1567+
"(seconds, at most {})", kMaxTimeout)));
1568+
m.hooks.timeoutSeconds = static_cast<int>(it->second.as_int());
1569+
}
1570+
1571+
for (auto [key, out] : std::initializer_list<
1572+
std::pair<std::string_view, bool*>>{
1573+
{"enabled", &m.hooks.enabled},
1574+
{"side_effect", &m.hooks.sideEffect}}) {
1575+
auto it = ht->find(key);
1576+
if (it == ht->end()) continue;
1577+
if (!it->second.is_bool())
1578+
return std::unexpected(error(origin, std::format(
1579+
"[hooks].{} must be a boolean", key)));
1580+
*out = it->second.as_bool();
1581+
}
1582+
1583+
static constexpr std::string_view kKnownHookKeys[] = {
1584+
"build_start", "build_failed", "build_finished",
1585+
"timeout_seconds", "enabled", "side_effect",
1586+
};
1587+
for (auto& [k, _] : *ht) {
1588+
bool known = false;
1589+
for (auto kk : kKnownHookKeys) if (k == kk) { known = true; break; }
1590+
if (!known)
1591+
m.schemaWarnings.push_back(std::format(
1592+
"[hooks] has unsupported key '{}' (ignored). Keys: "
1593+
"build_start, build_failed, build_finished, "
1594+
"timeout_seconds, enabled, side_effect.", k));
1595+
}
1596+
}
1597+
15231598
// [lib] — library root convention (cargo-style).
15241599
if (auto v = doc->get_string("lib.path")) {
15251600
m.lib.path = *v;

‎modules/manifest/src/types.cppm‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -919,6 +919,37 @@ struct WorkspaceConfig {
919919
bool present = false;
920920
};
921921

922+
// `[hooks]` — project build lifecycle commands (#496).
923+
//
924+
// The commands are host-shell strings written by the project author, run by
925+
// `mcpp build` around the build it performs. See docs/05-mcpp-toml.md §2.16.
926+
//
927+
// ⚠️ ONLY THE ROOT PROJECT'S HOOKS ARE EVER RUN. Every manifest mcpp parses
928+
// carries this field, including a DEPENDENCY's — and `mcpp build` reaches the
929+
// invoker (mcpp.hooks) with the root project's manifest alone. A dependency
930+
// that declares hooks is inert by construction, which is the only reason
931+
// `mcpp add` of a third-party package does not become "run their shell
932+
// command on my next build". Anything that adds a second call site inherits
933+
// that responsibility.
934+
struct Hooks {
935+
std::string buildStart;
936+
std::string buildFailed;
937+
std::string buildFinished;
938+
int timeoutSeconds = 10; // per command
939+
bool enabled = true; // whole table
940+
// Whether a hook failure fails the build. False downgrades it to a
941+
// warning and preserves whatever the build itself returned.
942+
bool sideEffect = true;
943+
944+
// "This project has work for `mcpp build` to do." Distinct from `enabled`:
945+
// a table that only sets policy keys declares no command, and must leave
946+
// the build path it would otherwise divert (the fast path) untouched.
947+
bool active() const {
948+
return enabled && !(buildStart.empty() && buildFailed.empty()
949+
&& buildFinished.empty());
950+
}
951+
};
952+
922953
// [profile.<name>] — bundled build settings (opt level, debug, lto, strip).
923954
struct Profile {
924955
std::string optLevel = "2";
@@ -985,6 +1016,7 @@ struct Manifest {
9851016
Resources resources; // [resources] (mcpp#365)
9861017
RuntimeConfig runtimeConfig;
9871018
XlingsConfig xlings; // [xlings] build environment (L-1)
1019+
Hooks hooks; // [hooks] lifecycle commands (#496)
9881020
std::vector<ConditionalConfig> conditionalConfigs; // [target.'cfg(...)'.build], deferred
9891021
std::map<std::string, Profile> profiles; // [profile.<name>]
9901022
// [features] — feature name → implied features ("default" = default set).

‎modules/platform/src/process.cppm‎

Lines changed: 63 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -112,11 +112,22 @@ int run_exec_deadline(const std::vector<std::string>& argv,
112112
std::chrono::milliseconds deadline,
113113
bool* timed_out);
114114

115-
// Run one host-shell command with inherited stdio and a real deadline.
116-
// POSIX uses /bin/sh; Windows uses cmd.exe. This is for user-authored command
117-
// strings such as project hooks — programmatic launches should keep using the
118-
// argv-based run_exec_deadline API above.
115+
// Run one host-shell command with inherited stdio, a working directory and a
116+
// real deadline. POSIX uses /bin/sh; Windows uses cmd.exe. This is for
117+
// user-authored command strings such as project hooks — programmatic launches
118+
// keep using the argv-based run_exec_deadline API above.
119+
//
120+
// `cwd` is where the command runs; empty means "inherit ours". It is a
121+
// PARAMETER rather than something the caller arranges with a chdir: the
122+
// process-wide working directory is shared state, and the launchers underneath
123+
// already carry a per-child cwd (posix_spawn_file_actions_addchdir_np /
124+
// CreateProcess's lpCurrentDirectory).
125+
//
126+
// Returns 127 when the shell itself could not be started — the same code a
127+
// shell uses for a command it cannot find, and never confusable with a
128+
// command that ran.
119129
int run_shell_deadline(std::string_view command,
130+
std::string_view cwd,
120131
std::chrono::milliseconds deadline,
121132
bool* timed_out);
122133

@@ -168,6 +179,23 @@ int extract_exit_code(int raw_status);
168179
std::string windows_command_from_argv(const std::vector<std::string>& argv);
169180
std::string windows_wrap_for_cmd_c(std::string_view cmd);
170181

182+
// The command line that runs a USER-AUTHORED shell command through cmd.exe.
183+
//
184+
// ⚠️ NOT `windows_command_from_argv({"cmd.exe", "/d", "/s", "/c", command})`.
185+
// That shape is for a program plus its argv, where CreateProcess's parsing is
186+
// what has to be satisfied. cmd.exe is not parsed that way: its switches must
187+
// arrive BARE (quoted, they are no longer switches), and the command tail is
188+
// governed by the /C quote rule above rather than by argv quoting — so an
189+
// argv-quoted command arrives carrying a pair cmd does not consume. That is
190+
// #425 one layer up, and it is why this is its own shape:
191+
//
192+
// cmd.exe /d /s /c "<command>"
193+
//
194+
// /s makes the rule unconditional (strip exactly the outer pair), so the
195+
// command reaches cmd verbatim no matter how many quotes it contains; /d skips
196+
// AutoRun so a user's registry-installed shell hook cannot alter it.
197+
std::string windows_shell_command_line(std::string_view command);
198+
171199
} // namespace mcpp::platform::process
172200

173201
// ─── Implementation ──────────────────────────────────────────────────────
@@ -189,6 +217,12 @@ std::string windows_wrap_for_cmd_c(std::string_view cmd) {
189217
return "\"" + std::string(cmd) + "\"";
190218
}
191219

220+
std::string windows_shell_command_line(std::string_view command) {
221+
// One derivation for the outer pair: the same wrap the /c rule above is
222+
// written against.
223+
return "cmd.exe /d /s /c " + windows_wrap_for_cmd_c(command);
224+
}
225+
192226
namespace {
193227

194228
// Append a non-interactive stdin redirect to prevent child processes from
@@ -593,12 +627,17 @@ struct BoundedOutcome {
593627
// `capture == false` runs the child on the caller's stdio: live output, and a
594628
// real terminal for anything that checks. `run_exec_deadline` needs that; the
595629
// capturing variants need the pipe.
630+
// `windowsCommandLine` overrides what the Windows branch launches. Empty (the
631+
// normal case) means "derive it from argv". A shell command is the one caller
632+
// that must NOT be derived that way — see windows_shell_command_line — and the
633+
// POSIX branch is unaffected either way, because it never flattens argv.
596634
BoundedOutcome dispatch_bounded(
597635
const std::vector<std::string>& argv,
598636
const std::vector<std::pair<std::string, std::string>>& extraEnv,
599637
std::string_view cwd,
600638
std::chrono::milliseconds deadline,
601-
bool capture)
639+
bool capture,
640+
std::string_view windowsCommandLine = {})
602641
{
603642
BoundedOutcome outcome;
604643

@@ -625,7 +664,9 @@ BoundedOutcome dispatch_bounded(
625664
: nullptr;
626665

627666
if constexpr (mcpp::platform::is_windows) {
628-
const auto cmd = windows_command_from_argv(argv);
667+
const auto cmd = windowsCommandLine.empty()
668+
? windows_command_from_argv(argv)
669+
: std::string(windowsCommandLine);
629670
auto r = mcpp::platform::winproc::capture_with_deadline(
630671
cmd.c_str(), envArg, envCount, cwdArg, ms, sink, &outcome.output);
631672
outcome.supported = r.supported;
@@ -667,15 +708,25 @@ int run_exec_deadline(const std::vector<std::string>& argv,
667708
}
668709

669710
int run_shell_deadline(std::string_view command,
711+
std::string_view cwd,
670712
std::chrono::milliseconds deadline,
671713
bool* timed_out)
672714
{
673-
std::vector<std::string> argv;
674-
if constexpr (mcpp::platform::is_windows)
675-
argv = {"cmd.exe", "/d", "/s", "/c", std::string(command)};
676-
else
677-
argv = {"/bin/sh", "-c", std::string(command)};
678-
return run_exec_deadline(argv, {}, deadline, timed_out);
715+
if (timed_out) *timed_out = false;
716+
if (command.empty() || deadline.count() <= 0) return 127;
717+
718+
// argv is what the POSIX branch launches; the Windows branch takes the
719+
// shaped command line instead. Both are built here so neither platform's
720+
// spelling can drift into a launcher that does not use it.
721+
const std::vector<std::string> argv{"/bin/sh", "-c", std::string(command)};
722+
auto r = dispatch_bounded(argv, {}, cwd, deadline, /*capture=*/false,
723+
windows_shell_command_line(command));
724+
// No fallback to the unbounded launcher here, unlike run_exec_deadline: a
725+
// hook's deadline and its working directory are both part of what the
726+
// caller asked for, and the unbounded path can honour neither.
727+
if (!r.supported) return 127;
728+
if (timed_out) *timed_out = r.timed_out;
729+
return r.exit_code;
679730
}
680731

681732
RunResult capture_exec_deadline(

0 commit comments

Comments
 (0)