Skip to content

/ PSScriptAnalyzer

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

Upgrade 'System.Automation.Management' NuGet package of version 6.0.0-alpha13 to official version 6.0.2 from powershell-core feed, which requires upgrade to netstandard2.0 #919

Merged
merged 16 commits into from Apr 5, 2018
Merged

Upgrade 'System.Automation.Management' NuGet package of version 6.0.0-alpha13 to official version 6.0.2 from powershell-core feed, which requires upgrade to netstandard2.0 #919

merged 16 commits into from Apr 5, 2018

Conversation

@bergmeister
Copy link
Collaborator

bergmeister commented Mar 3, 2018 •
edited

PR Summary

Closes #879

At the moment we use an unofficial package from Nuget.org, which is a self-contained version of System.Management.Automation (including dependencies) from 6.0.0-alpha13, which contains the Parser that PSSA uses.

For coreclr:

  • This required the upgrade from netstandard1.6 to netstandard2.0, which required some more NuGet packages: Microsoft.CSharp, Microsoft.Management.Infrastructure (for DSC rules, this is only available as a pre-release) and System.Reflection.TypeExtensions
  • I tested in on Windows (10) and a Ubuntu VM and I only had the same 9 local test failures that I currently also get for the development branch

For fullclr:

  • We are now using the reference assemblies instead. Because the rule UseIdenticalMandatoryParametersDSC compiles only in PSv4 I had to add a new PSV4Release build option.
  • In order to thoroughly test the fullclr change, I created 2 azure machines with WMF3 (WinServer2012) and WMF4 (WinServer2012R2) to run the test. The outcome was that for WMF3, the current release (or the development branch) have an awful lot (245) failures. This PR reduces it to only around 8, which is good and an improvement. For WMF4, there were no test failures for the development branch but 1 test in this PR. This test failure could be looked into but it seems to be minor because I can see from the test failure already that it is just a string that is slightly different (additional quote in an expected string) and therefore I think this might be negligible.

Old unused csproj files were also removed

The reason for the PowerShell Core Appveyor failure is because the Windows image is still using pwsh 6.0.0 but the system.management,automation reference is 6.0.2. I opened an issue at appveyor/ci#2230 and this will get fixed in the next 3 weeks, therefore a step was added to install pwsh 6.0.2 on the Windows image if necessary

PR Checklist

Note: Tick the boxes below that apply to this pull request by putting an x between the square brackets. Please mark anything not applicable to this PR NA.

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • [NA] User facing documentation needed
  • Change is not breaking: The test results on Ubuntu indicate breaking changes
  • [NA] Make sure you've added a new test if existing tests do not effectively test the code changed
  • This PR is ready to merge and is not work in progress
    • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready
bergmeister added 2 commits Mar 3, 2018
@bergmeister
Upgrade system.automation.management to 6.0.1.1 for netstandard, whic…
a17699b 
…h required upgrade from netstandard 1.6 to netstandard2.0,which included some breaking changes in netstandard2.0 that required some more nuget packges.
@bergmeister
use release version of TypeExtensions instead of pre-release
Loading status checks…
a48573c
@bergmeister bergmeister self-assigned this Mar 3, 2018
@bergmeister bergmeister requested a review from JamesWTruher Mar 3, 2018
@bergmeister
bergmeister reviewed Mar 3, 2018
View changes
Engine/ScriptAnalyzer.cs
@@ -1141,7 +1141,7 @@ private List<ExternalRule> GetExternalRule(string[] moduleNames)
{
dynamic description = helpContent[0].Properties["Description"];

if (null != description && null != description.Value && description.Value.GetType().IsArray)
if (description != null && description.Value != null && description.Value.GetType().IsArray)

This comment has been minimized.

Sign in to view
@bergmeister

bergmeister Mar 3, 2018 Author Collaborator

This is just to highlight why I needed the NuGet package for Microsoft.CSharp, which was included in netstandard1.6 but not netstandard2.0

This comment has been minimized.

Sign in to view
@JamesWTruher

JamesWTruher Apr 4, 2018 Member

yup - I found this as well when I was looking at building against PowerShellStandard.Library
@bergmeister
bergmeister reviewed Mar 3, 2018
View changes
Rules/Rules.csproj Outdated

<ItemGroup Condition="'$(TargetFramework)' == 'netstandard2.0'">
<PackageReference Include="System.Management.Automation" Version="6.0.1.1" />
<PackageReference Include="Microsoft.Management.Infrastructure" Version="1.0.0-alpha08" />

This comment has been minimized.

Sign in to view
@bergmeister

bergmeister Mar 3, 2018 Author Collaborator

This is a good example to show that the old System.Management.Automation package was self-contained and now we also need its other dependency Microsoft.Management.Infrastructure.
@iSazonov
Copy link

iSazonov commented Mar 5, 2018

I wonder - Could PSSA use current PowerShell installation? - there is SMA.dll with all dependencies.
@bergmeister
Copy link
Collaborator Author

bergmeister commented Mar 5, 2018

@iSazonov Can you please clarify what you mean by PowerShell installation? The issue is that currently there is a missing feature in the PowerShell Parser and I expect the change to come in about 1 month and therefore want to consume the parser via the system.management.automation nuget package in order for the update to work on any old or even current version of PowerShell such as e.g. Windows PowerShell that will not receive the parser update in the near future.
@iSazonov
Copy link

iSazonov commented Mar 6, 2018

My quesion is could PSSA load needed dlls from c:\program file\powershell or C:\Windows\System32\WindowsPowerShell\v1.0?
@bergmeister
Copy link
Collaborator Author

bergmeister commented Mar 6, 2018

It probably could but we don't wan't to. The whole point of this PR is being able to consume the latest version of system.management.automation with the latest required features of the parser.
bergmeister added 3 commits Mar 23, 2018
@bergmeister
Merge branch 'development' of https://github.com/PowerShell/PSScriptA…
Loading status checks…
83fcb7b 
…nalyzer into Parser

# Conflicts:
#	Rules/Rules.csproj
#	appveyor.yml
@bergmeister
Update System.Management.Automation from 6.0.1.1 to 6.0.2
Loading status checks…
7403578
@bergmeister
Merge branch 'development' of https://github.com/PowerShell/PSScriptA…
Loading status checks…
81bdc15 
…nalyzer into Parser
@bergmeister bergmeister changed the title WIP: Upgrade unofficial 'System.Automation.Management' NuGet package of version 6.0.0-alpha13 to official version 6.0.1.1 from powershell-core feed, which requires upgrade to netstandard2.0 WIP: Upgrade unofficial 'System.Automation.Management' NuGet package of version 6.0.0-alpha13 to official version 6.0.2 from powershell-core feed, which requires upgrade to netstandard2.0 Mar 31, 2018
@bergmeister
Copy link
Collaborator Author

bergmeister commented Apr 3, 2018 •
edited

The reason for the PowerShell Core Appveyor failure is because the Windows image is still using pwsh 6.0.0 but the system.management,automation reference is 6.0.1. I opened an issue at appveyor/ci#2230 and this will get fixed in the next 3 weeks, therefore it is probably best to disable the PowerShell Core build on Windows for the moment (the Ubuntu build should give us enough coverage in the meantime). Just using the 6.0.0 package instead is not a good either because the 6.0.0 package only works for netcore2.0 but only the 6.0.2 packages were fixed to work with netstandard2.0
@bergmeister
run PowerShell Core tests only if image has at least 6.0.2 as require…
Loading status checks…
511216c 
…d by the new system.management.automation package.
@bergmeister bergmeister changed the title WIP: Upgrade unofficial 'System.Automation.Management' NuGet package of version 6.0.0-alpha13 to official version 6.0.2 from powershell-core feed, which requires upgrade to netstandard2.0 Upgrade unofficial 'System.Automation.Management' NuGet package of version 6.0.0-alpha13 to official version 6.0.2 from powershell-core feed, which requires upgrade to netstandard2.0 Apr 3, 2018
@JamesWTruher
JamesWTruher reviewed Apr 4, 2018
View changes
Copy link
Member

JamesWTruher left a comment

just one real question down below. Otherwise, this looks fine.
Engine/ScriptAnalyzer.cs
@@ -1141,7 +1141,7 @@ private List<ExternalRule> GetExternalRule(string[] moduleNames)
{
dynamic description = helpContent[0].Properties["Description"];

if (null != description && null != description.Value && description.Value.GetType().IsArray)
if (description != null && description.Value != null && description.Value.GetType().IsArray)

This comment has been minimized.

Sign in to view
@JamesWTruher

JamesWTruher Apr 4, 2018 Member

yup - I found this as well when I was looking at building against PowerShellStandard.Library
Rules/Rules.csproj Outdated

<ItemGroup Condition="'$(TargetFramework)' == 'netstandard2.0'">
<PackageReference Include="System.Management.Automation" Version="6.0.2" />
<PackageReference Include="Microsoft.Management.Infrastructure" Version="1.0.0-alpha08" />

This comment has been minimized.

Sign in to view
@JamesWTruher

JamesWTruher Apr 4, 2018 Member

could this just be Version="1.0.0-alpha* (that's what it is in PowerShell repo)

This comment has been minimized.

Sign in to view
@bergmeister

bergmeister Apr 4, 2018 Author Collaborator

Yes, I think it is reasonable to always take the latest version of a pre-release package.
Engine/Engine.csproj Outdated
</ItemGroup>

<ItemGroup Condition="'$(TargetFramework)' == 'net451'">
<PackageReference Include="System.Management.Automation" Version="6.0.0-alpha13" />

This comment has been minimized.

Sign in to view
@JamesWTruher

JamesWTruher Apr 4, 2018 Member

since this is really about full CLR should we still point at this? Would Microsoft.PowerShell.5.ReferenceAssemblies be better? (or Microsoft.PowerShell.3.ReferenceAssemblies) rather than pointing off to that alpha13 thing?

This comment has been minimized.

Sign in to view
@bergmeister

bergmeister Apr 4, 2018 •
edited
Author Collaborator

At the moment this PR changes it only for the coreclr but I can look into how to do it for full CLR. I first wanted to rather start with the simpler task but maybe it is better to do both in one go.
@bergmeister bergmeister changed the title Upgrade unofficial 'System.Automation.Management' NuGet package of version 6.0.0-alpha13 to official version 6.0.2 from powershell-core feed, which requires upgrade to netstandard2.0 WIP Upgrade unofficial 'System.Automation.Management' NuGet package of version 6.0.0-alpha13 to official version 6.0.2 from powershell-core feed, which requires upgrade to netstandard2.0 Apr 4, 2018
bergmeister added 9 commits Apr 4, 2018
@bergmeister
install pwsh for windows if required instead of skipping tests. Thank…
Loading status checks…
4767772 
…s for Ilya for the help to get that working
@bergmeister
upgrade to net 461 and use in full clr as well
Loading status checks…
52d8521
@bergmeister
fix other references to net451 with net461
Loading status checks…
d31a55b
@bergmeister
Revert "fix other references to net451 with net461"
041c2a2 
This reverts commit d31a55b.
@bergmeister
Revert "upgrade to net 461 and use in full clr as well"
5e73dd9 
This reverts commit 52d8521.
@bergmeister
use v3/5 reference assemblies
e59178c
@bergmeister
Merge branch 'development' of https://github.com/PowerShell/PSScriptA…
Loading status checks…
09b8822 
…nalyzer into Parser
@bergmeister
Make UseIdenticalMandatoryParametersDSC rule compilable for v4 by int…
Loading status checks…
5bb6fed 
…roducing PSV4Release

Remove old csproj files that are not needed any more
@bergmeister
fix wmf4 build
Loading status checks…
c13f87b
@bergmeister bergmeister changed the title WIP Upgrade unofficial 'System.Automation.Management' NuGet package of version 6.0.0-alpha13 to official version 6.0.2 from powershell-core feed, which requires upgrade to netstandard2.0 Upgrade unofficial 'System.Automation.Management' NuGet package of version 6.0.0-alpha13 to official version 6.0.2 from powershell-core feed, which requires upgrade to netstandard2.0 Apr 4, 2018
@bergmeister
Use 1.0.0-alpha* of Microsoft.Management.Infrastructure nuget package…
Loading status checks…
0c59367 
… (i.e. latest instead of specific)
@bergmeister bergmeister changed the title Upgrade unofficial 'System.Automation.Management' NuGet package of version 6.0.0-alpha13 to official version 6.0.2 from powershell-core feed, which requires upgrade to netstandard2.0 Upgrade 'System.Automation.Management' NuGet package of version 6.0.0-alpha13 to official version 6.0.2 from powershell-core feed, which requires upgrade to netstandard2.0 Apr 4, 2018
@JamesWTruher
Copy link
Member

JamesWTruher commented Apr 5, 2018

I think we're ready to go - go ahead and merge
@bergmeister bergmeister merged commit 7304362 into PowerShell:development Apr 5, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla All CLA requirements met.
Details
@bergmeister bergmeister mentioned this pull request May 11, 2018
Merged
5 of 7 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JamesWTruher JamesWTruher

Assignees

@bergmeister bergmeister

Labels
Area - Build Area - Engine
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Upgrade from netstandard1.6 to netstandard2.0
3 participants
@bergmeister @iSazonov @JamesWTruher
You can’t perform that action at this time.