From 378b69f08ac44a424528ed1e760ad7ae4630865d Mon Sep 17 00:00:00 2001 From: speak-agent Date: Sat, 8 Aug 2026 06:48:57 +0800 Subject: [PATCH 1/3] fix(run): an upgraded mcpp replayed a pre-upgrade cache with no subos env MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 2026.8.8.1 taught `mcpp run` to hand a program its subos's environment, and recorded which subos in the build cache so the fast path could do it too. What it did not do is notice a cache entry written before that field existed. The reader's own comment claimed it did -- "absent ⇒ the run fast path treats the entry as a miss" -- and no such check was ever written. A comment describing behaviour the code does not have is worse than the missing check. The consequence is the whole #352 fix quietly not applying to anyone who upgrades, and not just for one run: the fast path's identity is the profile, the cache mode and the resource list, and its fingerprint check compares a cached entry against ITSELF. Nothing there notices that a different mcpp wrote the entry, so the pre-upgrade build is replayed until something else happens to invalidate it. Measured on a real upgrade from 2026.8.7.1 rather than reasoned about: build with the old binary, run with the new one, and the environment never arrives. An empty subosDir cannot stand in for "predates the field" -- a system toolchain outside the xpkgs store legitimately has no subos -- so presence is tracked separately. One rebuild after upgrading, and the field is there. The e2e ages a cache by stripping the line, which is exactly what an older mcpp's cache looks like, and asserts three things: the fast path is refused, the rebuild applies the environment, and the field is recorded so the next run does not repeat the work. --- src/build/execute.cppm | 23 ++++++++++++++++-- tests/e2e/200_subos_env_reaches_program.sh | 27 ++++++++++++++++++++++ 2 files changed, 48 insertions(+), 2 deletions(-) diff --git a/src/build/execute.cppm b/src/build/execute.cppm index b4cb5c1e..d31208fa 100644 --- a/src/build/execute.cppm +++ b/src/build/execute.cppm @@ -68,6 +68,11 @@ struct BuildCacheEntry { // property and would otherwise be unknowable on the fast path -- which // has no toolchain to derive it from. std::string subosDir; + // Was the line present at all? An EMPTY subosDir is a legitimate answer + // (a system toolchain outside the xpkgs store has no subos), so it cannot + // stand in for "this cache predates the field" -- and those two need + // opposite treatment: the first runs, the second must rebuild once. + bool subosRecorded = false; // The resolved profile this entry was built for. Entries used to be keyed // by target triple alone, and the fast paths only refuse to run when an // EXPLICIT --profile/--dev/--release is passed — so a bare `mcpp build` @@ -154,7 +159,8 @@ std::vector read_build_cache(const std::filesystem::path& proje // using the cache at all -- the program would work once and then // silently stop finding its runtime data. if (haveNextLine && line.starts_with("subos=")) { - e.subosDir = line.substr(6); + e.subosDir = line.substr(6); + e.subosRecorded = true; haveNextLine = static_cast(std::getline(f, line)); } // Optional profile line. Same back-compat contract as the two blocks @@ -201,7 +207,8 @@ void write_build_cache(const std::filesystem::path& projectRoot, // Insert at front (MRU). BuildCacheEntry newEntry{targetTriple, outputDir.string(), ninjaProgram, fingerprintHex, runtimeEnvKey, runtimeEnvValue, std::move(runTargets), - runEnvKey, runEnvValue, subosDir, profile, cacheMode}; + runEnvKey, runEnvValue, subosDir, /*subosRecorded=*/true, + profile, cacheMode}; entries.insert(entries.begin(), std::move(newEntry)); // Trim to LRU capacity. @@ -774,6 +781,18 @@ std::optional try_fast_run(const std::filesystem::path& projectRoot, ninjaProgram = ninjaProgram.substr(1, ninjaProgram.size() - 2); if (match->runtimeEnvKey.empty()) return std::nullopt; // old cache entry; go through prepare_build once + // Written before this mcpp knew about subos environments (mcpp#352). Taking + // the fast path here would run the program without them -- which is the + // defect this field exists to fix, surviving an upgrade. + // + // It survives it for a long time, too: the fast path's identity is the + // profile, the cache mode and the resource list, and its fingerprint check + // compares a cached entry against ITSELF. Neither notices that a different + // mcpp wrote the entry, so without this line an upgraded mcpp would reuse a + // pre-upgrade build until something else happened to invalidate it. Measured + // on a real upgrade from 2026.8.7.1, not reasoned about. + if (!match->subosRecorded) + return std::nullopt; // predates `subos=`; rebuild once, then it is there // P1: verify fingerprint matches the outputDir basename. if (!match->fingerprint.empty()) { diff --git a/tests/e2e/200_subos_env_reaches_program.sh b/tests/e2e/200_subos_env_reaches_program.sh index 81da7302..fd2a9d36 100755 --- a/tests/e2e/200_subos_env_reaches_program.sh +++ b/tests/e2e/200_subos_env_reaches_program.sh @@ -106,6 +106,33 @@ echo "$out2" | grep -q 'PROBE=(unset)' || { exit 1 } +# 2b. A cache written before this mcpp knew about subos environments must NOT +# be replayed by the fast path. Simulated by stripping the field, which is +# exactly what an older mcpp's cache looks like. +# +# Without this the fix survives an upgrade in name only: the fast path's +# identity is the profile, the cache mode and the resource list, and its +# fingerprint check compares a cached entry against itself -- so nothing +# notices that a different mcpp wrote it, and an upgraded mcpp would keep +# running the pre-upgrade build with no subos environment at all. +cache="$TMP/hello/target/.build_cache" +[ -f "$cache" ] || { echo "no build cache to age"; exit 1; } +grep -q '^subos=' "$cache" || { echo "cache has no subos= line to strip"; exit 1; } +grep -v '^subos=' "$cache" > "$cache.old" && mv "$cache.old" "$cache" +aged=$(MCPP_SUBOS_DIR="$subos" "$MCPP" run 2>&1) || { + echo "run against an aged cache failed:"; echo "$aged"; exit 1; } +echo "$aged" | grep -q 'Resolving toolchain' || { + echo "an aged cache was replayed by the fast path — the subos environment" + echo " would be missing for every run after an upgrade:" + echo "$aged" + exit 1 +} +echo "$aged" | grep -q "PROBE=$subos/usr/lib/dri" || { + echo "the rebuild after an aged cache did not apply the environment:" + echo "$aged"; exit 1; } +grep -q '^subos=' "$cache" || { + echo "the rebuild did not record subos= , so every later run repeats it"; exit 1; } + # 3. A subos with no self-description degrades quietly and still runs. This is # the state of every subos created before xlings grew the block, so it must # not be an error. From 56e580f51acb904072c55f17dbb60ceabaf1ac28 Mon Sep 17 00:00:00 2001 From: speak-agent Date: Sat, 8 Aug 2026 07:02:03 +0800 Subject: [PATCH 2/3] fix(xlings): read the subos_info wire format xlings actually writes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 2026.8.8.1 shipped a reader for a format that does not exist. `envs` is an OBJECT keyed by binding: "envs": { "mesa@25.0.7.1": [ {"var":…,"op":…,"value":…}, … ], … } The reader expected an array of {binding, decls} objects, so `is_array()` was false, the loop never ran, and every variable came back unset. Against a real subos on an NVIDIA host the released build applies nothing at all -- LIBGL_DRIVERS_PATH unset, __EGL_VENDOR_LIBRARY_DIRS unset, XDG_DATA_DIRS still the host's. The whole #352 fix is inert. Ten unit tests and an e2e passed the whole way, because every fixture was hand-written in the same invented shape the parser expected. Both sides agreed and both were wrong. A fixture composed from the same understanding as the parser cannot catch a misunderstanding of the wire format; only one taken from the writer can. RealXlingsCapture is that -- a verbatim copy of what a real xlings wrote, reformatted for width and nothing else. The parser is now transcribed from xlings's own reader rather than modelled on it, which also caught a second divergence: xlings drops a declaration whose op is neither "set" nor "prepend", and this accepted any op and treated everything non-"set" as a prepend. A reader more permissive than its writer eventually applies something the writer meant to reject. Verified the way it should have been the first time: the fixed binary against the subos a real `xlings install graphics` produced, with all three variables arriving and the shared EGL vendor directory de-duplicated to one entry. --- src/xlings/subos_info.cppm | 53 ++++++++++------- tests/unit/test_subos_info.cpp | 106 +++++++++++++++++++++++++++------ 2 files changed, 121 insertions(+), 38 deletions(-) diff --git a/src/xlings/subos_info.cppm b/src/xlings/subos_info.cppm index 0054fb2b..41aaac55 100644 --- a/src/xlings/subos_info.cppm +++ b/src/xlings/subos_info.cppm @@ -131,28 +131,39 @@ Info read(const std::filesystem::path& subosDir) { if (auto v = it->find("runtime"); v != it->end() && v->is_string()) info.runtime = v->get(); - if (auto envs = it->find("envs"); envs != it->end() && envs->is_array()) { - for (auto const& p : *envs) { - if (!p.is_object()) continue; + // `envs` is an OBJECT keyed by binding, whose values are arrays of + // declarations: + // + // "envs": { "mesa@25.0.7.1": [ {"var":…,"op":…,"value":…}, … ], … } + // + // Transcribed from xlings's own reader (core/subos/manifest.cppm), not + // from a model of it. The first version of this file expected an array of + // {binding, decls} objects — a shape xlings never writes — and its tests + // hand-wrote JSON in that same invented shape, so both agreed and both + // were wrong. Against a real subos the loop simply never ran and every + // variable came back unset, silently. That is why the fixture below is a + // verbatim capture of real output rather than something composed here. + if (auto envs = it->find("envs"); envs != it->end() && envs->is_object()) { + for (auto e = envs->begin(); e != envs->end(); ++e) { + if (!e.value().is_array()) continue; Provider prov; - if (auto b = p.find("binding"); b != p.end() && b->is_string()) - prov.binding = b->get(); - if (auto ds = p.find("decls"); ds != p.end() && ds->is_array()) { - for (auto const& d : *ds) { - if (!d.is_object()) continue; - EnvDecl e; - if (auto x = d.find("var"); x != d.end() && x->is_string()) - e.var = x->get(); - if (auto x = d.find("op"); x != d.end() && x->is_string()) - e.op = x->get(); - if (auto x = d.find("value"); x != d.end() && x->is_string()) - e.value = x->get(); - // A declaration with no variable name is not a partial - // declaration to be guessed at — it is malformed input, - // and the right thing is to leave it out rather than - // invent a name for it. - if (!e.var.empty()) prov.decls.push_back(std::move(e)); - } + prov.binding = e.key(); + for (auto const& d : e.value()) { + if (!d.is_object()) continue; + EnvDecl decl; + if (auto x = d.find("var"); x != d.end() && x->is_string()) + decl.var = x->get(); + if (auto x = d.find("op"); x != d.end() && x->is_string()) + decl.op = x->get(); + if (auto x = d.find("value"); x != d.end() && x->is_string()) + decl.value = x->get(); + // xlings drops a declaration whose var is empty or whose op is + // neither "set" nor "prepend". Matched exactly: a reader that + // is more permissive than the writer will one day apply + // something the writer considers malformed. + if (decl.var.empty()) continue; + if (decl.op != "set" && decl.op != "prepend") continue; + prov.decls.push_back(std::move(decl)); } info.providers.push_back(std::move(prov)); } diff --git a/tests/unit/test_subos_info.cpp b/tests/unit/test_subos_info.cpp index 95385778..a7f27af2 100644 --- a/tests/unit/test_subos_info.cpp +++ b/tests/unit/test_subos_info.cpp @@ -37,14 +37,14 @@ TEST(SubosInfo, ReadsRuntimeAndEnvDeclarations) { "subos_info": { "schema_version": 1, "runtime": "glibc@2.39", - "envs": [ - { "binding": "mesa@25.0.7.1", "decls": [ + "envs": { + "mesa@25.0.7.1": [ { "var": "LIBGL_DRIVERS_PATH", "op": "prepend", "value": "${subosdir}/usr/lib/dri" }, { "var": "XDG_DATA_DIRS", "op": "prepend", "value": "${subosdir}/share" } - ]} - ] + ] + } } })"); auto info = su::read(t.dir); @@ -64,8 +64,8 @@ TEST(SubosInfo, ReadsRuntimeAndEnvDeclarations) { TEST(SubosInfo, ResolvesSubosdirPlaceholder) { Tmp t; t.write(R"({"subos_info":{"schema_version":1,"runtime":"glibc@2.39", - "envs":[{"binding":"mesa@1","decls":[ - {"var":"LIBGL_DRIVERS_PATH","op":"prepend","value":"${subosdir}/usr/lib/dri"}]}]}})"); + "envs":{"mesa@1":[ + {"var":"LIBGL_DRIVERS_PATH","op":"prepend","value":"${subosdir}/usr/lib/dri"}]}}})"); auto env = su::resolve_env(su::read(t.dir), t.dir); ASSERT_EQ(env.size(), 1u); EXPECT_EQ(env[0].first, "LIBGL_DRIVERS_PATH"); @@ -81,11 +81,9 @@ TEST(SubosInfo, ResolvesSubosdirPlaceholder) { // EGL vendor directory. `prepend` joins them; it must not drop either. TEST(SubosInfo, PrependJoinsProvidersInOrder) { Tmp t; - t.write(R"({"subos_info":{"schema_version":1,"runtime":"glibc@2.39","envs":[ - {"binding":"a-mesa@1","decls":[ - {"var":"V","op":"prepend","value":"${subosdir}/one"}]}, - {"binding":"b-vendor@1","decls":[ - {"var":"V","op":"prepend","value":"${subosdir}/two"}]}]}})"); + t.write(R"({"subos_info":{"schema_version":1,"runtime":"glibc@2.39","envs":{ + "a-mesa@1":[{"var":"V","op":"prepend","value":"${subosdir}/one"}], + "b-vendor@1":[{"var":"V","op":"prepend","value":"${subosdir}/two"}]}}})"); auto env = su::resolve_env(su::read(t.dir), t.dir); ASSERT_EQ(env.size(), 1u); const auto sep = mcpp::platform::env::path_list_separator(); @@ -98,9 +96,9 @@ TEST(SubosInfo, PrependJoinsProvidersInOrder) { // would otherwise grow the variable without bound. TEST(SubosInfo, PrependDeduplicates) { Tmp t; - t.write(R"({"subos_info":{"schema_version":1,"runtime":"glibc@2.39","envs":[ - {"binding":"a@1","decls":[{"var":"V","op":"prepend","value":"${subosdir}/x"}]}, - {"binding":"b@1","decls":[{"var":"V","op":"prepend","value":"${subosdir}/x"}]}]}})"); + t.write(R"({"subos_info":{"schema_version":1,"runtime":"glibc@2.39","envs":{ + "a@1":[{"var":"V","op":"prepend","value":"${subosdir}/x"}], + "b@1":[{"var":"V","op":"prepend","value":"${subosdir}/x"}]}}})"); auto env = su::resolve_env(su::read(t.dir), t.dir); ASSERT_EQ(env.size(), 1u); // One entry, not two. The de-duplication has to split on the PLATFORM's @@ -114,9 +112,9 @@ TEST(SubosInfo, PrependDeduplicates) { // `set` replaces rather than joins — xlings's own precedence. TEST(SubosInfo, SetReplaces) { Tmp t; - t.write(R"({"subos_info":{"schema_version":1,"runtime":"glibc@2.39","envs":[ - {"binding":"a@1","decls":[{"var":"V","op":"prepend","value":"/one"}]}, - {"binding":"b@1","decls":[{"var":"V","op":"set","value":"/two"}]}]}})"); + t.write(R"({"subos_info":{"schema_version":1,"runtime":"glibc@2.39","envs":{ + "a@1":[{"var":"V","op":"prepend","value":"/one"}], + "b@1":[{"var":"V","op":"set","value":"/two"}]}}})"); auto env = su::resolve_env(su::read(t.dir), t.dir); ASSERT_EQ(env.size(), 1u); EXPECT_EQ(env[0].second, "/two"); @@ -177,4 +175,78 @@ TEST(SubosInfo, FamilyOfMirrorsXlings) { EXPECT_EQ(su::family_of("glibc"), "linux-x86_64-glibc"); } + +// A VERBATIM capture of what a real xlings wrote, after `xlings install +// graphics` on an NVIDIA host. Reformatted for width and nothing else -- keys, +// nesting and spelling are as found on disk. +// +// This test exists because its absence shipped a broken feature. The first +// version of this file hand-wrote every fixture in a shape the reader also +// expected and xlings never produces: `envs` as an array of {binding, decls}. +// Ten tests passed against a format that does not exist, and against a real +// subos the released build applied no variables at all -- silently, because +// "no providers" and "nothing declared" look identical. +// +// A fixture composed from the same understanding as the parser cannot catch +// that. Only one taken from the writer can. +TEST(SubosInfo, RealXlingsCapture) { + Tmp t; + t.write(R"({ + "subos_info": { + "created_at": "2026-08-08T01:40:00Z", + "created_by": "xlings 2026.8.7.1", + "runtime": "glibc@2.39", + "schema_version": 1, + "envs": { + "mesa@25.0.7.1": [ + {"op": "prepend", "value": "${subosdir}/usr/lib/dri", "var": "LIBGL_DRIVERS_PATH"}, + {"op": "prepend", "value": "${subosdir}/share/glvnd/egl_vendor.d", "var": "__EGL_VENDOR_LIBRARY_DIRS"}, + {"op": "prepend", "value": "${subosdir}/share", "var": "XDG_DATA_DIRS"} + ], + "nvidia-gl-host-link@0.1.1": [ + {"op": "prepend", "value": "${subosdir}/share/glvnd/egl_vendor.d", "var": "__EGL_VENDOR_LIBRARY_DIRS"} + ] + } + }, + "workspace": {} + })"); + + auto info = su::read(t.dir); + ASSERT_TRUE(info.present); + EXPECT_EQ(info.runtime, "glibc@2.39"); + ASSERT_EQ(info.providers.size(), 2u); + EXPECT_EQ(info.providers[0].binding, "mesa@25.0.7.1"); + EXPECT_EQ(info.providers[1].binding, "nvidia-gl-host-link@0.1.1"); + + auto env = su::resolve_env(info, t.dir); + ASSERT_EQ(env.size(), 3u) << "all three graphics variables must be produced"; + + std::map byVar; + for (auto& [k, v] : env) byVar[k] = v; + const auto sep = mcpp::platform::env::path_list_separator(); + EXPECT_EQ(byVar["LIBGL_DRIVERS_PATH"], t.dir.string() + "/usr/lib/dri"); + EXPECT_EQ(byVar["XDG_DATA_DIRS"], t.dir.string() + "/share"); + // Both providers name the same vendor directory; de-duplication must + // leave exactly one, or libglvnd sees it twice and enumerates the device + // twice -- which is a defect xlings hit on its own side. + EXPECT_EQ(byVar["__EGL_VENDOR_LIBRARY_DIRS"], + t.dir.string() + "/share/glvnd/egl_vendor.d"); + EXPECT_EQ(byVar["__EGL_VENDOR_LIBRARY_DIRS"].find(sep), std::string::npos); +} + +// xlings drops a declaration whose op it does not recognise. A reader more +// permissive than its writer eventually applies something the writer meant to +// reject, so this asserts the same refusal rather than a tolerant guess. +TEST(SubosInfo, UnknownOpIsDroppedLikeXlingsDrops) { + Tmp t; + t.write(R"({"subos_info":{"schema_version":1,"runtime":"glibc@2.39","envs":{ + "a@1":[{"var":"V","op":"append","value":"/nope"}, + {"var":"W","op":"prepend","value":"/yes"}, + {"var":"","op":"prepend","value":"/no-name"}]}}})"); + auto env = su::resolve_env(su::read(t.dir), t.dir); + ASSERT_EQ(env.size(), 1u); + EXPECT_EQ(env[0].first, "W"); + EXPECT_EQ(env[0].second, "/yes"); +} + } // namespace From 5a86eac1c0ff3b604e9f9d0a40e06e105c3299b0 Mon Sep 17 00:00:00 2001 From: speak-agent Date: Sat, 8 Aug 2026 07:02:48 +0800 Subject: [PATCH 3/3] test(e2e): the fixture was the invented wire format too The e2e wrote its subos manifest in the same shape the broken reader expected, so it kept passing while the feature did nothing against a real subos. Fixing the reader made it fail, which is the correct outcome and the reason the fix is trustworthy. The comment now says the structure is a wire format rather than a convenience, because the invented shape is the tidier-looking of the two and someone will want to "simplify" it back. --- tests/e2e/200_subos_env_reaches_program.sh | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/tests/e2e/200_subos_env_reaches_program.sh b/tests/e2e/200_subos_env_reaches_program.sh index fd2a9d36..4ce376df 100755 --- a/tests/e2e/200_subos_env_reaches_program.sh +++ b/tests/e2e/200_subos_env_reaches_program.sh @@ -14,6 +14,12 @@ # know what any of these variables mean -- it carries whatever the subos # declares -- and a test naming LIBGL_DRIVERS_PATH would quietly suggest # otherwise. +# +# The JSON below is xlings's REAL shape: `envs` is an object keyed by binding, +# whose values are arrays of declarations. The first version of this test wrote +# an array of {binding, decls} -- a shape xlings never produces -- and it +# passed, because the reader had been written from the same misunderstanding. +# Do not "simplify" this structure; it is a wire format, not a convenience. set -euo pipefail TMP=$(mktemp -d) @@ -28,9 +34,9 @@ mkdir -p "$subos/usr/lib/dri" cat > "$subos/.xlings.json" <<'EOF' { "workspace": {}, "subos_info": { "schema_version": 1, "runtime": "glibc@2.39", - "envs": [ { "binding": "probe@1", "decls": [ + "envs": { "probe@1": [ { "var": "MCPP_E2E_PROBE", "op": "prepend", - "value": "${subosdir}/usr/lib/dri" } ] } ] } } + "value": "${subosdir}/usr/lib/dri" } ] } } } EOF cd "$TMP"