Skip to content

Commit 661a7d1

Browse files
committed
fix(build): #390 修复补全 —— 摄入点收敛 + emitter 兜底 + 合并去重自愈
按 review(#391) 补全同一决策的全部推导点,不再依赖「所有摄入点都被找全」: - P1: 补上 plan.cppm expand_manifest_include_entry(绝对分支与 generated/ 裸拼接回退)、scanner.cppm rewrite_rel_copy(cxxflags 的 -Ithird_party/inc 通道)、flags.cppm [build] include_dirs 全局 cxxflags 通道;并在 emit_compile_commands 加最后一层兜底:file/directory/-c/-o/-I 统一 make_preferred,对 CDB 契约给出无条件保证。 - P2: merge_compile_commands 去重键改为归一化路径(lexically_normal + make_preferred),旧 CDB 里的混合分隔符条目与 fresh 原生拼写视为同一 文件 → 升级后第一次 build 即自愈,用户无需手删 compile_commands.json。 已在带旧条目的工程上实测:注入混合条目后重建,归零。 - P3: e2e 76 的 Windows 断言改为平台无关的「同时含 \ 与 / 即失败」 (消掉 os.name 依赖,避免 MSYS python 假绿);新增 extra.cpp 必须进 CDB 的 grep 守卫;python3 缺失时显式 SKIP。 - native_path_from_generic 改用标准库 make_preferred;include_dirs 绝对 分支不再做 generic_string 窄串往返(ANSI 代码页拼不出的名字会抛, mcpp#230)。 - 单测:NormalizedFileKeysHealStaleSeparatorSpellings(合并自愈)、 EmittedPathsUseNativeSeparators(emitter 兜底)、 Plan.ExpandManifestIncludeEntryNativeSpelling(plan.cppm 摄入点, expand_manifest_include_entry 为此从匿名命名空间提出并导出)。 注:directives::abs_against 归一化会改写 build.mcpp 指令路径的拼写, 声明输入指纹一次性失效 → 一次多余重建,属预期。
1 parent be3d36e commit 661a7d1

11 files changed

Lines changed: 273 additions & 60 deletions

src/build/compile_commands.cppm

Lines changed: 34 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -142,16 +142,28 @@ std::vector<std::string> split_flags(std::string_view s) {
142142

143143
namespace {
144144

145+
// The CDB's path contract: NATIVE separators, unconditionally. Every
146+
// ingestion point (manifest globs, include_dirs, build.mcpp directives) is
147+
// normalized at the source, but this is the LAST line — a path that slips
148+
// through with a mixed `root\a/b` spelling (MSVC keeps input `/` verbatim)
149+
// breaks CLion, and no amount of "all ingestion points are covered" can be
150+
// proven. make_preferred() is a no-op on POSIX.
151+
std::string native_string(const std::filesystem::path& p) {
152+
auto n = p;
153+
n.make_preferred();
154+
return n.string();
155+
}
156+
145157
std::vector<std::string> local_include_args(const CompileUnit& cu) {
146158
std::vector<std::string> args;
147159
args.reserve(cu.localIncludeDirs.size());
148160
for (auto const& inc : cu.localIncludeDirs) {
149-
args.push_back("-I" + inc.string());
161+
args.push_back("-I" + native_string(inc));
150162
}
151163
// #249: after-dirs keep their -idirafter spelling in the compile DB so
152164
// tooling (clangd) reproduces the compiler's search order.
153165
for (auto const& inc : cu.localIncludeDirsAfter) {
154-
args.push_back("-idirafter" + inc.string());
166+
args.push_back("-idirafter" + native_string(inc));
155167
}
156168
return args;
157169
}
@@ -186,7 +198,7 @@ std::string emit_compile_commands(const BuildPlan& plan, const CompileFlags& fla
186198
: isCSource ? flags.cc
187199
: flags.cxx;
188200

189-
auto output_path = (plan.outputDir / cu.object).string();
201+
auto output_path = native_string(plan.outputDir / cu.object);
190202

191203
// Build arguments array.
192204
nlohmann::json args = nlohmann::json::array();
@@ -198,13 +210,13 @@ std::string emit_compile_commands(const BuildPlan& plan, const CompileFlags& fla
198210
for (auto& f : package_flag_args(cu, isCSource))
199211
args.push_back(std::move(f));
200212
args.push_back("-c");
201-
args.push_back(cu.source.string());
213+
args.push_back(native_string(cu.source));
202214
args.push_back("-o");
203215
args.push_back(output_path);
204216

205217
nlohmann::json entry;
206-
entry["directory"] = plan.projectRoot.string();
207-
entry["file"] = cu.source.string();
218+
entry["directory"] = native_string(plan.projectRoot);
219+
entry["file"] = native_string(cu.source);
208220
entry["arguments"] = std::move(args);
209221
entry["output"] = output_path;
210222

@@ -222,11 +234,25 @@ std::string merge_compile_commands(
222234
if (freshJ.is_discarded() || !freshJ.is_array())
223235
return std::string(fresh);
224236

237+
// Dedup key = the file's PATH, spelled the way a fresh plan spells it
238+
// (native separators). A prior CDB written before the mixed-separator
239+
// fix (#390) carries `root\generated/modules\x.cppm` entries that are
240+
// the SAME file as the fresh `root\generated\modules\x.cppm` — a literal
241+
// string comparison would keep both and the user's upgrade would not
242+
// visibly fix anything. Normalizing makes the merge self-healing: the
243+
// stale mixed entry is skipped on the first `mcpp build` after upgrade.
244+
// fileExists still probes the raw spelling — Windows accepts both.
245+
auto norm_key = [](std::string_view f) {
246+
auto p = std::filesystem::path(std::string(f)).lexically_normal();
247+
p.make_preferred();
248+
return p.string();
249+
};
250+
225251
// Files the current plan already covers — those entries are authoritative.
226252
std::set<std::string> freshFiles;
227253
for (auto const& e : freshJ) {
228254
if (e.contains("file") && e["file"].is_string())
229-
freshFiles.insert(e["file"].get<std::string>());
255+
freshFiles.insert(norm_key(e["file"].get<std::string>()));
230256
}
231257

232258
// Keep fresh order, then append still-valid prior entries the plan doesn't
@@ -238,7 +264,7 @@ std::string merge_compile_commands(
238264
for (auto const& e : existingJ) {
239265
if (!e.contains("file") || !e["file"].is_string()) continue;
240266
auto f = e["file"].get<std::string>();
241-
if (freshFiles.contains(f)) continue; // fresh wins
267+
if (freshFiles.contains(norm_key(f))) continue; // fresh wins
242268
if (!fileExists(std::filesystem::path(f))) continue; // pruned
243269
merged.push_back(e);
244270
}

src/build/flags.cppm

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -317,7 +317,13 @@ CompileFlags compute_flags(const BuildPlan& plan) {
317317
// once ninja hands the resolved command line to the shell.
318318
std::vector<std::string> includeTokens;
319319
for (auto& inc : plan.manifest.buildConfig.includeDirs) {
320-
std::filesystem::path p = inc.has_root_path() ? inc : (plan.projectRoot / inc);
320+
// make_preferred: a multi-segment TOML entry like `generated/inc`
321+
// keeps its `/` on MSVC, and the bare `projectRoot / inc` join would
322+
// be MIXED — reaching both the ninja command line and the CDB's
323+
// arguments (via f.cxx → split_flags). Same rule as every other
324+
// manifest-path ingestion point (#390); no-op on POSIX.
325+
auto p = inc.has_root_path() ? inc : (plan.projectRoot / inc);
326+
p.make_preferred();
321327
includeTokens.push_back(include_token(d, p));
322328
}
323329
// #249: `[build] include_dirs_after` — searched AFTER the toolchain's
@@ -327,8 +333,8 @@ CompileFlags compute_flags(const BuildPlan& plan) {
327333
// (documented degradation; clang-MSVC uses the gnu dialect).
328334
const bool msvcInclude = d.includePrefix == std::string_view("/I");
329335
for (auto& inc : plan.manifest.buildConfig.includeDirsAfter) {
330-
std::filesystem::path ip(inc);
331-
std::filesystem::path p = ip.has_root_path() ? ip : (plan.projectRoot / ip);
336+
auto p = inc.has_root_path() ? inc : (plan.projectRoot / inc);
337+
p.make_preferred();
332338
includeTokens.push_back(
333339
include_token(d, p, msvcInclude ? "/I" : "-idirafter"));
334340
}

src/build/plan.cppm

Lines changed: 33 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -220,6 +220,14 @@ make_plan(const mcpp::manifest::Manifest& manifest,
220220
// simply makes those units uncacheable.
221221
const std::vector<std::filesystem::path>& storeRoots = {});
222222

223+
// Expand one manifest `include_dirs` entry against the project root — the
224+
// #249 consistency join + the expand_dir_glob the dep path uses. Exported
225+
// (like modgraph's glob_literal_prefix) so unit tests can assert its
226+
// native-separator contract directly; see the definition below.
227+
std::vector<std::filesystem::path>
228+
expand_manifest_include_entry(const std::filesystem::path& root,
229+
const std::filesystem::path& inc);
230+
223231
} // namespace mcpp::build
224232

225233
namespace mcpp::build {
@@ -368,22 +376,42 @@ std::vector<std::string> shared_library_link_flags(
368376
return flags;
369377
}
370378

379+
} // namespace
380+
371381
// #249 consistency fix: expand include_dirs entries with the same
372382
// `expand_dir_glob` the dep path (prepare.cppm) uses, so a main-manifest
373383
// `include_dirs = ["*/include"]` glob works identically here. For a literal
374384
// (wildcard-free) entry expand_dir_glob only returns EXISTING directories,
375385
// whereas this helper historically joined unconditionally — keep the plain
376386
// join as a fallback so an -I for a dir created later (e.g. by a build
377387
// step) isn't silently dropped.
388+
//
389+
// Deliberately OUTSIDE the anonymous namespace: it is exported for its unit
390+
// test (like modgraph's glob_literal_prefix), and the two
391+
// local_include_dirs_*_for_manifest consumers below ride along so a single
392+
// namespace split serves the whole trio.
378393
std::vector<std::filesystem::path>
379394
expand_manifest_include_entry(const std::filesystem::path& root,
380395
const std::filesystem::path& inc)
381396
{
382-
if (inc.is_absolute()) return { inc };
397+
if (inc.is_absolute()) {
398+
// A TOML value like `C:/SDL2/include` keeps its `/` on MSVC — make
399+
// it native so the CDB's -I (via local_include_args) is uniform.
400+
auto n = inc;
401+
n.make_preferred();
402+
return { std::move(n) };
403+
}
383404
const auto glob = inc.generic_string();
384405
auto expanded = mcpp::modgraph::expand_dir_glob(root, glob);
385-
if (expanded.empty() && glob.find('*') == std::string::npos)
386-
expanded.push_back(root / inc);
406+
if (expanded.empty() && glob.find('*') == std::string::npos) {
407+
// Same native-spelling rule for the bare join (see above): `root / p`
408+
// with a multi-segment `generated/inc` is MIXED on MSVC, and this
409+
// fallback exists precisely for dirs like `generated/` that a later
410+
// build step creates — the #390 shape.
411+
auto joined = root / inc;
412+
joined.make_preferred();
413+
expanded.push_back(std::move(joined));
414+
}
387415
return expanded;
388416
}
389417

@@ -412,6 +440,8 @@ local_include_dirs_after_for_manifest(const std::filesystem::path& root,
412440
return dirs;
413441
}
414442

443+
namespace {
444+
415445
void append_unique_path(std::vector<std::filesystem::path>& out,
416446
std::filesystem::path path)
417447
{

src/build/prepare.cppm

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2997,11 +2997,13 @@ prepare_build(bool print_fingerprint,
29972997
std::vector<std::filesystem::path> dirs;
29982998
for (auto const& inc : manifest.buildConfig.includeDirs) {
29992999
if (inc.is_absolute()) {
3000-
// Native spelling (see native_path_from_generic): a TOML
3001-
// `C:/SDL2/include` stays mixed on MSVC and leaks into the
3002-
// CDB's -I otherwise.
3003-
appendUniquePath(dirs,
3004-
mcpp::modgraph::native_path_from_generic(inc.generic_string()));
3000+
// Native spelling: a TOML `C:/SDL2/include` stays mixed on
3001+
// MSVC and leaks into the CDB's -I otherwise. Direct
3002+
// make_preferred — no generic_string round trip, which can
3003+
// throw for names the ANSI codepage cannot spell (mcpp#230).
3004+
auto n = inc;
3005+
n.make_preferred();
3006+
appendUniquePath(dirs, std::move(n));
30053007
continue;
30063008
}
30073009
for (auto& dir : mcpp::modgraph::expand_dir_glob(
@@ -3021,8 +3023,9 @@ prepare_build(bool print_fingerprint,
30213023
std::vector<std::filesystem::path> dirs;
30223024
for (auto const& inc : manifest.buildConfig.includeDirsAfter) {
30233025
if (inc.is_absolute()) {
3024-
appendUniquePath(dirs,
3025-
mcpp::modgraph::native_path_from_generic(inc.generic_string()));
3026+
auto n = inc;
3027+
n.make_preferred();
3028+
appendUniquePath(dirs, std::move(n));
30263029
continue;
30273030
}
30283031
for (auto& dir : mcpp::modgraph::expand_dir_glob(

src/modgraph/glob.cppm

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -24,16 +24,12 @@ export namespace mcpp::modgraph {
2424
// to parse. Ninja never notices because it renders everything via
2525
// generic_string(); the CDB is the first `.string()` consumer.
2626
//
27-
// POSIX is untouched (its native separator already is `/`). Replacing only
28-
// `/` is also safe for already-native Windows input: it never contains `/`.
27+
// POSIX is untouched (`make_preferred()` is a no-op there, and it is also
28+
// safe for already-native Windows input, which never contains `/`).
2929
std::filesystem::path native_path_from_generic(std::string_view s) {
30-
constexpr char kSep = std::filesystem::path::preferred_separator;
31-
if (kSep == '/') return std::filesystem::path(s);
32-
std::string p(s);
33-
for (auto& c : p) {
34-
if (c == '/') c = kSep;
35-
}
36-
return std::filesystem::path(std::move(p));
30+
std::filesystem::path p(s);
31+
p.make_preferred();
32+
return p;
3733
}
3834

3935
// Does `candidate` match `glob`, interpreted relative to `root`?

src/modgraph/scanner.cppm

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -501,10 +501,18 @@ namespace {
501501

502502
// has_root_path: leave absolute AND root-relative ("/x" on Windows)
503503
// spellings alone — only genuinely root-less paths are project-relative.
504+
// Both branches normalize to NATIVE separators: a `-Ithird_party/inc` cxxflag
505+
// would otherwise come back as `C:\proj\third_party/inc` on MSVC (path keeps
506+
// the input `/` verbatim) and reach the CDB's arguments via packageCxxflags.
504507
std::string rewrite_rel_copy(const std::string& p, const std::filesystem::path& root) {
505508
std::filesystem::path fp(p);
506-
if (fp.has_root_path()) return p;
507-
return (root / fp).string();
509+
if (fp.has_root_path()) {
510+
fp.make_preferred();
511+
return fp.string();
512+
}
513+
auto joined = root / fp;
514+
joined.make_preferred();
515+
return joined.string();
508516
}
509517

510518
void rewrite_rel(std::string& p, const std::filesystem::path& root) {
@@ -691,8 +699,12 @@ local_include_dirs_for(const std::filesystem::path& root,
691699
if (inc.is_absolute()) {
692700
// A TOML value like `C:/SDL2/include` keeps its `/` on MSVC —
693701
// normalize so the CDB's -I comes out native (mixed separators
694-
// break CLion). See mcpp::modgraph::native_path_from_generic.
695-
dirs.push_back(native_path_from_generic(inc.generic_string()));
702+
// break CLion). Direct make_preferred, no generic_string round
703+
// trip: the narrow conversion can throw for names the ANSI
704+
// codepage cannot spell (mcpp#230).
705+
auto n = inc;
706+
n.make_preferred();
707+
dirs.push_back(std::move(n));
696708
continue;
697709
}
698710
for (auto& d : expand_dir_glob(root, inc.generic_string())) {
@@ -711,7 +723,9 @@ local_include_dirs_after_for(const std::filesystem::path& root,
711723
std::vector<std::filesystem::path> dirs;
712724
for (auto const& inc : manifest.buildConfig.includeDirsAfter) {
713725
if (inc.is_absolute()) {
714-
dirs.push_back(native_path_from_generic(inc.generic_string()));
726+
auto n = inc;
727+
n.make_preferred();
728+
dirs.push_back(std::move(n));
715729
continue;
716730
}
717731
for (auto& d : expand_dir_glob(root, inc.generic_string())) {

tests/e2e/76_compile_commands_generated.sh

Lines changed: 20 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -55,26 +55,36 @@ grep -qE '"command"|"arguments"' "$cdb" || {
5555
# The minimal project's source (src/main.cpp) must have an entry.
5656
grep -q 'main\.cpp' "$cdb" || { echo "FAIL: $cdb has no entry for src/main.cpp"; cat "$cdb"; exit 1; }
5757

58+
# The multi-segment glob must ACTUALLY have contributed an entry — if the
59+
# glob silently missed, every separator assertion below is vacuous green.
60+
grep -q 'extra\.cpp' "$cdb" || {
61+
echo "FAIL: $cdb has no entry for generated/modules/extra.cpp — multi-segment glob missed"
62+
cat "$cdb"; exit 1
63+
}
64+
5865
# Deeper structural validation when a JSON parser is available (GitHub-hosted
59-
# runners ship python3). Skips cleanly where it isn't, keeping the grep checks
60-
# above as the portable baseline.
66+
# runners ship python3). Explicitly reports the skip where it isn't, so a
67+
# silent pass can never masquerade as validation coverage.
6168
if command -v python3 >/dev/null 2>&1; then
6269
python3 - "$cdb" <<'PY' || exit 1
63-
import json, sys, os
70+
import json, sys
6471
d = json.load(open(sys.argv[1], encoding="utf-8"))
6572
assert isinstance(d, list) and d, "CDB must be a non-empty JSON array"
6673
for e in d:
6774
assert "file" in e and "directory" in e, "entry missing file/directory: %r" % e
6875
assert ("command" in e) or ("arguments" in e), "entry missing command/arguments: %r" % e
69-
# Native separators on Windows: a multi-segment manifest glob used to
70-
# yield MIXED `root\generated/modules\x.cppm` file paths (MSVC's path
71-
# keeps the `/` from the glob prefix), which CLion refuses to parse.
72-
# Ninja hides the problem (it renders generic_string()); the CDB is
73-
# the .string() consumer.
74-
if os.name == "nt" and "/" in e["file"]:
75-
raise AssertionError("file must use native separators on Windows: %r" % e["file"])
76+
# Platform-independent mixed-separator check: the #390 bug spelled a
77+
# Windows file as `root\generated/modules\x.cppm` (MSVC's path keeps the
78+
# `/` from the manifest glob prefix, and the directory walk propagates
79+
# it). On POSIX a backslash never appears in a path, so the assertion is
80+
# trivially true there and catches exactly the bug on Windows — no
81+
# os.name / platform sniffing needed.
82+
f = e["file"]
83+
assert not ("\\" in f and "/" in f), "mixed separators in file: %r" % f
7684
print(" json validation OK (%d entries)" % len(d))
7785
PY
86+
else
87+
echo "SKIP: python3 not on PATH — JSON validation not run"
7888
fi
7989

8090
echo "OK"

tests/unit/test_build_flags.cpp

Lines changed: 23 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -97,35 +97,51 @@ TEST(BuildFlagsAtomic, StaticLinkEmittedWhenArchivePresent) {
9797
// BOTH the joined spelling (`-iquotehdr`) and the separated spelling
9898
// (`-isystem` followed by a standalone next element). All four of these
9999
// project-relative paths must resolve to the same "/proj/hdr" target.
100+
//
101+
// The expected spelling is NATIVE (#390): `-I/abs/hdr` written with forward
102+
// slashes keeps them on MSVC, and the old expectation `(root / "hdr").string()`
103+
// was itself the mixed `"/proj\hdr"` shape this family of bugs produced.
104+
// make_preferred() makes the expectation platform-correct on both sides.
100105
TEST(BuildFlags, NormalizeIncludeFlagsRewritesFullIncludeFamily) {
101106
std::filesystem::path root = "/proj";
107+
auto expected = [](const std::filesystem::path& p) {
108+
auto n = p;
109+
n.make_preferred();
110+
return n.string();
111+
};
102112
std::vector<std::string> flags = {
103113
"-Ihdr", "-iquotehdr", "-isystem", "hdr", "-idirafterhdr",
104114
};
105115

106116
mcpp::modgraph::normalize_include_flags(root, flags);
107117

108118
ASSERT_EQ(flags.size(), 5u);
109-
EXPECT_EQ(flags[0], "-I" + (root / "hdr").string());
110-
EXPECT_EQ(flags[1], "-iquote" + (root / "hdr").string());
119+
EXPECT_EQ(flags[0], "-I" + expected(root / "hdr"));
120+
EXPECT_EQ(flags[1], "-iquote" + expected(root / "hdr"));
111121
EXPECT_EQ(flags[2], "-isystem"); // prefix itself untouched
112-
EXPECT_EQ(flags[3], (root / "hdr").string()); // separated element rewritten
113-
EXPECT_EQ(flags[4], "-idirafter" + (root / "hdr").string());
122+
EXPECT_EQ(flags[3], expected(root / "hdr")); // separated element rewritten
123+
EXPECT_EQ(flags[4], "-idirafter" + expected(root / "hdr"));
114124
}
115125

116126
// Absolute paths and root-relative spellings are left alone (matches the
117-
// pre-#226 -I behavior), for both the joined and separated forms.
127+
// pre-#226 -I behavior), for both the joined and separated forms — only the
128+
// separator spelling is normalized to native (#390; a no-op on POSIX).
118129
TEST(BuildFlags, NormalizeIncludeFlagsLeavesAbsolutePathsAlone) {
119130
std::filesystem::path root = "/proj";
131+
auto expected = [](const std::filesystem::path& p) {
132+
auto n = p;
133+
n.make_preferred();
134+
return n.string();
135+
};
120136
std::vector<std::string> flags = {
121137
"-I/abs/hdr", "-isystem", "/abs/hdr", "-DKEEP",
122138
};
123139

124140
mcpp::modgraph::normalize_include_flags(root, flags);
125141

126-
EXPECT_EQ(flags[0], "-I/abs/hdr");
142+
EXPECT_EQ(flags[0], "-I" + expected("/abs/hdr"));
127143
EXPECT_EQ(flags[1], "-isystem");
128-
EXPECT_EQ(flags[2], "/abs/hdr");
144+
EXPECT_EQ(flags[2], expected("/abs/hdr"));
129145
EXPECT_EQ(flags[3], "-DKEEP");
130146
}
131147

0 commit comments

Comments
 (0)