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

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.

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.
Dongbo Wang (@daxian-dbw) Could you please help?

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.

Done.

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

Done.

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

Done.

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?

Done.

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

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.

Done.

same here.

Done.

Same here regarding the pattern.

Done.

same here regarding the code duplication.

Done.

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

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

<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

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.

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.

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.

I agree.

Please remove the extra blank line.

Done.

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?

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

Moved.

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

Done

extra line

Done.

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.

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

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

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).

Fixed.

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

Dongbo Wang (@daxian-dbw), James Truher (@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.

shouldn't this be UNIX?

Oh... fixed.

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

Dongbo Wang (@daxian-dbw) Steve Lee (@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.

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

Ilya (@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}
...

)

Michael Klement (@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).

I added comment about tests in first post.

Ilya (@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)

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

Ilya (@iSazonov) are you going to add tests to this PR?

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?

Ilya (@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 Michael Klement (@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.

Dongbo Wang (@daxian-dbw) Tracking Issue #3488.

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

Based on the conclusion in discussion PowerShell#3248, this PR fix "OEM" and "Default" encoding in powershell. 1. OEM -- We need an internal fix because .Net Core has no "OEM" encoding. The fix gives the following PowerShell Core behavior on "OEM" encoding: - 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 2. Default -- We need internal fix because CoreFX `Encoding.default` for all platforms is UTF8. Until we complete the `Encoding RFC` we should restore behavior as in Windows PowerShell. The fix gives the following PowerShell Core behavior on "Default" encoding: - 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](https://github.com/dotnet/coreclr/issues/10643))

Ilya (iSazonov) requested a review from Dongbo Wang (daxian-dbw) March 31, 2017 15:05

Microsoft Contribution License Agreements (msftclas) added the cla-already-signed label Mar 31, 2017

Fix Default/OEM encoding behavior PowerShell Core

9580b4a

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

Dongbo Wang (daxian-dbw) reviewed Mar 31, 2017

View reviewed changes

Dongbo Wang (daxian-dbw) requested a review from Mike Richmond (mirichmo) March 31, 2017 18:27

Ilya (iSazonov) added 2 commits March 31, 2017 22:32

Fixes after code review

64bceba

Fix errors

a26149c

Dongbo Wang (daxian-dbw) reviewed Mar 31, 2017

View reviewed changes

Dongbo Wang (daxian-dbw) requested a review from James Truher (JamesWTruher) March 31, 2017 19:58

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

b60e9eb

Michael Klement (mklement0) reviewed Mar 31, 2017

View reviewed changes

Fixed extra lines and default encoding

900311c

Dongbo Wang (daxian-dbw) reviewed Mar 31, 2017

View reviewed changes

Ilya (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

Steve Lee (SteveL-MSFT) requested changes Mar 31, 2017

View reviewed changes

Mike Richmond (mirichmo) reviewed Apr 1, 2017

View reviewed changes

Fix if UNIX and UTF8Encoding

0f04279

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

Steve Lee (SteveL-MSFT) approved these changes Apr 2, 2017

View reviewed changes

Ilya (iSazonov) mentioned this pull request Apr 5, 2017

Need a full set of encoding tests #3488

Closed

6 tasks

Mike Richmond (mirichmo) assigned Dongbo Wang (daxian-dbw) Apr 6, 2017

Dongbo Wang (daxian-dbw) merged commit 0883438 into PowerShell:master Apr 6, 2017

Ilya (iSazonov) deleted the fixdefaultencoding branch April 6, 2017 03:03

Ilya (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

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

Ilya (iSazonov) mentioned this pull request Jul 12, 2017

Unify file encoding when a cmdlet creates a file #4119

Closed

Ilya (iSazonov) mentioned this pull request Oct 11, 2017

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

Merged

Joey Aiello (joeyaiello) mentioned this pull request Oct 15, 2018

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

Closed

Joey Aiello (joeyaiello) removed Documentation Needed in this repo Documentation is needed in this repo labels Oct 15, 2018

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

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

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

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

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

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

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

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

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

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


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

                           }
                           return s_oemEncoding;
                       }


		private static volatile Encoding s_oemEncoding;

		private static EncodingRegisterProvider()

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

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

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

Navigation Menu

Uh oh!

Fix Default/OEM encoding behavior PowerShell Core - #3467

Fix Default/OEM encoding behavior PowerShell Core#3467Dongbo Wang (daxian-dbw) merged 8 commits intoPowerShell:masterPowerShell/PowerShell:masterfrom iSazonov:fixdefaultencodingiSazonov/PowerShell:fixdefaultencodingCopy head branch name to clipboard

Conversation

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

Uh oh!

Uh oh!

Ilya (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!

Dongbo Wang (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!

Dongbo Wang (daxian-dbw) commented Mar 31, 2017

Uh oh!

Dongbo Wang (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

Fix Default/OEM encoding behavior PowerShell Core#3467
Dongbo Wang (daxian-dbw) merged 8 commits into
PowerShell:masterfrom
iSazonov:fixdefaultencoding

Ilya (iSazonov) commented Mar 31, 2017 •
edited
Loading

Dongbo Wang (daxian-dbw) Mar 31, 2017 •
edited
Loading

Dongbo Wang (daxian-dbw) Apr 1, 2017 •
edited
Loading

Michael Klement (mklement0) commented Mar 31, 2017 •
edited
Loading