fix(flags): rewrite full -I/-iquote/-isystem include family to projec… · mcpp-community/mcpp@b71d93c · GitHub
Skip to content

Commit b71d93c

Browse files
SunrisepeakSunrisepeak
committed
fix(flags): rewrite full -I/-iquote/-isystem include family to project root + shell-quote flag values with spaces (#226 #234)
1 parent 69b8b33 commit b71d93c

7 files changed

Lines changed: 317 additions & 29 deletions

File tree

‎src/build/flags.cppm‎

Lines changed: 70 additions & 4 deletions

‎src/build/ninja_backend.cppm‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,10 +97,17 @@ std::string local_include_flags(const CompileUnit& cu) {
9797
}
9898

9999
std::string join_flags(const std::vector<std::string>& flags) {
100+
// mcpp#234: each vector element is already one argv token (e.g. a
101+
// manifest define `T=long long` arrives here as the single element
102+
// `-DT=long long`, pushed whole by apply_glob_flags) — but joining with
103+
// a bare space and no quoting let the embedded space split it into two
104+
// words once ninja handed the resolved command line to the shell.
105+
// shell_quote_arg is a no-op for tokens with nothing shell-significant
106+
// (`-std=c++23`, `-O2`, ...), so plain flags are untouched.
100107
std::string out;
101108
for (auto const& flag : flags) {
102109
out += ' ';
103-
out += flag;
110+
out += shell_quote_arg(flag);
104111
}
105112
return out;
106113
}

‎src/build/plan.cppm‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -704,8 +704,8 @@ BuildPlan make_plan(const mcpp::manifest::Manifest& manifest,
704704
}
705705
// Root-relative -I flags → absolute (G8b), mirroring the scanner's
706706
// treatment of every scanned unit.
707-
mcpp::modgraph::absolutize_include_flags(projectRoot, main_cu.packageCflags);
708-
mcpp::modgraph::absolutize_include_flags(projectRoot, main_cu.packageCxxflags);
707+
mcpp::modgraph::normalize_include_flags(projectRoot, main_cu.packageCflags);
708+
mcpp::modgraph::normalize_include_flags(projectRoot, main_cu.packageCxxflags);
709709

710710
// We didn't scan main.cpp earlier (it's not in scanner output unless globbed in).
711711
// Best-effort: scan its imports here.

‎src/modgraph/scanner.cppm‎

Lines changed: 56 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -68,13 +68,18 @@ struct ScanResult {
6868
ScanResult scan_package(const std::filesystem::path& root,
6969
const mcpp::manifest::Manifest& manifest);
7070

71-
// Absolutize relative `-I<path>` compile flags against the package root
72-
// (G8b). A manifest's relative -I means root-relative, but ninja runs
73-
// commands with cwd = the output dir, so a verbatim relative flag resolves
74-
// against the wrong base. Called at every point where per-unit flag vectors
75-
// are attached (the scanner here; plan.cppm for a target's entry unit).
76-
void absolutize_include_flags(const std::filesystem::path& root,
77-
std::vector<std::string>& flags);
71+
// Absolutize relative include/lib-search-path flags against the package root
72+
// (G8b, generalized by #226). A manifest's relative include flag means
73+
// root-relative, but ninja runs commands with cwd = the output dir, so a
74+
// verbatim relative flag resolves against the wrong base. Recognizes the
75+
// whole include-family prefix set — -I, -iquote, -isystem, -idirafter,
76+
// -iprefix, -L — in BOTH the joined spelling (`-iquotehdr`) and the
77+
// separated spelling (`-isystem` followed by a standalone `hdr` element).
78+
// Called at every point where per-unit flag vectors are attached (the
79+
// scanner here; plan.cppm for a target's entry unit; flags.cppm for the
80+
// manifest-global [build] include_dirs).
81+
void normalize_include_flags(const std::filesystem::path& root,
82+
std::vector<std::string>& flags);
7883

7984
enum class DependencyVisibility {
8085
Private,
@@ -543,16 +548,45 @@ std::vector<std::filesystem::path> expand_dir_glob(const std::filesystem::path&
543548
return out;
544549
}
545550

546-
void absolutize_include_flags(const std::filesystem::path& root,
547-
std::vector<std::string>& flags)
551+
namespace {
552+
553+
// has_root_path: leave absolute AND root-relative ("/x" on Windows)
554+
// spellings alone — only genuinely root-less paths are project-relative.
555+
std::string rewrite_rel_copy(const std::string& p, const std::filesystem::path& root) {
556+
std::filesystem::path fp(p);
557+
if (fp.has_root_path()) return p;
558+
return (root / fp).string();
559+
}
560+
561+
void rewrite_rel(std::string& p, const std::filesystem::path& root) {
562+
p = rewrite_rel_copy(p, root);
563+
}
564+
565+
} // namespace
566+
567+
void normalize_include_flags(const std::filesystem::path& root,
568+
std::vector<std::string>& flags)
548569
{
549-
for (auto& f : flags) {
550-
if (f.size() > 2 && f.starts_with("-I")) {
551-
std::filesystem::path p(f.substr(2));
552-
// has_root_path: leave absolute AND root-relative ("/x" on
553-
// Windows) spellings alone — only genuinely root-less paths are
554-
// project-relative.
555-
if (!p.has_root_path()) f = "-I" + (root / p).string();
570+
// #226: the whole include/lib-search-path family, not just -I. Each
571+
// prefix is checked in both spellings:
572+
// joined: "-iquotehdr" (element starts_with prefix, has a tail)
573+
// separated: "-isystem", "hdr" (element == bare prefix, rewrite next)
574+
static constexpr std::string_view kIncPrefixes[] =
575+
{"-I", "-iquote", "-isystem", "-idirafter", "-iprefix", "-L"};
576+
577+
for (std::size_t i = 0; i < flags.size(); ++i) {
578+
for (auto pre : kIncPrefixes) {
579+
if (flags[i] == pre && i + 1 < flags.size()) { // separated
580+
rewrite_rel(flags[i + 1], root);
581+
++i;
582+
break;
583+
}
584+
if (flags[i].size() > pre.size() && flags[i].starts_with(pre)) { // joined
585+
std::string tail = flags[i].substr(pre.size());
586+
std::string abs = rewrite_rel_copy(tail, root);
587+
if (abs != tail) flags[i] = std::string(pre) + abs;
588+
break;
589+
}
556590
}
557591
}
558592
}
@@ -836,9 +870,9 @@ void scan_one_into(ScanResult& result,
836870
u.packageCflags = packageCflags;
837871
u.packageCxxflags = packageCxxflags;
838872
apply_glob_flags(u);
839-
absolutize_include_flags(root, u.packageCflags);
840-
absolutize_include_flags(root, u.packageCxxflags);
841-
absolutize_include_flags(root, u.packageAsmflags);
873+
normalize_include_flags(root, u.packageCflags);
874+
normalize_include_flags(root, u.packageCxxflags);
875+
normalize_include_flags(root, u.packageAsmflags);
842876
result.graph.units.push_back(std::move(u));
843877
continue;
844878
}
@@ -851,9 +885,9 @@ void scan_one_into(ScanResult& result,
851885
r->packageCflags = packageCflags;
852886
r->packageCxxflags = packageCxxflags;
853887
apply_glob_flags(*r);
854-
absolutize_include_flags(root, r->packageCflags);
855-
absolutize_include_flags(root, r->packageCxxflags);
856-
absolutize_include_flags(root, r->packageAsmflags);
888+
normalize_include_flags(root, r->packageCflags);
889+
normalize_include_flags(root, r->packageCxxflags);
890+
normalize_include_flags(root, r->packageAsmflags);
857891
result.graph.units.push_back(std::move(*r));
858892
}
859893

‎tests/e2e/116_include_family_and_define_spaces.sh‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
#!/usr/bin/env bash
2+
# requires: gcc
3+
# mcpp#226 + mcpp#234: the full include-family flag spelling (-I/-iquote/
4+
# -isystem/-idirafter/-iprefix/-L) must root-relativize, not just -I; and any
5+
# flag-vector token that contains a space (e.g. `-DT=long long`, one manifest
6+
# `defines` entry) must survive as ONE shell argument through emission, not
7+
# silently split into two.
8+
set -e
9+
10+
TMP=$(mktemp -d)
11+
trap "rm -rf $TMP" EXIT
12+
13+
cd "$TMP"
14+
"$MCPP" new incfam > /dev/null
15+
cd incfam
16+
17+
# --- Part 1: #226 — -iquote (joined spelling) resolves root-relative -------
18+
# hdr/magic.h is NOT next to the including .c file, so plain quote-include
19+
# lookup (current-dir-of-includer, then quote search path) only finds it if
20+
# -iquotehdr got rewritten to an absolute, root-relative "-iquote<root>/hdr".
21+
mkdir -p hdr
22+
cat > hdr/magic.h <<'EOF'
23+
#define MAGIC 42
24+
EOF
25+
26+
cat > src/magic_user.c <<'EOF'
27+
#include "magic.h"
28+
int get_magic(void) { return MAGIC; }
29+
EOF
30+
31+
cat > src/main.cpp <<'EOF'
32+
import std;
33+
extern "C" int get_magic();
34+
extern long long get_big();
35+
36+
int main() {
37+
std::println("magic={} big={}", get_magic(), get_big());
38+
return 0;
39+
}
40+
EOF
41+
42+
# --- Part 2: #234 — a flag-vector token containing a space must reach the
43+
# compiler as ONE argv token, not split on the space. T is injected via a
44+
# per-glob `defines = ["T=long long"]` targeting this file specifically. If
45+
# the space split the token, the compiler would see a bogus positional
46+
# "long" argument and this translation unit would fail to build.
47+
cat > src/typed_user.cpp <<'EOF'
48+
typedef T MyLong;
49+
long long get_big() {
50+
MyLong big = 123456789012LL;
51+
return static_cast<long long>(big);
52+
}
53+
EOF
54+
55+
cat > mcpp.toml <<'EOF'
56+
[package]
57+
name = "incfam"
58+
version = "0.1.0"
59+
60+
[build]
61+
cflags = ["-iquotehdr"]
62+
flags = [
63+
{ glob = "src/typed_user.cpp", defines = ["T=long long"] },
64+
]
65+
EOF
66+
67+
"$MCPP" build > build.log 2>&1 || { cat build.log; echo "build failed"; exit 1; }
68+
69+
out="$("$MCPP" run 2>&1 | tail -1)"
70+
[[ "$out" == "magic=42 big=123456789012" ]] || {
71+
echo "unexpected output: $out"; exit 1; }
72+
73+
echo "OK"

‎tests/unit/test_build_flags.cpp‎

Lines changed: 38 additions & 0 deletions

0 commit comments

Comments
 (0)