Fix Default/OEM encoding behavior PowerShell Core by iSazonov · Pull Request #3467 · PowerShell/PowerShell

iSazonov · 2017-03-31T15:05:09Z

Based on conclusion in discussion #3248

- OEM

We need internal fix because .Net Core haven't "OEM" term.
The fix should provide PowerShell Core behavior:

on Windows - as Windows PowerShell based on GetOEMCP() and legacy encodings (System.Text.Encoding.CodePages)
on Unix - the same as default encoding in PowerShell Core
- Default

We need internal fix because CoreFX default for all platforms is UTF8
Until we have completed Encoding RFC we should restore behaivor as in Windows PowerShell.
The fix should provide PowerShell Core behavior:

on Windows - as Windows PowerShell based on GetACP() and legacy encodings (System.Text.Encoding.CodePages)
on Unix - UTF-8 without BOM (We are expecting that the same will be in CoreFX)
- Tests

Now we don't have a full set of encoding tests (only for redirections). I believe we need to create them during future RFC implementation, not now.

iSazonov · 2017-03-31T15:31:36Z

I was able to build locally with
<PackageReference Include="System.Text.Encodings.Web" Version="4.0.11" />
in Microsoft.PowerShell.SDK.csproj
but on CI build failed 😕 I revert to Version="4.3.0" but build failed again.
@daxian-dbw Could you please help?

daxian-dbw · 2017-03-31T18:10:28Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

                    {
-                        uint oemCP = NativeMethods.GetOEMCP();
-                        encoding = System.Text.Encoding.GetEncoding((int)oemCP);
+                        encoding = ClrFacade.GetOEMEncoding();


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

It is used in ClrFacade.GetOEMEncoding

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

daxian-dbw · 2017-03-31T18:11:48Z

src/System.Management.Automation/utils/ClrFacade.cs

+#if CORECLR
+
+    #if UNUX    // PowerShell Core on Unix
+                s_defaultEncoding = Encoding.GetEncoding(65001);


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

daxian-dbw · 2017-03-31T18:12:52Z

src/System.Management.Automation/utils/ClrFacade.cs

+    #if UNUX    // PowerShell Core on Unix
+                s_defaultEncoding = Encoding.GetEncoding(65001);
+    #else       // PowerShell Core on Windows
+                uint aCp = NativeMethods.GetACP();


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

daxian-dbw · 2017-03-31T18:20:44Z

src/System.Management.Automation/utils/ClrFacade.cs

-                // We will revisit this if it causes any failures when running tests on Core PS.
-                s_defaultEncoding = Encoding.GetEncoding(28591);
-#else
+#if CORECLR


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?

daxian-dbw · 2017-03-31T18:23:00Z

src/System.Management.Automation/utils/ClrFacade.cs

+                uint aCp = NativeMethods.GetACP();
+                if (s_oemEncoding == null)
+                {
+                    Encoding.RegisterProvider(System.Text.CodePagesEncodingProvider.Instance);


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

daxian-dbw · 2017-03-31T18:24:18Z

src/System.Management.Automation/utils/PInvokeDllNames.cs

        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*/


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.

daxian-dbw · 2017-03-31T18:24:25Z

src/System.Management.Automation/utils/PInvokeDllNames.cs

        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*/


daxian-dbw · 2017-03-31T18:24:45Z

src/System.Management.Automation/utils/ClrFacade.cs

            {
-#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


Same here regarding the pattern.

daxian-dbw · 2017-03-31T18:24:59Z

src/System.Management.Automation/utils/ClrFacade.cs

+                uint oemCp = NativeMethods.GetOEMCP();
+                if (s_defaultEncoding == null)
+                {
+                    Encoding.RegisterProvider(System.Text.CodePagesEncodingProvider.Instance);


same here regarding the code duplication.

daxian-dbw · 2017-03-31T18:29:11Z

Add @mirichmo to review the new windows API set added to PInvokeDllNames.cs, to see if that's fine for downlevel machines like win7.

@iSazonov I don't see you change the .csproj file. Did you miss that?

daxian-dbw · 2017-03-31T18:38:48Z

<PackageReference Include="System.Text.Encodings.Web" Version="4.0.11" />

Why are you using System.Text.Encoding.Web? The CodePagesEncodingProvider type is available in System.Text.Encoding.CodePages

daxian-dbw · 2017-03-31T19:44:57Z

src/Microsoft.PowerShell.SDK/Microsoft.PowerShell.SDK.csproj

    <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.Encoding.CodePages" Version="4.0.11" />


Why change it to 4.0.11? I don't even find such a version on nuget.org

With 4.0.11 I can compile locally on my computer. Now I changed to 4.0.1 but failed too.

daxian-dbw · 2017-03-31T19:47:42Z

src/System.Management.Automation/utils/ClrFacade.cs

+                EncodingRegisterProvider()
+
+                uint currentAnsiCp = NativeMethods.GetACP();
+                s_defaultEncoding = Encoding.GetEncoding((int)currentAnsiCp);


Encoding.Default is coming back in netstandard2.0, are we going to change to it at that point?

We have RFC for that. No we only recover "Windows PowerShell" behavior.

daxian-dbw · 2017-03-31T19:48:52Z

src/System.Management.Automation/utils/ClrFacade.cs


        /// <summary>
-        /// Facade for Encoding.Default
+        /// Facade for getting Encoding.Default


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.

daxian-dbw · 2017-03-31T19:49:12Z

src/System.Management.Automation/utils/ClrFacade.cs

+
+                uint currentAnsiCp = NativeMethods.GetACP();
+                s_defaultEncoding = Encoding.GetEncoding((int)currentAnsiCp);
+


Please remove the extra blank line.

daxian-dbw · 2017-03-31T19:52:13Z

src/System.Management.Automation/utils/ClrFacade.cs

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


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

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

daxian-dbw · 2017-03-31T19:54:52Z

src/System.Management.Automation/utils/ClrFacade.cs

            }
            return s_oemEncoding;
        }
+


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

daxian-dbw · 2017-03-31T19:56:27Z

src/System.Management.Automation/utils/ClrFacade.cs

+
        private static volatile Encoding s_oemEncoding;

+        private static EncodingRegisterProvider()


This method should be enclosed with #if CoreCLR, as it won't build for fullclr

daxian-dbw · 2017-03-31T19:56:36Z

src/System.Management.Automation/utils/ClrFacade.cs

+            {
+                Encoding.RegisterProvider(CodePagesEncodingProvider.Instance);
+            }
+


daxian-dbw · 2017-03-31T19:57:21Z

src/System.Management.Automation/utils/ClrFacade.cs

+        {
+            if (s_defaultEncoding == null && s_oemEncoding == null)
+            {
+                Encoding.RegisterProvider(CodePagesEncodingProvider.Instance);


It's possible this line will be called more than once when there are multiple runspace. Will it fail at the second call?

First I put the call in GetDefaultEncoding and GetOEMEncoding without "if" and second call was well.

iSazonov · 2017-03-31T19:59:30Z

@daxian-dbw
I change CodePages version on 4.0.11 but CodePagesEncodingProvider cannot be finded. 😕

daxian-dbw · 2017-03-31T19:59:56Z

@JamesWTruher @mklement0, you have been closely following up with this issue (and related), could you please take a look at the fix?

mklement0 · 2017-03-31T20:36:06Z

src/System.Management.Automation/utils/ClrFacade.cs

-                s_defaultEncoding = Encoding.GetEncoding(28591);
-#else
+#if UNUX        // PowerShell Core on Unix
+                s_defaultEncoding = Encoding.UTF8;


I think it's a mistake to default to a UTF-8 encoding with BOM.
While .NET Core, at least currently, also reports the same BOM-based encoding - unofficially in Encoding.Default (not part of the contract yet) and officially in Encoding.GetDefaultEncoding(0) (and I consider that misguided too) -
at least it doesn't govern the actual default behavior in the sense of what happens if no encoding is specified. The methods of type System.IO.File default to BOM-less UTF-8 - both in .NET Framework since its inception and in .NET Core.
It think it's imperative that we use a BOM-less UTF-8 encoding in the Unix world (and, I would argue, even make that the default on Windows (Core) as well, with opt-in to legacy behavior).

daxian-dbw · 2017-03-31T20:53:08Z

src/Microsoft.PowerShell.SDK/Microsoft.PowerShell.SDK.csproj

    <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.Encoding.CodePages" Version="4.0.1" />


Why not use 4.3.0? The type System.Text.CodePagesEncodingProvider is available in that package.
You need to update SMA.csproj to add this package reference since that's the project depending on it. Then you can remove this entry from Microsoft.PowerShell.SDK.csproj.

Many thanks! It is solved the problem and PowerShell is builded well.

But currently Travis is failed on test phase with

Import-Module : Unable to load DLL 'api-ms-win-core-localization-l1-2-0.dll'

Could you comment please?

#if UNUX

As Steve pointed out, this is why 😄

…n="4.3.0" />

mklement0 · 2017-03-31T21:32:29Z

@daxian-dbw, @JamesWTruher : I know this debate is cumbersome, but I think it's a very important one to have, and I don't think we're ready to move ahead yet - I've just added further thoughts here.

SteveL-MSFT · 2017-03-31T22:24:16Z

src/System.Management.Automation/utils/ClrFacade.cs

-                // We will revisit this if it causes any failures when running tests on Core PS.
-                s_defaultEncoding = Encoding.GetEncoding(28591);
-#else
+#if UNUX        // PowerShell Core on Unix


shouldn't this be UNIX?

Oh... fixed.

mirichmo · 2017-04-01T01:31:14Z

src/System.Management.Automation/utils/PInvokeDllNames.cs

        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*/


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>

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

Nano api-ms-win-core-localization-l1-2-2.dll https://msdn.microsoft.com/en-us/library/mt588480%28v=vs.85%29.aspx?#api-ms-win-core-localization-l1-2-2.dll

Desktop api-ms-win-core-localization-l1-2-1.dll https://msdn.microsoft.com/en-us/library/windows/desktop/dn933214(v=vs.85).aspx

What version we should use?

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

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

Thanks! Clear.
Is Nano still under question?

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

iSazonov · 2017-04-01T10:19:37Z

@daxian-dbw @SteveL-MSFT CI have passed - many thanks! Sorry for many typos (make the PR in the weekend night was bad my idea). Please continue code review.

@mklement0 Can you build the branch and check our expectations? If no, could you please attach files with your tests?

mklement0 · 2017-04-01T19:21:36Z

@iSazonov: I built on my macOS 10.12 machine and I can confirm that the following ad-hoc tests return true on macOS (and, judging by the source, therefore all Unix platforms), as desired:

# Setup: *create* a  BOM-less UTF-8 file.
'ö' | set-content -nonewline /tmp/$pid.txt

# Tests: Both should output $True

# Compare the raw bytes of the new file to the UTF-8 encoding of 'ö' (0xc3 0xb6)
# With the current alpha17, this would return $False, because Set-Content creates an
# ISO-8859-1 file.
$null -eq (Compare-Object (Get-Content -Encoding Byte -Raw /tmp/$pid.txt) (0xc3, 0xb6))

# See if the BOM-less UTF-8 file is *read* correctly.
'ö' -eq (Get-Content -Raw /tmp/$pid.txt)

(I wasn't able to run the tests - the powershell binary crashed at the following point:

Describing New-PSSession basic test
 [-] New-PSSession should not crash powershell 228ms
   Expected string length 66 but was 77. Strings differ at index 0.
   Expected: {InvalidOperation,Microsoft.PowerShell.Commands.NewPSSessionCommand}
   But was:  {System.DllNotFoundException,Microsoft.PowerShell.Commands.NewPSSessionCommand}
...

)

iSazonov · 2017-04-01T19:55:11Z

@mklement0 Thanks for confirmation! It means the code is step in right direction!

All tests have passed on CI. I believe "New-PSSession basic test" too and you can ignore the error in context of the PR (or try discover a root of the error: copy-past test code in your session and see $error[0] - I think you don't install any package).

iSazonov · 2017-04-02T08:13:17Z

I added comment about tests in first post.

mklement0 · 2017-04-02T12:24:42Z

@iSazonov:

Great idea, but I suggest, to avoid ambiguity, that you explicitly mention in your comment that this PR implements BOM-less UTF-8, especially given that your comment links to the .NET Core Encoding.Default property, which is currently UTF-8 with BOM (which, as stated, is not the actual default encoding (though, fortunately, it looks like getting Encoding.Default in line with that actual default is coming)).

To add some more fodder for upcoming tests, here's a simple one that tests whether PowerShell itself correctly reads BOM-less UTF-8 source code (such as scripts) by default in PS Core on Unix:

# Setup:
# Create a no-BOM UTF-8 file with the following literal content:
#   'ö' 
# which, when executed as a PS script, should echo 'ö' back, IF the script
# was correctly decoded from UTF-8 by PS.
# UTF-8 bytes: 0x27 (single quote), 0xc3 0xb6 (encoding of 'ö', U+00F6), 0x27 (single quote)
[byte[]] (0x27, 0xc3, 0xb6, 0x27) | Set-Content -Encoding Byte /tmp/$PID.ps1

# Test: See if the 'ö' is echoed back correctly.
#       Should return $True
'ö' -eq (& /tmp/$PID.ps1)

iSazonov · 2017-04-02T12:42:30Z

@mklement0 I corrected first post about BOM-less, in the code it is obvious.

daxian-dbw · 2017-04-04T16:38:35Z

@iSazonov are you going to add tests to this PR?

iSazonov · 2017-04-04T17:58:15Z

I added in first post:

Now we don't have a full set of encoding tests (only for redirections). I believe we need to create them during future RFC implementation, not now.

I am confused. Partial test is meaningless. The full test also does not make sense until the approval of the Encoding RFC. What is compromise?

daxian-dbw · 2017-04-04T19:44:23Z

@iSazonov the code changes look good to me. If the corresponding tests will come later, then we should keep that work item tracked in an issue. Is #3248 considered fixed with this PR? If not, the test work item should be tracked in the first post of that issue, similar to how you are tracking OEM, Default and tests in this PR.
And @mklement0 provides some good testing snippets here, we should also keep them in the same issue that tracks the test work item, so they can be easily referenced when it's time to complete the tests.

iSazonov · 2017-04-05T14:04:00Z

@daxian-dbw Tracking Issue #3488.

daxian-dbw · 2017-04-05T16:03:56Z

@iSazonov Thank you! I will merge this PR later today.

iSazonov requested a review from daxian-dbw March 31, 2017 15:05

msftclas added the cla-already-signed label Mar 31, 2017

Fix Default/OEM encoding behavior PowerShell Core

9580b4a

iSazonov force-pushed the fixdefaultencoding branch from 36ab1e2 to 9580b4a Compare March 31, 2017 15:20

daxian-dbw reviewed Mar 31, 2017

View reviewed changes

daxian-dbw requested a review from mirichmo March 31, 2017 18:27

iSazonov added 2 commits March 31, 2017 22:32

Fixes after code review

64bceba

Fix errors

a26149c

daxian-dbw reviewed Mar 31, 2017

View reviewed changes

daxian-dbw requested a review from JamesWTruher March 31, 2017 19:58

Change CodePages version to Version="4.0.1" and change comment

b60e9eb

mklement0 reviewed Mar 31, 2017

View reviewed changes

Fixed extra lines and default encoding

900311c

daxian-dbw reviewed Mar 31, 2017

View reviewed changes

iSazonov added 2 commits March 31, 2017 23:56

FIx <PackageReference Include="System.Text.Encoding.CodePages" Versio…

957f01b

…n="4.3.0" />

Fix #endif

595b728

SteveL-MSFT requested changes Mar 31, 2017

View reviewed changes

mirichmo reviewed Apr 1, 2017

View reviewed changes

Fix if UNIX and UTF8Encoding

0f04279

iSazonov force-pushed the fixdefaultencoding branch from 3a8d68b to 0f04279 Compare April 1, 2017 09:48

SteveL-MSFT approved these changes Apr 2, 2017

View reviewed changes

iSazonov mentioned this pull request Apr 5, 2017

Need a full set of encoding tests #3488

Closed

6 tasks

mirichmo assigned daxian-dbw Apr 6, 2017

daxian-dbw merged commit 0883438 into PowerShell:master Apr 6, 2017

iSazonov deleted the fixdefaultencoding branch April 6, 2017 03:03

iSazonov mentioned this pull request Apr 6, 2017

Set-Content/Add-Content/Get-Content use an 8-bit character encoding by default, but the help topics state ASCII; problematic Core default file encoding #3248

Closed

iSazonov added the Documentation Needed in this repo Documentation is needed in this repo label Jun 17, 2017

iSazonov mentioned this pull request Jul 12, 2017

Unify file encoding when a cmdlet creates a file #4119

Closed

iSazonov mentioned this pull request Oct 11, 2017

Unify cmdlets with parameter 'Encoding' to be of type System.Text.Encoding #5080

Merged

joeyaiello mentioned this pull request Oct 15, 2018

Docs needed for 'Fix Default/OEM encoding behavior PowerShell Core' MicrosoftDocs/PowerShell-Docs#3105

Closed

joeyaiello removed Documentation Needed in this repo Documentation is needed in this repo labels Oct 15, 2018


		uint currentAnsiCp = NativeMethods.GetACP();
		s_defaultEncoding = Encoding.GetEncoding((int)currentAnsiCp);


		private static volatile Encoding s_oemEncoding;

		private static EncodingRegisterProvider()

Conversation

iSazonov commented Mar 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Mar 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Mar 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw commented Mar 31, 2017

Uh oh!

daxian-dbw commented Mar 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Mar 31, 2017 •

edited

Loading

daxian-dbw Mar 31, 2017 •

edited

Loading

daxian-dbw Apr 1, 2017 •

edited

Loading

mklement0 commented Mar 31, 2017 •

edited

Loading