-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
fix(wsl): Non-English system users cannot use wsl machine #22932
base: main
Are you sure you want to change the base?
Conversation
in general, i agree /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: baude, BlackHole1 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
updated release note as well. |
} | ||
|
||
if user := os.Getenv("USERPROFILE"); user != "" { | ||
return filepath.Join(user, "AppData", "Local"), true |
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.
is this defined anywhere as a constant; or is it something we use more than once around our code?
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.
is this defined anywhere as a constant;
Not defined elsewhere
is it something we use more than once around our code?
Currently, it is only being used here.
return `C:\Windows\System32` | ||
} | ||
|
||
func getProgramFiles() string { |
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.
seems like some of these might be more usefully placed in general windows utils thing. nit only, no worries
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.
Do you mean it's better to put: getLocalAppData
getSystem32Root
getProgramFiles
in https://github.com/containers/podman/blob/main/pkg/machine/wsl/util_windows.go?
The reason I didn't put them in util_windows.go
is because they belong to two different packages, and these three functions are only used here.
If you think it's necessary, I will move them to util_windows.go
.
if strings.Contains(line, match) { | ||
return true | ||
} | ||
func existsKernel() bool { |
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.
would you mind a little more concise naming of this function? like WSL something or another.
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.
Do you have any recommendations? Or move the code to the IsWSLInstalled
function?
@BlackHole1 tyvm for the contribution. couple of nitty comments but else generally LGTM |
@baude Thank you for your review. I am on vacation and will update this PR in 3-4 days :) |
@baude Friendly ping :) |
well Im on PTO too! :) most of my comments were optional. need someone else from the team to review as well. |
When checking if WSL is already installed, podman relies on the English string returned by WSL. However, in non-English systems, WSL will return content in the current system language (such as Chinese). To avoid this situation, we cannot simply rely on the content returned by WSL and need to manually check if the kernel exists. Signed-off-by: Kevin Cui <bh@bugs.cc>
@n1hility PTAL |
Tried to test on an non-English VM but I don't meet the min system requirements.. Anyway, tested on my Windows box locally and it seems to work. LGTM other than @baude's concerns. |
So far, there has been no further discussion. Should we perhaps continue to wait for the code review from @n1hility 👀 ? |
When checking if WSL is already installed, podman relies on the English string returned by WSL.
However, in non-English systems, WSL will return content in the current system language (such as Chinese).
To avoid this situation, we cannot simply rely on the content returned by WSL and need to manually check if the kernel exists.
The English translation of the Chinese in the above image is:
Does this PR introduce a user-facing change?
[NO NEW TESTS NEEDED]