Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
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
10 changes: 8 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,11 @@ 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 (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(
Expand All @@ -3016,7 +3021,8 @@ 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);
appendUniquePath(dirs,
mcpp::modgraph::native_path_from_generic(inc.generic_string()));
continue;
}
for (auto& dir : mcpp::modgraph::expand_dir_glob(
Expand Down
24 changes: 24 additions & 0 deletions src/modgraph/glob.cppm
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down
23 changes: 18 additions & 5 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 @@ -682,7 +689,10 @@ 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). 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())) {
Expand All @@ -701,7 +711,7 @@ 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);
dirs.push_back(native_path_from_generic(inc.generic_string()));
continue;
}
for (auto& d : expand_dir_glob(root, inc.generic_string())) {
Expand Down Expand Up @@ -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;
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
24 changes: 23 additions & 1 deletion tests/e2e/76_compile_commands_generated.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
69 changes: 68 additions & 1 deletion tests/unit/test_modgraph.cpp
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#include <gtest/gtest.h>

import std;
import mcpp.modgraph.glob;
import mcpp.modgraph.graph;
import mcpp.modgraph.scanner;
import mcpp.modgraph.validate;
Expand Down Expand Up @@ -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");
Expand All @@ -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
Expand Down
Loading