Skip to content

Commit b4088c0

Browse files
committed
fix(build.mcpp): ask the prerequisite question before the cache is consulted
Self-review found the refusal sitting after the cache fast path returns. Withdrawing `reexport = true` from an edge leaves the module set and every interface hash identical, so nothing in `compilerIdentity` moves -- the cached program would be replayed and the now-illegal import would never be reported. Measured: it fires anyway today, because `ctxHash` happens to change too. That is an incidental coupling between two keys that answer different questions, and this codebase has paid for that shape repeatedly. Adding `importable` to the cache identity would have been a second place deriving the same decision; asking before the cache is consulted needs no key at all. `srcText` is hoisted to where the check now lives and read once. e2e 311 gains a section that runs on a warm cache, with the measurement above written into it: the assertion pins the behaviour, and the placement is what makes it hold.
1 parent dd8c254 commit b4088c0

2 files changed

Lines changed: 59 additions & 23 deletions

File tree

src/build/build_program.cppm

Lines changed: 36 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -703,6 +703,40 @@ std::expected<void, std::string> run_build_program(
703703
std::string programHash = mcpp::toolchain::hash_file(src);
704704
std::string compilerHash = mcpp::toolchain::hash_string(compilerIdentity);
705705

706+
// Read once, here, because the check below has to run BEFORE the cache
707+
// fast path returns.
708+
std::string srcText;
709+
{ std::ifstream is(src); std::ostringstream ss; ss << is.rdbuf(); srcText = ss.str(); }
710+
711+
// A module that is present only as another rule's prerequisite is not part
712+
// of this package's declared surface. Refusing the import rather than
713+
// letting it work is the difference between a rule that travels and one
714+
// that happens to build on whoever's machine: the same source stops
715+
// compiling the moment the intermediate rule stops depending on it, or the
716+
// moment a Clang user tries it, since only GCC makes the BMI reachable
717+
// without the flags.
718+
//
719+
// ⚠️ AHEAD OF THE FAST PATH ON PURPOSE. Withdrawing `reexport = true` from
720+
// an edge leaves the module SET and every interface hash identical, so
721+
// nothing in `compilerIdentity` moves. Measured: the replay is prevented
722+
// today only because `ctxHash` happens to change too — an incidental
723+
// coupling, and the shape this codebase keeps paying for. Asking the
724+
// question before the cache is consulted needs no key at all.
725+
for (auto const& hm : env.hostModules) {
726+
if (hm.importable) continue;
727+
if (!imports_module(srcText, hm.logical)) continue;
728+
return std::unexpected(std::format(
729+
"build.mcpp imports '{}', which reaches this build only as a "
730+
"prerequisite of another build rule.\n"
731+
" A rule's build-time provisions cross one further edge only "
732+
"when that edge says so.\n"
733+
" Either depend on the package providing '{}' directly, or "
734+
"have the rule that owns it re-export it:\n"
735+
" <that rule's package> = {{ ..., host-module = true, "
736+
"reexport = true }}",
737+
hm.logical, hm.logical));
738+
}
739+
706740
// Fast path: declared inputs + contract unchanged → reapply cached
707741
// directives, no run.
708742
CacheRecord cache = read_cache(bdir);
@@ -763,8 +797,8 @@ std::expected<void, std::string> run_build_program(
763797
// -fmodules, cwd = project root). When it does `import mcpp;`, compile the
764798
// module, link its object, and run the build.mcpp compile from `bdir` so GCC
765799
// finds gcm.cache/mcpp.gcm.
766-
std::string srcText;
767-
{ std::ifstream is(src); std::ostringstream ss; ss << is.rdbuf(); srcText = ss.str(); }
800+
// `srcText` was read above the cache fast path, which the prerequisite
801+
// check needs to run ahead of.
768802
bool usesModule = srcText.find("import mcpp") != std::string::npos;
769803
bool usesStdCompat = imports_module(srcText, "std.compat");
770804
bool usesStd = usesStdCompat || imports_module(srcText, "std");
@@ -784,27 +818,6 @@ std::expected<void, std::string> run_build_program(
784818
if (imports_module(t, "std")) usesStd = true;
785819
}
786820

787-
// A module that is present only as another rule's prerequisite is not part
788-
// of this package's declared surface. Refusing the import here rather than
789-
// letting it work is the difference between a rule that travels and one
790-
// that happens to build on whoever's machine: the same source stops
791-
// compiling the moment the intermediate rule stops depending on it, or the
792-
// moment a Clang user tries it, since only GCC makes the BMI reachable
793-
// without the flags.
794-
for (auto const& hm : env.hostModules) {
795-
if (hm.importable) continue;
796-
if (!imports_module(srcText, hm.logical)) continue;
797-
return std::unexpected(std::format(
798-
"build.mcpp imports '{}', which reaches this build only as a "
799-
"prerequisite of another build rule.\n"
800-
" A rule's build-time provisions cross one further edge only "
801-
"when that edge says so.\n"
802-
" Either depend on the package providing '{}' directly, or "
803-
"have the rule that owns it re-export it:\n"
804-
" <that rule's package> = {{ ..., host-module = true, "
805-
"reexport = true }}",
806-
hm.logical, hm.logical));
807-
}
808821
usesStd = usesStd || usesStdCompat;
809822

810823
// The toolchain's own environment (MSVC's INCLUDE / LIB / VSLANG, which

tests/e2e/311_rule_build_dependencies.sh

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,29 @@ out="$("$MCPP" run 2>&1 | grep '^ANSWER=' | tail -1)"
136136
[[ "$out" == "ANSWER=7" ]] || {
137137
echo "FAIL: the re-exported path broke the value: $out"; exit 1; }
138138

139+
# ── 3b. withdrawing reexport is caught on a warm cache ─────────────────────
140+
# Ordered right after the successful build above, so build.mcpp's cache and the
141+
# whole-project fast path are both warm. Dropping `reexport` leaves the module
142+
# SET and every interface hash identical, so nothing in the cache identity
143+
# moves; the refusal is reached only because it is asked BEFORE the cache is
144+
# consulted.
145+
#
146+
# ⚠️ This assertion does not prove that placement on its own. Measured: with
147+
# the check below the fast path it still fired here, because `ctxHash` happens
148+
# to change as well. The placement is what makes the property hold; this pins
149+
# the behaviour, not the reason.
150+
cd "$TMP"
151+
mk_outer ""
152+
touch app/src/main.cpp # the whole-project fast path never reaches build.mcpp
153+
cd app
154+
if "$MCPP" build > b3b.log 2>&1; then
155+
cat b3b.log
156+
echo "FAIL: withdrawing reexport was replayed from the build.mcpp cache"
157+
exit 1
158+
fi
159+
grep -qF "globbing" b3b.log || {
160+
cat b3b.log; echo "FAIL: the refusal after cache invalidation does not name the module"; exit 1; }
161+
139162
# ── 4. a cycle is reported as a cycle ──────────────────────────────────────
140163
cd "$TMP"
141164
cat >> globbing/mcpp.toml <<'EOF'

0 commit comments

Comments
 (0)