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..4cf9c96d 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,45 @@ 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") { + // `set` wins, ambient or not. + // + // 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; + } + // `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..1223d35f 100644 --- a/tests/unit/test_subos_info.cpp +++ b/tests/unit/test_subos_info.cpp @@ -249,4 +249,65 @@ TEST(SubosInfo, UnknownOpIsDroppedLikeXlingsDrops) { EXPECT_EQ(env[0].second, "/yes"); } + + + +// `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); +} + +// `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