CLI: Address review feedback for environment options (#40830)
* Update ScopedEnvironmentVariable, CLI environment variable set and tests
David Bennett committed
Jun 17, 2026 at 11:41 UTC
74ba07fc20b134961d38dd7181768ca597671654
4 files changed
+63
-50
src/windows/wslc/core/EnvironmentOptions.cpp
+5
-13
@@ -14,21 +14,18 @@ namespace wsl::windows::wslc {
14
namespace {
15
16
// nullopt iff the variable is not defined; engaged (possibly empty) otherwise.
17
- std::optional<std::wstring> ReadEnv(const wchar_t* name) noexcept
18
- try
17
+ std::optional<std::wstring> ReadEnv(const wchar_t* name)
18
{
19
std::wstring value;
20
const HRESULT hr = wil::GetEnvironmentVariableW(name, value);
22
- if (FAILED(hr))
21
+ if (hr == HRESULT_FROM_WIN32(ERROR_ENVVAR_NOT_FOUND))
22
{
23
return std::nullopt;
24
}
25
+
26
+ THROW_IF_FAILED(hr);
27
return value;
28
}
28
- catch (...)
29
- {
30
- return std::nullopt;
31
- }
29
30
} // namespace
31
@@ -69,11 +66,6 @@ try
66
}
67
}
68
}
72
-catch (...)
73
-{
74
- // Must not throw: runs before NO_COLOR is applied, so a throw could
75
- // surface as colored error output from the parser's error path.
76
- LOG_CAUGHT_EXCEPTION();
77
-}
69
+CATCH_LOG()
70
71
} // namespace wsl::windows::wslc
test/windows/Common.cpp
+31
-3
@@ -2517,14 +2517,42 @@ void Trim(std::wstring& string)
2517
std::erase_if(string, [](auto c) { return !isalnum(c); });
2518
}
2519
2520
-ScopedEnvVariable::ScopedEnvVariable(const std::wstring& Name, const std::wstring& Value) : m_name(Name)
2520
+static std::optional<std::wstring> CaptureEnvValue(const std::wstring& Name)
2521
{
2522
- VERIFY_IS_TRUE(SetEnvironmentVariable(Name.c_str(), Value.c_str()));
2522
+ std::wstring value;
2523
+ HRESULT hr = wil::GetEnvironmentVariableW(Name.c_str(), value);
2524
+ if (hr == HRESULT_FROM_WIN32(ERROR_ENVVAR_NOT_FOUND))
2525
+ {
2526
+ return std::nullopt;
2527
+ }
2528
+ THROW_IF_FAILED(hr);
2529
+ return value;
2530
+}
2531
+
2532
+ScopedEnvVariable::ScopedEnvVariable(const std::wstring& Name) : m_name(Name), m_originalValue(CaptureEnvValue(Name))
2533
+{
2534
+ VERIFY_IS_TRUE(SetEnvironmentVariableW(Name.c_str(), nullptr));
2535
+}
2536
+
2537
+ScopedEnvVariable::ScopedEnvVariable(const std::wstring& Name, const std::wstring& Value) :
2538
+ m_name(Name), m_originalValue(CaptureEnvValue(Name))
2539
+{
2540
+ VERIFY_IS_TRUE(SetEnvironmentVariableW(Name.c_str(), Value.c_str()));
2541
}
2542
2543
ScopedEnvVariable::~ScopedEnvVariable()
2544
{
2527
- VERIFY_IS_TRUE(SetEnvironmentVariable(m_name.c_str(), nullptr));
2545
+ VERIFY_IS_TRUE(SetEnvironmentVariableW(m_name.c_str(), m_originalValue.has_value() ? m_originalValue->c_str() : nullptr));
2546
+}
2547
+
2548
+void ScopedEnvVariable::Set(const std::wstring& Value)
2549
+{
2550
+ VERIFY_IS_TRUE(SetEnvironmentVariableW(m_name.c_str(), Value.c_str()));
2551
+}
2552
+
2553
+void ScopedEnvVariable::Clear()
2554
+{
2555
+ VERIFY_IS_TRUE(SetEnvironmentVariableW(m_name.c_str(), nullptr));
2556
}
2557
2558
UniqueWebServer::UniqueWebServer(LPCWSTR Endpoint, LPCWSTR Content)
test/windows/Common.h
+15
-4
@@ -330,16 +330,27 @@ private:
330
class ScopedEnvVariable
331
{
332
public:
333
+ // Captures any existing value and clears the variable.
334
+ explicit ScopedEnvVariable(const std::wstring& Name);
335
+
336
+ // Captures any existing value and sets the variable to Value.
337
ScopedEnvVariable(const std::wstring& Name, const std::wstring& Value);
338
+
339
+ // Restores the original value.
340
~ScopedEnvVariable();
341
336
- ScopedEnvVariable(const WslConfigChange&) = delete;
337
- ScopedEnvVariable(WslConfigChange&&) = delete;
338
- const ScopedEnvVariable& operator=(ScopedEnvVariable&&) = delete;
339
- const ScopedEnvVariable& operator=(ScopedEnvVariable&) = delete;
342
+ NON_COPYABLE(ScopedEnvVariable);
343
+ NON_MOVABLE(ScopedEnvVariable);
344
+
345
+ // Sets the variable to a new value.
346
+ void Set(const std::wstring& Value);
347
+
348
+ // Clears (unsets) the variable.
349
+ void Clear();
350
351
private:
352
std::wstring m_name;
353
+ std::optional<std::wstring> m_originalValue;
354
};
355
356
class UniqueWebServer
test/windows/wslc/WSLCCLIEnvironmentOptionsUnitTests.cpp
+12
-30
@@ -34,25 +34,24 @@ class WSLCCLIEnvironmentOptionsUnitTests
34
{
35
WSLC_TEST_CLASS(WSLCCLIEnvironmentOptionsUnitTests)
36
37
- // Tests touch process-wide env state. Capture pre-existing values in setup
38
- // and restore them in cleanup so the suite is hermetic and doesn't clobber
39
- // values the test host (or CI) may have set.
37
+ // Tests touch process-wide env state. ScopedEnvVariable captures any
38
+ // pre-existing value in setup, clears it, and restores it in cleanup so
39
+ // the suite is hermetic.
40
TEST_METHOD_SETUP(TestMethodSetup)
41
{
42
- m_savedNoColor = CaptureEnv(L"NO_COLOR");
43
- VERIFY_IS_TRUE(SetEnvironmentVariableW(L"NO_COLOR", nullptr));
42
+ m_noColor = std::make_unique<ScopedEnvVariable>(L"NO_COLOR");
43
return true;
44
}
45
46
TEST_METHOD_CLEANUP(TestMethodCleanup)
47
{
49
- RestoreEnv(L"NO_COLOR", m_savedNoColor);
48
+ m_noColor.reset();
49
return true;
50
}
51
52
TEST_METHOD(ApplyEnvironmentOptions_NoColorEmptyValue_SetsFlag)
53
{
55
- VERIFY_IS_TRUE(SetEnvironmentVariableW(L"NO_COLOR", L""));
54
+ m_noColor->Set(L"");
55
56
ArgMap target;
57
ApplyEnvironmentOptions(target, NoColorDefs());
@@ -66,7 +65,7 @@ class WSLCCLIEnvironmentOptionsUnitTests
65
{
66
for (const auto* value : {L"0", L"false", L"FALSE", L"no", L"off"})
67
{
69
- VERIFY_IS_TRUE(SetEnvironmentVariableW(L"NO_COLOR", value));
68
+ m_noColor->Set(value);
69
70
ArgMap target;
71
ApplyEnvironmentOptions(target, NoColorDefs());
@@ -75,13 +74,13 @@ class WSLCCLIEnvironmentOptionsUnitTests
74
VERIFY_IS_TRUE(target.Contains(ArgType::NoColor));
75
VERIFY_IS_TRUE(target.Get<ArgType::NoColor>());
76
78
- VERIFY_IS_TRUE(SetEnvironmentVariableW(L"NO_COLOR", nullptr));
77
+ m_noColor->Clear();
78
}
79
}
80
81
TEST_METHOD(ApplyEnvironmentOptions_NoColorArbitraryValue_SetsFlag)
82
{
84
- VERIFY_IS_TRUE(SetEnvironmentVariableW(L"NO_COLOR", L"1"));
83
+ m_noColor->Set(L"1");
84
85
ArgMap target;
86
ApplyEnvironmentOptions(target, NoColorDefs());
@@ -101,7 +100,7 @@ class WSLCCLIEnvironmentOptionsUnitTests
100
// Env-derived defaults are lowest precedence and must not overwrite.
101
TEST_METHOD(ApplyEnvironmentOptions_TargetAlreadyContainsArg_LeavesItUntouched)
102
{
104
- VERIFY_IS_TRUE(SetEnvironmentVariableW(L"NO_COLOR", L""));
103
+ m_noColor->Set(L"");
104
105
ArgMap target;
106
target.Add<ArgType::NoColor>(false);
@@ -118,7 +117,7 @@ class WSLCCLIEnvironmentOptionsUnitTests
117
// cause NO_COLOR to leak into target.
118
TEST_METHOD(ApplyEnvironmentOptions_UndeclaredArg_IsIgnored)
119
{
121
- VERIFY_IS_TRUE(SetEnvironmentVariableW(L"NO_COLOR", L""));
120
+ m_noColor->Set(L"");
121
122
std::vector<Argument> defs;
123
defs.push_back(Argument::Create(ArgType::Verbose));
@@ -130,7 +129,7 @@ class WSLCCLIEnvironmentOptionsUnitTests
129
}
130
131
private:
133
- std::optional<std::wstring> m_savedNoColor;
132
+ std::unique_ptr<ScopedEnvVariable> m_noColor;
133
134
static std::vector<Argument> NoColorDefs()
135
{
@@ -138,23 +137,6 @@ private:
137
defs.push_back(Argument::Create(ArgType::NoColor));
138
return defs;
139
}
141
-
142
- // Snapshot a process env var. nullopt means the variable was not defined;
143
- // an empty string means it was defined as "".
144
- static std::optional<std::wstring> CaptureEnv(const wchar_t* name)
145
- {
146
- std::wstring value;
147
- if (FAILED(wil::GetEnvironmentVariableW(name, value)))
148
- {
149
- return std::nullopt;
150
- }
151
- return value;
152
- }
153
-
154
- static void RestoreEnv(const wchar_t* name, const std::optional<std::wstring>& saved)
155
- {
156
- VERIFY_IS_TRUE(SetEnvironmentVariableW(name, saved.has_value() ? saved->c_str() : nullptr));
157
- }
140
};
141
142
} // namespace WSLCCLIEnvironmentOptionsUnitTests