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 all commits
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
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/Microsoft.PowerShell.SDK/Microsoft.PowerShell.SDK.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,6 @@
<PackageReference Include="System.ServiceModel.Primitives" Version="4.3.0" />
<PackageReference Include="System.ServiceModel.Security" Version="4.3.0" />
<PackageReference Include="System.ServiceProcess.ServiceController" Version="4.3.0" />
<PackageReference Include="System.Text.Encoding.CodePages" Version="4.3.0" />
<PackageReference Include="System.Text.Encodings.Web" Version="4.3.0" />
<PackageReference Include="System.Threading.AccessControl" Version="4.3.0" />
<PackageReference Include="System.Threading.Overlapped" Version="4.3.0" />
Expand Down
1 change: 1 addition & 0 deletions src/System.Management.Automation/System.Management.Automation.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
<PackageReference Include="System.Security.Cryptography.Algorithms" Version="4.3.0" />
<PackageReference Include="System.Security.Cryptography.Pkcs" Version="4.3.0" />
<PackageReference Include="System.Security.Cryptography.X509Certificates" Version="4.3.0" />
<PackageReference Include="System.Text.Encoding.CodePages" Version="4.3.0" />
<PackageReference Include="System.Threading.Thread" Version="4.3.0" />
<PackageReference Include="System.Threading.Tasks.Parallel" Version="4.3.0" />
<PackageReference Include="System.Xml.XPath.XmlDocument" Version="4.3.0" />
Expand Down
8 changes: 1 addition & 7 deletions src/System.Management.Automation/namespaces/FileSystemProvider.cs
View file
Open in desktop
10000
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 Expand Up @@ -7607,11 +7606,6 @@ public bool WasStreamTypeSpecified
} // get
} // WasStreamTypeSpecified

private static class NativeMethods
{
[DllImport(PinvokeDllNames.GetOEMCPDllName, SetLastError = false, CharSet = CharSet.Unicode)]
internal static extern uint GetOEMCP();
}
} // class FileSystemContentDynamicParametersBase

/// <summary>
Expand Down
44 changes: 35 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,22 +530,26 @@ internal static object[] GetCustomAttributes<T>(Assembly assembly)
#region Encoding

/// <summary>
/// Facade for Encoding.Default
/// Facade for getting default encoding
/// </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 UNIX // PowerShell Core on Unix
s_defaultEncoding = new UTF8Encoding(false);
#elif CORECLR // PowerShell Core on Windows
EncodingRegisterProvider();

uint currentAnsiCp = NativeMethods.GetACP();
s_defaultEncoding = Encoding.GetEncoding((int)currentAnsiCp);
#else // Windows PowerShell
s_defaultEncoding = Encoding.Default;
#endif
}
return s_defaultEncoding;
}

private static volatile Encoding s_defaultEncoding;

/// <summary>
Expand All @@ -555,18 +559,34 @@ 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 UNIX // PowerShell Core on Unix
s_oemEncoding = GetDefaultEncoding();
#else
#elif CORECLR // PowerShell Core on Windows
EncodingRegisterProvider();

uint oemCp = NativeMethods.GetOEMCP();
s_oemEncoding = Encoding.GetEncoding((int)oemCp);

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

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?

}
return s_oemEncoding;
}

Copy link
Member

Choose a reason for hiding this comment

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

Remove extra line, or move private static volatile Encoding s_defaultEncoding; here as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved.

private static volatile Encoding s_oemEncoding;

#if CORECLR
private static void EncodingRegisterProvider()
{
if (s_defaultEncoding == null && s_oemEncoding == null)
{
Encoding.RegisterProvider(CodePagesEncodingProvider.Instance);
}
}
#endif

#endregion Encoding

#region Security
Expand Down Expand Up @@ -1033,6 +1053,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 @@ -135,6 +135,7 @@ internal static class PinvokeDllNames
internal const string CreateToolhelp32SnapshotDllName = "api-ms-win-core-toolhelp-l1-1-0"; /*120*/
internal const string Process32FirstDllName = "api-ms-win-core-toolhelp-l1-1-0"; /*121*/
internal const string Process32NextDllName = "api-ms-win-core-toolhelp-l1-1-0"; /*122*/
internal const string GetACPDllName = "api-ms-win-core-localization-l1-2-0.dll"; /*123*/
Copy link
Member

Choose a reason for hiding this comment

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

The API set matches the API on my laptop. I don't have access to a Nano VM at the moment to verify it there. On Nano, it must match the exporting API set exactly. You can verify it with dumpbin.exe /exports <path to api set>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now I don't run still dumpbin.exe /exports
MSDN docs say about:

What version we should use?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

GetACP is in api-ms-win-core-localization-l1-2-0.dll of runtime.win7-x64.Microsoft.NETCore.Windows.ApiSets dump.
But It seems Nano use runtime.win10-x64.Microsoft.NETCore.Windows.ApiSets

Copy link
Member

Choose a reason for hiding this comment

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

It is correct "as-is" here. No change needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! Clear.
Is Nano still under question?

Copy link
Member

Choose a reason for hiding this comment

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

No. Nano Server contains the 1-2-0 version of the API set

#else
internal const string QueryDosDeviceDllName = "kernel32.dll"; /*1*/
internal const string CreateSymbolicLinkDllName = "kernel32.dll"; /*2*/
Expand Down Expand Up @@ -257,6 +258,7 @@ internal static class PinvokeDllNames
internal const string CreateToolhelp32SnapshotDllName = "kernel32.dll"; /*120*/
internal const string Process32FirstDllName = "kernel32.dll"; /*121*/
internal const string Process32NextDllName = "kernel32.dll"; /*122*/
internal const string GetACPDllName = "kernel32.dll"; /*123*/
#endif
}
}
0