Skip to content

Commit 8eedf15

Browse files
committed
fix(build): #261 keep only $local_includes in the response file — CI caught the #247 trap one level deeper
The Windows leg failed with module file 'D:amcppmcpptargetx86_64-windows-msvcc1ffdc8b90b687f4pcm.cachestd.pcm' not found i.e. D:\a\mcpp\mcpp\target\...\pcm.cache\std.pcm with every separator eaten. Response-file content is tokenized GNU-style, where backslash is an ESCAPE character. Switching escape_flag_path to generic_string() covered the -I paths this file builds itself, but $cxxflags carries paths produced elsewhere — flags.cppm's -fprebuilt-module-path= and module-file mappings — which are still native-separated. Moving those off the command line broke every `import std;`. The fix is to scope the response file to what this file can guarantee the form of: $local_includes only. $cxxflags / $unit_cxxflags / $asmflags stay inline, where a backslash is just a character. That is also sufficient for #261: the unbounded axis is one -I per dependency include dir, and the rest of the payload is bounded. Making every flag producer emit forward slashes would be the more general fix, but it is a much wider change than this batch should carry, and it is not needed for the ceiling. Only the `mcpp test` step failed, not the build: the build step runs the bootstrap mcpp, so the freshly-built binary is first exercised when it compiles the test targets. Also makes e2e 118 use a portable in-place edit. Dropping its `# requires: gcc` gate in #257 meant it ran on macOS for the first time, where BSD sed reads `-i`'s next argument as a backup suffix and swallows the script. The other three new e2e tests pass on macOS as-is.
1 parent 58294b4 commit 8eedf15

3 files changed

Lines changed: 51 additions & 21 deletions

File tree

src/build/ninja_backend.cppm

Lines changed: 28 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -554,6 +554,18 @@ std::string emit_ninja_string(const BuildPlan& plan) {
554554
//
555555
// POSIX keeps the inline form byte-identical: ARG_MAX is ample and an
556556
// inline command is far easier to re-run by hand when debugging.
557+
//
558+
// ONLY $local_includes goes in. Response-file content is tokenized
559+
// GNU-style, where backslash is an ESCAPE character, so every path that
560+
// moves off the command line must be forward-slashed — and the only
561+
// paths whose form this file controls are the -I/-idirafter entries it
562+
// builds via escape_flag_path(). $cxxflags carries paths produced
563+
// elsewhere (flags.cppm's -fprebuilt-module-path=, module-file mappings)
564+
// that are still native-separated; routing those through the rsp ate the
565+
// separators of the std.pcm path and broke every `import std;` on the
566+
// Windows CI leg. They stay inline, where a backslash is just a
567+
// character. Sufficient, too: the unbounded axis #261 is about is one -I
568+
// per dependency include dir, and the rest of the payload is bounded.
557569
// Also keyed on the msvc dialect, which only ever runs on Windows: it
558570
// makes the response-file shape reachable from a non-Windows test host,
559571
// the same over-approximation the link rules use (`separateLinker ||
@@ -576,11 +588,10 @@ std::string emit_ninja_string(const BuildPlan& plan) {
576588
append("rule cxx_module\n");
577589
if constexpr (mcpp::platform::is_windows) {
578590
// Windows: skip BMI restat optimization (requires POSIX shell).
579-
const auto payload = std::format(
580-
" $local_includes $cxxflags $unit_cxxflags{}{}",
581-
module_output_flag, module_src_flags);
582-
append(std::format(" command = $cxx{} {}\n",
583-
rsp_ref(payload), compile_tail));
591+
const std::string payload = " $local_includes";
592+
append(std::format(" command = $cxx{} $cxxflags $unit_cxxflags{}{} {}\n",
593+
rsp_ref(payload), module_output_flag,
594+
module_src_flags, compile_tail));
584595
append_rspfile(payload);
585596
append_cxx_deps();
586597
} else {
@@ -604,8 +615,8 @@ std::string emit_ninja_string(const BuildPlan& plan) {
604615

605616
append("rule cxx_object\n");
606617
if constexpr (mcpp::platform::is_windows) {
607-
const std::string payload = " $local_includes $cxxflags $unit_cxxflags";
608-
append(std::format(" command = $cxx{} {}\n",
618+
const std::string payload = " $local_includes";
619+
append(std::format(" command = $cxx{} $cxxflags $unit_cxxflags {}\n",
609620
rsp_ref(payload), compile_tail));
610621
append_rspfile(payload);
611622
} else {
@@ -621,8 +632,8 @@ std::string emit_ninja_string(const BuildPlan& plan) {
621632

622633
if (need_c_rule) {
623634
append("rule c_object\n");
624-
const std::string payload = " $local_includes $cflags $unit_cflags";
625-
append(std::format(" command = $cc{} {}{}\n",
635+
const std::string payload = " $local_includes";
636+
append(std::format(" command = $cc{} $cflags $unit_cflags {}{}\n",
626637
rsp_ref(payload), c_mmd_flag, compile_tail));
627638
append_rspfile(payload);
628639
append(" description = CC $out\n");
@@ -641,17 +652,17 @@ std::string emit_ninja_string(const BuildPlan& plan) {
641652
// clang emits `argument unused during compilation: '-MMD'` for every
642653
// such file and writes nothing, and ninja's `deps = gcc` treats an
643654
// absent depfile as an error. So `.s` keeps the pre-#257 shape.
644-
const std::string payload = " $local_includes $asmflags $unit_asmflags";
655+
const std::string payload = " $local_includes";
645656
append("rule asm_object\n"); // .S — preprocessed, tracks #include
646-
append(std::format(" command = $cc{} {}{}\n",
657+
append(std::format(" command = $cc{} $asmflags $unit_asmflags {}{}\n",
647658
rsp_ref(payload), c_mmd_flag, compile_tail));
648659
append_rspfile(payload);
649660
append(" description = AS $out\n");
650661
append_cxx_deps();
651662
append("\n");
652663

653664
append("rule asm_object_raw\n"); // .s — not preprocessed, no depfile
654-
append(std::format(" command = $cc{} {}\n",
665+
append(std::format(" command = $cc{} $asmflags $unit_asmflags {}\n",
655666
rsp_ref(payload), compile_tail));
656667
append_rspfile(payload);
657668
append(" description = AS $out\n\n");
@@ -733,16 +744,16 @@ std::string emit_ninja_string(const BuildPlan& plan) {
733744
// The scan command is strictly LONGER than the compile command for
734745
// the same TU (it wraps it), so it carries the same unbounded
735746
// include payload through the same response file (#261).
736-
const std::string scanPayload = " $local_includes $cxxflags $unit_cxxflags";
747+
const std::string scanPayload = " $local_includes";
737748
if (msvcDeps) {
738749
// MSVC: compiler-integrated P1689 via /scanDependencies (scan
739750
// only — no codegen); /TP because our module units are .cppm.
740-
append(std::format(" command = $cxx{} "
751+
append(std::format(" command = $cxx{} $cxxflags $unit_cxxflags "
741752
"/scanDependencies $out /TP /c $in /Fo:$compile_target\n",
742753
rsp_ref(scanPayload)));
743754
} else if (plan.scanDepsPath.empty()) {
744755
// GCC path: compiler-integrated P1689 scanning.
745-
append(std::format(" command = $cxx{} -fmodules "
756+
append(std::format(" command = $cxx{} $cxxflags $unit_cxxflags -fmodules "
746757
"-fdeps-format=p1689r5 "
747758
"-fdeps-file=$out -fdeps-target=$compile_target "
748759
"-M -MM -MF $out.dep -E $in -o $compile_target\n",
@@ -757,7 +768,8 @@ std::string emit_ninja_string(const BuildPlan& plan) {
757768
// overruns (#261: 48 -I entries at a deep consumer path).
758769
append(std::format(
759770
" command = $scan_deps -format=p1689 -o $out -- "
760-
"$cxx{} -c $in -o $compile_target\n", rsp_ref(scanPayload)));
771+
"$cxx{} $cxxflags $unit_cxxflags -c $in -o $compile_target\n",
772+
rsp_ref(scanPayload)));
761773
}
762774
append_rspfile(scanPayload);
763775
append(" description = SCAN $out\n\n");

tests/e2e/118_purview_include_rebuild.sh

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,13 @@ set -e
1717
# Resolved before any cd: the script changes directory below.
1818
SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)"
1919

20+
# Portable in-place edit. This test lost its `# requires: gcc` gate in #257,
21+
# so it now runs on macOS too, where BSD sed reads `-i`'s next argument as a
22+
# backup suffix and swallows the script.
23+
subst() { # subst <sed-expr> <file>
24+
sed "$1" "$2" > "$2.tmp" && mv "$2.tmp" "$2"
25+
}
26+
2027
TMP=$(mktemp -d)
2128
trap "rm -rf $TMP" EXIT
2229

@@ -51,7 +58,7 @@ EOF
5158
out="$("$MCPP" run 2>&1 | tail -1)"
5259
[[ "$out" == "41" ]] || { echo "unexpected initial output: $out"; exit 1; }
5360

54-
sed -i 's/41/42/' src/vals.inc
61+
subst 's/41/42/' src/vals.inc
5562

5663
out="$("$MCPP" run 2>&1 | tail -1)"
5764
[[ "$out" == "42" ]] || {
@@ -83,7 +90,7 @@ EOF
8390
out="$("$MCPP" run 2>&1 | tail -1)"
8491
[[ "$out" == "42 100" ]] || { echo "unexpected output before header edit: $out"; exit 1; }
8592

86-
sed -i 's/100/200/' src/helper.h
93+
subst 's/100/200/' src/helper.h
8794

8895
out="$("$MCPP" run 2>&1 | tail -1)"
8996
[[ "$out" == "42 200" ]] || {
@@ -130,7 +137,7 @@ EOF
130137
out="$("$MCPP" run 2>&1 | tail -1)"
131138
[[ "$out" == "41" ]] || { echo "clang: unexpected initial output: $out"; exit 1; }
132139

133-
sed -i 's/41/42/' src/vals.inc
140+
subst 's/41/42/' src/vals.inc
134141

135142
out="$("$MCPP" run 2>&1 | tail -1)"
136143
[[ "$out" == "42" ]] || {
@@ -158,7 +165,7 @@ EOF
158165
out="$("$MCPP" run 2>&1 | tail -1)"
159166
[[ "$out" == "42 100" ]] || { echo "clang: unexpected output before header edit: $out"; exit 1; }
160167

161-
sed -i 's/100/200/' src/helper.h
168+
subst 's/100/200/' src/helper.h
162169

163170
out="$("$MCPP" run 2>&1 | tail -1)"
164171
[[ "$out" == "42 200" ]] || {

tests/unit/test_ninja_backend.cpp

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -637,13 +637,24 @@ TEST(NinjaBackend, CompileAndScanRulesRouteFlagsThroughRspfileUnderMsvcDialect)
637637

638638
EXPECT_NE(body.find("@$out.rsp"), std::string::npos) << body;
639639
EXPECT_NE(body.find("rspfile = $out.rsp"), std::string::npos) << body;
640-
EXPECT_NE(body.find("rspfile_content = $local_includes"),
640+
// ONLY $local_includes may live in the response file: its content
641+
// is tokenized GNU-style (backslash = escape), and those are the
642+
// only paths this file forward-slashes itself. $cxxflags carries
643+
// native-separated paths from flags.cppm and must stay inline —
644+
// routing it through the rsp ate the separators of the std.pcm path
645+
// and broke every `import std;` on Windows.
646+
EXPECT_NE(body.find("rspfile_content = $local_includes\n"),
641647
std::string::npos) << body;
642648
// The payload must not ALSO remain inline, or the ceiling stands.
643649
auto cmdStart = body.find("command = ");
644650
auto cmdEnd = body.find('\n', cmdStart);
645651
auto cmd = body.substr(cmdStart, cmdEnd - cmdStart);
646652
EXPECT_EQ(cmd.find("$local_includes"), std::string::npos) << cmd;
653+
// ...and the flags that must NOT move off the command line stay there.
654+
if (rule != "rule c_object\n")
655+
EXPECT_NE(cmd.find("$cxxflags"), std::string::npos) << cmd;
656+
else
657+
EXPECT_NE(cmd.find("$cflags"), std::string::npos) << cmd;
647658
}
648659
}
649660

0 commit comments

Comments
 (0)