Skip to content

Commit 8aa42f7

Browse files
SunrisepeakSunrisepeak
committed
fix(build): architecture-review pass — strict policy actually enforced, .s depfile split, feature axis folds through append
Three findings from the whole-branch review, two of them defects introduced by this batch. 1. diag::flush() was never called outside its own unit test. The one new degradation this branch introduces (#257's windows + non-MSVC "no depfile" case) would print its warning and then be ignored, and `mcpp build --strict` would still exit 0. The channel built to stop silent degradation was itself silently degrading. BuildContext now carries `strict` and run_build_plan calls flush() after the build — after, because backend emission is where degradations are discovered, so end-of-prepare_build would be too early. 2. `-MMD` was applied to `.s` as well as `.S`. The C driver preprocesses `.S` but not `.s`, so clang emits two `argument unused during compilation` warnings per file and writes no depfile (measured on 20.1.7; gcc writes none either, just quietly) — and ninja's `deps = gcc` treats an absent depfile as an error, so the cases cannot share a rule. Split into asm_object (.S, tracked) and asm_object_raw (.s, pre-#257 shape). 3. The feature axis was still folding its per-glob flags field-by-field rather than through append(BuildInputs&), so the "one funnel for both axes" claim held for only one axis. Now routed through append. The other two feature tables stay out on purpose and the comment says why: feature `sources` carry DROP-then-ADD semantics, and feature `defines` are interface contributions that propagate along Public edges — neither is a plain append, so neither belongs in BuildInputs. Also fixes an ordering bug found while re-reading the #254 change: the target platform was computed at the top of prepare_build, but overrides.target_triple is only filled in from `[build] target` / the config default and canonicalized some 200 lines later. Any project that sets its target in the manifest rather than on the command line would have silently fallen back to the host axis — the exact bug #254 is about, reintroduced one level up. Moved below the resolution block, with a comment recording why it cannot move back.
1 parent e84e8fb commit 8aa42f7

6 files changed

Lines changed: 250 additions & 12 deletions

File tree

‎src/build/execute.cppm‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ export module mcpp.build.execute;
1010

1111
import std;
1212
import mcpp.build.prepare;
13+
import mcpp.diag;
1314
import mcpp.build.plan;
1415
import mcpp.build.backend;
1516
import mcpp.build.ninja;
@@ -334,6 +335,14 @@ export int run_build_plan(BuildContext& ctx, bool verbose, bool no_cache,
334335
std::move(runTargets), runEnvKey, runEnvValue);
335336
}
336337

338+
// The one place the --strict policy is settled. Degradations reported by
339+
// the backend (e.g. a toolchain/platform combination that cannot emit a
340+
// depfile, #257) are discovered during emission, so this has to come
341+
// after the build rather than at the end of prepare_build. Without this
342+
// call the whole diag channel would report and then be ignored — the
343+
// exact failure mode it exists to prevent.
344+
if (!mcpp::diag::flush(ctx.strict)) return 1;
345+
337346
mcpp::ui::finished("release", r->elapsed);
338347
return 0;
339348
}

‎src/build/ninja_backend.cppm‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -636,14 +636,25 @@ std::string emit_ninja_string(const BuildPlan& plan) {
636636
// GAS assembly (.S/.s) through the C driver: it preprocesses .S (cpp)
637637
// and assembles both, dispatching by extension. $asmflags is the
638638
// asm-safe flag subset (no -std / no -O — see flags.cppm).
639-
append("rule asm_object\n");
639+
// TWO rules, split by case: the C driver preprocesses `.S` but not
640+
// `.s`. Asking for a depfile on a `.s` unit is not merely useless —
641+
// clang emits `argument unused during compilation: '-MMD'` for every
642+
// such file and writes nothing, and ninja's `deps = gcc` treats an
643+
// absent depfile as an error. So `.s` keeps the pre-#257 shape.
640644
const std::string payload = " $local_includes $asmflags $unit_asmflags";
645+
append("rule asm_object\n"); // .S — preprocessed, tracks #include
641646
append(std::format(" command = $cc{} {}{}\n",
642647
rsp_ref(payload), c_mmd_flag, compile_tail));
643648
append_rspfile(payload);
644649
append(" description = AS $out\n");
645650
append_cxx_deps();
646651
append("\n");
652+
653+
append("rule asm_object_raw\n"); // .s — not preprocessed, no depfile
654+
append(std::format(" command = $cc{} {}\n",
655+
rsp_ref(payload), compile_tail));
656+
append_rspfile(payload);
657+
append(" description = AS $out\n\n");
647658
}
648659

649660
if (need_nasm_rule) {
@@ -803,8 +814,10 @@ std::string emit_ninja_string(const BuildPlan& plan) {
803814
return "cxx_module";
804815
if (ext == ".c" || ext == ".m")
805816
return "c_object";
806-
if (ext == ".S" || ext == ".s")
817+
if (ext == ".S")
807818
return "asm_object";
819+
if (ext == ".s")
820+
return "asm_object_raw";
808821
if (ext == ".asm")
809822
return "nasm_object";
810823
return "cxx_object";

‎src/build/prepare.cppm‎

Lines changed: 33 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -547,6 +547,11 @@ bool graph_or_targets_import_std(const mcpp::modgraph::Graph& graph,
547547
}
548548

549549
export struct BuildContext {
550+
// --strict: degradations reported through mcpp::diag become errors.
551+
// Carried on the context because the build's degradations are discovered
552+
// during backend emission, i.e. after prepare_build has returned — the
553+
// single place that settles the policy is run_build_plan (execute.cppm).
554+
bool strict = false;
550555
mcpp::manifest::Manifest manifest;
551556
mcpp::toolchain::Toolchain tc;
552557
mcpp::toolchain::Fingerprint fp;
@@ -596,15 +601,6 @@ prepare_build(bool print_fingerprint,
596601
return std::unexpected("no mcpp.toml found in current directory or any parent");
597602
}
598603

599-
// #254: everything compiled INTO this build is resolved for the TARGET —
600-
// an xpkg descriptor's per-OS sections (sources, flags, deps) and its xpm
601-
// asset/version table all describe code that will run on the target, not
602-
// on the machine building it. This used to be a compile-time host
603-
// constant, which is invisible natively (host == target) and picks the
604-
// wrong leg under --target.
605-
const auto targetPlatform = mcpp::platform::TargetPlatform::for_os(
606-
cfgpred::context_for(overrides.target_triple).os);
607-
608604
auto m = mcpp::manifest::load(*root / "mcpp.toml");
609605
if (!m) return std::unexpected(m.error().format());
610606

@@ -902,6 +898,21 @@ prepare_build(bool print_fingerprint,
902898
}
903899
if (overrides.force_static) m->buildConfig.linkage = "static";
904900

901+
// #254: everything compiled INTO this build is resolved for the TARGET —
902+
// an xpkg descriptor's per-OS sections (sources, flags, deps) and its xpm
903+
// asset/version table all describe code that will run on the target, not
904+
// on the machine building it. Previously a compile-time host constant,
905+
// which is invisible natively (host == target) and picks the wrong leg
906+
// under --target.
907+
//
908+
// Computed HERE, not earlier: `overrides.target_triple` is only complete
909+
// above — it is filled from `[build] target` and the config default, then
910+
// canonicalized. Reading it before that point would silently fall back to
911+
// the host for any project that sets its target in the manifest rather
912+
// than on the command line.
913+
const auto targetPlatform = mcpp::platform::TargetPlatform::for_os(
914+
cfgpred::context_for(overrides.target_triple).os);
915+
905916
// ── L1: merge conditional [target.'cfg(...)'.build] sources/flags AND
906917
// root-only [target.'cfg(...)'.dependencies] ─────────────────────────────
907918
// Evaluated now (target resolved) against the resolved target — the
@@ -3009,13 +3020,24 @@ prepare_build(bool print_fingerprint,
30093020
// sources ADD above, `mcpp build` and `mcpp test` must agree
30103021
// (0.0.94 dual-path invariant). featureOrigin tags the entry so
30113022
// the scanner's zero-hit warning can name the owning feature.
3023+
//
3024+
// Routed through the SAME append(BuildInputs&) the cfg axis uses
3025+
// (#258): both axes are contributing additive build inputs, so
3026+
// "how does a contribution combine with the base" must have one
3027+
// answer. Only the flags half of the feature axis is expressible
3028+
// that way — feature `sources` above carry DROP-then-ADD
3029+
// semantics, and feature `defines` are interface contributions
3030+
// that propagate along Public edges, so neither is a plain
3031+
// append and neither belongs in BuildInputs.
30123032
for (auto& [f, entries] : bc.featureFlags) {
30133033
if (std::ranges::find(active, f) == active.end()) continue;
3034+
mcpp::manifest::BuildInputs contribution;
30143035
for (auto const& gf : entries) {
30153036
auto tagged = gf;
30163037
tagged.featureOrigin = f;
3017-
bc.globFlags.push_back(std::move(tagged));
3038+
contribution.globFlags.push_back(std::move(tagged));
30183039
}
3040+
mcpp::manifest::append(bc, contribution);
30193041
}
30203042
};
30213043
if (!packages.empty()) {
@@ -3450,6 +3472,7 @@ prepare_build(bool print_fingerprint,
34503472
}
34513473

34523474
BuildContext ctx;
3475+
ctx.strict = overrides.strict;
34533476
ctx.manifest = *m;
34543477
ctx.tc = *tc;
34553478
ctx.fp = fp;

‎tests/e2e/151_cross_per_os_xpkg_splice.sh‎

Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,125 @@
1+
#!/usr/bin/env bash
2+
# requires: mingw-cross
3+
# #254: an xpkg dependency's per-OS section must be spliced for the RESOLVED
4+
# TARGET, not for the host.
5+
#
6+
# The per-OS sections carry sources, flags, deps and the xpm asset table —
7+
# all describing code compiled INTO the user's build, so they belong to the
8+
# platform the artifacts will run on. They used to key on a compile-time HOST
9+
# constant, so a Linux -> Windows cross build silently got the linux leg.
10+
#
11+
# Native builds cannot observe this (host == target), which is exactly why
12+
# the three-platform CI never caught it: this test has to cross-compile to
13+
# see the difference at all. It uses a local path-index descriptor so it
14+
# needs no network and no real package.
15+
set -e
16+
17+
TRIPLE=x86_64-windows-gnu
18+
19+
TMP=$(mktemp -d)
20+
trap "rm -rf $TMP" EXIT
21+
cd "$TMP"
22+
23+
# A local xpkg index with one descriptor whose per-OS sections differ in a
24+
# way the compiler can prove: each leg defines its own macro, and the TU
25+
# errors out unless exactly the windows leg was spliced.
26+
mkdir -p idx/pkgs/t
27+
cat > idx/pkgs/t/testsplice.lua <<'LUA'
28+
package = {
29+
name = "testsplice",
30+
namespace = "test",
31+
}
32+
33+
xpm = {
34+
linux = { ["1.0.0"] = { url = "unused", sha256 = "unused" } },
35+
windows = { ["1.0.0"] = { url = "unused", sha256 = "unused" } },
36+
macosx = { ["1.0.0"] = { url = "unused", sha256 = "unused" } },
37+
}
38+
39+
mcpp = {
40+
sources = { "src/**/*.cpp" },
41+
include_dirs = { "include" },
42+
linux = { cxxflags = { "-DSPLICED_LEG=1" } },
43+
macosx = { cxxflags = { "-DSPLICED_LEG=2" } },
44+
windows = { cxxflags = { "-DSPLICED_LEG=3" } },
45+
}
46+
LUA
47+
48+
# The "installed" payload for that descriptor.
49+
PAYLOAD="$TMP/payload/test-x-testsplice/1.0.0"
50+
mkdir -p "$PAYLOAD/src" "$PAYLOAD/include"
51+
cat > "$PAYLOAD/include/splice.h" <<'EOF'
52+
#pragma once
53+
int spliced_leg();
54+
EOF
55+
cat > "$PAYLOAD/src/splice.cpp" <<'EOF'
56+
#include "splice.h"
57+
#ifndef SPLICED_LEG
58+
#error "no per-OS section was spliced at all"
59+
#endif
60+
int spliced_leg() { return SPLICED_LEG; }
61+
EOF
62+
touch "$PAYLOAD/.mcpp_ok"
63+
64+
"$MCPP" new crosssplice > /dev/null
65+
cd crosssplice
66+
67+
cat > src/main.cpp <<'EOF'
68+
#include "splice.h"
69+
int main() {
70+
// 3 == the windows leg. Anything else means the wrong per-OS section
71+
// was spliced for this target.
72+
return spliced_leg() == 3 ? 0 : 1;
73+
}
74+
EOF
75+
76+
cat > mcpp.toml <<EOF
77+
[package]
78+
name = "crosssplice"
79+
version = "0.1.0"
80+
81+
[dependencies]
82+
"test.testsplice" = "1.0.0"
83+
EOF
84+
85+
# Point mcpp at the local index + pre-installed payload.
86+
export MCPP_HOME="$TMP/home"
87+
mkdir -p "$MCPP_HOME/registry/data"
88+
cp -r "$TMP/idx" "$MCPP_HOME/registry/data/xim-pkgindex"
89+
mkdir -p "$MCPP_HOME/registry/data/xpkgs"
90+
cp -r "$TMP/payload/test-x-testsplice" "$MCPP_HOME/registry/data/xpkgs/"
91+
92+
set +e
93+
"$MCPP" build --target "$TRIPLE" > build.log 2>&1
94+
RC=$?
95+
set -e
96+
97+
if [[ $RC -ne 0 ]]; then
98+
# A wrongly-spliced leg shows up as SPLICED_LEG=1 (linux) reaching the
99+
# windows TU; distinguish that from an unrelated setup failure.
100+
if grep -q "SPLICED_LEG" build.log; then
101+
cat build.log
102+
echo "FAIL: the wrong per-OS section was spliced for target $TRIPLE"
103+
exit 1
104+
fi
105+
echo "SKIP: cross build could not run in this environment"
106+
sed -n '1,15p' build.log
107+
exit 0
108+
fi
109+
110+
# The build succeeding is not enough — prove the windows leg's flag is what
111+
# landed, by checking the recorded compile command for the dep TU.
112+
CC_JSON=$(find target -name compile_commands.json | head -1)
113+
if [[ -n "$CC_JSON" ]]; then
114+
grep -q "SPLICED_LEG=3" "$CC_JSON" || {
115+
grep -o "SPLICED_LEG=[0-9]" "$CC_JSON" | sort -u
116+
echo "FAIL: dep TU was not compiled with the windows leg's flags"
117+
exit 1
118+
}
119+
grep -q "SPLICED_LEG=1" "$CC_JSON" && {
120+
echo "FAIL: the host (linux) leg leaked into a $TRIPLE build"
121+
exit 1
122+
}
123+
fi
124+
125+
echo "OK"

‎tests/unit/test_diag.cpp‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,3 +79,24 @@ TEST_F(DiagTest, FlushClearsRunState) {
7979
EXPECT_EQ(records().size(), 0u);
8080
EXPECT_EQ(count(Severity::Degraded), 0u);
8181
}
82+
83+
// Regression guard for the review finding on this batch: flush() is the ONLY
84+
// place the --strict policy is settled, and it was initially never called
85+
// outside this file — the channel reported degradations and then everyone
86+
// ignored them, which is precisely the failure mode it exists to prevent.
87+
// run_build_plan now calls it; this pins the contract flush() must honour.
88+
TEST_F(DiagTest, FlushIsTheSolePolicyPointAndReportsFailureToTheCaller) {
89+
// No records at all: strict must not fail a clean build.
90+
EXPECT_TRUE(flush(/*strict=*/true));
91+
92+
// A degradation under --strict must tell the caller to fail. The caller
93+
// (run_build_plan) turns this into a non-zero exit.
94+
degraded("build/depfile", "no depfile on this toolchain",
95+
"stale BMI possible after editing an included file");
96+
EXPECT_FALSE(flush(/*strict=*/true));
97+
98+
// Same degradation without --strict: reported, not fatal.
99+
degraded("build/depfile", "no depfile on this toolchain",
100+
"stale BMI possible after editing an included file");
101+
EXPECT_TRUE(flush(/*strict=*/false));
102+
}

‎tests/unit/test_ninja_backend.cpp‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -727,6 +727,8 @@ TEST(NinjaBackend, CAndAsmRulesAlsoTrackHeaderDeps) {
727727
auto ninja = emit_ninja_string(plan);
728728

729729
for (std::string_view rule : {"rule c_object\n", "rule asm_object\n"}) {
730+
// NOTE: asm_object is the `.S` rule. `.s` uses asm_object_raw and
731+
// deliberately has no depfile — see below.
730732
auto start = ninja.find(rule);
731733
ASSERT_NE(start, std::string::npos) << rule << "\n" << ninja;
732734
auto end = ninja.find("\n\n", start);
@@ -741,3 +743,48 @@ TEST(NinjaBackend, CAndAsmRulesAlsoTrackHeaderDeps) {
741743
EXPECT_EQ(body.find("awk"), std::string::npos) << body;
742744
}
743745
}
746+
747+
// The C driver preprocesses `.S` but not `.s`, so only `.S` can produce a
748+
// depfile. Asking anyway makes clang emit "argument unused during
749+
// compilation: '-MMD'" for every such file and write nothing, and ninja's
750+
// `deps = gcc` treats an absent depfile as an error — so the two cases need
751+
// separate rules.
752+
TEST(NinjaBackend, LowercaseAsmHasNoDepfileAndItsOwnRule) {
753+
if constexpr (mcpp::platform::is_windows)
754+
GTEST_SKIP() << "POSIX depfile shape only";
755+
756+
auto plan = minimal_plan();
757+
plan.compileUnits.push_back({
758+
.source = "src/upper.S",
759+
.object = "obj/upper.o",
760+
.packageName = "objc_rule_test",
761+
});
762+
plan.compileUnits.push_back({
763+
.source = "src/lower.s",
764+
.object = "obj/lower.o",
765+
.packageName = "objc_rule_test",
766+
});
767+
768+
auto ninja = emit_ninja_string(plan);
769+
770+
auto body_of = [&](std::string_view rule) {
771+
auto start = ninja.find(rule);
772+
EXPECT_NE(start, std::string::npos) << rule << "\n" << ninja;
773+
auto end = ninja.find("\n\n", start);
774+
return ninja.substr(start, end - start);
775+
};
776+
777+
auto upper = body_of("rule asm_object\n");
778+
EXPECT_NE(upper.find("-MMD -MF $out.d"), std::string::npos) << upper;
779+
EXPECT_NE(upper.find("depfile = $out.d"), std::string::npos) << upper;
780+
781+
auto lower = body_of("rule asm_object_raw\n");
782+
EXPECT_EQ(lower.find("-MMD"), std::string::npos) << lower;
783+
EXPECT_EQ(lower.find("depfile"), std::string::npos) << lower;
784+
785+
// And the edges must be routed to the matching rule.
786+
EXPECT_NE(ninja.find("build obj/upper.o : asm_object src/upper.S"),
787+
std::string::npos) << ninja;
788+
EXPECT_NE(ninja.find("build obj/lower.o : asm_object_raw src/lower.s"),
789+
std::string::npos) << ninja;
790+
}

0 commit comments

Comments
 (0)