fix(shim): Manipulating shims with UTF8 encoding #4791
Conversation
Signed-off-by: Chawye Hsu <[email protected]>
Signed-off-by: Chawye Hsu <[email protected]>
Signed-off-by: Chawye Hsu <[email protected]>
| @@ -682,7 +682,7 @@ function shim($path, $global, $name, $arg) { | |||
| ") else (", | |||
| " powershell -noprofile -ex unrestricted -file `"$resolved_path`" $arg %args%", | |||
| ")" | |||
| ) -join "`r`n" | Out-File "$shim.cmd" -Encoding ASCII | |||
| ) -join "`r`n" | Out-UTF8File "$shim.cmd" | |||
There was a problem hiding this comment.
If the PATH contains special characters, the user might need to enable support for them, like this: lptstr/winfetch#132 (comment)
There was a problem hiding this comment.
That will not work on Windows 7.
There was a problem hiding this comment.
Does Windows 7 lack that setting or does it not work on 7 even after enabling that setting? Idk anything about 7.
There was a problem hiding this comment.
This setting was introduced on Windows 10, so only Windows 10 (build 17134 or above explicitly) can use this setting to turn on the UTF-8 as ASCII feature. Otherwise, users need to use trick like chcp 65001 to work with UTF-8. https://en.wikipedia.org/wiki/Unicode_in_Microsoft_Windows
This is a well-known issue, and chcp 65001 has been talked about widely as a workaround to enable UTF-8 support for CMD/PowerShell(old versions), though this trick has bad user experiences.
There was a problem hiding this comment.
This is Win7/CMD bug, not on PowerShell, right? So it's okay and ready to be merged IMO.
There was a problem hiding this comment.
Does this matter?
It depends. I'm here just saying it's a breaking change, we have no idea of how many Windows 7 users in our community, I tend to push it forword though. The PR is ready ofc.
There was a problem hiding this comment.
There are certainly some Win 7 users, I mean is it only causes bug on CMD and run well on PowerShell? If so, then it's okay.
|
So |
|
No it adds newline, but it does not matter in this case. |
|
See #4637 (comment), so does it matter here if adding newline? |
|
Was not aware that, I tested the sh shim on the Git-Bash/MSYS2 and it worked. |
|
MSYS2/Git-Bash recognize CRLF (because they're Windows apps). The problem was in WSL. |
Signed-off-by: Chawye Hsu <[email protected]>
|
Just added the switch of |
| Out-File -FilePath $LogPath -Encoding Default -Append -InputObject $Process.StandardOutput.ReadToEnd() | ||
| Out-File -FilePath $LogPath -Encoding Default -Append -InputObject $Process.StandardError.ReadToEnd() | ||
| Out-UTF8File -FilePath $LogPath -Append -InputObject $Process.StandardOutput.ReadToEnd() | ||
| Out-UTF8File -FilePath $LogPath -Append -InputObject $Process.StandardError.ReadToEnd() |
There was a problem hiding this comment.
It seems that tarball decompression function breaks after this change on decompression logging. The decompression function failed to get the nested tarball filename from the log file. And I found the relevant commits are from #4582
cd6d31d#diff-8408a53252c7afe378fec3971ffb95781f58079a5e50bf901bcd7e5fd632093eR113
@niheaven Reason of getting the nested tarball filename from log file, instead of using a trimmed one by the outer archive filename? According to https://regex101.com/r/dlvXi2/1, the regex replacement does not work as expected.
Description
Manipulating shims with UTF8 encoding
Motivation and Context
Closes #4784
Closes #2573
Closes #2230
How Has This Been Tested?
Checklist: