Skip to content
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(shim): Manipulating shims with UTF8 encoding #4791

Merged
merged 6 commits into from
Mar 10, 2022

Conversation

chawyehsu
Copy link
Member

@chawyehsu chawyehsu commented Mar 9, 2022

Description

Manipulating shims with UTF8 encoding

Motivation and Context

Closes #4784
Closes #2573
Closes #2230

How Has This Been Tested?

Checklist:

  • I have read the Contributing Guide.
  • I have updated the documentation accordingly.
  • I have updated the tests accordingly.

Signed-off-by: Chawye Hsu <chawyehsu@hotmail.com>
Signed-off-by: Chawye Hsu <chawyehsu@hotmail.com>
Signed-off-by: Chawye Hsu <chawyehsu@hotmail.com>
@@ -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"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks on CMD,
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the PATH contains special characters, the user might need to enable support for them, like this: lptstr/winfetch#132 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That will not work on Windows 7.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this matter?

Copy link
Member

@rashil2000 rashil2000 Mar 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does Windows 7 lack that setting or does it not work on 7 even after enabling that setting? Idk anything about 7.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@rashil2000 rashil2000 Mar 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how many Win 7 users are out there, but this feature will allow us to close #2573 and #2230 as well

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is Win7/CMD bug, not on PowerShell, right? So it's okay and ready to be merged IMO.

Copy link
Member Author

@chawyehsu chawyehsu Mar 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@niheaven
Copy link
Member

niheaven commented Mar 9, 2022

So Out-UTF8File now will not add newline at the file end?

@chawyehsu
Copy link
Member Author

No it adds newline, but it does not matter in this case.

@niheaven
Copy link
Member

niheaven commented Mar 9, 2022

See #4637 (comment), so does it matter here if adding newline?

@chawyehsu
Copy link
Member Author

Was not aware that, I tested the sh shim on the Git-Bash/MSYS2 and it worked.

@rashil2000
Copy link
Member

MSYS2/Git-Bash recognize CRLF (because they're Windows apps). The problem was in WSL.

Signed-off-by: Chawye Hsu <chawyehsu@hotmail.com>
@chawyehsu
Copy link
Member Author

Just added the switch of NoNewLine

@niheaven niheaven merged commit 5025661 into ScoopInstaller:develop Mar 10, 2022
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()
Copy link
Member Author

@chawyehsu chawyehsu Mar 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants