Changes to build analyzer for PS7 and PS6 and ship in separate directories and bump version to 1.19.0 #1425

JamesWTruher · 2020-03-09T08:05:08Z

PR Summary

With changes in PS7 parser, rules will likely be created which will not even compile on PS6 (Ast changes). This changes our build to release assemblies for both PS6 and PS7. PS6 assemblie stays as netstandard assemblies, but the new assemblies target netcoreapp3.1.

Other changes include needed changes to sign and release via our build system (which had some breaking changes recently). Also add defines for PS7 and PS6 so new rules targeting PS7 may be more easily created.

Update to release version 1.19.0

PR Checklist

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
Change is not breaking
Make sure all .cs, .ps1 and .psm1 files have the correct copyright header
Make sure you've added a new test if existing tests do not effectively test the code changed and/or updated documentation
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.

Since there are language changes in ps7, it seemed reasonable that distinct rules would be created to support these, which are not going to be in ps6

…into ps7

Rules/Rules.csproj

bergmeister

I am OK with adding a separate conditional compilation for PS7 but I would merge this PR only once we have a rule that needs it. Or is there some value in doing that for 1.19.0, I can only imagine having newer NuGet dependencies for v7 and therefore more predictable behaviour in terms of which assembly gets loaded first.
I think it would be better if we have a separate PR just for bumping the PSSA version and NuGet references that got updated in 6.2.4 so that we can separate concerns, which is what we need for 1.19.0 and what comes in the future afterwards.

rjmholt · 2020-03-09T21:56:41Z

global.json

@@ -1,5 +1,5 @@
 {
    "sdk": {
-        "version": "3.1.102"


What's the reason for this change? This seems to be a downgrade

Jim mentioned on Slack it didn't work with 102 but only 101. Def something that would be interesting to know but I am not too worried about being one patch behind since PSSA is not a self-contained app

it was a vsts build problem - for some reason the VSTS build machine couldn't download 102 - i didn't spend the time to find out why since it was just a single patch (for the SDK)

rjmholt · 2020-03-09T21:57:32Z

tools/releaseBuild/Image/DockerFile

    Install-ChocolateyPackage -PackageName git -Executable git.exe; `
    Install-ChocolateyPackage -PackageName nuget.commandline -Executable nuget.exe -Cleanup; `
    Install-Module -Force -Name platyPS -Repository PSGallery; `
    Invoke-WebRequest -Uri https://raw.githubusercontent.com/dotnet/cli/master/scripts/obtain/dotnet-install.ps1 -outfile C:/dotnet-install.ps1; `
    C:/dotnet-install.ps1 -Channel Release -Version 2.1.4; `
    Add-Path C:/Users/ContainerAdministrator/AppData/Local/Microsoft/dotnet;

-RUN Import-Module ./containerFiles/dockerInstall.psm1; `
+#RUN Import-Module ./containerFiles/dockerInstall.psm1; `


Might be worth removing this entirely if we're no longer using this

rjmholt · 2020-03-09T21:58:50Z

but I would merge this PR only once we have a rule that needs it.

That's something of a chicken and egg problem. PSUseCompatibleSyntax could use this today, but doesn't have an ifdef to target currently. If it did, we could add visitor methods for new syntaxes that issue warnings for other versions

bergmeister · 2020-03-09T22:46:46Z

Ok, let me re-phrase it then: Do we want to get 1.19.0 out of the door in 1-2 weeks or 1-2 months? Because taking advantage of those new APIs will take some time or do you guys plan to spend much more time on PSSA for those PRs since PS and the extension have shipped now? I'm up for either approach (fast releases or wait longer for just 1 release), just want to be sure we are all on the same page.

rjmholt · 2020-03-09T23:20:11Z

Ok, let me re-phrase it then: Do we want to get 1.19.0 out of the door in 1-2 weeks or 1-2 months? Because taking advantage of those new APIs will take some time or do you guys plan to spend much more time on PSSA for those PRs since PS and the extension have shipped now? I'm up for either approach (fast releases or wait longer for just 1 release), just want to be sure we are all on the same page.

Could you expand on this some more? Which PRs are you referring to? Which changes do you mean will take advantage of the new APIs?

As far as I can think, the only rule that would easily take advantage of the new AST elements is the compatible syntax rule. Currently most of the rules just pick up the ASTs they're normally looking for, and they'll continue to do that when targeting PS7 APIs. Are there other rules you have in mind for changes?

bergmeister · 2020-03-11T17:26:44Z

OK, that sounds good if it is just your compatibility syntax rule, I thought the scope could've been bigger. Totally OK if you want to have this change as part of 1.19.0
In a few days, Azure Pipelines images are likely going to update PowerShell to 7.0 so testing is going to be interesting for 6.2 in the next months until it goes out of support in 6 months...
Will have a re-review around some of the build related changes. So far the only problem that I could spot is the issue that we also need conditions for the version of Microsoft.Management.Infrastructure.

bergmeister · 2020-03-11T22:04:50Z

Engine/PSScriptAnalyzer.psm1

@@ -11,12 +11,16 @@ $PSModuleRoot = $PSModule.ModuleBase
 $binaryModuleRoot = $PSModuleRoot


-if (($PSVersionTable.Keys -contains "PSEdition") -and ($PSVersionTable.PSEdition -ne 'Desktop')) {
-    $binaryModuleRoot = Join-Path -Path $PSModuleRoot -ChildPath 'coreclr'
+# if (($PSVersionTable.Keys -contains "PSEdition") -and ($PSVersionTable.PSEdition -ne 'Desktop')) {


Please cleanup this commented out line

bergmeister · 2020-03-11T22:07:53Z

Rules/Rules.csproj

+  </ItemGroup>
+
+  <ItemGroup Condition="'$(TargetFramework)' == 'netcoreapp3.1' ">
+    <ProjectReference Include="..\Engine\Engine.csproj" />


This line is common amongst all compilations and is therefore duplicated. Please move it into its own ItemGroup

bergmeister · 2020-03-11T22:09:22Z

Rules/Rules.csproj

    <ProjectReference Include="..\Engine\Engine.csproj" />
    <PackageReference Include="Newtonsoft.Json" Version="12.0.2" />
-    <PackageReference Include="Microsoft.Management.Infrastructure" Version="1.0.0" />
+    <PackageReference Include="Microsoft.Management.Infrastructure" Version="2.0.0" />


Shouldn't this be 1.0.0 for non PS7 versions? How come the build is green? Is the reference not needed any more maybe?

Looks like it's used in UseIdentiticalMandatoryParametersDSC. I don't know any of the details wrt to MMI, but can insert whatever might be needed

bergmeister · 2020-03-11T22:10:06Z

Rules/Rules.csproj

+    <DefineConstants>$(DefineConstants);PSV7;CORECLR</DefineConstants>
+  </PropertyGroup>
+  <ItemGroup Condition="'$(TargetFramework)' == 'netcoreapp3.1' AND '$(Configuration)' == 'PSV7Release'">
+    <PackageReference Include="Microsoft.PowerShell.SDK" Version="7.0.0" />


Why the SDK and not SMA?

strictly speaking, SMA should have never been used, but the SDK did not exist. Using the SDK is the guidance we're providing.

bergmeister · 2020-03-11T22:11:04Z

build.ps1

-        # due to netstandard2.0 we do not need to treat PS version 7 differently
-        $PSVersion = 6
-    }
+    #if ($PSVersion -gt 6) {


Please remove commented out code

bergmeister · 2020-03-11T22:15:55Z

build.psm1

@@ -240,7 +245,7 @@ function Start-ScriptAnalyzerBuild
        }

        $buildConfiguration = $Configuration
-        if ((3, 4) -contains $PSVersion) {
+        if ((3, 4, 6, 7) -contains $PSVersion) {


Because only 7 is special, wouldn't code be simpler if we didn't add 6 so that we would only need the PSVersion7 constant in the csproj file but not PSVersion6?

possibly, but i thought it would be better to be more complete and treat 5 as the only special one. Since assemblies are put in special directories, i thought it was best to provide constants that could be used

rjmholt

@JamesWTruher if you can follow up on @bergmeister's comments I can re-review and approve

rjmholt · 2020-03-31T16:45:33Z

@JamesWTruher @bergmeister this should be ready to go now, if you're able to sign off or let me know what else should be changed

bergmeister

LGTM
Just a question for my own understanding/education: Jim said that one should use the Microsoft.PowerShell.SDK NuGet package and not the SMA one, why? I don't fully understand why we reference the SDK in this case. If that is the case, shouldn't we the theoretically replace the SMA reference for the PS6 build with the SDK as well and compile against netcoreapp2.1? I am not saying we should do since it doesn't provide any value in this case since the PS6 build works and PS6 will be dead in 5 months anyway

rjmholt · 2020-03-31T18:55:44Z

Just a question for my own understanding/education: Jim said that one should use the Microsoft.PowerShell.SDK NuGet package and not the SMA one, why?

Ok I've now had a discussion with members of the PowerShell team on this one, and I think for our purposes, SMA is the right target for now. I'm hoping to write documentation of some kind to address this in the next week or so

bergmeister

Thanks, still looks good to me

bergmeister · 2020-03-31T21:06:16Z

Ok I've now had a discussion with members of the PowerShell team on this one, and I think for our purposes, SMA is the right target for now. I'm hoping to write documentation of some kind to address this in the next week or so

Interesting, thanks.
My simplistic understanding as a user is that SMA is to be used for when .net code is executed from within PowerShell itself as a reference assembly of SMA APIs since PowerShell itself already has all required assemblies. I'd use the SDK if the code was running as part of a 'normal' .net app that doesn't have the PowerShell dependencies like the SMA Dll and its other dependent assemblies so that a dotnet publish contains those DLLs and therefore ships a self-contained version of the PowerShell 'runtime' (also meaning that the author would be in charge of patching)

rjmholt · 2020-03-31T21:12:01Z

Basically the outcome of our discussion was:

Use SMA for just parsing, minimal hosting using SessionState.CreateDefault2() and only commands in the Microsoft.PowerShell.Core module, or when you need a reference assembly with version-specific APIs
Use the SDK for hosted scenarios where arbitrary commands or modules may be used or where code references are required to other Microsoft.PowerShell.* module types
Use PSStd for all reference (rather than hosted) scenarios where version-specific APIs are not required

Basically PSStd is a facade with no implementation, but it's a common denominator facade so omits version-specific APIs.

SMA is just the engine DLL, so doesn't come with the whole of what ships today as "PowerShell". It's enough to parse and run very basic PowerShell scripts, but doesn't have the modules or other required bits like the refs folder needed for Add-Type.

The SDK is the whole circus, with everything needed to run PowerShell just like pwsh.exe. But if you're just trying to compile against PowerShell as a module, it's probably overkill.

JamesWTruher added 7 commits March 3, 2020 15:11

Changes to break up coreclr build into PS6 and PS7

c485f58

Since there are language changes in ps7, it seemed reasonable that distinct rules would be created to support these, which are not going to be in ps6

move docker file changes from buildfeb2020 branch

0ec56c1

use final version of 7 sdk

d9e7138

back to sdk 3.1.101

c15e809

back off to 3.1.101

9a0cb55

Merge branch 'ps7' of https://github.com/JamesWTruher/PSScriptAnalyzer …

23d19f8

…into ps7

Bring version up to 1.19.0

cb178e1

JamesWTruher requested review from rjmholt and bergmeister March 9, 2020 08:05

bergmeister changed the title ~~Changes to build analyzer for PS7 and PS6 and ship in separate directories~~ Changes to build analyzer for PS7 and PS6 and ship in separate directories and bump version to 1.19.0 Mar 9, 2020

bergmeister reviewed Mar 9, 2020

View reviewed changes

Rules/Rules.csproj Show resolved Hide resolved

bergmeister reviewed Mar 9, 2020

View reviewed changes

Rules/Rules.csproj Show resolved Hide resolved

bergmeister reviewed Mar 9, 2020

View reviewed changes

bergmeister added the Area - Build label Mar 9, 2020

rjmholt reviewed Mar 9, 2020

View reviewed changes

JamesWTruher added 3 commits March 11, 2020 13:04

bump minimum ps6 to 6.2.4

c3b2a4c

use downlevel newtonsoft library for PS6

255b70d

remove commented code in dockerfile

147700b

rjmholt mentioned this pull request Mar 11, 2020

Ps7 syntax #1426

Merged

6 tasks

bergmeister reviewed Mar 11, 2020

View reviewed changes

rjmholt reviewed Mar 18, 2020

View reviewed changes

Address @bergmeister's comments

3178e74

bergmeister approved these changes Mar 31, 2020

View reviewed changes

Move to SMA reference

64577dd

Ensure SafeDirectoryCatalogue is not compiled in netcoreapp3.1

94c0ecd

bergmeister approved these changes Mar 31, 2020

View reviewed changes

rjmholt merged commit d3cd1f7 into PowerShell:master Mar 31, 2020

This was referenced Apr 17, 2020

Update Newtonsoft.Json from 12.0.2 to 12.0.3 in Compatibility project as well #1447

Merged

Cleanup csproj files: Remove code duplication for Configuration and entries in Rules project, since it depends on Engine project #1450

Merged

bergmeister mentioned this pull request Apr 23, 2022

Upgrade from net452 to net462 for Windows PowerShell #1789

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes to build analyzer for PS7 and PS6 and ship in separate directories and bump version to 1.19.0 #1425

Changes to build analyzer for PS7 and PS6 and ship in separate directories and bump version to 1.19.0 #1425

JamesWTruher commented Mar 9, 2020 •

edited

Loading

bergmeister left a comment •

edited

Loading

rjmholt Mar 9, 2020

bergmeister Mar 9, 2020

JamesWTruher Mar 11, 2020

rjmholt Mar 9, 2020

JamesWTruher Mar 11, 2020

rjmholt commented Mar 9, 2020

bergmeister commented Mar 9, 2020 •

edited

Loading

rjmholt commented Mar 9, 2020

bergmeister commented Mar 11, 2020

bergmeister Mar 11, 2020

bergmeister Mar 11, 2020 •

edited

Loading

JamesWTruher Mar 13, 2020

bergmeister Mar 11, 2020

rjmholt Mar 31, 2020

bergmeister Mar 11, 2020

JamesWTruher Mar 13, 2020

bergmeister Mar 11, 2020

JamesWTruher Mar 13, 2020

bergmeister Mar 11, 2020

JamesWTruher Mar 13, 2020

rjmholt left a comment

rjmholt commented Mar 31, 2020

bergmeister left a comment •

edited

Loading

rjmholt commented Mar 31, 2020

bergmeister left a comment

bergmeister commented Mar 31, 2020

rjmholt commented Mar 31, 2020 •

edited

Loading

Changes to build analyzer for PS7 and PS6 and ship in separate directories and bump version to 1.19.0 #1425

Changes to build analyzer for PS7 and PS6 and ship in separate directories and bump version to 1.19.0 #1425

Conversation

JamesWTruher commented Mar 9, 2020 • edited Loading

PR Summary

PR Checklist

bergmeister left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjmholt commented Mar 9, 2020

bergmeister commented Mar 9, 2020 • edited Loading

rjmholt commented Mar 9, 2020

bergmeister commented Mar 11, 2020

Choose a reason for hiding this comment

bergmeister Mar 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjmholt left a comment

Choose a reason for hiding this comment

rjmholt commented Mar 31, 2020

bergmeister left a comment • edited Loading

Choose a reason for hiding this comment

rjmholt commented Mar 31, 2020

bergmeister left a comment

Choose a reason for hiding this comment

bergmeister commented Mar 31, 2020

rjmholt commented Mar 31, 2020 • edited Loading

JamesWTruher commented Mar 9, 2020 •

edited

Loading

bergmeister left a comment •

edited

Loading

bergmeister commented Mar 9, 2020 •

edited

Loading

bergmeister Mar 11, 2020 •

edited

Loading

bergmeister left a comment •

edited

Loading

rjmholt commented Mar 31, 2020 •

edited

Loading