Skip to content

Commit 1cc1052

Browse files
committed
fix(subos): 撤回 set 让位;只留 prepend 那半 —— 它才是 mcpp 的缺陷
上一版把 `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 条)
1 parent 73ab179 commit 1cc1052

2 files changed

Lines changed: 42 additions & 55 deletions

File tree

‎src/xlings/subos_info.cppm‎

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -256,18 +256,26 @@ resolve_env(const Info& info, const std::filesystem::path& subosDir,
256256
if (!hit) {
257257
auto amb = ambient_of(d.var);
258258
if (d.op == "set") {
259-
// A `set` is a DEFAULT, not an order. If the caller
260-
// already exported the variable, that wins.
259+
// `set` wins, ambient or not.
261260
//
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; }
261+
// Deliberately NOT "yield to an exported value". That
262+
// reading was written here first and withdrawn: `set` and
263+
// "default" are two different intentions, and a subos has
264+
// real need of the first -- a variable naming its own
265+
// loader configuration must not be overridable by a stale
266+
// value in the caller's shell. Collapsing them here would
267+
// remove the ability to express it.
268+
//
269+
// It is also not mcpp's vocabulary to redefine. `envs` is
270+
// xlings' wire format; a consumer that quietly gives an op
271+
// a second meaning makes the same subos behave differently
272+
// depending on which tool launched the program.
273+
//
274+
// The escape hatch mcpp#382 asks for (a recipe declaring a
275+
// DEFAULT the user can override) therefore wants a new op
276+
// from xlings, not a reinterpretation of this one. When it
277+
// exists, it is honoured here -- unknown ops are dropped
278+
// today, which is why it has to arrive on both sides.
271279
out.emplace_back(d.var, value);
272280
continue;
273281
}

‎tests/unit/test_subos_info.cpp‎

Lines changed: 23 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -249,52 +249,8 @@ 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-
}
275252

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-
}
286253

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-
}
298254

299255
// `prepend` prepends TO the caller's value rather than replacing it. Same
300256
// reason: the pair replaces the variable, so emitting the declared value alone
@@ -331,4 +287,27 @@ TEST(SubosResolveEnv, PrependIsIdempotentAgainstTheExportedValue) {
331287
EXPECT_EQ(out[0].second, existing);
332288
}
333289

290+
// `set` wins over an exported value, and that is deliberate.
291+
//
292+
// This assertion exists to stop the opposite reading from being reintroduced
293+
// -- it was, once, as a fix for mcpp#382, and withdrawn: `set` and "a default
294+
// the user may override" are two intentions, and a subos needs the first for
295+
// variables naming its own configuration. The escape hatch that issue wants is
296+
// a NEW op from xlings, whose wire format this is, not a second meaning for
297+
// this one applied by one consumer.
298+
TEST(SubosResolveEnv, SetWinsOverAnExportedValue) {
299+
Tmp t;
300+
t.write(R"({"workspace":{},"subos_info":{"schema_version":1,
301+
"runtime":"glibc@2.39",
302+
"envs":{"glibc@2.39":[{"var":"GALLIUM_DRIVER","op":"set","value":"d3d12"}]}}})");
303+
auto info = su::read(t.dir);
304+
auto out = su::resolve_env(
305+
info, t.dir, [](std::string_view v) -> std::optional<std::string> {
306+
if (v == "GALLIUM_DRIVER") return std::string("llvmpipe");
307+
return std::nullopt;
308+
});
309+
ASSERT_EQ(out.size(), 1u);
310+
EXPECT_EQ(out[0].second, "d3d12");
311+
}
312+
334313
} // namespace

0 commit comments

Comments
 (0)