Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 34 additions & 8 deletions src/build/compile_commands.cppm
Original file line number Diff line number Diff line change
Expand Up @@ -142,16 +142,28 @@ std::vector<std::string> 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<std::string> local_include_args(const CompileUnit& cu) {
std::vector<std::string> 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;
}
Expand Down Expand Up @@ -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();
Expand All @@ -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;

Expand All @@ -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<std::string> freshFiles;
for (auto const& e : freshJ) {
if (e.contains("file") && e["file"].is_string())
freshFiles.insert(e["file"].get<std::string>());
freshFiles.insert(norm_key(e["file"].get<std::string>()));
}

// Keep fresh order, then append still-valid prior entries the plan doesn't
Expand All @@ -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<std::string>();
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);
}
Expand Down
5 changes: 4 additions & 1 deletion src/build/directives.cppm
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down
12 changes: 9 additions & 3 deletions src/build/flags.cppm
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,13 @@ CompileFlags compute_flags(const BuildPlan& plan) {
// once ninja hands the resolved command line to the shell.
std::vector<std::string> 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
Expand All @@ -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"));
}
Expand Down
36 changes: 33 additions & 3 deletions src/build/plan.cppm
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,14 @@ make_plan(const mcpp::manifest::Manifest& manifest,
// simply makes those units uncacheable.
const std::vector<std::filesystem::path>& 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<std::filesystem::path>
expand_manifest_include_entry(const std::filesystem::path& root,
const std::filesystem::path& inc);

} // namespace mcpp::build

namespace mcpp::build {
Expand Down Expand Up @@ -368,22 +376,42 @@ std::vector<std::string> 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
// (wildcard-free) entry expand_dir_glob only returns EXISTING directories,
// 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<std::filesystem::path>
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;
}

Expand Down Expand Up @@ -412,6 +440,8 @@ local_include_dirs_after_for_manifest(const std::filesystem::path& root,
return dirs;
}

namespace {

void append_unique_path(std::vector<std::filesystem::path>& out,
std::filesystem::path path)
{
Expand Down
13 changes: 11 additions & 2 deletions src/build/prepare.cppm
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -2996,7 +2997,13 @@ prepare_build(bool print_fingerprint,
std::vector<std::filesystem::path> dirs;
for (auto const& inc : manifest.buildConfig.includeDirs) {
if (inc.is_absolute()) {
appendUniquePath(dirs, inc);
// 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(
Expand All @@ -3016,7 +3023,9 @@ prepare_build(bool print_fingerprint,
std::vector<std::filesystem::path> dirs;
for (auto const& inc : manifest.buildConfig.includeDirsAfter) {
if (inc.is_absolute()) {
appendUniquePath(dirs, inc);
auto n = inc;
n.make_preferred();
appendUniquePath(dirs, std::move(n));
continue;
}
for (auto& dir : mcpp::modgraph::expand_dir_glob(
Expand Down
20 changes: 20 additions & 0 deletions src/modgraph/glob.cppm
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,26 @@ 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 (`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) {
std::filesystem::path p(s);
p.make_preferred();
return p;
}

// Does `candidate` match `glob`, interpreted relative to `root`?
//
// Supports "**" (any number of directory levels) and "*" (within one segment).
Expand Down
41 changes: 34 additions & 7 deletions src/modgraph/scanner.cppm
Original file line number Diff line number Diff line change
Expand Up @@ -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 `{`,
Expand Down Expand Up @@ -442,7 +447,9 @@ std::vector<std::filesystem::path> 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;
}
Expand Down Expand Up @@ -494,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) {
Expand Down Expand Up @@ -682,7 +697,14 @@ local_include_dirs_for(const std::filesystem::path& root,
std::vector<std::filesystem::path> 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). 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())) {
Expand All @@ -701,7 +723,9 @@ local_include_dirs_after_for(const std::filesystem::path& root,
std::vector<std::filesystem::path> dirs;
for (auto const& inc : manifest.buildConfig.includeDirsAfter) {
if (inc.is_absolute()) {
dirs.push_back(inc);
auto n = inc;
n.make_preferred();
dirs.push_back(std::move(n));
continue;
}
for (auto& d : expand_dir_glob(root, inc.generic_string())) {
Expand Down Expand Up @@ -738,7 +762,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;
Expand Down
21 changes: 21 additions & 0 deletions tests/e2e/47_cdb_prebuilt_module_path_abs.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
:
Expand Down
Loading
Loading