Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions src/build/execute.cppm
Original file line number Diff line number Diff line change
Expand Up @@ -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<std::string> {
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`
Expand Down Expand Up @@ -877,7 +885,13 @@ std::optional<int> 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<std::string> {
if (const char* e = std::getenv(std::string(v).c_str()))
return std::string(e);
return std::nullopt;
}))
childEnv.push_back(std::move(kv));
}
}
Expand Down
54 changes: 52 additions & 2 deletions src/xlings/subos_info.cppm
Original file line number Diff line number Diff line change
Expand Up @@ -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<std::pair<std::string, std::string>>
resolve_env(const Info& info, const std::filesystem::path& subosDir) {
resolve_env(const Info& info, const std::filesystem::path& subosDir,
const std::function<std::optional<std::string>(std::string_view)>&
ambient = {}) {
std::vector<std::pair<std::string, std::string>> 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<std::string> {
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}";
Expand Down Expand Up @@ -241,7 +253,45 @@ resolve_env(const Info& info, const std::filesystem::path& subosDir) {
std::pair<std::string, std::string>* 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;
Expand Down
61 changes: 61 additions & 0 deletions tests/unit/test_subos_info.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<std::string> {
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<std::string> {
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<std::string> {
if (v == "GALLIUM_DRIVER") return std::string("llvmpipe");
return std::nullopt;
});
ASSERT_EQ(out.size(), 1u);
EXPECT_EQ(out[0].second, "d3d12");
}

} // namespace
Loading