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/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/e2e/200_subos_env_reaches_program.sh b/tests/e2e/200_subos_env_reaches_program.sh index 81da7302..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" @@ -106,6 +112,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. 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