fix(wsl): Non-English system users cannot use wsl machine #22932
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 |
openshift-ci
bot
added
the
approved
|
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.
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.
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.
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.
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.
would you mind a little more concise naming of this function? like WSL something or another.
There was a problem hiding this comment.
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. |
openshift-merge-robot
added
the
needs-rebase
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>
openshift-merge-robot
removed
the
needs-rebase
|
@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]