From be3d36e57b8a08554f395748eeb7e8d5b54f42df Mon Sep 17 00:00:00 2001 From: Zhe Feng Date: Sat, 8 Aug 2026 21:01:01 +0800 Subject: [PATCH 1/3] =?UTF-8?q?fix(build):=20=E5=A4=9A=E6=AE=B5=20glob=20?= =?UTF-8?q?=E6=BA=90=E6=96=87=E4=BB=B6=E8=B7=AF=E5=BE=84=E5=9C=A8=20Window?= =?UTF-8?q?s=20=E4=B8=8A=E4=BF=9D=E6=8C=81=E5=8E=9F=E7=94=9F=E5=88=86?= =?UTF-8?q?=E9=9A=94=E7=AC=A6=20(#390)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit manifest glob(如 `generated/modules/**/*.cppm`)用 `/` 拼路径; MSVC 的 std::filesystem::path 保留输入分隔符原样,于是 `root / prefix` 与目录迭代子路径都是混合形式 (`...\9bca0b44ae3aa660\generated/modules\ccc.when.cppm`), 一路流入 CompileUnit::source → compile_commands.json 的 file/-c 字段, CLion 拒绝解析。ninja 侧一律 generic_string()(全正斜杠)所以构建正常, CDB 是第一个 .string() 消费者,缺陷只在那里显现。 新增 mcpp::modgraph::native_path_from_generic(glob.cppm),在 glob_literal_prefix / expand_dir_glob / scan_one_into / 绝对 include dir 分支 / directives::abs_against 摄入点归一化;顺带修复 build.mcpp 指令 路径与 TOML 绝对 include dir(C:/SDL2/include)同类混合问题。 单测:GlobLiteralPrefixUsesNativeSeparators / NativePathFromGeneric / ExpandGlobMultiSegmentUsesNativeSeparators / ExpandDirGlobMultiSegmentUsesNativeSeparators。 e2e:76 增加多段 glob 源 + Windows 原生分隔符断言;47 增加无残留引号断言。 --- src/build/directives.cppm | 5 +- src/build/prepare.cppm | 10 ++- src/modgraph/glob.cppm | 24 +++++++ src/modgraph/scanner.cppm | 23 +++++-- tests/e2e/47_cdb_prebuilt_module_path_abs.sh | 21 ++++++ tests/e2e/76_compile_commands_generated.sh | 24 ++++++- tests/unit/test_modgraph.cpp | 69 +++++++++++++++++++- 7 files changed, 166 insertions(+), 10 deletions(-) diff --git a/src/build/directives.cppm b/src/build/directives.cppm index 85fda563..41014758 100644 --- a/src/build/directives.cppm +++ b/src/build/directives.cppm @@ -366,7 +366,10 @@ const Def* find_by_tag(std::string_view tag) { } std::string abs_against(const fs::path& base, std::string_view p) { - fs::path pp(p); + // Native spelling (see mcpp::modgraph::native_path_from_generic): a + // directive path like `generated/modules/x` would otherwise stay mixed + // on MSVC and leak into include flags / the CDB. + fs::path pp = mcpp::modgraph::native_path_from_generic(p); if (pp.is_relative()) pp = base / pp; return pp.lexically_normal().string(); } diff --git a/src/build/prepare.cppm b/src/build/prepare.cppm index 0d61b018..2f7acdba 100644 --- a/src/build/prepare.cppm +++ b/src/build/prepare.cppm @@ -16,6 +16,7 @@ import mcpp.platform.axis; import mcpp.libs.json; import mcpp.log; import mcpp.manifest; +import mcpp.modgraph.glob; import mcpp.modgraph.graph; import mcpp.modgraph.scanner; import mcpp.modgraph.validate; @@ -2996,7 +2997,11 @@ prepare_build(bool print_fingerprint, std::vector dirs; for (auto const& inc : manifest.buildConfig.includeDirs) { if (inc.is_absolute()) { - appendUniquePath(dirs, inc); + // Native spelling (see native_path_from_generic): a TOML + // `C:/SDL2/include` stays mixed on MSVC and leaks into the + // CDB's -I otherwise. + appendUniquePath(dirs, + mcpp::modgraph::native_path_from_generic(inc.generic_string())); continue; } for (auto& dir : mcpp::modgraph::expand_dir_glob( @@ -3016,7 +3021,8 @@ prepare_build(bool print_fingerprint, std::vector dirs; for (auto const& inc : manifest.buildConfig.includeDirsAfter) { if (inc.is_absolute()) { - appendUniquePath(dirs, inc); + appendUniquePath(dirs, + mcpp::modgraph::native_path_from_generic(inc.generic_string())); continue; } for (auto& dir : mcpp::modgraph::expand_dir_glob( diff --git a/src/modgraph/glob.cppm b/src/modgraph/glob.cppm index a2262735..73a04b9a 100644 --- a/src/modgraph/glob.cppm +++ b/src/modgraph/glob.cppm @@ -12,6 +12,30 @@ import std; export namespace mcpp::modgraph { +// Convert a manifest-style path or glob prefix (always spelled with the +// generic `/` separator) to the platform's native spelling. +// +// MSVC's std::filesystem::path preserves the separators of the string it +// was constructed from instead of normalizing them, so wrapping a raw +// `generated/modules` in a path and joining it with `root / p` yields the +// MIXED `C:\...\generated/modules` — and the directory-walk children built +// on top of that stay mixed. `.string()` then carries the mixed form into +// `compile_commands.json` (its `file` / `-c` fields), which CLion refuses +// to parse. Ninja never notices because it renders everything via +// generic_string(); the CDB is the first `.string()` consumer. +// +// POSIX is untouched (its native separator already is `/`). Replacing only +// `/` is also safe for already-native Windows input: it never contains `/`. +std::filesystem::path native_path_from_generic(std::string_view s) { + constexpr char kSep = std::filesystem::path::preferred_separator; + if (kSep == '/') return std::filesystem::path(s); + std::string p(s); + for (auto& c : p) { + if (c == '/') c = kSep; + } + return std::filesystem::path(std::move(p)); +} + // Does `candidate` match `glob`, interpreted relative to `root`? // // Supports "**" (any number of directory levels) and "*" (within one segment). diff --git a/src/modgraph/scanner.cppm b/src/modgraph/scanner.cppm index 6ae3232e..2f3755c6 100644 --- a/src/modgraph/scanner.cppm +++ b/src/modgraph/scanner.cppm @@ -268,7 +268,12 @@ std::filesystem::path glob_literal_prefix(std::string_view glob) { ? glob : glob.substr(0, wildcard); auto slash = literal.find_last_of('/'); if (slash == std::string_view::npos) return {}; - return std::filesystem::path(literal.substr(0, slash)); + // Native separators, not the raw generic form: MSVC keeps the input's + // `/` verbatim, and `root / p` plus the directory walk then propagate a + // MIXED `root\generated/modules` into every downstream path — which is + // what `compile_commands.json`'s `file` field showed on Windows for + // multi-segment globs. See mcpp::modgraph::native_path_from_generic. + return native_path_from_generic(literal.substr(0, slash)); } // mcpp#228: `{a,b}` alternation, recursively. Finds the first top-level `{`, @@ -442,7 +447,9 @@ std::vector expand_dir_glob(const std::filesystem::path& // expand_glob) — include_dirs entries are meant to name one literal // directory each; a caller wanting alternatives lists multiple entries. if (glob.find('*') == std::string_view::npos) { - auto p = root / std::filesystem::path(glob); + // Native spelling (see native_path_from_generic — a raw `a/b` would + // come back mixed from .string() on MSVC). + auto p = root / native_path_from_generic(glob); if (std::filesystem::is_directory(p, ec)) out.push_back(p); return out; } @@ -682,7 +689,10 @@ local_include_dirs_for(const std::filesystem::path& root, std::vector dirs; for (auto const& inc : manifest.buildConfig.includeDirs) { if (inc.is_absolute()) { - dirs.push_back(inc); + // A TOML value like `C:/SDL2/include` keeps its `/` on MSVC — + // normalize so the CDB's -I comes out native (mixed separators + // break CLion). See mcpp::modgraph::native_path_from_generic. + dirs.push_back(native_path_from_generic(inc.generic_string())); continue; } for (auto& d : expand_dir_glob(root, inc.generic_string())) { @@ -701,7 +711,7 @@ local_include_dirs_after_for(const std::filesystem::path& root, std::vector dirs; for (auto const& inc : manifest.buildConfig.includeDirsAfter) { if (inc.is_absolute()) { - dirs.push_back(inc); + dirs.push_back(native_path_from_generic(inc.generic_string())); continue; } for (auto& d : expand_dir_glob(root, inc.generic_string())) { @@ -738,7 +748,10 @@ void scan_one_into(ScanResult& result, // Literal absolute entry — e.g. a dependency build.mcpp's OUT_DIR // generated source, which lives OUTSIDE the (possibly read-only) // package root. No glob expansion; taken as-is when it exists. - if (std::filesystem::path gp(g); gp.is_absolute()) { + // Native spelling: a raw `C:/abs/x.cppm` would stay mixed on MSVC + // (see native_path_from_generic) and leak into the CDB. + auto gp = native_path_from_generic(g); + if (gp.is_absolute()) { std::error_code aec; if (std::filesystem::is_regular_file(gp, aec)) all_files.insert(gp); continue; diff --git a/tests/e2e/47_cdb_prebuilt_module_path_abs.sh b/tests/e2e/47_cdb_prebuilt_module_path_abs.sh index 62833223..4b0cf2fb 100755 --- a/tests/e2e/47_cdb_prebuilt_module_path_abs.sh +++ b/tests/e2e/47_cdb_prebuilt_module_path_abs.sh @@ -22,6 +22,17 @@ cd app cdb=compile_commands.json [[ -f "$cdb" ]] || { echo "FAIL: no $cdb generated"; exit 1; } +# jq-independent early guard for the stray-quote bug: before the CDB +# splitter understood shell quoting, flags.cppm's ninja-side quoting leaked +# into the raw JSON as `\"-fprebuilt-module-path=...` (Windows) / `'-...` +# (POSIX). The GCC flow emits no such flag at all, so no-match is the +# expected pass there. +if grep -q '\\"-fprebuilt-module-path' "$cdb" \ + || grep -q "'-fprebuilt-module-path" "$cdb"; then + echo "FAIL: -fprebuilt-module-path retains shell quoting in raw CDB" + exit 1 +fi + command -v jq >/dev/null 2>&1 || { echo "SKIP: jq not on PATH (preinstalled on GitHub-hosted runners)" exit 0 @@ -63,6 +74,16 @@ while IFS= read -r v; do fail=1 fi + # Nor shell quoting: the flags string is assembled for the NINJA command + # line, where shell_quote_arg wraps every token containing a Windows `\` + # in double quotes — and those quotes used to land VERBATIM in the CDB + # (`"-fprebuilt-module-path=C:\...\pcm.cache"`), which clangd execs + # literally and cannot resolve. The CDB splitter must have undone them. + if [[ "$v" == '"'* || "$v" == "'"* || "$v" == *'"' || "$v" == *"'" ]]; then + echo "FAIL: value retains shell quoting: '$v'" + fail=1 + fi + # Absolute: POSIX (starts with '/') or Windows drive (e.g. 'C:'). if [[ "$v" =~ ^/ || "$v" =~ ^[A-Za-z]: ]]; then : diff --git a/tests/e2e/76_compile_commands_generated.sh b/tests/e2e/76_compile_commands_generated.sh index 6b953ad4..57253642 100755 --- a/tests/e2e/76_compile_commands_generated.sh +++ b/tests/e2e/76_compile_commands_generated.sh @@ -16,6 +16,21 @@ trap "rm -rf $TMP" EXIT cd "$TMP" "$MCPP" new app > /dev/null cd app + +# A second source reached through a MULTI-SEGMENT glob (literal prefix +# "generated/modules") — the shape that used to leak MIXED separators into +# the CDB's `file`/`-c` on Windows (`root\generated/modules\extra.cpp`), +# because MSVC's std::filesystem::path keeps the `/` from the manifest glob. +mkdir -p generated/modules +cat > generated/modules/extra.cpp <<'EOF' +int mcpp_extra_anchor() { return 1; } +EOF +cat >> mcpp.toml <<'EOF' + +[build] +sources = ["src/**/*.cpp", "generated/modules/**/*.cpp"] +EOF + "$MCPP" build > /dev/null cdb=compile_commands.json @@ -45,12 +60,19 @@ grep -q 'main\.cpp' "$cdb" || { echo "FAIL: $cdb has no entry for src/main.cpp"; # above as the portable baseline. if command -v python3 >/dev/null 2>&1; then python3 - "$cdb" <<'PY' || exit 1 -import json, sys +import json, sys, os d = json.load(open(sys.argv[1], encoding="utf-8")) assert isinstance(d, list) and d, "CDB must be a non-empty JSON array" for e in d: assert "file" in e and "directory" in e, "entry missing file/directory: %r" % e assert ("command" in e) or ("arguments" in e), "entry missing command/arguments: %r" % e + # Native separators on Windows: a multi-segment manifest glob used to + # yield MIXED `root\generated/modules\x.cppm` file paths (MSVC's path + # keeps the `/` from the glob prefix), which CLion refuses to parse. + # Ninja hides the problem (it renders generic_string()); the CDB is + # the .string() consumer. + if os.name == "nt" and "/" in e["file"]: + raise AssertionError("file must use native separators on Windows: %r" % e["file"]) print(" json validation OK (%d entries)" % len(d)) PY fi diff --git a/tests/unit/test_modgraph.cpp b/tests/unit/test_modgraph.cpp index 5b104dc0..4bd1a304 100644 --- a/tests/unit/test_modgraph.cpp +++ b/tests/unit/test_modgraph.cpp @@ -1,6 +1,7 @@ #include import std; +import mcpp.modgraph.glob; import mcpp.modgraph.graph; import mcpp.modgraph.scanner; import mcpp.modgraph.validate; @@ -151,7 +152,7 @@ TEST(Scanner, GlobLiteralPrefixDerivation) { // Wildcard already in the first segment: no literal directory to bound to. EXPECT_EQ(glob_literal_prefix("**/*.c"), ""); // No wildcard at all: the full parent directory path is the prefix. - EXPECT_EQ(glob_literal_prefix("a/b/c.cpp"), "a/b"); + EXPECT_EQ(glob_literal_prefix("a/b/c.cpp").generic_string(), "a/b"); // Truncate back to the last COMPLETE '/' before the first wildcard char — // "x*.cpp" is a partial segment, not a real directory named "x". EXPECT_EQ(glob_literal_prefix("src/x*.cpp"), "src"); @@ -161,6 +162,72 @@ TEST(Scanner, GlobLiteralPrefixDerivation) { EXPECT_EQ(glob_literal_prefix("a/{x,y}/z"), "a"); } +// MSVC's std::filesystem::path preserves the separators of the string it was +// constructed from, so a raw `a/b` prefix stays generic and `root / p` turns +// into a MIXED `root\a/b` — which used to leak into compile_commands.json +// (`file` / `-c` for every source under a multi-segment glob) and break CLion. +// glob_literal_prefix must return NATIVE separators so the walk and everything +// downstream is native too. +TEST(Scanner, GlobLiteralPrefixUsesNativeSeparators) { + EXPECT_EQ(glob_literal_prefix("a/b/c.cpp").generic_string(), "a/b"); + if constexpr (std::filesystem::path::preferred_separator == '\\') { + EXPECT_EQ(glob_literal_prefix("a/b/c.cpp").string(), "a\\b"); + EXPECT_EQ(glob_literal_prefix("a/b/c.cpp").string().find('/'), + std::string::npos); + } +} + +// The exported converter itself — both spelling directions. +TEST(Glob, NativePathFromGeneric) { + auto p = mcpp::modgraph::native_path_from_generic("a/b/c"); + EXPECT_EQ(p.generic_string(), "a/b/c"); + if constexpr (std::filesystem::path::preferred_separator == '\\') { + EXPECT_EQ(p.string(), "a\\b\\c"); + // Already-native input is untouched. + EXPECT_EQ(mcpp::modgraph::native_path_from_generic("C:\\x\\y").string(), + "C:\\x\\y"); + } +} + +// The end-to-end shape of the reported bug: a source under a multi-segment +// glob (`generated/modules/**/*.cppm`) must come out of expand_glob with +// NATIVE separators on Windows — the mixed `root\generated/modules\a.cppm` +// was what compile_commands.json's `file` field showed before the fix. +TEST(Scanner, ExpandGlobMultiSegmentPrefixUsesNativeSeparators) { + auto dir = make_tempdir("mcpp-scanner-multi"); + write(dir / "generated" / "modules" / "a.cppm", "export module a;\n"); + + auto files = expand_glob(dir, "generated/modules/**/*.cppm"); + + ASSERT_EQ(files.size(), 1u); + if constexpr (std::filesystem::path::preferred_separator == '\\') { + EXPECT_EQ(files[0].string().find('/'), std::string::npos) << files[0]; + } + EXPECT_EQ(files[0].generic_string(), + (dir / "generated" / "modules" / "a.cppm").generic_string()); + + std::filesystem::remove_all(dir); +} + +// Same contract for the INCLUDE-DIR channel (expand_dir_glob): a multi-segment +// `third_party/inc` entry must yield a native path or the CDB's -I carries the +// mixed form. +TEST(Scanner, ExpandDirGlobMultiSegmentUsesNativeSeparators) { + auto dir = make_tempdir("mcpp-scanner-dirglob"); + std::filesystem::create_directories(dir / "third_party" / "inc"); + + auto dirs = expand_dir_glob(dir, "third_party/inc"); + + ASSERT_EQ(dirs.size(), 1u); + if constexpr (std::filesystem::path::preferred_separator == '\\') { + EXPECT_EQ(dirs[0].string().find('/'), std::string::npos) << dirs[0]; + } + EXPECT_EQ(dirs[0].generic_string(), + (dir / "third_party" / "inc").generic_string()); + + std::filesystem::remove_all(dir); +} + // mcpp#225: expand_glob must bound its walk to the glob's literal directory // prefix ("src" for "src/**/*.cppm") instead of always walking the whole // root and lexically filtering afterward. This is the FUNCTIONAL half of the From 661a7d1adaadb60e8190b743560b5cd622df218d Mon Sep 17 00:00:00 2001 From: Zhe Feng Date: Sat, 8 Aug 2026 22:44:36 +0800 Subject: [PATCH 2/3] =?UTF-8?q?fix(build):=20#390=20=E4=BF=AE=E5=A4=8D?= =?UTF-8?q?=E8=A1=A5=E5=85=A8=20=E2=80=94=E2=80=94=20=E6=91=84=E5=85=A5?= =?UTF-8?q?=E7=82=B9=E6=94=B6=E6=95=9B=20+=20emitter=20=E5=85=9C=E5=BA=95?= =?UTF-8?q?=20+=20=E5=90=88=E5=B9=B6=E5=8E=BB=E9=87=8D=E8=87=AA=E6=84=88?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 按 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 指令路径的拼写, 声明输入指纹一次性失效 → 一次多余重建,属预期。 --- src/build/compile_commands.cppm | 42 ++++++++++++--- src/build/flags.cppm | 12 +++-- src/build/plan.cppm | 36 +++++++++++-- src/build/prepare.cppm | 17 +++--- src/modgraph/glob.cppm | 14 ++--- src/modgraph/scanner.cppm | 24 +++++++-- tests/e2e/76_compile_commands_generated.sh | 30 +++++++---- tests/unit/test_build_flags.cpp | 30 ++++++++--- tests/unit/test_compile_commands.cpp | 60 ++++++++++++++++++++++ tests/unit/test_modgraph.cpp | 14 +++-- tests/unit/test_ninja_backend.cpp | 54 +++++++++++++++++-- 11 files changed, 273 insertions(+), 60 deletions(-) diff --git a/src/build/compile_commands.cppm b/src/build/compile_commands.cppm index d02d3cfc..a02fb6ac 100644 --- a/src/build/compile_commands.cppm +++ b/src/build/compile_commands.cppm @@ -142,16 +142,28 @@ std::vector split_flags(std::string_view s) { namespace { +// The CDB's path contract: NATIVE separators, unconditionally. Every +// ingestion point (manifest globs, include_dirs, build.mcpp directives) is +// normalized at the source, but this is the LAST line — a path that slips +// through with a mixed `root\a/b` spelling (MSVC keeps input `/` verbatim) +// breaks CLion, and no amount of "all ingestion points are covered" can be +// proven. make_preferred() is a no-op on POSIX. +std::string native_string(const std::filesystem::path& p) { + auto n = p; + n.make_preferred(); + return n.string(); +} + std::vector local_include_args(const CompileUnit& cu) { std::vector args; args.reserve(cu.localIncludeDirs.size()); for (auto const& inc : cu.localIncludeDirs) { - args.push_back("-I" + inc.string()); + args.push_back("-I" + native_string(inc)); } // #249: after-dirs keep their -idirafter spelling in the compile DB so // tooling (clangd) reproduces the compiler's search order. for (auto const& inc : cu.localIncludeDirsAfter) { - args.push_back("-idirafter" + inc.string()); + args.push_back("-idirafter" + native_string(inc)); } return args; } @@ -186,7 +198,7 @@ std::string emit_compile_commands(const BuildPlan& plan, const CompileFlags& fla : isCSource ? flags.cc : flags.cxx; - auto output_path = (plan.outputDir / cu.object).string(); + auto output_path = native_string(plan.outputDir / cu.object); // Build arguments array. nlohmann::json args = nlohmann::json::array(); @@ -198,13 +210,13 @@ std::string emit_compile_commands(const BuildPlan& plan, const CompileFlags& fla for (auto& f : package_flag_args(cu, isCSource)) args.push_back(std::move(f)); args.push_back("-c"); - args.push_back(cu.source.string()); + args.push_back(native_string(cu.source)); args.push_back("-o"); args.push_back(output_path); nlohmann::json entry; - entry["directory"] = plan.projectRoot.string(); - entry["file"] = cu.source.string(); + entry["directory"] = native_string(plan.projectRoot); + entry["file"] = native_string(cu.source); entry["arguments"] = std::move(args); entry["output"] = output_path; @@ -222,11 +234,25 @@ std::string merge_compile_commands( if (freshJ.is_discarded() || !freshJ.is_array()) return std::string(fresh); + // Dedup key = the file's PATH, spelled the way a fresh plan spells it + // (native separators). A prior CDB written before the mixed-separator + // fix (#390) carries `root\generated/modules\x.cppm` entries that are + // the SAME file as the fresh `root\generated\modules\x.cppm` — a literal + // string comparison would keep both and the user's upgrade would not + // visibly fix anything. Normalizing makes the merge self-healing: the + // stale mixed entry is skipped on the first `mcpp build` after upgrade. + // fileExists still probes the raw spelling — Windows accepts both. + auto norm_key = [](std::string_view f) { + auto p = std::filesystem::path(std::string(f)).lexically_normal(); + p.make_preferred(); + return p.string(); + }; + // Files the current plan already covers — those entries are authoritative. std::set freshFiles; for (auto const& e : freshJ) { if (e.contains("file") && e["file"].is_string()) - freshFiles.insert(e["file"].get()); + freshFiles.insert(norm_key(e["file"].get())); } // Keep fresh order, then append still-valid prior entries the plan doesn't @@ -238,7 +264,7 @@ std::string merge_compile_commands( for (auto const& e : existingJ) { if (!e.contains("file") || !e["file"].is_string()) continue; auto f = e["file"].get(); - if (freshFiles.contains(f)) continue; // fresh wins + if (freshFiles.contains(norm_key(f))) continue; // fresh wins if (!fileExists(std::filesystem::path(f))) continue; // pruned merged.push_back(e); } diff --git a/src/build/flags.cppm b/src/build/flags.cppm index 40fa3fbe..709c1900 100644 --- a/src/build/flags.cppm +++ b/src/build/flags.cppm @@ -317,7 +317,13 @@ CompileFlags compute_flags(const BuildPlan& plan) { // once ninja hands the resolved command line to the shell. std::vector includeTokens; for (auto& inc : plan.manifest.buildConfig.includeDirs) { - std::filesystem::path p = inc.has_root_path() ? inc : (plan.projectRoot / inc); + // make_preferred: a multi-segment TOML entry like `generated/inc` + // keeps its `/` on MSVC, and the bare `projectRoot / inc` join would + // be MIXED — reaching both the ninja command line and the CDB's + // arguments (via f.cxx → split_flags). Same rule as every other + // manifest-path ingestion point (#390); no-op on POSIX. + auto p = inc.has_root_path() ? inc : (plan.projectRoot / inc); + p.make_preferred(); includeTokens.push_back(include_token(d, p)); } // #249: `[build] include_dirs_after` — searched AFTER the toolchain's @@ -327,8 +333,8 @@ CompileFlags compute_flags(const BuildPlan& plan) { // (documented degradation; clang-MSVC uses the gnu dialect). const bool msvcInclude = d.includePrefix == std::string_view("/I"); for (auto& inc : plan.manifest.buildConfig.includeDirsAfter) { - std::filesystem::path ip(inc); - std::filesystem::path p = ip.has_root_path() ? ip : (plan.projectRoot / ip); + auto p = inc.has_root_path() ? inc : (plan.projectRoot / inc); + p.make_preferred(); includeTokens.push_back( include_token(d, p, msvcInclude ? "/I" : "-idirafter")); } diff --git a/src/build/plan.cppm b/src/build/plan.cppm index 45a30fa9..795cc5b0 100644 --- a/src/build/plan.cppm +++ b/src/build/plan.cppm @@ -220,6 +220,14 @@ make_plan(const mcpp::manifest::Manifest& manifest, // simply makes those units uncacheable. const std::vector& storeRoots = {}); +// Expand one manifest `include_dirs` entry against the project root — the +// #249 consistency join + the expand_dir_glob the dep path uses. Exported +// (like modgraph's glob_literal_prefix) so unit tests can assert its +// native-separator contract directly; see the definition below. +std::vector +expand_manifest_include_entry(const std::filesystem::path& root, + const std::filesystem::path& inc); + } // namespace mcpp::build namespace mcpp::build { @@ -368,6 +376,8 @@ std::vector shared_library_link_flags( return flags; } +} // namespace + // #249 consistency fix: expand include_dirs entries with the same // `expand_dir_glob` the dep path (prepare.cppm) uses, so a main-manifest // `include_dirs = ["*/include"]` glob works identically here. For a literal @@ -375,15 +385,33 @@ std::vector shared_library_link_flags( // whereas this helper historically joined unconditionally — keep the plain // join as a fallback so an -I for a dir created later (e.g. by a build // step) isn't silently dropped. +// +// Deliberately OUTSIDE the anonymous namespace: it is exported for its unit +// test (like modgraph's glob_literal_prefix), and the two +// local_include_dirs_*_for_manifest consumers below ride along so a single +// namespace split serves the whole trio. std::vector expand_manifest_include_entry(const std::filesystem::path& root, const std::filesystem::path& inc) { - if (inc.is_absolute()) return { inc }; + if (inc.is_absolute()) { + // A TOML value like `C:/SDL2/include` keeps its `/` on MSVC — make + // it native so the CDB's -I (via local_include_args) is uniform. + auto n = inc; + n.make_preferred(); + return { std::move(n) }; + } const auto glob = inc.generic_string(); auto expanded = mcpp::modgraph::expand_dir_glob(root, glob); - if (expanded.empty() && glob.find('*') == std::string::npos) - expanded.push_back(root / inc); + if (expanded.empty() && glob.find('*') == std::string::npos) { + // Same native-spelling rule for the bare join (see above): `root / p` + // with a multi-segment `generated/inc` is MIXED on MSVC, and this + // fallback exists precisely for dirs like `generated/` that a later + // build step creates — the #390 shape. + auto joined = root / inc; + joined.make_preferred(); + expanded.push_back(std::move(joined)); + } return expanded; } @@ -412,6 +440,8 @@ local_include_dirs_after_for_manifest(const std::filesystem::path& root, return dirs; } +namespace { + void append_unique_path(std::vector& out, std::filesystem::path path) { diff --git a/src/build/prepare.cppm b/src/build/prepare.cppm index 2f7acdba..3a7d2649 100644 --- a/src/build/prepare.cppm +++ b/src/build/prepare.cppm @@ -2997,11 +2997,13 @@ prepare_build(bool print_fingerprint, std::vector dirs; for (auto const& inc : manifest.buildConfig.includeDirs) { if (inc.is_absolute()) { - // Native spelling (see native_path_from_generic): a TOML - // `C:/SDL2/include` stays mixed on MSVC and leaks into the - // CDB's -I otherwise. - appendUniquePath(dirs, - mcpp::modgraph::native_path_from_generic(inc.generic_string())); + // Native spelling: a TOML `C:/SDL2/include` stays mixed on + // MSVC and leaks into the CDB's -I otherwise. Direct + // make_preferred — no generic_string round trip, which can + // throw for names the ANSI codepage cannot spell (mcpp#230). + auto n = inc; + n.make_preferred(); + appendUniquePath(dirs, std::move(n)); continue; } for (auto& dir : mcpp::modgraph::expand_dir_glob( @@ -3021,8 +3023,9 @@ prepare_build(bool print_fingerprint, std::vector dirs; for (auto const& inc : manifest.buildConfig.includeDirsAfter) { if (inc.is_absolute()) { - appendUniquePath(dirs, - mcpp::modgraph::native_path_from_generic(inc.generic_string())); + auto n = inc; + n.make_preferred(); + appendUniquePath(dirs, std::move(n)); continue; } for (auto& dir : mcpp::modgraph::expand_dir_glob( diff --git a/src/modgraph/glob.cppm b/src/modgraph/glob.cppm index 73a04b9a..7d929c54 100644 --- a/src/modgraph/glob.cppm +++ b/src/modgraph/glob.cppm @@ -24,16 +24,12 @@ export namespace mcpp::modgraph { // to parse. Ninja never notices because it renders everything via // generic_string(); the CDB is the first `.string()` consumer. // -// POSIX is untouched (its native separator already is `/`). Replacing only -// `/` is also safe for already-native Windows input: it never contains `/`. +// POSIX is untouched (`make_preferred()` is a no-op there, and it is also +// safe for already-native Windows input, which never contains `/`). std::filesystem::path native_path_from_generic(std::string_view s) { - constexpr char kSep = std::filesystem::path::preferred_separator; - if (kSep == '/') return std::filesystem::path(s); - std::string p(s); - for (auto& c : p) { - if (c == '/') c = kSep; - } - return std::filesystem::path(std::move(p)); + std::filesystem::path p(s); + p.make_preferred(); + return p; } // Does `candidate` match `glob`, interpreted relative to `root`? diff --git a/src/modgraph/scanner.cppm b/src/modgraph/scanner.cppm index 2f3755c6..df04639e 100644 --- a/src/modgraph/scanner.cppm +++ b/src/modgraph/scanner.cppm @@ -501,10 +501,18 @@ namespace { // has_root_path: leave absolute AND root-relative ("/x" on Windows) // spellings alone — only genuinely root-less paths are project-relative. +// Both branches normalize to NATIVE separators: a `-Ithird_party/inc` cxxflag +// would otherwise come back as `C:\proj\third_party/inc` on MSVC (path keeps +// the input `/` verbatim) and reach the CDB's arguments via packageCxxflags. std::string rewrite_rel_copy(const std::string& p, const std::filesystem::path& root) { std::filesystem::path fp(p); - if (fp.has_root_path()) return p; - return (root / fp).string(); + if (fp.has_root_path()) { + fp.make_preferred(); + return fp.string(); + } + auto joined = root / fp; + joined.make_preferred(); + return joined.string(); } void rewrite_rel(std::string& p, const std::filesystem::path& root) { @@ -691,8 +699,12 @@ local_include_dirs_for(const std::filesystem::path& root, if (inc.is_absolute()) { // A TOML value like `C:/SDL2/include` keeps its `/` on MSVC — // normalize so the CDB's -I comes out native (mixed separators - // break CLion). See mcpp::modgraph::native_path_from_generic. - dirs.push_back(native_path_from_generic(inc.generic_string())); + // break CLion). Direct make_preferred, no generic_string round + // trip: the narrow conversion can throw for names the ANSI + // codepage cannot spell (mcpp#230). + auto n = inc; + n.make_preferred(); + dirs.push_back(std::move(n)); continue; } for (auto& d : expand_dir_glob(root, inc.generic_string())) { @@ -711,7 +723,9 @@ local_include_dirs_after_for(const std::filesystem::path& root, std::vector dirs; for (auto const& inc : manifest.buildConfig.includeDirsAfter) { if (inc.is_absolute()) { - dirs.push_back(native_path_from_generic(inc.generic_string())); + auto n = inc; + n.make_preferred(); + dirs.push_back(std::move(n)); continue; } for (auto& d : expand_dir_glob(root, inc.generic_string())) { diff --git a/tests/e2e/76_compile_commands_generated.sh b/tests/e2e/76_compile_commands_generated.sh index 57253642..b812ff77 100755 --- a/tests/e2e/76_compile_commands_generated.sh +++ b/tests/e2e/76_compile_commands_generated.sh @@ -55,26 +55,36 @@ grep -qE '"command"|"arguments"' "$cdb" || { # The minimal project's source (src/main.cpp) must have an entry. grep -q 'main\.cpp' "$cdb" || { echo "FAIL: $cdb has no entry for src/main.cpp"; cat "$cdb"; exit 1; } +# The multi-segment glob must ACTUALLY have contributed an entry — if the +# glob silently missed, every separator assertion below is vacuous green. +grep -q 'extra\.cpp' "$cdb" || { + echo "FAIL: $cdb has no entry for generated/modules/extra.cpp — multi-segment glob missed" + cat "$cdb"; exit 1 +} + # Deeper structural validation when a JSON parser is available (GitHub-hosted -# runners ship python3). Skips cleanly where it isn't, keeping the grep checks -# above as the portable baseline. +# runners ship python3). Explicitly reports the skip where it isn't, so a +# silent pass can never masquerade as validation coverage. if command -v python3 >/dev/null 2>&1; then python3 - "$cdb" <<'PY' || exit 1 -import json, sys, os +import json, sys d = json.load(open(sys.argv[1], encoding="utf-8")) assert isinstance(d, list) and d, "CDB must be a non-empty JSON array" for e in d: assert "file" in e and "directory" in e, "entry missing file/directory: %r" % e assert ("command" in e) or ("arguments" in e), "entry missing command/arguments: %r" % e - # Native separators on Windows: a multi-segment manifest glob used to - # yield MIXED `root\generated/modules\x.cppm` file paths (MSVC's path - # keeps the `/` from the glob prefix), which CLion refuses to parse. - # Ninja hides the problem (it renders generic_string()); the CDB is - # the .string() consumer. - if os.name == "nt" and "/" in e["file"]: - raise AssertionError("file must use native separators on Windows: %r" % e["file"]) + # Platform-independent mixed-separator check: the #390 bug spelled a + # Windows file as `root\generated/modules\x.cppm` (MSVC's path keeps the + # `/` from the manifest glob prefix, and the directory walk propagates + # it). On POSIX a backslash never appears in a path, so the assertion is + # trivially true there and catches exactly the bug on Windows — no + # os.name / platform sniffing needed. + f = e["file"] + assert not ("\\" in f and "/" in f), "mixed separators in file: %r" % f print(" json validation OK (%d entries)" % len(d)) PY +else + echo "SKIP: python3 not on PATH — JSON validation not run" fi echo "OK" diff --git a/tests/unit/test_build_flags.cpp b/tests/unit/test_build_flags.cpp index 507b9e0a..d67773da 100644 --- a/tests/unit/test_build_flags.cpp +++ b/tests/unit/test_build_flags.cpp @@ -97,8 +97,18 @@ TEST(BuildFlagsAtomic, StaticLinkEmittedWhenArchivePresent) { // BOTH the joined spelling (`-iquotehdr`) and the separated spelling // (`-isystem` followed by a standalone next element). All four of these // project-relative paths must resolve to the same "/proj/hdr" target. +// +// The expected spelling is NATIVE (#390): `-I/abs/hdr` written with forward +// slashes keeps them on MSVC, and the old expectation `(root / "hdr").string()` +// was itself the mixed `"/proj\hdr"` shape this family of bugs produced. +// make_preferred() makes the expectation platform-correct on both sides. TEST(BuildFlags, NormalizeIncludeFlagsRewritesFullIncludeFamily) { std::filesystem::path root = "/proj"; + auto expected = [](const std::filesystem::path& p) { + auto n = p; + n.make_preferred(); + return n.string(); + }; std::vector flags = { "-Ihdr", "-iquotehdr", "-isystem", "hdr", "-idirafterhdr", }; @@ -106,26 +116,32 @@ TEST(BuildFlags, NormalizeIncludeFlagsRewritesFullIncludeFamily) { mcpp::modgraph::normalize_include_flags(root, flags); ASSERT_EQ(flags.size(), 5u); - EXPECT_EQ(flags[0], "-I" + (root / "hdr").string()); - EXPECT_EQ(flags[1], "-iquote" + (root / "hdr").string()); + EXPECT_EQ(flags[0], "-I" + expected(root / "hdr")); + EXPECT_EQ(flags[1], "-iquote" + expected(root / "hdr")); EXPECT_EQ(flags[2], "-isystem"); // prefix itself untouched - EXPECT_EQ(flags[3], (root / "hdr").string()); // separated element rewritten - EXPECT_EQ(flags[4], "-idirafter" + (root / "hdr").string()); + EXPECT_EQ(flags[3], expected(root / "hdr")); // separated element rewritten + EXPECT_EQ(flags[4], "-idirafter" + expected(root / "hdr")); } // Absolute paths and root-relative spellings are left alone (matches the -// pre-#226 -I behavior), for both the joined and separated forms. +// pre-#226 -I behavior), for both the joined and separated forms — only the +// separator spelling is normalized to native (#390; a no-op on POSIX). TEST(BuildFlags, NormalizeIncludeFlagsLeavesAbsolutePathsAlone) { std::filesystem::path root = "/proj"; + auto expected = [](const std::filesystem::path& p) { + auto n = p; + n.make_preferred(); + return n.string(); + }; std::vector flags = { "-I/abs/hdr", "-isystem", "/abs/hdr", "-DKEEP", }; mcpp::modgraph::normalize_include_flags(root, flags); - EXPECT_EQ(flags[0], "-I/abs/hdr"); + EXPECT_EQ(flags[0], "-I" + expected("/abs/hdr")); EXPECT_EQ(flags[1], "-isystem"); - EXPECT_EQ(flags[2], "/abs/hdr"); + EXPECT_EQ(flags[2], expected("/abs/hdr")); EXPECT_EQ(flags[3], "-DKEEP"); } diff --git a/tests/unit/test_compile_commands.cpp b/tests/unit/test_compile_commands.cpp index 7be570db..d13455eb 100644 --- a/tests/unit/test_compile_commands.cpp +++ b/tests/unit/test_compile_commands.cpp @@ -2,6 +2,9 @@ import std; import mcpp.build.compile_commands; +import mcpp.build.flags; +import mcpp.build.plan; +import mcpp.libs.json; using namespace mcpp::build; @@ -93,6 +96,27 @@ TEST(CompileCommandsMerge, MalformedExistingFallsBackToFresh) { EXPECT_NE(merged.find("-O2"), std::string::npos) << merged; } +// #390 self-heal: a CDB written BEFORE the mixed-separator fix spells the +// same file with the generic `/` spelling (`/p/generated/modules/a.cpp` — +// on Windows this is exactly `root\generated/modules\a.cpp` vs the native +// `root\generated\modules\a.cpp`). The merge key is the NORMALIZED path, so +// the stale entry is recognized as the same file and dropped on the first +// build after upgrade — the user never has to delete compile_commands.json +// by hand. (On POSIX the two spellings are byte-identical; the test then +// still verifies the plain fresh-wins contract.) +TEST(CompileCommandsMerge, NormalizedFileKeysHealStaleSeparatorSpellings) { + auto p = std::filesystem::path("/p") / "generated" / "modules" / "a.cpp"; + auto fresh = cdb({ entry(p.string(), "-O2-FRESH") }); + auto existing = cdb({ entry(p.generic_string(), "-O0-STALE") }); + + auto merged = merge_compile_commands( + fresh, existing, [](const std::filesystem::path&) { return true; }); + + EXPECT_EQ(count(merged, "generated"), 1u) << merged; + EXPECT_NE(merged.find("-O2-FRESH"), std::string::npos) << merged; + EXPECT_EQ(merged.find("-O0-STALE"), std::string::npos) << merged; +} + // ── CDB arguments must be argv, not shell words ───────────────────────────── // // A consumer (clangd) execs `arguments` LITERALLY — no shell. So a token that @@ -172,3 +196,39 @@ TEST(CompileCommandsArgs, InnerQuotesAreNotStripped) { EXPECT_EQ(out[0], R"(-DGREETING="hi")"); EXPECT_FALSE(is_quoted(out[0])); } + +// ── Emitted paths use native separators, unconditionally ──────────────────── +// +// The last line of the #390 defence: every ingestion point is normalized at +// the source, but a path that still slips through with a mixed spelling +// (MSVC keeps the input `/` verbatim) would land in `file`/`-c`/`-o`/`-I` +// and break CLion. emit_compile_commands therefore makes ALL of them native +// — make_preferred(), a no-op on POSIX. +TEST(CompileCommandsEmit, EmittedPathsUseNativeSeparators) { + BuildPlan plan; + plan.projectRoot = "/p"; + plan.outputDir = "/p/target"; + plan.compileUnits.push_back({ + // A path whose spelling carries `/` segments (what the manifest + // glob ingestion used to produce on MSVC). + .source = std::filesystem::path("C:/Users/x/src/main.cpp"), + .object = std::filesystem::path("obj") / "main.o", + .packageName = "demo", + }); + CompileFlags flags; + flags.cxxBinary = std::filesystem::path("/usr/bin/g++"); + + auto j = nlohmann::json::parse(emit_compile_commands(plan, flags)); + auto const& e = j[0]; + if constexpr (std::filesystem::path::preferred_separator == '\\') { + EXPECT_EQ(e["file"].get(), "C:\\Users\\x\\src\\main.cpp"); + EXPECT_EQ(e["directory"].get(), "\\p"); + EXPECT_EQ(e["output"].get(), "\\p\\target\\obj\\main.o"); + for (auto const& a : e["arguments"]) { + if (a.is_string() && a.get().starts_with("-")) + EXPECT_EQ(a.get().find('/'), std::string::npos); + } + } else { + EXPECT_EQ(e["file"].get(), "C:/Users/x/src/main.cpp"); + } +} diff --git a/tests/unit/test_modgraph.cpp b/tests/unit/test_modgraph.cpp index 4bd1a304..d74051a5 100644 --- a/tests/unit/test_modgraph.cpp +++ b/tests/unit/test_modgraph.cpp @@ -167,9 +167,9 @@ TEST(Scanner, GlobLiteralPrefixDerivation) { // into a MIXED `root\a/b` — which used to leak into compile_commands.json // (`file` / `-c` for every source under a multi-segment glob) and break CLion. // glob_literal_prefix must return NATIVE separators so the walk and everything -// downstream is native too. +// downstream is native too. (The generic spelling is already locked down by +// Scanner.GlobLiteralPrefixDerivation above.) TEST(Scanner, GlobLiteralPrefixUsesNativeSeparators) { - EXPECT_EQ(glob_literal_prefix("a/b/c.cpp").generic_string(), "a/b"); if constexpr (std::filesystem::path::preferred_separator == '\\') { EXPECT_EQ(glob_literal_prefix("a/b/c.cpp").string(), "a\\b"); EXPECT_EQ(glob_literal_prefix("a/b/c.cpp").string().find('/'), @@ -757,6 +757,9 @@ TEST(Scanner, PerGlobFlagsMatchBraceAlternation) { // G8b: relative -I flags are root-relative in the manifest but ninja runs // with cwd = output dir — the scanner absolutizes them on every unit. +// The absolute spelling is NORMALIZED to native separators (#390): MSVC's +// path keeps the input `/` verbatim, and `-I/abs/path` written with forward +// slashes used to survive into the CDB's arguments as a mixed path. TEST(Scanner, RelativeIncludeFlagsAbsolutized) { auto dir = make_tempdir("mcpp-scanner-relinc"); write(dir / "src" / "a.cpp", "int a();\n"); @@ -774,8 +777,11 @@ TEST(Scanner, RelativeIncludeFlagsAbsolutized) { ASSERT_EQ(res.graph.units.size(), 1u); auto& fl = res.graph.units[0].packageCxxflags; EXPECT_EQ(fl[0], "-I" + (dir / "inc").string()); - EXPECT_EQ(fl[1], "-I/abs/path"); // absolute stays - EXPECT_EQ(fl[2], "-DKEEP"); // non-include untouched + if constexpr (std::filesystem::path::preferred_separator == '\\') + EXPECT_EQ(fl[1], "-I\\abs\\path"); // absolute stays, but native + else + EXPECT_EQ(fl[1], "-I/abs/path"); // absolute stays + EXPECT_EQ(fl[2], "-DKEEP"); // non-include untouched std::filesystem::remove_all(dir); } diff --git a/tests/unit/test_ninja_backend.cpp b/tests/unit/test_ninja_backend.cpp index be803b8f..894cac41 100644 --- a/tests/unit/test_ninja_backend.cpp +++ b/tests/unit/test_ninja_backend.cpp @@ -5,6 +5,7 @@ import mcpp.build.compile_commands; import mcpp.build.flags; import mcpp.build.ninja; import mcpp.build.plan; +import mcpp.libs.json; import mcpp.manifest; import mcpp.toolchain.dialect; import mcpp.toolchain.model; @@ -163,8 +164,11 @@ TEST(NinjaBackend, CxxFlagsIncludeBuildIncludeDirs) { EXPECT_NE(flags.cxx.find(escaped_include_flag(plan.projectRoot / "include")), std::string::npos) << flags.cxx; - EXPECT_NE(flags.cxx.find(escaped_include_flag( - plan.projectRoot / std::filesystem::path{"third_party/imgui"})), + // #390: a multi-segment entry is normalized to NATIVE separators before + // the -I token is built (a mixed `...\third_party/imgui` used to reach + // the CDB through f.cxx). Build the expected path natively too. + auto imgui = plan.projectRoot / "third_party" / "imgui"; + EXPECT_NE(flags.cxx.find(escaped_include_flag(imgui)), std::string::npos) << flags.cxx; } @@ -1148,8 +1152,22 @@ TEST(NinjaBackend, CachedUnitsStillAppearInCompileCommands) { auto flags = compute_flags(plan); auto cdb = emit_compile_commands(plan, flags); - EXPECT_NE(cdb.find("/store/dep/src/dep.c"), std::string::npos) << cdb; - EXPECT_NE(cdb.find("src/main.cpp"), std::string::npos) << cdb; + // #390: the emitter spells every path natively, so the expected source + // spelling is make_preferred'ed (a no-op on POSIX). Compared on the + // DECODED file field — the raw JSON text doubles every backslash. + auto dep = std::filesystem::path("/store/dep/src/dep.c"); + dep.make_preferred(); + auto main = std::filesystem::path("src/main.cpp"); + main.make_preferred(); + auto j = nlohmann::json::parse(cdb); + bool sawDep = false, sawMain = false; + for (auto const& e : j) { + auto f = e["file"].get(); + sawDep = sawDep || f == dep.string(); + sawMain = sawMain || f == main.string(); + } + EXPECT_TRUE(sawDep) << cdb; + EXPECT_TRUE(sawMain) << cdb; } // Replacing a package's compile edges with stage edges also removes the ordering @@ -1228,3 +1246,31 @@ TEST(NinjaBackend, NoStagedPhonyWhenNothingIsCached) { auto ninja = emit_ninja_string(plan); EXPECT_EQ(ninja.find("_mcpp_staged_cache"), std::string::npos) << ninja; } + +// #390: the main-manifest include channel's fallback join — `root / inc` for +// a directory a build step will create LATER (the `generated/inc` shape) — +// must spell its result NATIVELY on Windows. MSVC keeps the `/` of a TOML +// `generated/inc` verbatim, so the bare join yields `C:\proj\generated/inc`, +// which reaches the CDB's -I via local_include_args and breaks CLion. Same +// rule for an absolute entry written with forward slashes (`C:/SDL2/include`). +// (The existing-directory path runs through expand_dir_glob, covered by +// Scanner.ExpandDirGlobMultiSegmentUsesNativeSeparators.) +TEST(Plan, ExpandManifestIncludeEntryNativeSpelling) { + namespace fs = std::filesystem; + auto root = fs::temp_directory_path() / "mcpp-plan-inc-entry"; + fs::remove_all(root); + fs::create_directories(root); // generated/inc deliberately does NOT exist + + auto fb = expand_manifest_include_entry(root, fs::path("generated/inc")); + ASSERT_EQ(fb.size(), 1u); + if constexpr (fs::path::preferred_separator == '\\') + EXPECT_EQ(fb[0].string().find('/'), std::string::npos) << fb[0]; + EXPECT_EQ(fb[0], root / "generated" / "inc"); + + auto abs = expand_manifest_include_entry(root, fs::path("C:/SDL2/include")); + ASSERT_EQ(abs.size(), 1u); + if constexpr (fs::path::preferred_separator == '\\') + EXPECT_EQ(abs[0].string(), "C:\\SDL2\\include"); + + fs::remove_all(root); +} From fe82dd39c0c68e0247a785416a08087017af0d26 Mon Sep 17 00:00:00 2001 From: speak-agent Date: Sun, 9 Aug 2026 03:29:30 +0800 Subject: [PATCH 3/3] =?UTF-8?q?test(cdb):=20=E8=87=AA=E6=84=88=E5=AE=88?= =?UTF-8?q?=E5=8D=AB=E6=AD=A4=E5=89=8D=E5=9C=A8=E4=B8=A4=E4=B8=AA=E5=B9=B3?= =?UTF-8?q?=E5=8F=B0=E9=83=BD=E7=A9=BA=E8=BD=AC=20=E2=80=94=E2=80=94=20?= =?UTF-8?q?=E4=BF=AE=E5=A5=BD=E5=AE=83,=E9=A1=BA=E5=B8=A6=E6=94=B6?= =?UTF-8?q?=E6=95=9B=20flags.cppm=20=E7=9A=84=E7=AC=AC=E5=9B=9B=E4=BB=BD?= =?UTF-8?q?=20join?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit review 第二轮的四条: 1) NormalizedFileKeysHealStaleSeparatorSpellings 之前测不到东西。 测试 helper `entry()` 用 std::format 手拼 JSON,而 Windows 的 `file` 值带反斜杠 —— `...\generated\...` 里的 `\g` 不是合法 JSON 转义,整份 fixture 无法解析。merge_compile_commands 对解析失败的 回答是 `return std::string(fresh)`,于是三条断言在没有跑到 norm_key 的情况下全部通过。POSIX 上则是另一种空转:stale 用 p.generic_string(),与 fresh 的 p.string() 逐字节相同,换回修改前 的字面串键一样能过。 修法两处:entry() 改由 nlohmann 序列化(转义按构造保证);stale 拼写换成 `/p/generated/./modules/a.cpp` —— `/./` 在 POSIX 上也必须 经 lexically_normal 折叠才能与 fresh 对齐,Windows 上再叠一层 make_preferred。cdb() 里加一条 fixture 自检,让「拼出来的 JSON 不 可解析」这类空转以后直接红。 已验证先红:把 norm_key 临时退回字面串键,该测试在 Linux 上失败 (正是它原来空转的平台),恢复后 68/68。 2) emitter 兜底的范围说清楚。它覆盖 CDB schema 的路径字段与 emitter 自己构造的 argv 位置,不覆盖 split_flags(f.cxx) 与 package 的 cflags/cxxflags —— 对任意 flag 载荷做路径归一化本来就不安全 (`-DPATH="/etc/x"` 里的斜杠是真的)。注释按实际范围改写。 EmittedPathsUseNativeSeparators 里那个「以 - 开头的参数不得含 /」 的循环在原 fixture 下遍历不到任何带路径的参数(flags 全空,只剩 裸 -c/-o),换成填上 localIncludeDirs / localIncludeDirsAfter 后 逐条断言 -I 与 -idirafter,POSIX 分支也一并断言。 3) flags.cppm 的 include join 收敛成一个 abs_native lambda,四处共用 ({include_dirs, include_dirs_after} × {C/C++ 通道, NASM 通道})。 NASM 那两处此前既没归一化分隔符,谓词也用的是 is_absolute() 而非 has_root_path() —— 同一个 manifest 键在两个通道产出不同的路径。 两者只在 Windows 的根相对拼法(`/x`)上有别,现在与编译器通道一致。 够不到 CDB(.asm 单元被 emitter 显式跳过),但这正是本 PR 要终结的 那类债。新增 NasmIncludeDirsMatchTheCxxChannelSpelling 钉住契约 (POSIX 上是防回归守卫,Windows 上才是新断言)。 4) rewrite_rel_copy 的绝对分支不再无谓往返:没有 `/` 可改时直接返回 原字节,不经 path 的窄转换 —— 那个转换对 ANSI 代码页拼不出的名字 会抛 std::system_error(mcpp#230),而本 PR 刚以同一理由删掉别处的 generic_string() 往返。 本机:mcpp build 自举通过,mcpp test 68/68,e2e 47 / 76 / 105 / 141 / 148 / 179 / 25 / 51 通过。 --- src/build/compile_commands.cppm | 18 ++++-- src/build/flags.cppm | 43 +++++++++----- src/modgraph/scanner.cppm | 6 ++ tests/unit/test_compile_commands.cpp | 88 +++++++++++++++++++++------- tests/unit/test_ninja_backend.cpp | 26 ++++++++ 5 files changed, 137 insertions(+), 44 deletions(-) diff --git a/src/build/compile_commands.cppm b/src/build/compile_commands.cppm index a02fb6ac..ed60c2a9 100644 --- a/src/build/compile_commands.cppm +++ b/src/build/compile_commands.cppm @@ -142,12 +142,18 @@ std::vector split_flags(std::string_view s) { namespace { -// The CDB's path contract: NATIVE separators, unconditionally. Every -// ingestion point (manifest globs, include_dirs, build.mcpp directives) is -// normalized at the source, but this is the LAST line — a path that slips -// through with a mixed `root\a/b` spelling (MSVC keeps input `/` verbatim) -// breaks CLion, and no amount of "all ingestion points are covered" can be -// proven. make_preferred() is a no-op on POSIX. +// NATIVE separators for every path this emitter SPELLS ITSELF. Each ingestion +// point (manifest globs, include_dirs, build.mcpp directives) is normalized at +// the source, but this is the last line for the fields the CDB schema defines +// — a path that slips through with a mixed `root\a/b` spelling (MSVC keeps +// input `/` verbatim) breaks CLion, and "all ingestion points are covered" is +// not a claim that can be proven once and stay true. +// +// It is NOT a whole-argv guarantee: the flag strings (split_flags(f.cxx), the +// package cflags/cxxflags) pass through untouched, because normalizing an +// arbitrary flag payload is unsafe — `-DPATH="/etc/x"` holds real slashes. +// Those channels are normalized where they are ingested instead. +// make_preferred() is a no-op on POSIX. std::string native_string(const std::filesystem::path& p) { auto n = p; n.make_preferred(); diff --git a/src/build/flags.cppm b/src/build/flags.cppm index 709c1900..d2aed189 100644 --- a/src/build/flags.cppm +++ b/src/build/flags.cppm @@ -315,16 +315,24 @@ CompileFlags compute_flags(const BuildPlan& plan) { // ninja-$-escape and shell-quote per token (#234) so an include dir // whose name contains a space can't silently split into two shell words // once ninja hands the resolved command line to the shell. - std::vector includeTokens; - for (auto& inc : plan.manifest.buildConfig.includeDirs) { - // make_preferred: a multi-segment TOML entry like `generated/inc` - // keeps its `/` on MSVC, and the bare `projectRoot / inc` join would - // be MIXED — reaching both the ninja command line and the CDB's - // arguments (via f.cxx → split_flags). Same rule as every other - // manifest-path ingestion point (#390); no-op on POSIX. + // The one place this file turns a manifest include entry into a path. + // make_preferred: a multi-segment TOML entry like `generated/inc` keeps + // its `/` on MSVC, and the bare `projectRoot / inc` join would be MIXED — + // reaching both the ninja command line and the CDB's arguments (via + // f.cxx → split_flags). Same rule as every other manifest-path ingestion + // point (#390); no-op on POSIX. ONE lambda because the same join is needed + // four times in this function — {include_dirs, include_dirs_after} × {the + // C/C++ token list, the NASM one} — and re-deriving it per site is how the + // two channels drifted apart in the first place. + auto abs_native = [&](const std::filesystem::path& inc) { auto p = inc.has_root_path() ? inc : (plan.projectRoot / inc); p.make_preferred(); - includeTokens.push_back(include_token(d, p)); + return p; + }; + + std::vector includeTokens; + for (auto& inc : plan.manifest.buildConfig.includeDirs) { + includeTokens.push_back(include_token(d, abs_native(inc))); } // #249: `[build] include_dirs_after` — searched AFTER the toolchain's // system dirs via -idirafter (gcc+clang), so entries can't shadow @@ -333,10 +341,8 @@ CompileFlags compute_flags(const BuildPlan& plan) { // (documented degradation; clang-MSVC uses the gnu dialect). const bool msvcInclude = d.includePrefix == std::string_view("/I"); for (auto& inc : plan.manifest.buildConfig.includeDirsAfter) { - auto p = inc.has_root_path() ? inc : (plan.projectRoot / inc); - p.make_preferred(); includeTokens.push_back( - include_token(d, p, msvcInclude ? "/I" : "-idirafter")); + include_token(d, abs_native(inc), msvcInclude ? "/I" : "-idirafter")); } std::string include_flags; for (auto& t : includeTokens) { @@ -535,17 +541,22 @@ CompileFlags compute_flags(const BuildPlan& plan) { // re-spelt with -I regardless of dialect (nasm ≥2.14 inserts a missing // path separator itself); DWARF debug info exists on ELF only. if (!plan.nasmPath.empty()) { + // Same abs_native join as the C/C++ channel above — one decision, one + // implementation. Two knock-on effects, both wanted: the entry is now + // spelt with native separators (#390), and the "already rooted?" test + // becomes has_root_path() instead of is_absolute(), so a root-relative + // `/x` entry is left alone here exactly as it is for the C/C++ include + // list. The two predicates only differ on Windows, and only for that + // spelling — where NASM disagreeing with the compiler about the SAME + // `include_dirs` key was the bug, not the feature. std::string nasm_includes; for (auto& inc : plan.manifest.buildConfig.includeDirs) { - auto abs = inc.is_absolute() ? inc : (plan.projectRoot / inc); - nasm_includes += " -I" + escape_path(abs); + nasm_includes += " -I" + escape_path(abs_native(inc)); } // #249: nasm has no system header dirs to defer to — after-dirs // degrade to plain -I appended at the end. for (auto& inc : plan.manifest.buildConfig.includeDirsAfter) { - std::filesystem::path ip(inc); - auto abs = ip.is_absolute() ? ip : (plan.projectRoot / ip); - nasm_includes += " -I" + escape_path(abs); + nasm_includes += " -I" + escape_path(abs_native(inc)); } std::string nasm_debug; if (prof.debug && plan.nasmFormat.starts_with("elf")) diff --git a/src/modgraph/scanner.cppm b/src/modgraph/scanner.cppm index df04639e..47e94dcf 100644 --- a/src/modgraph/scanner.cppm +++ b/src/modgraph/scanner.cppm @@ -507,6 +507,12 @@ namespace { std::string rewrite_rel_copy(const std::string& p, const std::filesystem::path& root) { std::filesystem::path fp(p); if (fp.has_root_path()) { + // Nothing to re-spell → hand back the ORIGINAL bytes rather than + // round-tripping them through path's narrow conversion, which throws + // std::system_error for names the ANSI codepage cannot express + // (mcpp#230 — see path_matches_glob). A rooted path with no '/' is + // already native on both platform families. + if (p.find('/') == std::string::npos) return p; fp.make_preferred(); return fp.string(); } diff --git a/tests/unit/test_compile_commands.cpp b/tests/unit/test_compile_commands.cpp index d13455eb..50e14a83 100644 --- a/tests/unit/test_compile_commands.cpp +++ b/tests/unit/test_compile_commands.cpp @@ -11,12 +11,22 @@ using namespace mcpp::build; namespace { // Build a single CDB entry as JSON text. `flag` is a marker we can grep for. +// +// Serialized THROUGH nlohmann, never hand-formatted: a Windows `file` value +// carries backslashes, and `\g` (from `...\generated\...`) is not a legal JSON +// escape. A format-string version produced text that no parser accepts, which +// merge_compile_commands answers by returning `fresh` verbatim — every +// assertion below then passes without the code under test ever running. std::string entry(std::string_view file, std::string_view flag) { // Keep the file path out of `arguments` so it appears exactly once (in // "file") — lets tests count entries per file unambiguously. - return std::format( - R"({{"directory":"/p","file":"{}","arguments":["g++","{}","-c","src.cpp"],"output":"o"}})", - file, flag); + nlohmann::json e; + e["directory"] = "/p"; + e["file"] = std::string(file); + e["arguments"] = nlohmann::json::array( + { "g++", std::string(flag), "-c", "src.cpp" }); + e["output"] = "o"; + return e.dump(); } std::string cdb(std::initializer_list entries) { @@ -28,6 +38,12 @@ std::string cdb(std::initializer_list entries) { first = false; } s += "\n]\n"; + // A fixture that does not PARSE makes every merge test vacuously green + // (merge_compile_commands short-circuits to `fresh` on a discarded parse), + // so the fixture itself is checked here rather than trusted. Tests that + // WANT malformed input pass it directly, not through this builder. + EXPECT_FALSE(nlohmann::json::parse(s, nullptr, /*allow_exceptions=*/false) + .is_discarded()) << s; return s; } @@ -96,18 +112,24 @@ TEST(CompileCommandsMerge, MalformedExistingFallsBackToFresh) { EXPECT_NE(merged.find("-O2"), std::string::npos) << merged; } -// #390 self-heal: a CDB written BEFORE the mixed-separator fix spells the -// same file with the generic `/` spelling (`/p/generated/modules/a.cpp` — -// on Windows this is exactly `root\generated/modules\a.cpp` vs the native -// `root\generated\modules\a.cpp`). The merge key is the NORMALIZED path, so -// the stale entry is recognized as the same file and dropped on the first -// build after upgrade — the user never has to delete compile_commands.json -// by hand. (On POSIX the two spellings are byte-identical; the test then -// still verifies the plain fresh-wins contract.) +// #390 self-heal: a CDB written BEFORE the mixed-separator fix spells the same +// file differently from the way a fresh plan spells it. Because the merge key +// is the NORMALIZED path, the stale entry is recognized as the same file and +// dropped on the first build after upgrade — the user never has to delete +// compile_commands.json by hand. +// +// The stale spelling deliberately differs from fresh on EVERY platform: the +// `/./` segment needs lexically_normal to collapse (POSIX included), and on +// Windows the `/` separators need make_preferred on top. A stale value of just +// `p.generic_string()` would be byte-identical to fresh on POSIX, and the +// literal-string key this test exists to replace would pass it unchanged. TEST(CompileCommandsMerge, NormalizedFileKeysHealStaleSeparatorSpellings) { auto p = std::filesystem::path("/p") / "generated" / "modules" / "a.cpp"; + auto stale = "/p/generated/./modules/a.cpp"; + ASSERT_NE(p.string(), stale) << "fixture must differ from the fresh spelling"; + auto fresh = cdb({ entry(p.string(), "-O2-FRESH") }); - auto existing = cdb({ entry(p.generic_string(), "-O0-STALE") }); + auto existing = cdb({ entry(stale, "-O0-STALE") }); auto merged = merge_compile_commands( fresh, existing, [](const std::filesystem::path&) { return true; }); @@ -197,38 +219,60 @@ TEST(CompileCommandsArgs, InnerQuotesAreNotStripped) { EXPECT_FALSE(is_quoted(out[0])); } -// ── Emitted paths use native separators, unconditionally ──────────────────── +// ── Emitted paths use native separators ───────────────────────────────────── // // The last line of the #390 defence: every ingestion point is normalized at // the source, but a path that still slips through with a mixed spelling -// (MSVC keeps the input `/` verbatim) would land in `file`/`-c`/`-o`/`-I` -// and break CLion. emit_compile_commands therefore makes ALL of them native -// — make_preferred(), a no-op on POSIX. +// (MSVC keeps the input `/` verbatim) would land in the CDB and break CLion. +// +// Scope, precisely: the emitter owns the CDB schema's path fields (`file`, +// `directory`, `output`) and the argv positions it builds itself (`-c`'s and +// `-o`'s values, plus `-I`/`-idirafter` from localIncludeDirs). The flag +// STRINGS — split_flags(f.cxx) and the package cflags/cxxflags — pass through +// untouched and stay the ingestion points' responsibility; normalizing an +// arbitrary flag payload is not safe (`-DPATH="/etc/x"` holds real slashes). TEST(CompileCommandsEmit, EmittedPathsUseNativeSeparators) { BuildPlan plan; plan.projectRoot = "/p"; plan.outputDir = "/p/target"; plan.compileUnits.push_back({ - // A path whose spelling carries `/` segments (what the manifest - // glob ingestion used to produce on MSVC). + // Paths whose spelling carries `/` segments — what the manifest glob + // and the include_dirs channels used to produce on MSVC. .source = std::filesystem::path("C:/Users/x/src/main.cpp"), .object = std::filesystem::path("obj") / "main.o", .packageName = "demo", + .localIncludeDirs = { std::filesystem::path("C:/proj/generated/inc") }, + .localIncludeDirsAfter = { std::filesystem::path("C:/proj/third_party/inc") }, }); CompileFlags flags; flags.cxxBinary = std::filesystem::path("/usr/bin/g++"); auto j = nlohmann::json::parse(emit_compile_commands(plan, flags)); auto const& e = j[0]; + + // Assert on the two include tokens by name. The earlier shape of this + // test ("no `-`-prefixed argument may contain `/`") looked stronger but + // was vacuous — with no flags configured, the only `-` tokens are the + // bare `-c` and `-o`, which have no path in them at all. + auto arg_with = [&](std::string_view pre) { + for (auto const& a : e["arguments"]) { + auto s = a.get(); + if (s.starts_with(pre)) return s; + } + return std::string{}; + }; + const auto inc = arg_with("-I"); + const auto incAfter = arg_with("-idirafter"); + if constexpr (std::filesystem::path::preferred_separator == '\\') { EXPECT_EQ(e["file"].get(), "C:\\Users\\x\\src\\main.cpp"); EXPECT_EQ(e["directory"].get(), "\\p"); EXPECT_EQ(e["output"].get(), "\\p\\target\\obj\\main.o"); - for (auto const& a : e["arguments"]) { - if (a.is_string() && a.get().starts_with("-")) - EXPECT_EQ(a.get().find('/'), std::string::npos); - } + EXPECT_EQ(inc, "-IC:\\proj\\generated\\inc"); + EXPECT_EQ(incAfter, "-idirafterC:\\proj\\third_party\\inc"); } else { EXPECT_EQ(e["file"].get(), "C:/Users/x/src/main.cpp"); + EXPECT_EQ(inc, "-IC:/proj/generated/inc"); + EXPECT_EQ(incAfter, "-idirafterC:/proj/third_party/inc"); } } diff --git a/tests/unit/test_ninja_backend.cpp b/tests/unit/test_ninja_backend.cpp index 894cac41..2ae7014b 100644 --- a/tests/unit/test_ninja_backend.cpp +++ b/tests/unit/test_ninja_backend.cpp @@ -173,6 +173,32 @@ TEST(NinjaBackend, CxxFlagsIncludeBuildIncludeDirs) { << flags.cxx; } +// #390: the NASM include list is built from the SAME `[build] include_dirs` +// key as the C/C++ one, so it must absolutize and spell entries identically — +// it used to re-derive the join on its own (and with a different "already +// rooted?" predicate). Nothing here is nasm-specific except the channel: the +// point is that one manifest key cannot produce two different paths. +TEST(NinjaBackend, NasmIncludeDirsMatchTheCxxChannelSpelling) { + auto plan = minimal_plan(); + plan.nasmPath = "/usr/bin/nasm"; + plan.manifest.buildConfig.includeDirs = {"third_party/imgui"}; + plan.manifest.buildConfig.includeDirsAfter = {"generated/inc"}; + + auto flags = compute_flags(plan); + + auto native = [](std::filesystem::path p) { p.make_preferred(); return p; }; + auto imgui = native(plan.projectRoot / "third_party" / "imgui"); + auto gen = native(plan.projectRoot / "generated" / "inc"); + + // Absolutized against projectRoot, natively spelt, and -I for BOTH keys + // (nasm has no system-header chain to defer to, so after-dirs degrade). + EXPECT_NE(flags.nasm.find(escaped_include_flag(imgui)), std::string::npos) + << flags.nasm; + EXPECT_NE(flags.nasm.find(escaped_include_flag(gen)), std::string::npos) + << flags.nasm; + EXPECT_EQ(flags.nasm.find("-idirafter"), std::string::npos) << flags.nasm; +} + // #249: a compile unit's localIncludeDirsAfter emit as -idirafter into the // same $local_includes variable, APPENDED after the -I entries. -idirafter // dirs are searched after the toolchain's system dirs (gcc+clang), so a dep