Skip to content

Commit 73ab179

Browse files
committed
fix(subos): set 是默认值,不是命令;prepend 要接在用户的值前面
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 通过
1 parent 6e6baa4 commit 73ab179

3 files changed

Lines changed: 142 additions & 4 deletions

File tree

src/build/execute.cppm

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -361,7 +361,15 @@ compute_subos_env(const mcpp::build::BuildPlan& plan) {
361361
// diagnostic path in doctor.
362362
if (!info.note.empty())
363363
mcpp::log::verbose("subos", info.note);
364-
return mcpp::xlings::subos::resolve_env(info, dir);
364+
// Resolved AGAINST the caller's environment, not in a vacuum: these
365+
// entries replace the variable in the child, so a `set` that ignores an
366+
// exported value overwrites it and a `prepend` that ignores it drops it.
367+
return mcpp::xlings::subos::resolve_env(
368+
info, dir, [](std::string_view v) -> std::optional<std::string> {
369+
if (const char* e = std::getenv(std::string(v).c_str()))
370+
return std::string(e);
371+
return std::nullopt;
372+
});
365373
}
366374

367375
// Compile a prepared BuildContext. Shared between `mcpp build` and `mcpp run`
@@ -877,7 +885,13 @@ std::optional<int> try_fast_run(const std::filesystem::path& projectRoot,
877885
auto subosDir = subos_dir_for_run(std::filesystem::path(match->subosDir));
878886
if (!subosDir.empty()) {
879887
auto info = mcpp::xlings::subos::read(subosDir);
880-
for (auto& kv : mcpp::xlings::subos::resolve_env(info, subosDir))
888+
for (auto& kv : mcpp::xlings::subos::resolve_env(
889+
info, subosDir,
890+
[](std::string_view v) -> std::optional<std::string> {
891+
if (const char* e = std::getenv(std::string(v).c_str()))
892+
return std::string(e);
893+
return std::nullopt;
894+
}))
881895
childEnv.push_back(std::move(kv));
882896
}
883897
}

src/xlings/subos_info.cppm

Lines changed: 44 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -201,9 +201,21 @@ Info read(const std::filesystem::path& subosDir) {
201201
// never matches and the joined value is a corrupt list. Caught by CI on
202202
// Windows, not by any amount of reading.
203203
std::vector<std::pair<std::string, std::string>>
204-
resolve_env(const Info& info, const std::filesystem::path& subosDir) {
204+
resolve_env(const Info& info, const std::filesystem::path& subosDir,
205+
const std::function<std::optional<std::string>(std::string_view)>&
206+
ambient = {}) {
205207
std::vector<std::pair<std::string, std::string>> out;
206208

209+
// What the caller's environment already says about a variable.
210+
//
211+
// The declarations are merged against this, not in a vacuum, because the
212+
// result REPLACES the variable in the child (it goes in as extraEnv). A
213+
// resolution that ignores the ambient value silently discards it.
214+
auto ambient_of = [&](std::string_view var) -> std::optional<std::string> {
215+
if (!ambient) return std::nullopt;
216+
return ambient(var);
217+
};
218+
207219
const std::string subos = subosDir.string();
208220
auto expand = [&](std::string v) {
209221
constexpr std::string_view kPh = "${subosdir}";
@@ -241,7 +253,37 @@ resolve_env(const Info& info, const std::filesystem::path& subosDir) {
241253
std::pair<std::string, std::string>* hit = nullptr;
242254
for (auto& kv : out)
243255
if (kv.first == d.var) { hit = &kv; break; }
244-
if (!hit) { out.emplace_back(d.var, value); continue; }
256+
if (!hit) {
257+
auto amb = ambient_of(d.var);
258+
if (d.op == "set") {
259+
// A `set` is a DEFAULT, not an order. If the caller
260+
// already exported the variable, that wins.
261+
//
262+
// Recipes document this as the escape hatch -- xlings'
263+
// wsl-gl-host-link says in so many words that a user who
264+
// exports GALLIUM_DRIVER=llvmpipe keeps it. Before this,
265+
// the subos value overwrote it and the hatch did not
266+
// exist: `export GALLIUM_DRIVER=llvmpipe; mcpp run` still
267+
// ran with d3d12 and still failed (mcpp#382). An
268+
// environment a user set deliberately is the one piece of
269+
// input a build environment must not quietly overrule.
270+
if (amb && !amb->empty()) { out.emplace_back(d.var, *amb); continue; }
271+
out.emplace_back(d.var, value);
272+
continue;
273+
}
274+
// `prepend` against the ambient value, not instead of it.
275+
// These entries replace the variable in the child, so
276+
// emitting the declared value alone DROPS whatever the caller
277+
// had -- for a PATH-shaped variable that is the user's whole
278+
// search path.
279+
if (amb && !amb->empty() && !contains_element(*amb, value))
280+
out.emplace_back(d.var, value + sep + *amb);
281+
else if (amb && !amb->empty())
282+
out.emplace_back(d.var, *amb);
283+
else
284+
out.emplace_back(d.var, value);
285+
continue;
286+
}
245287
if (d.op == "set") { hit->second = value; continue; }
246288
if (!contains_element(hit->second, value))
247289
hit->second = value + sep + hit->second;

tests/unit/test_subos_info.cpp

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -249,4 +249,86 @@ TEST(SubosInfo, UnknownOpIsDroppedLikeXlingsDrops) {
249249
EXPECT_EQ(env[0].second, "/yes");
250250
}
251251

252+
// A `set` is a default; an exported value wins.
253+
//
254+
// The resolved pairs REPLACE the variable in the child (they go in as
255+
// extraEnv), so a `set` that ignores the caller's environment overwrites it
256+
// silently. Recipes are written on the opposite assumption: xlings'
257+
// wsl-gl-host-link documents that a user who exports GALLIUM_DRIVER=llvmpipe
258+
// keeps it, and that escape hatch did not exist -- `export
259+
// GALLIUM_DRIVER=llvmpipe; mcpp run` still ran with the subos value and still
260+
// failed (mcpp#382).
261+
TEST(SubosResolveEnv, SetDoesNotOverrideAnExportedValue) {
262+
Tmp t;
263+
t.write(R"({"workspace":{},"subos_info":{"schema_version":1,"runtime":"glibc@2.39",
264+
"envs":{"glibc@2.39":[{"var":"GALLIUM_DRIVER","op":"set","value":"d3d12"}]}}})");
265+
auto info = su::read(t.dir);
266+
auto out = su::resolve_env(
267+
info, t.dir, [](std::string_view v) -> std::optional<std::string> {
268+
if (v == "GALLIUM_DRIVER") return std::string("llvmpipe");
269+
return std::nullopt;
270+
});
271+
ASSERT_EQ(out.size(), 1u);
272+
EXPECT_EQ(out[0].first, "GALLIUM_DRIVER");
273+
EXPECT_EQ(out[0].second, "llvmpipe") << "the user's export must survive";
274+
}
275+
276+
TEST(SubosResolveEnv, SetAppliesWhenNothingIsExported) {
277+
Tmp t;
278+
t.write(R"({"workspace":{},"subos_info":{"schema_version":1,"runtime":"glibc@2.39",
279+
"envs":{"glibc@2.39":[{"var":"GALLIUM_DRIVER","op":"set","value":"d3d12"}]}}})");
280+
auto info = su::read(t.dir);
281+
auto out = su::resolve_env(
282+
info, t.dir, [](std::string_view) { return std::optional<std::string>{}; });
283+
ASSERT_EQ(out.size(), 1u);
284+
EXPECT_EQ(out[0].second, "d3d12");
285+
}
286+
287+
// An empty export is not an export.
288+
TEST(SubosResolveEnv, EmptyAmbientDoesNotBlockASet) {
289+
Tmp t;
290+
t.write(R"({"workspace":{},"subos_info":{"schema_version":1,"runtime":"glibc@2.39",
291+
"envs":{"glibc@2.39":[{"var":"X","op":"set","value":"v"}]}}})");
292+
auto info = su::read(t.dir);
293+
auto out = su::resolve_env(
294+
info, t.dir, [](std::string_view) { return std::optional<std::string>(""); });
295+
ASSERT_EQ(out.size(), 1u);
296+
EXPECT_EQ(out[0].second, "v");
297+
}
298+
299+
// `prepend` prepends TO the caller's value rather than replacing it. Same
300+
// reason: the pair replaces the variable, so emitting the declared value alone
301+
// discards whatever search path the user had.
302+
TEST(SubosResolveEnv, PrependKeepsTheExportedValue) {
303+
Tmp t;
304+
t.write(R"({"workspace":{},"subos_info":{"schema_version":1,"runtime":"glibc@2.39",
305+
"envs":{"glibc@2.39":[{"var":"LD_LIBRARY_PATH","op":"prepend","value":"/sub/lib"}]}}})");
306+
auto info = su::read(t.dir);
307+
auto out = su::resolve_env(
308+
info, t.dir, [](std::string_view v) -> std::optional<std::string> {
309+
if (v == "LD_LIBRARY_PATH") return std::string("/user/lib");
310+
return std::nullopt;
311+
});
312+
ASSERT_EQ(out.size(), 1u);
313+
const auto sep = mcpp::platform::env::path_list_separator();
314+
EXPECT_EQ(out[0].second, std::string("/sub/lib") + sep + "/user/lib");
315+
}
316+
317+
// Already there: prepending again would grow the list on every nested run.
318+
TEST(SubosResolveEnv, PrependIsIdempotentAgainstTheExportedValue) {
319+
Tmp t;
320+
t.write(R"({"workspace":{},"subos_info":{"schema_version":1,"runtime":"glibc@2.39",
321+
"envs":{"glibc@2.39":[{"var":"LD_LIBRARY_PATH","op":"prepend","value":"/sub/lib"}]}}})");
322+
auto info = su::read(t.dir);
323+
const auto sep = mcpp::platform::env::path_list_separator();
324+
const auto existing = std::string("/sub/lib") + sep + "/user/lib";
325+
auto out = su::resolve_env(
326+
info, t.dir, [&](std::string_view v) -> std::optional<std::string> {
327+
if (v == "LD_LIBRARY_PATH") return existing;
328+
return std::nullopt;
329+
});
330+
ASSERT_EQ(out.size(), 1u);
331+
EXPECT_EQ(out[0].second, existing);
332+
}
333+
252334
} // namespace

0 commit comments

Comments
 (0)