-
Notifications
You must be signed in to change notification settings - Fork 54
fix(wslc): clarify WSL runtime prerequisite #656
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -317,6 +317,22 @@ fn sdk_error(context: &str, hr: HRESULT, sdk_msg: &str) -> ScriptResponse { | |
| ScriptResponse::error(&msg) | ||
| } | ||
|
|
||
| fn wslc_prerequisite_error(missing: WslcComponentFlags) -> String { | ||
| if missing as u32 & WslcComponentFlags::WslPackage as u32 != 0 { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WslcComponentFlags is a fieldless repr(u32) enum, but wslcsdk.h declares it as combinable flags. WslcCanRun can therefore return 3 when both components are missing, which is not a valid Rust enum discriminant and causes undefined behavior when read here. Please represent this FFI type as a transparent integer-backed bitmask and add coverage for combined values. |
||
| return format!( | ||
| "WSLC runtime unavailable. Missing components: {:?}. WSL 2.8.1 or newer \ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This string says 'runtime unavailable', the one at line 330 says 'runtime not available' as does line 241 of the .md file. Let's make them consistent.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Jfyi, we are moving to the public release of 2.9 soon. That is the minimum publicly available version. |
||
| is required. Run `wsl --update` and check `wsl --version`.", | ||
| missing | ||
| ); | ||
| } | ||
|
|
||
| format!( | ||
| "WSLC runtime not available. Missing components: {:?}. Ensure WSL2 and the \ | ||
| WSLC SDK are installed.", | ||
| missing | ||
| ) | ||
| } | ||
|
|
||
| impl ScriptRunner for WSLContainerRunner { | ||
| fn execute(&mut self, request: &ExecutionRequest, logger: &mut Logger) -> ScriptResponse { | ||
| unsafe { self.run_internal(request, logger) } | ||
|
|
@@ -356,11 +372,7 @@ impl WSLContainerRunner { | |
| return Err(sdk_error("WslcCanRun failed", hr, "")); | ||
| } | ||
| if can_run == 0 { | ||
| return Err(ScriptResponse::error(&format!( | ||
| "WSLC runtime not available. Missing components: {:?}. \ | ||
| Ensure WSL2 and the WSLC SDK are installed.", | ||
| missing | ||
| ))); | ||
| return Err(ScriptResponse::error(&wslc_prerequisite_error(missing))); | ||
| } | ||
| let _ = writeln!(logger, "[WSLC] Runtime check passed"); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new helper has two user-facing message branches, but this PR adds no tests for either one. Please add table-driven tests covering WslPackage, VirtualMachinePlatform, and their combined value, asserting the version and remediation guidance selected for each case.