Update PowerShell to build with .NET Core SDK 2.1.300-rc1-008662 #6718

iSazonov · 2018-04-24T04:34:08Z

PR Summary

TargetFramework changed to netcoreapp21 and removed unnecessary RuntimeFrameworkVersion from PowerShell.Common.props
Update sdk to 2.1.300-rc1-008662
Updated TypeGen temporary proj target in Build.psm1 to work with 2.1
Rename macOS runtime to just osx-x64 as the current build logic expects 10.12 and breaks running on 10.13 system.
Remove assemblyreference to System.Memory as it's part of dotnetcore 2.1
Update packaging.psm1 to dotnetcore 2.1
??? Update search for crossgen executable to find matching version to sdk

Test updates:

Update test tools (WebListener and TestExe) to latest asp.net core
(!) Marked AuthHeader Redirect tests as Pending due to change in corefx
Using [string]::contains("string") instead of [string]::contains(int) due to overloads introduced in 2.1

PR Checklist

SteveL-MSFT · 2018-04-25T00:46:44Z

Working with dotnetcore team to figure out crossgen issue. Temporarily turned start-crossgen to a no-op so we can see the test results.

SteveL-MSFT · 2018-04-25T01:41:01Z

A bunch of webcmdlet tests are failing because the User-Agent string has a regression in dotnetcore 2.1-preview2. Looks like they already fixed it though.

iSazonov · 2018-04-25T07:00:59Z

Locally I tried use the older HttpClientHandler adding

AppContext.SetSwitch("System.Net.Http.UseSocketsHttpHandler", false);

and other web cmdlet tests failed. It seems better not to use the former HttpClientHandler and investigate current test failures. @markekraus Could you please help?

We could set pending for User-Agent tests until we move to .Net Core 2.1 RC.

iSazonov · 2018-04-25T09:42:27Z

I see that another test completes abnormally.
They added new overlaps:

before 2.1

$tableOutput.Contains

OverloadDefinitions
-------------------
bool Contains(string value)

in 2.1

$tableOutput.Contains

OverloadDefinitions
-------------------
bool Contains(string value)
bool Contains(string value, System.StringComparison comparisonType)
bool Contains(char value)
bool Contains(char value, System.StringComparison comparisonType)

I'm fixing the test, but it seems that it can cause problems in scripts and not only in this case because other APIs can also be extended.

markekraus · 2018-04-25T09:57:13Z

@iSazonov @SteveL-MSFT

I looked into the authorization issues. It looks like there is a change in HttpClient's behavior between 2.0 and 2.1 WRT authorization headers on redirect.

using this code in 6.0.2 and a build from @iSazonov branch:

$Handler = [System.Net.Http.HttpClientHandler]::new()
$client = [System.Net.Http.HttpClient]::new($Handler)
$req = [System.Net.Http.HttpRequestMessage]::new()
$req.Headers.Add('Authorization','test')
$req.RequestUri = 'https://httpbin.org/redirect-to?url=https://httpbin.org/get'
$res = $client.SendAsync($req).GetAwaiter().GetResult()
$res.Content.ReadAsStringAsync().GetAwaiter().GetResult()

in 6.0.2 (using dotnet 2.0):

{
  "args": {},
  "headers": {
    "Authorization": "test",
    "Connection": "close",
    "Host": "httpbin.org"
  },
  "origin": "173.239.232.139",
  "url": "https://httpbin.org/get"
}

in the build of this PR (dotnet 2.1):

{
  "args": {},
  "headers": {
    "Connection": "close",
    "Host": "httpbin.org"
  },
  "origin": "173.239.232.139",
  "url": "https://httpbin.org/get"
}

So it appears HttpClient no longer sends Authorization headers on redirect.

We can handle this in the web cmdlets with some change in logic. But I'd like to find out if this is a regression or by design breaking change in CoreFX. I might not have any free time until this weekend. if one of you could do a quick look or talk with the CoreFX folks that would be great. if not, i will look into deeper this weekend.

iSazonov · 2018-04-25T10:08:07Z

@markekraus Many thanks! Could you please look https://stackoverflow.com/questions/28564961/authorization-header-is-lost-on-redirect ? My understanding is that a http client remove authorization headers by default because of security.

markekraus · 2018-04-25T10:55:57Z

@iSazonov Yes. In .NET Framework (which is what that link is about) HttpClient did strip authorization by default. You can see this for yourself in Windows PowerShell 5.1 with the same code after running Add-Type -AssemblyName 'System.Net.Http'

In .NET Core 1.0-2.0, however, Authorization was not stripped by default. Code was added to the cmdlets to handle that. By default, we intercept the response if the authorization header is present. The pwsh web cmdlets do that by default and it can be overridden with -PreserveAuthorizationOnRedirect. This is a new feature we added to Core and it is something that we will need to continue to support as there were even recent issues opened about APIs requiring authorization headers on redirect.

If the change in 2.1 is by design we need to switch our logic around and instead intercept the redirect when the user wants to preserve the auth header.

markekraus · 2018-04-25T11:04:43Z

@iSazonov @SteveL-MSFT ugh.. the change in the String.Contains() method in 2.1 is going to cause some pain in PowerShell.

https://docs.microsoft.com/en-us/dotnet/api/system.string.contains?view=netcore-2.1
vs
https://docs.microsoft.com/en-us/dotnet/api/system.string.contains?view=netcore-2.0

I hope there is something we can point users to in our release notes for 6.1 to changes in 2.1 APIs.

iSazonov · 2018-04-25T11:29:48Z

Clear Authorization Headers on Redirect dotnet/corefx#26864

markekraus · 2018-04-25T12:04:53Z

@iSazonov thanks for finding that. It seems clear this is an intentional change so we will need to modify or logic.

The good news is that the current way was fragile (we fixed 2 issues with it since 6.0.0) and since it was default it ran more often. Now we can rely on .NET and the fragile code will run less often.

markekraus · 2018-04-25T13:00:03Z

created #6728 to track the authorization logic changes needed.

SteveL-MSFT · 2018-04-25T16:47:19Z

My thought is that we shouldn't move to dotnetcore until PSCore6+2.1 is as good or better than current PSCore6 build. So I would wait until the fixes are available to pick up in this PR than get into a state where some things work and some things don't and we accidentally miss something on release of 6.1. I would be fine moving ahead to something newer than 2.1-Preview2 since it appears dotnetcore is moving towards RC and the window of opportunity to get them to fix issues in 2.1 before they declare GA is getting smaller. For this reason, I prefer not to mark tests as Pending specifically because of 2.1.

iSazonov · 2018-05-01T18:00:34Z

@daxian-dbw Could you please review the important PR too and merge?

daxian-dbw · 2018-05-01T18:30:41Z

src/powershell-native/nativemsh/pwrshcommon/pwrshcommon.cpp

        "System.Globalization.Calendars",
        "System.Globalization.Extensions",
        "System.IO",
        "System.IO.Compression",


It seems System.ComponentModel.Composition.dll is gone. Then it should be removed from this list now.

A note about Microsoft.CodeAnalysis.VisualBasic.dll we added from the Add-Type refactoring. Since we plan to remove the VB support, we shouldn't add that dll in this list.

I'll take care of this #Closed.

daxian-dbw · 2018-05-01T18:39:26Z

test/tools/WebListener/NuGet.config

+    <add key="dotnet-core" value="https://dotnet.myget.org/F/dotnet-core/api/v3/index.json" />
+    <add key="nuget.org" value="https://api.nuget.org/v3/index.json" />
+  </packageSources>
+</configuration>


This nuget.config file doesn't introduce a new feed. Do we need it only because we need to try dotnet-core before nuget.org?

daxian-dbw · 2018-05-01T18:42:23Z

tools/packaging/packaging.psm1

            $Script:Options.Runtime -ne $Runtime -or                ## Last build wasn't for the required RID
            $Script:Options.Configuration -ne $Configuration -or    ## Last build was with configuration other than 'Release'
-            $Script:Options.Framework -ne "netcoreapp2.0")          ## Last build wasn't for CoreCLR
+            $Script:Options.Framework -ne "netcoreapp2.1")          ## Last build wasn't for CoreCLR


I see other places using netcoreapp2.0. They all need to be updated.

PS:103> dir *.psm1 -Recurse | sls -SimpleMatch netcoreapp2.0 test\tools\OpenCover\OpenCover.psm1:627: [parameter()]$PowerShellExeDirectory = "${script:psRepoPath}/src/powershell-win-core/bin/CodeCoverage/netcoreapp2.0/win7-x64/publish", tools\packaging\packaging.psm1:2221: New-MSIPackage -Verbose -ProductCode (New-Guid) -ProductSourcePath '.\src\powershell-win-core\bin\Debug\netcoreapp2.0\win7-x64\publish' -ProductTargetArchitecture x64 -ProductVersion '1.2.3' build.psm1:560: # publish netcoreapp2.0 reference assemblies

Fixed. #Closed.

daxian-dbw · 2018-05-01T18:52:50Z

build.psm1

        throw "crossgen is not available for this platform"
    }

+    $dotnetRuntimeVersion = $dotnetCLIRequiredVersion -match "(\d\.\d)\..*" | ForEach-Object { $matches[1] }


This will give us the Major.Minor version of the sdk, not the runtime. The sdk version is different from the runtime version, e.g. we use sdk 2.1.4 currently with runtime version 2.0. It may happen in future that sdk moved to 2.2 while we are still using 2.1 runtime.

Seems we need review this after .Net Core 2.1 release because it introduce new policy - always use latest version. It is still not work for Preview versions. And seems they want adjust this versions for simplicity.

It is still not work for Preview versions.

What is not working for preview versions?

The problem we had in CI w/o this is that we pick up the first version that matches which was crossgen 2.2 (some preview version) which is not compatible with the 2.1 built binaries.

I wonder how come we have the 2.2 runtime packages in our CI runs ... I wish we can figure this out.

This line of script gets the Major.Minor of the sdk version, not the runtime version, which is not what we really want (not future-proof). Maybe explicitly use 2.1* for now (which is also not future-proof, but at least intuitive than the regex matching)?

Perhaps we should rename the variable to $dotnetSdkVersion. Keeping this regex would mean that when we move to 2.2, but there's a 2.3 preview out we won't be broken.

It also means that when we move to sdk 2.2 but still building against runtime 2.1 (similar to what we have today: sdk 2.1.4, runtime 2.0), we will be broken.

Using dotnet --list-runtimes shows Microsoft.NETCore.App 2.1.0-rc1-26423-06 which matches the runtime version of crossgen we need. So perhaps we can use dotnet --list-runtimes and select the newest one and match that runtime version to find appropriate crossgen.

We can use $Script:Options.Framework to get the runtime version. The framework will always be netcoreapp2.1 until we update it again.

$dotnetRuntimeVersion = $script:Options.Framework -replace 'netcoreapp'

@daxian-dbw

What is not working for preview versions?

I meant that .Net Core starting with 2.0 has new model and always uses latest runtime versions (roll-forward minor versions) with dotnet publish but it is not working for preview versions.

daxian-dbw · 2018-05-01T18:57:00Z

build.psm1

    # Get the CrossGen.exe for the correct runtime with the latest version
    $crossGenPath = Get-ChildItem $script:Environment.nugetPackagesRoot $crossGenExe -Recurse | `
                        Where-Object { $_.FullName -match $crossGenRuntime } | `
+                        Where-Object { $_.FullName -match $dotnetRuntimeVersion } | `


Is this really necessary? I didn't try on Linux and macOS, but on Windows, the 2.1 folder is the first one after sort (I have 2.0.6, 2.0.5 and 2.0.0 runtime package installed):

PS:107> dir crossgen.exe -Recurse | ? FullName -Match win-x64 | sort -Property FullName -Descending | select -First 1 Directory: C:\Users\dongbow\.nuget\packages\runtime.win-x64.microsoft.netcore.app\2.1.0-rc1-26423-06\tools Mode LastWriteTime Length Name ---- ------------- ------ ---- -a---- 4/23/2018 9:07 AM 2916176 crossgen.exe

This was happening in CI. #Closed.

…sFromRAR' when moving to new sdk

daxian-dbw · 2018-05-01T21:49:20Z

We also need to update the versions of the dotnet NuGet packages referred by PackageReference in our .csproj files. For example, Microsoft.Win32.Registry.AccessControl in system.management.automation.csproj should be updated to 4.5.0-rc1-26423-06.

Note that, we should pick the rc1-26423 ones, which are from the same build of the netcoreapp we are using with the new sdk (AssemblyFileVersion("4.6.26423.06")).

remove System.ComponentModel.Composition from trusted list

iSazonov · 2018-05-02T12:05:37Z

LGTM.

Seems we don't open a tracking issue for web cmdlet proxy tests.
Need review and make cleanups our workarounds for curl.
Need review packaging. Perhaps in the PR (We've already missed the Preview2 time).

daxian-dbw · 2018-05-02T16:49:18Z

@iSazonov I'm not familiar with the issue regarding web cmdlet proxy tests and workaround for curl. You probably have more context, so can you please open a tracking issue for that?

As to packaging review, do you have any specific concerns about how this change would affect packaging?
Both the [Feature] and [Package] CI runs passed. We can kick off a release build after merging this PR to test the release build pipeline. /cc @TravisEz13, feel free to weigh in.

@SteveL-MSFT, @adityapatwardhan and @markekraus, since new commits were pushed after you signed off, could you please take another look at this PR? Thanks!

TravisEz13 · 2018-05-02T16:51:07Z

@daxian-dbw I agree with starting a release build after this is merged.

daxian-dbw · 2018-05-02T16:57:04Z

About the Visual Studio build, I don't know if it will still work after this merge ... @TravisEz13 Does VS2017 allow configuring which dotnet sdk to use?

TravisEz13 · 2018-05-02T18:18:58Z

For Visual Studio you cannot specify the SDK.

SteveL-MSFT · 2018-05-02T18:33:24Z

This has been a long thread, @iSazonov can you summarize the proxy issue with the webcmdlet tests? I changed the tests as they were originally just wrong and incomplete. I think we should probably remove the tests for http_proxy env var which we skip on Windows because that is a side effect of corefx taking dependency on libcurl for non-Windows and not something supported explicitly by corefx nor the webcmdlets.

iSazonov · 2018-05-02T18:42:39Z

Currently I agree to remove this proxy tests. But I'll look this in depth only tomorrow. If I'll discover something important I'll open new tracking Issue.

iSazonov · 2018-05-02T18:43:16Z

The PR LGTM.

SteveL-MSFT · 2018-05-02T18:46:15Z

@iSazonov just to be clear, only the proxy tests for the http_proxy and https_proxy env vars. The ones using -proxy should stay and currently work. Created #6804

SteveL-MSFT

LGTM

daxian-dbw · 2018-05-02T23:59:09Z

PR merged. Thanks everyone for getting this done!

iSazonov · 2018-05-03T06:21:57Z

Thanks everyone for this work!

Maybe it's a good experience of working together.

iSazonov requested review from TravisEz13, adityapatwardhan and daxian-dbw as code owners April 24, 2018 04:34

iSazonov mentioned this pull request Apr 24, 2018

Move to .Net Core 2.1 RC1-Final #6632

Closed

iSazonov force-pushed the move-to-netcore210preview2 branch 2 times, most recently from d007b1a to f585424 Compare April 24, 2018 11:55

SteveL-MSFT force-pushed the move-to-netcore210preview2 branch 2 times, most recently from 8bdf8a5 to d3c5edf Compare April 24, 2018 23:00

SteveL-MSFT force-pushed the move-to-netcore210preview2 branch from 3e071a6 to 1e60cd9 Compare April 25, 2018 00:56

iSazonov force-pushed the move-to-netcore210preview2 branch 3 times, most recently from 6eae8be to 7b5c542 Compare April 25, 2018 06:13

iSazonov force-pushed the move-to-netcore210preview2 branch from ac8906f to 12da43b Compare April 25, 2018 10:20

iSazonov force-pushed the move-to-netcore210preview2 branch from cff8bdc to 4ec4f91 Compare April 25, 2018 12:13

markekraus mentioned this pull request Apr 25, 2018

Adjust logic of Web Cmdlet Authorization Header Stripping to work with .NET Core 2.1 #6728

Closed

iSazonov force-pushed the move-to-netcore210preview2 branch 2 times, most recently from 0fa3ed9 to 1f4ea23 Compare April 25, 2018 14:24

SteveL-MSFT force-pushed the move-to-netcore210preview2 branch from 1f4ea23 to f4bbdf2 Compare April 25, 2018 19:05

markekraus approved these changes May 1, 2018

View reviewed changes

[package] Minor update the script figuring out default Runtime

752e133

daxian-dbw requested changes May 1, 2018

View reviewed changes

iSazonov and others added 2 commits May 2, 2018 00:28

Replace netcoreapp2.0 with netcoreapp2.1 in scripts

d322c46

Suppress the preview sdk warning

fcbed2d

PowerShell deleted a comment from TravisEz13 May 1, 2018

Add comment about how to find the available properties of '_Reference…

183def7

…sFromRAR' when moving to new sdk

SteveL-MSFT and others added 4 commits May 1, 2018 15:31

[feature]

11e18bf

remove System.ComponentModel.Composition from trusted list

[Feature] Update NuGet package versions used in 'PackageReference' tags

e042397

[Feature] Update Files.wxs as instructed by the build failure

4956c77

[Package] Use $script:Options.Framework to get the runtime version

cb41ffc

daxian-dbw approved these changes May 2, 2018

View reviewed changes

SteveL-MSFT approved these changes May 2, 2018

View reviewed changes

adityapatwardhan approved these changes May 2, 2018

View reviewed changes

daxian-dbw merged commit 84344cb into PowerShell:master May 2, 2018

iSazonov mentioned this pull request May 3, 2018

Quote Multipart/Form-Data Field Names #6782

Merged

11 tasks

iSazonov deleted the move-to-netcore210preview2 branch September 28, 2018 12:45

Update PowerShell to build with .NET Core SDK 2.1.300-rc1-008662 #6718

Update PowerShell to build with .NET Core SDK 2.1.300-rc1-008662 #6718

Uh oh!

Conversation

iSazonov commented Apr 24, 2018 • edited by SteveL-MSFT Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

Test updates:

PR Checklist

Uh oh!

SteveL-MSFT commented Apr 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SteveL-MSFT commented Apr 25, 2018

Uh oh!

iSazonov commented Apr 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Apr 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markekraus commented Apr 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Apr 25, 2018

Uh oh!

markekraus commented Apr 25, 2018

Uh oh!

markekraus commented Apr 25, 2018

Uh oh!

iSazonov commented Apr 25, 2018

Uh oh!

markekraus commented Apr 25, 2018

Uh oh!

markekraus commented Apr 25, 2018

Uh oh!

SteveL-MSFT commented Apr 25, 2018

Uh oh!

iSazonov commented May 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT May 1, 2018 • edited by daxian-dbw 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!

iSazonov May 1, 2018 • edited by daxian-dbw 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!

iSazonov May 2, 2018 • 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!

iSazonov commented Apr 24, 2018 •

edited by SteveL-MSFT

Loading

SteveL-MSFT commented Apr 25, 2018 •

edited

Loading

iSazonov commented Apr 25, 2018 •

edited

Loading

iSazonov commented Apr 25, 2018 •

edited

Loading

markekraus commented Apr 25, 2018 •

edited

Loading

SteveL-MSFT May 1, 2018 •

edited by daxian-dbw

Loading

iSazonov May 1, 2018 •

edited by daxian-dbw

Loading

iSazonov May 2, 2018 •

edited

Loading

SteveL-MSFT May 1, 2018 •

edited by daxian-dbw

Loading

iSazonov commented May 2, 2018 •

edited

Loading

daxian-dbw commented May 2, 2018 •

edited

Loading

SteveL-MSFT commented May 2, 2018 •

edited

Loading