8000 Fix Default/OEM encoding behavior PowerShell Core by iSazonov · Pull Request #3467 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]
Skip to content

Fix Default/OEM encoding behavior PowerShell Core #3467

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Apr 6, 2017
Merged

Fix Default/OEM encoding behavior PowerShell Core #3467

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9580b4a
Fix Default/OEM encoding behavior PowerShell Core
iSazonov Mar 31, 2017
64bceba
Fixes after code review
iSazonov Mar 31, 2017
a26149c
Fix errors
iSazonov Mar 31, 2017
b60e9eb
Change CodePages version to Version="4.0.1" and change comment
iSazonov Mar 31, 2017
900311c
Fixed extra lines and default encoding
iSazonov Mar 31, 2017
957f01b
FIx <PackageReference Include="System.Text.Encoding.CodePages" Ve…
iSazonov Mar 31, 2017
595b728
Fix #endif
iSazonov Mar 31, 2017
0f04279
Fix if UNIX and UTF8Encoding
iSazonov Apr 1, 2017
File filter

Filter by extension

Filter by extension 8000
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/System.Management.Automation/namespaces/FileSystemProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7569,8 +7569,7 @@ private static Encoding GetEncodingFromEnum(FileSystemCmdletProviderEncoding typ

case FileSystemCmdletProviderEncoding.Oem:
{
uint oemCP = NativeMethods.GetOEMCP();
encoding = System.Text.Encoding.GetEncoding((int)oemCP);
encoding = ClrFacade.GetOEMEncoding();
Copy link
Member

Choose a reason for hiding this comment

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

The NativeMethods.GetOEMCP in this file can be removed if it's no longer used.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is used in ClrFacade.GetOEMEncoding

Copy link
Member

Choose a reason for hiding this comment

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

ClrFacade has its own copy of GetOEMEncoding, so the one in FileSystemProvider.cs can be removed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

}
break;

Expand Down
43 changes: 34 additions & 9 deletions src/System.Management.Automation/utils/ClrFacade.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -530,17 +530,26 @@ internal static object[] GetCustomAttributes<T>(Assembly assembly)
#region Encoding

/// <summary>
/// Facade for Encoding.Default
/// Facade for getting Encoding.Default
Copy link
Member

Choose a reason for hiding this comment

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

Depending on what we will do when Encoding.Default is back in netstandard2.0. If we are not going to change to Encoding.Default in powershell core, then the comment should be Facade for getting default encoding.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree.

/// </summary>
internal static Encoding GetDefaultEncoding()
{
if (s_defaultEncoding == null)
{
#if CORECLR // Encoding.Default is not in CoreCLR
// As suggested by CoreCLR team (tarekms), use latin1 (ISO-8859-1, CodePage 28591) as the default encoding.
// We will revisit this if it causes any failures when running tests on Core PS.
s_defaultEncoding = Encoding.GetEncoding(28591);
#else
#if CORECLR
Copy link
Member
@daxian-dbw daxian-dbw Mar 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The if/def pattern used in other places is

#if UNIX
// unix behavior
#elif CORECLR
// win-core behavior
#else
// win-full behavior
#endif

Could you please follow the same?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.


#if UNUX // PowerShell Core on Unix
s_defaultEncoding = Encoding.GetEncoding(65001);
Copy link
Member

Choose a reason for hiding this comment

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

Why not using Encoding.UTF8? Isn't 65001 the same as utf8?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

#else // PowerShell Core on Windows
uint aCp = NativeMethods.GetACP();
Copy link
Member

Choose a reason for hiding this comment

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

Maybe rename the variable to currentAnsiCp? It's helpful to know what GetACP does.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

if (s_oemEncoding == null)
{
Encoding.RegisterProvider(System.Text.CodePagesEncodingProvider.Instance);
Copy link
Member

Choose a reason for hiding this comment

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

Is it better to separate this into a method to reduce code duplication?

}
s_defaultEncoding = Encoding.GetEncoding((int)aCp);
#endif

Copy link
Member

Choose a reason for hiding this comment

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

Please remove the extra blank line.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

#else // Windows PowerShell
s_defaultEncoding = Encoding.Default;
#endif
}
Expand All @@ -555,10 +564,20 @@ internal static Encoding GetOEMEncoding()
{
if (s_oemEncoding == null)
{
#if CORECLR // The OEM code page '437' is not supported by CoreCLR.
// Use the default encoding (ISO-8859-1, CodePage 28591) as the OEM encoding in OneCore powershell.
#if CORECLR
Copy link
Member

Choose a reason for hiding this comment

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

Same here regarding the pattern.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.


#if UNUX // PowerShell Core on Unix
s_oemEncoding = GetDefaultEncoding();
#else
#else // PowerShell Core on Windows
uint oemCp = NativeMethods.GetOEMCP();
if (s_defaultEncoding == null)
{
Encoding.RegisterProvider(System.Text.CodePagesEncodingProvider.Instance);
Copy link
Member

Choose a reason for hiding this comment

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

same here regarding the code duplication.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

}
s_oemEncoding = Encoding.GetEncoding((int)oemCp);
#endif

#else // Windows PowerShell
uint oemCp = NativeMethods.GetOEMCP();
s_oemEncoding = Encoding.GetEncoding((int)oemCp);
#endif
Copy link
Member

Choose a reason for hiding this comment

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

I didn't notice the duplicate code between coreclr and fullclr. In this case, maybe the following is better?

#if UNIX
         s_oemEncoding = GetDefaultEncoding();
#else
#if CORECLR
         EncodingRegisterProvider()
#endif
         uint oemCp = NativeMethods.GetOEMCP();
         s_oemEncoding = Encoding.GetEncoding((int)oemCp);
#end

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried - it is difficult to read. In compile time no duplicate code is. So maybe leave as is?

Expand Down Expand Up @@ -1033,6 +1052,12 @@ private static class NativeMethods
[DllImport(PinvokeDllNames.GetOEMCPDllName, SetLastError = false, CharSet = CharSet.Unicode)]
internal static extern uint GetOEMCP();

/// <summary>
/// Pinvoke for GetACP to get the Windows operating system code page.
/// </summary>
[DllImport(PinvokeDllNames.GetACPDllName, SetLastError = false, CharSet = CharSet.Unicode)]
internal static extern uint GetACP();

public const int S_OK = 0x00000000;

/// <summary>
Expand Down
2 changes: 2 additions & 0 deletions src/System.Management.Automation/utils/PInvokeDllNames.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ internal static class PinvokeDllNames
internal const string QueryDosDeviceDllName = "api-ms-win-core-file-l1-1-0.dll"; /*1*/
internal const string CreateSymbolicLinkDllName = "api-ms-win-core-file-l2-1-0.dll"; /*2*/
internal const string GetOEMCPDllName = "api-ms-win-core-localization-l1-2-0.dll"; /*3*/
internal const string GetACPDllName = "api-ms-win-core-localization-l1-2-0.dll"; /*3*/
Copy link
Member

Choose a reason for hiding this comment

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

The /*3*/ at the end is important to track if there is a mismatch. Please add this API to the end of the list and use an incremented number.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

internal const string DeviceIoControlDllName = "api-ms-win-core-io-l1-1-0.dll"; /*4*/
internal const string CreateFileDllName = "api-ms-win-core-file-l1-1-0.dll"; /*5*/
internal const string DeleteFileDllName = "api-ms-win-core-file-l1-1-0.dll"; /*6*/
Expand Down Expand Up @@ -139,6 +140,7 @@ internal static class PinvokeDllNames
internal const string QueryDosDeviceDllName = "kernel32.dll"; /*1*/
internal const string CreateSymbolicLinkDllName = "kernel32.dll"; /*2*/
internal const string GetOEMCPDllName = "kernel32.dll"; /*3*/
internal const string GetACPDllName = "kernel32.dll"; /*3*/
Copy link
Member

Choose a reason for hiding this comment

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

same here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

internal const string DeviceIoControlDllName = "kernel32.dll"; /*4*/
internal const string CreateFileDllName = "kernel32.dll"; /*5*/
internal const string DeleteFileDllName = "kernel32.dll"; /*6*/
Expand Down
0