Update powershell to consume the latest preview packages for .NET Core 1.1.0 release (November) #2472

daxian-dbw · 2016-10-13T19:36:52Z

Resolving #2343

Updating with the latest .NET Core preview packages causes some regressions:

Latest System.Runtime.Loader.AssemblyLoadContext causes a regression in PSAssemblyLoadContext. The regression has been fixed on PS side.
With .NET Core 1.0.0 RTM, dotnet.exe doesn't verify the existence of assemblies that are built from the local projects when check the TPA deps assemblies, and thus we are able to remove IL assemblies after crossgen. However, the latest dotnet.exe checks the existence of locally built assemblies, so IL assemblies cannot be removed anymore.
The new dotnet.exe loads IL assemblies in precedence, instead of NI assemblies. The symptom is that you will find PowerShellAssemblyLoadContext.dll is loaded by the default load context when both the IL and NI assemblies exist. But with RTM packages, PowerShellAssemblyLoadContext.ni.dll gets loaded by the default load context under the same circumstance. This doesn't affect us too much now because PowerShellAssemblyLoadContext handles the loading of other PS assemblies and it prefers NI over IL assemblies. However, when the legacy APIs are back and it's time to retire PSALC, our crossgen story will be broken if the default context prefer IL over NI. I will start a conversation with .NET Core team on this.
Latest Microsoft.Win32.Registry will now raise PlatformNotSupportedException when you call any static members of it on UNIX. In PS code, we call some of its static properties when initializing some types (e.g. System.Management.Automation.Utils), and that caused PS to crash in UNIX. This is fixed.
When working on #4, I found both Registry and Certificate providers are shown up in UNIX PS. They don't work in UNIX at all, and should not be exposed (until we have a UNIX implementation of Certificate provider someday). They are now excluded from compilation for UNIX PS.
[System.Text.Encoding]::Default is available via reflection at runtime (exposed by System.Private.CoreLib), but it's not exposed in the reference contract of System.Text.Encoding, and thus we cannot use it in PS Code. This mismatch causes a redirection test to fail. The test is fixed to skip Encoding.Default for now. I will check with .NET Core team to clarify this mismatch.
The exclusion of 'Certificate' provider causes some failures on UNIX PS, because some tests are using Cert:\ to test the error behavior when not using a FileSystemProvider drive. The tests are fixed.

…24530-04 from 9/30/2016)

…the latest dotnet.exe checks the existence of all TPA assemblies, including those built from the local projects

… especially the code that would run in TypeInitializer. This is because with the latest .NET Core packages (1.1.0 preview), calling static members from 'Microsoft.Win32.Registry' will raise 'PlatformNotSupportedException'.

…y don't work on UNIX platform. With this change, 'Registry' and 'Certificate' providers won't be compiled for UNIX PS.

…ertificate' provider in UNIX PS

msftclas · 2016-10-13T19:36:57Z

Hi @daxian-dbw, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

It looks like you're a Microsoft contributor (Dongbo Wang). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

daxian-dbw · 2016-10-13T19:38:47Z

@andschwa @vors @lzybkr @mirichmo Could you please review this PR?

andyleejordan

I reviewed it all; just had no comments. Thanks for fixing the whitespace in the project.json files 😄

daxian-dbw · 2016-10-13T19:56:28Z

@andschwa Thanks Andy! It's quite easy to fix indentations in VIM, just gg=G 😄

mirichmo · 2016-10-13T22:05:17Z

src/Microsoft.PowerShell.Security/security/CertificateProvider.cs

@@ -1,3 +1,4 @@
+#if !UNIX


It'd be nice to do this within the project.json, but it looks like project.json does not support conditional inclusion and exclusion

Exactly. You can only exclude a file per target framework.

mirichmo · 2016-10-13T22:20:00Z

src/Microsoft.PowerShell.Security/security/AclCommands.cs

        private string[] _exclude = new string[0];
    }

+#if !UNIX


I'd like to see comments added explaining why it is excluded. It'll help us determine when it is appropriate to remove these ifdefs. This comment applies to all the new "#if !UNIX" additions.

I added comments to the instances that I think there might be a confusion. For rest of the #if !UNIX it's very obvious that the enclosing code doesn't apply to UNIX platform, and I think they are self-explained :)

mirichmo · 2016-10-13T22:21:32Z

src/System.Management.Automation/engine/Utils.cs

-        // Retrieves group policy settings based on the preference order provided:
-        // Dictionary<string, object> settings = GetGroupPolicySetting("Transcription", Registry.LocalMachine, Registry.CurrentUser);
-
+        // Calling static members of 'Registry' on UNIX will raise 'PlatformNotSupportedException' 


This is exactly what I am looking for on the other #if !UNIX additions.

Yes, this is an example where I think it might raise confusion, so I added comments.

mirichmo · 2016-10-13T22:24:08Z

test/powershell/Language/Parser/RedirectionOperator.Tests.ps1

+            # runtime via reflection. However,it isn't exposed in the reference contract of
+            # 'System.Text.Encoding', and therefore we cannot use 'Encoding.Default' in our
+            # code. So we need to skip this encoding in the test.
+            continue


Can you set a variable that controls whether the It in this loop is skipped? It'll satisfy the tracked skipping requirement that way.

Good point. Will do it.

Fixed. Thanks!

…ad of ignoring it, so that we can keep track of this encoding issue

daxian-dbw added 5 commits October 13, 2016 11:55

Update powershell to depend on latest dotnet core packages (preview1-…

db07a32

…24530-04 from 9/30/2016)

Update build.psm1 to not remove IL assemblies after crossgen because …

d155f7e

…the latest dotnet.exe checks the existence of all TPA assemblies, including those built from the local projects

Exclude 'Registry' and 'Certificate' providers from UNIX PS since the…

ea40c8b

…y don't work on UNIX platform. With this change, 'Registry' and 'Certificate' providers won't be compiled for UNIX PS.

Fix test failures due to changes in .NET Core and the exclusion of 'C…

dcd8c3d

…ertificate' provider in UNIX PS

msftclas added the cla-not-required label Oct 13, 2016

daxian-dbw added Review - Needed labels Oct 13, 2016

andyleejordan approved these changes Oct 13, 2016

View reviewed changes

mirichmo self-assigned this Oct 13, 2016

mirichmo mentioned this pull request Oct 13, 2016

Fix Background Jobs in *nix and Windows #1972

Merged

mirichmo suggested changes Oct 13, 2016

View reviewed changes

mirichmo added Review - Waiting on Author and removed Review - Needed labels Oct 13, 2016

Make Pester skip the redirection test for '[Encoding]::Default' inste…

e807b4f

…ad of ignoring it, so that we can keep track of this encoding issue

daxian-dbw added Review - Needed and removed Review - Waiting on Author labels Oct 13, 2016

mirichmo approved these changes Oct 14, 2016

View reviewed changes

mirichmo removed the Review - Needed label Oct 14, 2016

mirichmo merged commit e240c0c into PowerShell:master Oct 14, 2016

daxian-dbw deleted the dotnet-version branch October 14, 2016 17:03

This was referenced Oct 14, 2016

Update CoreCLR to the latest packages #2343

Closed

GetCustomAttributes fails to retrieve CmdletAttribute with ConfirmImpact property #1252

Closed

Update powershell to consume the latest preview packages for .NET Core 1.1.0 release (November) #2472

Update powershell to consume the latest preview packages for .NET Core 1.1.0 release (November) #2472

Uh oh!

Conversation

daxian-dbw commented Oct 13, 2016

Uh oh!

msftclas commented Oct 13, 2016

Uh oh!

daxian-dbw commented Oct 13, 2016

Uh oh!

andyleejordan left a comment

Choose a reason for hiding this comment

Uh oh!

daxian-dbw commented Oct 13, 2016

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants