Skip to content

Commit f651ed8

Browse files
committed
fix(actions): ninja 转义与未解析目标 —— 自审第二轮
## 1. action 命令里的 `$` 会被 ninja 吃掉 命令 token 只做了 shell 引号,没做 ninja 转义。**引号救不了它**:ninja 在 shell 被 调用之前就会展开 `$foo`,所以带字面 `$` 的 token(含 `$` 的路径、 `-Wl,-rpath,$ORIGIN`、一段 awk 程序)会被当成变量引用。 改成本文件其余地方一直用的那个配对(与 `include_dir_token` 同序):**先 ninja 转义, 再 shell 引号**。对普通 token 无影响 —— `escape_ninja_chars` 只碰空格 / `$` / `:`, 而 `shell_quote_arg` 对不含元字符的串逐字节原样返回。 这里每个元素**按构造就是一个 argv token**(类型化 builder 一次追加一个),这正是 逐 token 引号成立的前提 —— #331 表明手工拼出来的 flag blob **不**满足这个前提。 顺带把 `escape_ninja_chars` 从匿名 namespace 移出并导出:它此前是内部的,而 ninja_backend 需要它。让它继续内部化就意味着 ninja 转义规则第四份手写副本,而那 正是它们会漂的原因。 ## 2. `${mcpp.target_file:拼错}` 静默变成空串 未解析的引擎变量原本会被替换成空字符串,于是生成一条路径为空的边,ninja 在离 错误很远的地方报出来。现在是**硬错误**,并列出本次构建里存在的 target,还提示 「被 required_features 门住的 target 在那些 feature 未激活时不存在」。 ## 测试 188 补两条:字面 `$` 能原样到达工具;未知 target 引用报错并**点名**。 两条放在自己的最小工程里 —— `app` 的 main.cpp 故意依赖生成出来的符号,在它上面 换掉 build.mcpp 会让**链接**失败,那说明不了这两条在测什么(我第一版就是这么写的)。 验证:单测 57/57;13 个 e2e 全绿,含先前红的 35_workspace / 120_ws_root_indices。
1 parent fa5e914 commit f651ed8

4 files changed

Lines changed: 114 additions & 7 deletions

File tree

src/build/flags.cppm

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,14 @@ std::string atomic_link_flag(const std::vector<std::filesystem::path>& linkDirs,
109109
// escaped as `\"`) — cmd.exe/CreateProcess argv convention.
110110
std::string shell_quote_arg(std::string_view arg);
111111

112+
// Ninja's own escaping for a value that will sit on a `command = ` line:
113+
// ` `, `$` and `:` get a leading `$`. Exported because it is needed WITH
114+
// shell_quote_arg, not instead of it — quoting stops the SHELL from splitting
115+
// a token, but ninja expands `$foo` before the shell is ever invoked, so a
116+
// token carrying a literal `$` needs both. Callers apply ninja escaping first,
117+
// then shell quoting (see include_dir_token).
118+
std::string escape_ninja_chars(std::string_view s);
119+
112120
// One include-directory token, fully prepared for a ninja command line:
113121
// dialect prefix, ninja `$` escaping, and shell quoting — in that order.
114122
//
@@ -146,16 +154,15 @@ std::string include_token(const mcpp::toolchain::CommandDialect& d,
146154

147155
namespace mcpp::build {
148156

149-
namespace {
150-
151-
std::filesystem::path staged_std_bmi_path(const BuildPlan& plan) {
152-
return mcpp::toolchain::staged_std_bmi_path(plan.toolchain, plan.outputDir);
153-
}
154-
155157
// Escape a string for embedding in ninja rule strings. Takes the text, not a
156158
// path: round-tripping through std::filesystem::path would re-normalize the
157159
// separators on Windows, which silently undoes a caller that deliberately
158160
// chose generic_string() for a response-file token (#261).
161+
//
162+
// Deliberately OUTSIDE the anonymous namespace below: it is declared in this
163+
// module's export block so ninja_backend can pair it with shell_quote_arg for
164+
// action command tokens. Leaving it internal would mean a fourth hand-written
165+
// copy of ninja's escaping rules, which is how they drift.
159166
std::string escape_ninja_chars(std::string_view s) {
160167
std::string out;
161168
out.reserve(s.size());
@@ -167,6 +174,12 @@ std::string escape_ninja_chars(std::string_view s) {
167174
return out;
168175
}
169176

177+
namespace {
178+
179+
std::filesystem::path staged_std_bmi_path(const BuildPlan& plan) {
180+
return mcpp::toolchain::staged_std_bmi_path(plan.toolchain, plan.outputDir);
181+
}
182+
170183
// Escape a path for embedding in ninja rule strings (native separators).
171184
std::string escape_path(const std::filesystem::path& p) {
172185
return escape_ninja_chars(p.string());

src/build/ninja_backend.cppm

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1314,7 +1314,21 @@ std::string emit_ninja_string(const BuildPlan& plan) {
13141314
std::string cmd;
13151315
for (auto const& tok : a.command) {
13161316
if (!cmd.empty()) cmd += ' ';
1317-
cmd += shell_quote_arg(tok);
1317+
// BOTH escapes, in the order the rest of this file uses
1318+
// (include_dir_token does the same): ninja first, shell second.
1319+
// Shell-quoting alone is not enough — a literal `$` in a token
1320+
// (a path containing one, or an argument like `-Wl,-rpath,$ORIGIN`)
1321+
// is a VARIABLE REFERENCE to ninja, and quoting does not stop
1322+
// ninja from expanding it before the shell ever sees it.
1323+
//
1324+
// Safe for ordinary tokens: escape_ninja_chars only touches
1325+
// ` `, `$` and `:`, and shell_quote_arg returns anything without a
1326+
// metacharacter byte-for-byte, so a plain `--cpp_out=gen` is
1327+
// unchanged. Each element here is exactly one argv token by
1328+
// construction (the typed builder appends them one at a time),
1329+
// which is the assumption per-token quoting needs and which #331
1330+
// showed is NOT true of hand-assembled flag blobs.
1331+
cmd += shell_quote_arg(escape_ninja_chars(tok));
13181332
}
13191333
append(std::format("rule mcpp_action_{}\n", i));
13201334
append(std::format(" command = {}\n", cmd));

src/build/prepare.cppm

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4644,6 +4644,11 @@ prepare_build(bool print_fingerprint,
46444644
// what makes an action portable (Windows has no shell to assume) and
46454645
// cacheable (nothing can smuggle in ambient state).
46464646
{
4647+
// An engine variable that resolves to nothing must be an ERROR, not an
4648+
// empty string: `${mcpp.target_file:tpyo}` would otherwise silently
4649+
// become an edge with a blank path, and ninja reports that far away
4650+
// from the typo that caused it.
4651+
std::set<std::string> unresolvedTargets;
46474652
auto substitute = [&](std::string s) {
46484653
auto rep = [&](std::string_view what, const std::string& with) {
46494654
for (std::size_t p; (p = s.find(what)) != std::string::npos; )
@@ -4668,6 +4673,7 @@ prepare_build(bool print_fingerprint,
46684673
for (auto const& lu : ctx.plan.linkUnits)
46694674
if (lu.targetName == name)
46704675
resolved = lu.output.generic_string();
4676+
if (resolved.empty()) unresolvedTargets.insert(name);
46714677
s.replace(p, close - p + 1, resolved);
46724678
}
46734679
return s;
@@ -4683,6 +4689,19 @@ prepare_build(bool print_fingerprint,
46834689
collect(*m);
46844690
for (std::size_t i = 1; i < packages.size(); ++i)
46854691
collect(packages[i].manifest);
4692+
if (!unresolvedTargets.empty()) {
4693+
std::string bad, known;
4694+
for (auto const& n : unresolvedTargets) bad += (bad.empty() ? "" : ", ") + n;
4695+
for (auto const& lu : ctx.plan.linkUnits)
4696+
known += (known.empty() ? "" : ", ") + lu.targetName;
4697+
return std::unexpected(std::format(
4698+
"build.mcpp action references unknown target(s) via "
4699+
"${{mcpp.target_file:...}}: {}\n"
4700+
" targets in this build: [{}]\n"
4701+
" (a target gated by required_features is absent unless those "
4702+
"features are active)",
4703+
bad, known.empty() ? std::string("none") : known));
4704+
}
46864705
}
46874706
ctx.plan.stdCompatBmiPath = stdCompatBmiPath;
46884707
ctx.plan.stdCompatObjectPath = stdCompatObjectPath;

tests/e2e/188_build_actions.sh

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,67 @@ if "$MCPP" build > b4.log 2>&1; then
144144
fi
145145
rm -f FAIL_THE_CHECK
146146

147+
# ── 3b/3c: their own minimal project ───────────────────────────────────────
148+
# Separate from `app`, whose main.cpp deliberately depends on the generated
149+
# symbol — swapping its build.mcpp out would fail the LINK and say nothing
150+
# about what these two are actually testing.
151+
mkdir -p "$TMP/edge/src"
152+
cd "$TMP/edge"
153+
cat > mcpp.toml <<'EOF'
154+
[package]
155+
name = "edge"
156+
version = "0.1.0"
157+
EOF
158+
printf 'int main() {}\n' > src/main.cpp
159+
160+
# ── 3b. a literal `$` in a command survives to the tool ────────────────────
161+
# Shell-quoting alone does not save it: ninja expands `$foo` BEFORE the shell
162+
# runs, so a token carrying a `$` (a path containing one, `-Wl,-rpath,$ORIGIN`,
163+
# an awk program) needs ninja escaping too.
164+
cat > dollar.sh <<'EOF'
165+
#!/usr/bin/env bash
166+
# $1 must arrive containing a literal dollar sign
167+
case "$1" in *'$'*) : > "$2";; *) echo "lost the dollar: [$1]" >&2; exit 1;; esac
168+
EOF
169+
chmod +x dollar.sh
170+
cat > build.mcpp <<'EOF'
171+
#include <cstdio>
172+
#include <string>
173+
import mcpp;
174+
int main() {
175+
const std::string root = mcpp::manifest_dir();
176+
mcpp::action a;
177+
a.id = "dollar"; a.role = "check";
178+
a.arg((root + "/dollar.sh").c_str()).arg("-Wl,-rpath,$ORIGIN")
179+
.arg("${mcpp.out_dir}/dollar.stamp")
180+
.output("${mcpp.out_dir}/dollar.stamp")
181+
.submit();
182+
}
183+
EOF
184+
rm -rf target
185+
"$MCPP" build > b3b.log 2>&1 || {
186+
cat b3b.log; echo "FAIL: a literal \$ in an action command did not survive"; exit 1; }
187+
188+
# ── 3c. an unknown target reference is an error, not an empty path ─────────
189+
cat > build.mcpp <<'EOF'
190+
#include <cstdio>
191+
import mcpp;
192+
int main() {
193+
mcpp::action a;
194+
a.id = "bad-ref"; a.role = "artifact";
195+
a.arg("/bin/true").arg("${mcpp.target_file:no_such_target}")
196+
.input("${mcpp.target_file:no_such_target}")
197+
.output("${mcpp.out_dir}/x.out")
198+
.submit();
199+
}
200+
EOF
201+
rm -rf target
202+
if "$MCPP" build > b3c.log 2>&1; then
203+
cat b3c.log; echo "FAIL: an unknown target reference was accepted"; exit 1
204+
fi
205+
grep -q "no_such_target" b3c.log || {
206+
cat b3c.log; echo "FAIL: error does not name the unknown target"; exit 1; }
207+
147208
# ── 4. a malformed action is refused, not skipped ──────────────────────────
148209
cat > build.mcpp <<'EOF'
149210
#include <cstdio>

0 commit comments

Comments
 (0)