Move powershell to .NET Core 2.0 #3556

daxian-dbw · 2017-04-13T21:33:37Z

This change moves powershell to .NET Core 2.0. Major changes are:

PowerShell assemblies are now targeting netcoreapp2.0. We are using microsoft.netcore.app-2.0.0-preview1-001913-00, which is from dotnet-core build 4/4/17. We cannot target netstandard2.0 because the packages System.Reflection.Emit and System.Reflection.Emit.Lightweight, which are needed for powershell class, cannot be referenced when targeting netstandard2.0.
Refactor code to remove most CLR stub types and extension types.
Update build scripts to enable CI builds. ~~Note that the -cache option is removed from appveyor.yml, but it probably should be added back after this PR is merged.~~ The -cache section is added back and specified to depend on appveyor.yml, so the cache will be invalidated if appveyor.yml is changed. Thanks to @TravisEz13
Ship netcoreapp reference assemblies with powershell to fix the issues in Add-Type (Add-Type failed to create a type that implements IWebProxy #2764). By default Add-Type will reference all those reference assemblies when compiling C# code. If -ReferenceAssembly is specified, then we search reference assemblies first, then the framework runtime assemblies, and lastly the loaded assemblies (possibly a third-party one that was already loaded).
dotnet publish generates executable on Unix platforms, but doesn't set "x" permission and thus it cannot execute. Currently, the "x" permission is set in the build script, dotnet/cli issue #6286 is tracking this.
Replace the use of some APIs with the ones that take SecureString.
osx.10.12 is required to update to netcoreapp2.0 because dotnet-cli 2.0.0-preview only works on osx.10.12.
Add dependency to System.ValueTuple to work around a ambiguous type identity issue in coreclr. The issue is tracked by dotnet/corefx #17797. When moving to newer version of netcoreapp2.0, we need to verify if this dependency is still needed.

…coreapp2.0

TravisEz13 · 2017-04-13T21:40:03Z

appveyor.yml

We don't want to cache these at all?

Can't we clear the cache with a REST call instead of this change?

There is also a language to describe when to clear the cache https://www.appveyor.com/docs/build-cache/#cache-dependencies

As I mentioned in the description:

Note that the -cache option is removed from appveyor.yml, but it probably should be added back after this PR is merged.

I planned to added it back after this PR gets merged, and add one more directory path: %HOMEDRIVE%%HOMEPATH%\.dotnet. dotnet-cli 2.0.0 depends on such a folder now.

@TravisEz13 talked to me offline and it turns out it's quite easy to invalidate the cache, I will do that.

lzybkr · 2017-04-14T01:44:36Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Start-Sleep.Tests.ps1

A comment would be nice - because 1000 seems like a more reasonable value.

Comment added.

lzybkr · 2017-04-14T01:45:50Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Get-Member.Tests.ps1

I don't think this is a good or useful test - a better test would sample for some expected members.

Change the test to sample for 3 members: a, CreateNode, CreateElement.

lzybkr · 2017-04-14T01:55:45Z

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

We probably shouldn't delete !CORECLR code in the PR.

This class is added back.

lzybkr · 2017-04-14T01:59:29Z

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

I thought registry apis would throw on Unix - so why is this changed?

SafeRegistryHandle is used for transactioned registry, which is not enabled in powershell core. Actually the file SafeRegistryHandle.cs should be excluded from building powershell core.

lzybkr · 2017-04-14T02:04:51Z

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

Why is this deleted? If it's Windows PowerShell only now - it should be a different PR.

lzybkr · 2017-04-14T02:06:45Z

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

Oh, I see, you just moved this type, but it's hard to see that for sure. Can it stay in the same place to make it easier to see if anything changed - and if it should move, do that in another refactoring PR?

Can I make this an exception? DataRow/DataRowView adapters were enabled in MshObject.cs when I was working on the first cut of refactoring to enable the build. ExtraAdapter.cs contains the adapters that are not supported in powershell core and is excluded from the build, that's why I moved those 2 adapters to CoreAdatper.cs.

Never mind, I will revert this change, as I didn't add any tests for the DataRow/DataRowView adapters anyway.

lzybkr · 2017-04-14T02:10:25Z

src/System.Management.Automation/CoreCLR/CorePsPlatform.cs

So this code just moved files too? Again, it's hard to tell, and reviewing this for the first time, some of it is weird, like ProgramFiles is /bin, but ProgramFilesX86 is /sur/bin. Weird.

This code is moved from System.Management.Automation.Environment to SMA.Platform because the former class is completely removed because all environment members we extended now come back in System.Environment. Maybe we need to open a bug to review all if/defs and Unix specific code? -- #3565 is opened.

lzybkr · 2017-04-14T02:13:05Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/AddType.cs

predicted [](start = 16, length = 9)

predictable

lzybkr · 2017-04-14T02:16:59Z

@vors - can you review AddType.cs?

- Fix Start-Sleep test - Delay initializing PowerShellGet tests until they are actually about to run

…ageManagement depends on it

WaitHandle.WaitOne(milliseconds, exitContext) is not accurate. The wait time varies from 980ms to 1020ms in my observation. I change our test to check the wait time to be greater than 950ms.

daxian-dbw · 2017-04-14T20:23:34Z

@TravisEz13 @lzybkr @vors comments have been addressed, could you please continue the review? Thanks!

vors

This is soooo good! 🎉

I'm signing up mostly on the Add-Type changes with minor comments.

vors · 2017-04-15T03:11:47Z

build.psm1

    }

+    # add 'x' permission when building the standalone application
+    # this is temporary workaround to a bug in dotnet.exe, tracking by dotnet/cli issue #6286


that's a pretty hilarious bug

vors · 2017-04-15T03:22:32Z

appveyor.yml


 image: Visual Studio 2015

+# cache version - netcoreapp.2.0.0-preview1-001913-00


Should it be '005724' ?

005724 is the build of dotnet-cli we are currently using, but the netcoreapp version is actually 2.0.0-preview1-001913-00.

vors · 2017-04-15T03:35:50Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/AddType.cs

+
+            // We look up in reference/framework only when it's for resolving reference assemblies.
+            // In case of 'Add-Type -AssemblyName' scenario, we don't attempt to resolve against framework assemblies because
+            //   2. Explicitly loading a framework assembly usually is not necessary in PowerShell Core.


Good catch. Will fix.

vors · 2017-04-15T03:41:26Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/AddType.cs

+        /// </summary>
+        private static PortableExecutableReference[] InitDefaultRefAssemblies()
+        {
+            var defaultRefAssemblies = new List<PortableExecutableReference>(150);


I don't like this hardcoded value. Maybe just leave it to a default?

Currently, netcoreapp2.0 comes with 137 reference assemblies, and that's why I choose 150 as the default value. I will add a comment.
By default, the capacity of List is 0, and it increases the size as you add more items. So if the size is roughly known, it's better to specify.

vors · 2017-04-15T03:43:44Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/AddType.cs


+                    // Ignore some specified reference assemblies
+                    string fileName = PathType.GetFileName(resolvedAssemblyPath);
+                    if (s_refAssemblyNamesToIgnore.Value.Contains(fileName)) { continue; }


Lets add WriteVerbose() here about the ignoring the file names.

Will add it.

vors · 2017-04-15T03:50:59Z

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

+    <PackageReference Include="System.ServiceModel.Security" Version="4.4.0-beta-25205-01" />
+    <PackageReference Include="System.Text.Encodings.Web" Version="4.4.0-preview1-25204-02" />
+    <PackageReference Include="System.Threading.AccessControl" Version="4.4.0-preview1-25204-02" />
+    <PackageReference Include="System.Private.ServiceModel" Version="4.4.0-beta-25205-01" />


Can you double check that?
As we mentioned in https://github.com/PowerShell/PowerShell/tree/master/src/Microsoft.PowerShell.SDK

This second set includes packages that we do not necessarily require at compile-time, but must provide for our users at runtime. For example, we include the library System.Runtime.Serialization.Json.dll so that users of PowerShell can utilize its types and methods, even though PowerShell does not directly depend on the library.

I don't know much about the netstandart 2.0 model, but it's suspicious that we removing so many of them.
This list should be our SDK surface (subset of dotnet that we are promising to all applications).
It should not rely on our cross dependencies in the project.

netcoreapp2.0 now includes most of those dependencies by default, and that's why many get removed from SDK.csproj. Now I'm inclined to use SDK.csproj as:

meta project that references other projects

meta project that references those packages that are not directly depended by powershell projects.

daxian-dbw · 2017-04-17T18:53:22Z

@TravisEz13 @vors @lzybkr Thanks for the review!

iSazonov · 2017-04-18T02:59:45Z

@daxian-dbw Great work! Thanks!

Update .csproj files, build script and TypeCatalogGen to adopt to net…

45967c4

…coreapp2.0

msftclas added the cla-not-required label Apr 13, 2017

daxian-dbw requested review from TravisEz13 and lzybkr April 13, 2017 21:34

TravisEz13 reviewed Apr 13, 2017

View reviewed changes

daxian-dbw force-pushed the netstandard2 branch from 507087e to 7f065f0 Compare April 13, 2017 22:49

lzybkr reviewed Apr 14, 2017

View reviewed changes

daxian-dbw added 15 commits April 14, 2017 11:38

Update source code to have a successful build

bc09e7e

Fix Start-PSBuild

62f54c8

Update dotnet SDK to the latest

ca3003f

Update the url to obtain dotnet install script

d386e7b

appveyor.yml - remove cache

def5db0

Crossgen - remove unnecessary dlls for Add-Type

4197508

Fix Add-Type

4a14a36

Fix Get-Process, Start-Sleep and some tests

8db99dc

Fix build on Linux and some tests

db1c54f

- Fix Start-Sleep test - Delay initializing PowerShellGet tests until they are actually about to run

Revive 'Microsoft.PowerShell.CoreCLR.AssemblyExtensions' because Pack…

8a0d113

…ageManagement depends on it

Update 'build.psm1' and '.travis.yml' to fix Travis CI

068c41f

Update '.travis.yml' to install ruby-2.3.1

3fa431d

Run 'chmod u+x' on 'echoargs' and 'createchildprocess'

586bc45

Update 'Start-Sleep' test

1a6a0f5

WaitHandle.WaitOne(milliseconds, exitContext) is not accurate. The wait time varies from 980ms to 1020ms in my observation. I change our test to check the wait time to be greater than 950ms.

Fix crossgen on Linux/OSX

7170188

daxian-dbw added 3 commits April 14, 2017 11:38

Using SecureString APIs

8203f6a

Add back cache section and make the cache depend on 'appveyor.yml'

a748ad8

Address review comments

e00a2b5

daxian-dbw force-pushed the netstandard2 branch from d7e74bb to e00a2b5 Compare April 14, 2017 18:38

daxian-dbw mentioned this pull request Apr 14, 2017

Review all if/def and Unix specific code after moving to .NET Core 2.0 #3565

Closed

daxian-dbw requested a review from vors April 14, 2017 20:22

vors approved these changes Apr 15, 2017

View reviewed changes

lzybkr approved these changes Apr 17, 2017

View reviewed changes

Address more review comments

13dc9c3

daxian-dbw merged commit 7a55bf9 into PowerShell:master Apr 17, 2017

daxian-dbw deleted the netstandard2 branch April 17, 2017 18:53

This was referenced May 11, 2017

Catalog signing verification is not working in v6.0.0-beta.1 release of PowerShellCore PowerShell/PowerShellGetv2#125

Closed

Add-Type should not fail when default referenced assemblies are specified with -ReferencedAssemblies #3779

Closed

iSazonov mentioned this pull request Jan 12, 2018

Update BREAKINGCHANGES.md to Include Web Cmdlets Breaking Changes #5852

Merged

5 tasks


		image: Visual Studio 2015

		# cache version - netcoreapp.2.0.0-preview1-001913-00

Move powershell to .NET Core 2.0 #3556

Move powershell to .NET Core 2.0 #3556

Uh oh!

Conversation

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

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lzybkr Apr 14, 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!

daxian-dbw Apr 14, 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!

lzybkr commented Apr 14, 2017

Uh oh!

daxian-dbw commented Apr 14, 2017

Uh oh!

vors left a comment

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 Apr 17, 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!

daxian-dbw commented Apr 13, 2017 •

edited

Loading

lzybkr Apr 14, 2017 •

edited

Loading

daxian-dbw Apr 14, 2017 •

edited

Loading

daxian-dbw Apr 17, 2017 •

edited

Loading