From 73ab179987092c691ab0f7977e504c620e7c5db6 Mon Sep 17 00:00:00 2001 From: speak-agent Date: Sat, 8 Aug 2026 16:20:34 +0800 Subject: [PATCH 1/2] =?UTF-8?q?fix(subos):=20`set`=20=E6=98=AF=E9=BB=98?= =?UTF-8?q?=E8=AE=A4=E5=80=BC,=E4=B8=8D=E6=98=AF=E5=91=BD=E4=BB=A4;`prepen?= =?UTF-8?q?d`=20=E8=A6=81=E6=8E=A5=E5=9C=A8=E7=94=A8=E6=88=B7=E7=9A=84?= =?UTF-8?q?=E5=80=BC=E5=89=8D=E9=9D=A2?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit mcpp#382 的次要观察,而它把配方写明的逃生通道整个作废了。 解析出来的键值对是**替换**子进程里的变量的(它们作为 extraEnv 进去)。于是: - 一条 `set` 会覆盖用户已经 export 的值。xlings 的 wsl-gl-host-link 白纸黑字写着 「用户自己 export GALLIUM_DRIVER=llvmpipe 会保留」—— 实际不保留: `export GALLIUM_DRIVER=llvmpipe; mcpp run` 仍然带着 subos 的 d3d12 跑,仍然失败。 - 一条 `prepend` 只发出声明值,**丢掉**用户原有的内容。对 PATH 形状的变量,那是 用户的整条搜索路径。第二个缺陷,没人报过。 现在两者都对着调用方的环境解析:`set` 在变量已有非空值时让位,`prepend` 接在其 前面(且对已存在的元素幂等)。空值不算「用户设过」。 用户刻意设定的环境,是构建环境**唯一**不该悄悄推翻的输入。 tests/unit/test_subos_info.cpp +5(17 条);已验证去掉让位逻辑后该断言变红 e2e 200 通过 --- src/build/execute.cppm | 18 +++++++- src/xlings/subos_info.cppm | 46 ++++++++++++++++++- tests/unit/test_subos_info.cpp | 82 ++++++++++++++++++++++++++++++++++ 3 files changed, 142 insertions(+), 4 deletions(-) diff --git a/src/build/execute.cppm b/src/build/execute.cppm index b572f967..960c45d4 100644 --- a/src/build/execute.cppm +++ b/src/build/execute.cppm @@ -361,7 +361,15 @@ compute_subos_env(const mcpp::build::BuildPlan& plan) { // diagnostic path in doctor. if (!info.note.empty()) mcpp::log::verbose("subos", info.note); - return mcpp::xlings::subos::resolve_env(info, dir); + // Resolved AGAINST the caller's environment, not in a vacuum: these + // entries replace the variable in the child, so a `set` that ignores an + // exported value overwrites it and a `prepend` that ignores it drops it. + return mcpp::xlings::subos::resolve_env( + info, dir, [](std::string_view v) -> std::optional { + if (const char* e = std::getenv(std::string(v).c_str())) + return std::string(e); + return std::nullopt; + }); } // Compile a prepared BuildContext. Shared between `mcpp build` and `mcpp run` @@ -877,7 +885,13 @@ std::optional try_fast_run(const std::filesystem::path& projectRoot, auto subosDir = subos_dir_for_run(std::filesystem::path(match->subosDir)); if (!subosDir.empty()) { auto info = mcpp::xlings::subos::read(subosDir); - for (auto& kv : mcpp::xlings::subos::resolve_env(info, subosDir)) + for (auto& kv : mcpp::xlings::subos::resolve_env( + info, subosDir, + [](std::string_view v) -> std::optional { + if (const char* e = std::getenv(std::string(v).c_str())) + return std::string(e); + return std::nullopt; + })) childEnv.push_back(std::move(kv)); } } diff --git a/src/xlings/subos_info.cppm b/src/xlings/subos_info.cppm index 41aaac55..15788f0e 100644 --- a/src/xlings/subos_info.cppm +++ b/src/xlings/subos_info.cppm @@ -201,9 +201,21 @@ Info read(const std::filesystem::path& subosDir) { // never matches and the joined value is a corrupt list. Caught by CI on // Windows, not by any amount of reading. std::vector> -resolve_env(const Info& info, const std::filesystem::path& subosDir) { +resolve_env(const Info& info, const std::filesystem::path& subosDir, + const std::function(std::string_view)>& + ambient = {}) { std::vector> out; + // What the caller's environment already says about a variable. + // + // The declarations are merged against this, not in a vacuum, because the + // result REPLACES the variable in the child (it goes in as extraEnv). A + // resolution that ignores the ambient value silently discards it. + auto ambient_of = [&](std::string_view var) -> std::optional { + if (!ambient) return std::nullopt; + return ambient(var); + }; + const std::string subos = subosDir.string(); auto expand = [&](std::string v) { constexpr std::string_view kPh = "${subosdir}"; @@ -241,7 +253,37 @@ resolve_env(const Info& info, const std::filesystem::path& subosDir) { std::pair* hit = nullptr; for (auto& kv : out) if (kv.first == d.var) { hit = &kv; break; } - if (!hit) { out.emplace_back(d.var, value); continue; } + if (!hit) { + auto amb = ambient_of(d.var); + if (d.op == "set") { + // A `set` is a DEFAULT, not an order. If the caller + // already exported the variable, that wins. + // + // Recipes document this as the escape hatch -- xlings' + // wsl-gl-host-link says in so many words that a user who + // exports GALLIUM_DRIVER=llvmpipe keeps it. Before this, + // the subos value overwrote it and the hatch did not + // exist: `export GALLIUM_DRIVER=llvmpipe; mcpp run` still + // ran with d3d12 and still failed (mcpp#382). An + // environment a user set deliberately is the one piece of + // input a build environment must not quietly overrule. + if (amb && !amb->empty()) { out.emplace_back(d.var, *amb); continue; } + out.emplace_back(d.var, value); + continue; + } + // `prepend` against the ambient value, not instead of it. + // These entries replace the variable in the child, so + // emitting the declared value alone DROPS whatever the caller + // had -- for a PATH-shaped variable that is the user's whole + // search path. + if (amb && !amb->empty() && !contains_element(*amb, value)) + out.emplace_back(d.var, value + sep + *amb); + else if (amb && !amb->empty()) + out.emplace_back(d.var, *amb); + else + out.emplace_back(d.var, value); + continue; + } if (d.op == "set") { hit->second = value; continue; } if (!contains_element(hit->second, value)) hit->second = value + sep + hit->second; diff --git a/tests/unit/test_subos_info.cpp b/tests/unit/test_subos_info.cpp index a7f27af2..78eda0b8 100644 --- a/tests/unit/test_subos_info.cpp +++ b/tests/unit/test_subos_info.cpp @@ -249,4 +249,86 @@ TEST(SubosInfo, UnknownOpIsDroppedLikeXlingsDrops) { EXPECT_EQ(env[0].second, "/yes"); } +// A `set` is a default; an exported value wins. +// +// The resolved pairs REPLACE the variable in the child (they go in as +// extraEnv), so a `set` that ignores the caller's environment overwrites it +// silently. Recipes are written on the opposite assumption: xlings' +// wsl-gl-host-link documents that a user who exports GALLIUM_DRIVER=llvmpipe +// keeps it, and that escape hatch did not exist -- `export +// GALLIUM_DRIVER=llvmpipe; mcpp run` still ran with the subos value and still +// failed (mcpp#382). +TEST(SubosResolveEnv, SetDoesNotOverrideAnExportedValue) { + Tmp t; + t.write(R"({"workspace":{},"subos_info":{"schema_version":1,"runtime":"glibc@2.39", + "envs":{"glibc@2.39":[{"var":"GALLIUM_DRIVER","op":"set","value":"d3d12"}]}}})"); + auto info = su::read(t.dir); + auto out = su::resolve_env( + info, t.dir, [](std::string_view v) -> std::optional { + if (v == "GALLIUM_DRIVER") return std::string("llvmpipe"); + return std::nullopt; + }); + ASSERT_EQ(out.size(), 1u); + EXPECT_EQ(out[0].first, "GALLIUM_DRIVER"); + EXPECT_EQ(out[0].second, "llvmpipe") << "the user's export must survive"; +} + +TEST(SubosResolveEnv, SetAppliesWhenNothingIsExported) { + Tmp t; + t.write(R"({"workspace":{},"subos_info":{"schema_version":1,"runtime":"glibc@2.39", + "envs":{"glibc@2.39":[{"var":"GALLIUM_DRIVER","op":"set","value":"d3d12"}]}}})"); + auto info = su::read(t.dir); + auto out = su::resolve_env( + info, t.dir, [](std::string_view) { return std::optional{}; }); + ASSERT_EQ(out.size(), 1u); + EXPECT_EQ(out[0].second, "d3d12"); +} + +// An empty export is not an export. +TEST(SubosResolveEnv, EmptyAmbientDoesNotBlockASet) { + Tmp t; + t.write(R"({"workspace":{},"subos_info":{"schema_version":1,"runtime":"glibc@2.39", + "envs":{"glibc@2.39":[{"var":"X","op":"set","value":"v"}]}}})"); + auto info = su::read(t.dir); + auto out = su::resolve_env( + info, t.dir, [](std::string_view) { return std::optional(""); }); + ASSERT_EQ(out.size(), 1u); + EXPECT_EQ(out[0].second, "v"); +} + +// `prepend` prepends TO the caller's value rather than replacing it. Same +// reason: the pair replaces the variable, so emitting the declared value alone +// discards whatever search path the user had. +TEST(SubosResolveEnv, PrependKeepsTheExportedValue) { + Tmp t; + t.write(R"({"workspace":{},"subos_info":{"schema_version":1,"runtime":"glibc@2.39", + "envs":{"glibc@2.39":[{"var":"LD_LIBRARY_PATH","op":"prepend","value":"/sub/lib"}]}}})"); + auto info = su::read(t.dir); + auto out = su::resolve_env( + info, t.dir, [](std::string_view v) -> std::optional { + if (v == "LD_LIBRARY_PATH") return std::string("/user/lib"); + return std::nullopt; + }); + ASSERT_EQ(out.size(), 1u); + const auto sep = mcpp::platform::env::path_list_separator(); + EXPECT_EQ(out[0].second, std::string("/sub/lib") + sep + "/user/lib"); +} + +// Already there: prepending again would grow the list on every nested run. +TEST(SubosResolveEnv, PrependIsIdempotentAgainstTheExportedValue) { + Tmp t; + t.write(R"({"workspace":{},"subos_info":{"schema_version":1,"runtime":"glibc@2.39", + "envs":{"glibc@2.39":[{"var":"LD_LIBRARY_PATH","op":"prepend","value":"/sub/lib"}]}}})"); + auto info = su::read(t.dir); + const auto sep = mcpp::platform::env::path_list_separator(); + const auto existing = std::string("/sub/lib") + sep + "/user/lib"; + auto out = su::resolve_env( + info, t.dir, [&](std::string_view v) -> std::optional { + if (v == "LD_LIBRARY_PATH") return existing; + return std::nullopt; + }); + ASSERT_EQ(out.size(), 1u); + EXPECT_EQ(out[0].second, existing); +} + } // namespace From 1cc1052e9365f68b09f25124865cd8e06fe05b6f Mon Sep 17 00:00:00 2001 From: speak-agent Date: Sat, 8 Aug 2026 16:22:40 +0800 Subject: [PATCH 2/2] =?UTF-8?q?fix(subos):=20=E6=92=A4=E5=9B=9E=20`set`=20?= =?UTF-8?q?=E8=AE=A9=E4=BD=8D;=E5=8F=AA=E7=95=99=20`prepend`=20=E9=82=A3?= =?UTF-8?q?=E5=8D=8A=20=E2=80=94=E2=80=94=20=E5=AE=83=E6=89=8D=E6=98=AF=20?= =?UTF-8?q?mcpp=20=E7=9A=84=E7=BC=BA=E9=99=B7?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 上一版把 `set` 改成「用户已 export 就让位」。撤回,因为它错在两个层面: **`set` 与「默认值」是两种意图。** 有些变量 subos 必须说了算 —— 指向它自己 loader 配置的那类,用户 shell 里一个陈旧值就能把环境弄坏。把两者塌成一个,等于从此无法 表达前者。 **op 词汇表是 xlings 的,不是 mcpp 的。** `envs` 是 xlings 的线格式;消费方悄悄给 一个 op 加第二种含义,会让同一个 subos 因为「由谁启动」而行为不同。 mcpp#382 想要的逃生通道应当是 xlings 新增一个 op(「声明默认值,用户可覆盖」), mcpp 认它 —— 而不是在这里重解 `set`。现在未知 op 会被丢弃,所以那个 op 必须两侧 同时到位。已加断言把这个决定钉住,免得再被「修」回去。 留下的是 `prepend`:它字面意思就是接在已有值前面,而这里只发声明值、把调用方原有 内容整个丢掉。对 PATH 形状的变量,丢掉的是用户的整条搜索路径。这与 xlings 怎么修 无关,是 mcpp 自己的实现缺陷。 tests/unit/test_subos_info.cpp 15 条(prepend 2 条 + set 语义 1 条) --- src/xlings/subos_info.cppm | 30 +++++++++------ tests/unit/test_subos_info.cpp | 67 ++++++++++++---------------------- 2 files changed, 42 insertions(+), 55 deletions(-) diff --git a/src/xlings/subos_info.cppm b/src/xlings/subos_info.cppm index 15788f0e..4cf9c96d 100644 --- a/src/xlings/subos_info.cppm +++ b/src/xlings/subos_info.cppm @@ -256,18 +256,26 @@ resolve_env(const Info& info, const std::filesystem::path& subosDir, if (!hit) { auto amb = ambient_of(d.var); if (d.op == "set") { - // A `set` is a DEFAULT, not an order. If the caller - // already exported the variable, that wins. + // `set` wins, ambient or not. // - // Recipes document this as the escape hatch -- xlings' - // wsl-gl-host-link says in so many words that a user who - // exports GALLIUM_DRIVER=llvmpipe keeps it. Before this, - // the subos value overwrote it and the hatch did not - // exist: `export GALLIUM_DRIVER=llvmpipe; mcpp run` still - // ran with d3d12 and still failed (mcpp#382). An - // environment a user set deliberately is the one piece of - // input a build environment must not quietly overrule. - if (amb && !amb->empty()) { out.emplace_back(d.var, *amb); continue; } + // Deliberately NOT "yield to an exported value". That + // reading was written here first and withdrawn: `set` and + // "default" are two different intentions, and a subos has + // real need of the first -- a variable naming its own + // loader configuration must not be overridable by a stale + // value in the caller's shell. Collapsing them here would + // remove the ability to express it. + // + // It is also not mcpp's vocabulary to redefine. `envs` is + // xlings' wire format; a consumer that quietly gives an op + // a second meaning makes the same subos behave differently + // depending on which tool launched the program. + // + // The escape hatch mcpp#382 asks for (a recipe declaring a + // DEFAULT the user can override) therefore wants a new op + // from xlings, not a reinterpretation of this one. When it + // exists, it is honoured here -- unknown ops are dropped + // today, which is why it has to arrive on both sides. out.emplace_back(d.var, value); continue; } diff --git a/tests/unit/test_subos_info.cpp b/tests/unit/test_subos_info.cpp index 78eda0b8..1223d35f 100644 --- a/tests/unit/test_subos_info.cpp +++ b/tests/unit/test_subos_info.cpp @@ -249,52 +249,8 @@ TEST(SubosInfo, UnknownOpIsDroppedLikeXlingsDrops) { EXPECT_EQ(env[0].second, "/yes"); } -// A `set` is a default; an exported value wins. -// -// The resolved pairs REPLACE the variable in the child (they go in as -// extraEnv), so a `set` that ignores the caller's environment overwrites it -// silently. Recipes are written on the opposite assumption: xlings' -// wsl-gl-host-link documents that a user who exports GALLIUM_DRIVER=llvmpipe -// keeps it, and that escape hatch did not exist -- `export -// GALLIUM_DRIVER=llvmpipe; mcpp run` still ran with the subos value and still -// failed (mcpp#382). -TEST(SubosResolveEnv, SetDoesNotOverrideAnExportedValue) { - Tmp t; - t.write(R"({"workspace":{},"subos_info":{"schema_version":1,"runtime":"glibc@2.39", - "envs":{"glibc@2.39":[{"var":"GALLIUM_DRIVER","op":"set","value":"d3d12"}]}}})"); - auto info = su::read(t.dir); - auto out = su::resolve_env( - info, t.dir, [](std::string_view v) -> std::optional { - if (v == "GALLIUM_DRIVER") return std::string("llvmpipe"); - return std::nullopt; - }); - ASSERT_EQ(out.size(), 1u); - EXPECT_EQ(out[0].first, "GALLIUM_DRIVER"); - EXPECT_EQ(out[0].second, "llvmpipe") << "the user's export must survive"; -} -TEST(SubosResolveEnv, SetAppliesWhenNothingIsExported) { - Tmp t; - t.write(R"({"workspace":{},"subos_info":{"schema_version":1,"runtime":"glibc@2.39", - "envs":{"glibc@2.39":[{"var":"GALLIUM_DRIVER","op":"set","value":"d3d12"}]}}})"); - auto info = su::read(t.dir); - auto out = su::resolve_env( - info, t.dir, [](std::string_view) { return std::optional{}; }); - ASSERT_EQ(out.size(), 1u); - EXPECT_EQ(out[0].second, "d3d12"); -} -// An empty export is not an export. -TEST(SubosResolveEnv, EmptyAmbientDoesNotBlockASet) { - Tmp t; - t.write(R"({"workspace":{},"subos_info":{"schema_version":1,"runtime":"glibc@2.39", - "envs":{"glibc@2.39":[{"var":"X","op":"set","value":"v"}]}}})"); - auto info = su::read(t.dir); - auto out = su::resolve_env( - info, t.dir, [](std::string_view) { return std::optional(""); }); - ASSERT_EQ(out.size(), 1u); - EXPECT_EQ(out[0].second, "v"); -} // `prepend` prepends TO the caller's value rather than replacing it. Same // reason: the pair replaces the variable, so emitting the declared value alone @@ -331,4 +287,27 @@ TEST(SubosResolveEnv, PrependIsIdempotentAgainstTheExportedValue) { EXPECT_EQ(out[0].second, existing); } +// `set` wins over an exported value, and that is deliberate. +// +// This assertion exists to stop the opposite reading from being reintroduced +// -- it was, once, as a fix for mcpp#382, and withdrawn: `set` and "a default +// the user may override" are two intentions, and a subos needs the first for +// variables naming its own configuration. The escape hatch that issue wants is +// a NEW op from xlings, whose wire format this is, not a second meaning for +// this one applied by one consumer. +TEST(SubosResolveEnv, SetWinsOverAnExportedValue) { + Tmp t; + t.write(R"({"workspace":{},"subos_info":{"schema_version":1, + "runtime":"glibc@2.39", + "envs":{"glibc@2.39":[{"var":"GALLIUM_DRIVER","op":"set","value":"d3d12"}]}}})"); + auto info = su::read(t.dir); + auto out = su::resolve_env( + info, t.dir, [](std::string_view v) -> std::optional { + if (v == "GALLIUM_DRIVER") return std::string("llvmpipe"); + return std::nullopt; + }); + ASSERT_EQ(out.size(), 1u); + EXPECT_EQ(out[0].second, "d3d12"); +} + } // namespace