-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Add Windows Compat Pack 2.0.0 to PSCore6 and update package references to final versions including dotnetcore 2.1 #6958
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| <PackageReference Include="PSDesiredStateConfiguration" Version="6.0.0-beta.8" /> | ||
| <PackageReference Include="PowerShellHelpFiles" Version="1.0.0-*" /> | ||
| <PackageReference Include="psrp.windows" Version="6.1.0-*" /> | ||
| <PackageReference Include="Microsoft.Windows.Compatibility" Version="2.0.0" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this reference should be put in SDK.csproj with a Condition attribute, like <PackageReference Include="Microsoft.Windows.Compatibility" Version="2.0.0" Condition=" '$(IsWindows)' != 'true' " />. In this way, application that refernce to SDK nuget package will get those extended pack too.
IsWindows is defined in https://github.com/PowerShell/PowerShell/blob/master/PowerShell.Common.props#L113
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/cc @adityapatwardhan, in case I'm wrong about the SDK reference part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I don't know if we should even have the condition part. Some of the compat assemblies work on Unix platforms too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this should be part of SDK, so apps hosting PowerShell can use the modules with WCP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we don't need the condition as @daxian-dbw is correct that some of the assemblies from the WCP meta-package work on non-Windows so we should make those available. I'll make the change.
updated all refs of 4.5.0-RC1 to 4.5.0 final
|
Ha, it looks dotnet core 2.1 is officially out: https://www.nuget.org/packages/Microsoft.NETCore.App/2.1.0 |
|
@SteveL-MSFT Can you push a commit with |
|
@SteveL-MSFT I believe you also need to update the installer for .Net Core 2.1 RTM and cleanup comments, see my commit here |
|
We have a issue with |
|
We could use Version="4.5.*" for packages. |
Can we use it directly - to access registry, perfcounters and so on? |
|
@iSazonov System.Text.Encoding.CodePages 4.5.0 is published: https://www.nuget.org/packages/System.Text.Encoding.CodePages/ |
| "Microsoft.CodeAnalysis", | ||
| "Microsoft.CodeAnalysis.CSharp", | ||
| "Microsoft.CodeAnalysis", | ||
| "Microsoft.CodeAnalysis.VisualBasic", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to include this one? I think we will remove VB support before 6.1 RC. If we include it here now, we will have to update the windows.psrp package again when removing the VB support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will remove. We can always add it back later if needed.
| </ItemGroup> | ||
|
|
||
| <ItemGroup> | ||
| <PackageReference Include="Microsoft.Windows.Compatibility" Version="2.0.0" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't know the source location of this package, maybe put it at the end, in the section with the comment the source could not be found for the following package(s), along with Microsoft.NETCore.Windows.ApiSets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
|
Can't tell why System.Text.Encoding.CodePages is failing to load as the signature matches the assembly. |
|
I suspect this is a publishing error. Could you ask .Net team? |
|
@iSazonov that xunit test passes fine on my desktop machine, just not in AppVeyor though |
|
I see the following from $requiredDependencies = @(
$nativeLib,
"$Content/Microsoft.Management.Infrastructure.dll",
"$Content/System.Text.Encoding.CodePages.dll"
)The |
|
Errors in Appveyor tests: |
|
Ok, figured out the problem with System.Text.Encoding.CodePages.dll. The nupkg doesn't contain unix runtime only Windows. Older version has both. Created https://github.com/dotnet/corefx/issues/30059 Moving back to v4.3.0 fixes that issue. Will update files.wxs and submit new commit. (v4.4.0 also doesn't have unix runtime) |
|
PSXunit tests passed on my machine (consistently failing previously) after I update the xunit packages. So push a change to see if that also fixes the issue in CI. |
|
AppVeyor failure is due to recent PR to migrate the install docs, the aka.ms link now returned 404. I updated the aka.ms link so restarting AppVeyor. |
daxian-dbw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
iSazonov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leave a comment
| <PackageReference Include="System.Text.Encoding.CodePages" Version="4.5.0" /> | ||
| <!-- the following package(s) are from the powershell org --> | ||
| <PackageReference Include="Microsoft.Management.Infrastructure" Version="1.0.0-alpha*" /> | ||
| <PackageReference Include="PowerShell.Core.Instrumentation" Version="6.0.0-beta*" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we this APIs in WCP?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are pkgs PowerShell Team publishes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we migrate to WCP assemblies if we have them on long time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean include them in WCP? Probably not.
| "Microsoft.ApplicationInsights", | ||
| "Microsoft.CodeAnalysis", | ||
| "Microsoft.CodeAnalysis.CSharp", | ||
| "Microsoft.CodeAnalysis", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was the order changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The list was generated rather than trying to insert a large number of new assemblies into the list. Sort-Object seems to prefer the period over end of string.
| <!-- DotNetCliToolReference element specifies the CLI tool that the user wants to restore in the context of the project. --> | ||
| <DotNetCliToolReference Include="dotnet-xunit" Version="2.3.1" /> | ||
| <PackageReference Include="Xunit.SkippableFact" Version="1.3.3" /> | ||
| <DotNetCliToolReference Include="dotnet-xunit" Version="2.4.0-beta.1.build3958" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should open new tracking issue to update the beta to release.
And Microsoft.VisualStudio.Web.CodeGeneration.Tools too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created #6977
| 'Microsoft.PowerShell.Commands.Management' { | ||
| $deps.Add([tuple]::Create([tuple]::Create('id', 'Microsoft.PowerShell.Security'), [tuple]::Create('version', $PackageVersion))) > $null | ||
| $deps.Add([tuple]::Create([tuple]::Create('id', 'System.ServiceProcess.ServiceController'), [tuple]::Create('version', '4.4.1'))) > $null | ||
| $deps.Add([tuple]::Create([tuple]::Create('id', 'System.ServiceProcess.ServiceController'), [tuple]::Create('version', '4.5.0'))) > $null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not related the PR - why we set the versions manually and do not take from csproj files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| <DirectoryRef Id="$(var.ProductDirectoryName)"> | ||
| <Component Id="cmpCBE3857586454EB3B634B118E8EBD1D1" Guid="{CBE38575-8645-4EB3-B634-B118E8EBD1D1}"> | ||
| <File Id="filCBE3857586454EB3B634B118E8EBD1D1" KeyPath="yes" Source="$(env.ProductSourcePath)\Microsoft.Win32.SystemEvents.dll" /> | ||
| </Component> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I lost understanding - why do we update the file manually? Can we generate it automatically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The WiX tools will break patching. When you build, I added errors to tell you what to add or remove, but didn't fully automate the update. I updated the issue with the requirements of the update.
|
Newtonsoft.Json latest version is 11.0.1 NJsonSchema latest version is 9.10.50 |
PR Summary
To support PowerShell Modules built with .Net Windows Compatibility Pack, we decided that it was best to ship the WCP assemblies with PSCore6. This also adds many new .Net apis be default while only adding ~3.5 MB additional disk footprint (to a ~137 MB install currently).
Added ref to WCP 2.0.0 to SDK csproj.
Updated all refs of 4.5.0-RC1 to 4.5.0 final.
Update dotnetcore2.1 to final build
Update WebListener to aspnetcore.app 2.1.0 final
(Microsoft.VisualStudio.Web.CodeGeneration.Tools is not final yet, so no change)
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.[feature]if the change is significant or affects feature tests