Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upChanges to build analyzer for PS7 and PS6 and ship in separate directories and bump version to 1.19.0 #1425
Conversation
|
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. |
| @@ -1,5 +1,5 @@ | |||
| { | |||
| "sdk": { | |||
| "version": "3.1.102" | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
bergmeister
Mar 9, 2020
Collaborator
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
This comment has been minimized.
This comment has been minimized.
JamesWTruher
Mar 11, 2020
Author
Member
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)
| 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; ` |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
That's something of a chicken and egg problem. |
|
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? |
|
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 |
| @@ -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')) { | |||
This comment has been minimized.
This comment has been minimized.
| </ItemGroup> | ||
|
|
||
| <ItemGroup Condition="'$(TargetFramework)' == 'netcoreapp3.1' "> | ||
| <ProjectReference Include="..\Engine\Engine.csproj" /> |
This comment has been minimized.
This comment has been minimized.
bergmeister
Mar 11, 2020
•
Collaborator
This line is common amongst all compilations and is therefore duplicated. Please move it into its own ItemGroup
This comment has been minimized.
This comment has been minimized.
| <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" /> |
This comment has been minimized.
This comment has been minimized.
bergmeister
Mar 11, 2020
Collaborator
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?
This comment has been minimized.
This comment has been minimized.
rjmholt
Mar 31, 2020
Member
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
| <DefineConstants>$(DefineConstants);PSV7;CORECLR</DefineConstants> | ||
| </PropertyGroup> | ||
| <ItemGroup Condition="'$(TargetFramework)' == 'netcoreapp3.1' AND '$(Configuration)' == 'PSV7Release'"> | ||
| <PackageReference Include="Microsoft.PowerShell.SDK" Version="7.0.0" /> |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
JamesWTruher
Mar 13, 2020
Author
Member
strictly speaking, SMA should have never been used, but the SDK did not exist. Using the SDK is the guidance we're providing.
| # due to netstandard2.0 we do not need to treat PS version 7 differently | ||
| $PSVersion = 6 | ||
| } | ||
| #if ($PSVersion -gt 6) { |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| @@ -240,7 +245,7 @@ function Start-ScriptAnalyzerBuild | |||
| } | |||
|
|
|||
| $buildConfiguration = $Configuration | |||
| if ((3, 4) -contains $PSVersion) { | |||
| if ((3, 4, 6, 7) -contains $PSVersion) { | |||
This comment has been minimized.
This comment has been minimized.
bergmeister
Mar 11, 2020
Collaborator
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?
This comment has been minimized.
This comment has been minimized.
JamesWTruher
Mar 13, 2020
Author
Member
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
|
@JamesWTruher if you can follow up on @bergmeister's comments I can re-review and approve |
|
@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 |
|
LGTM |
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 |
|
Thanks, still looks good to me |
Interesting, thanks. |
|
Basically the outcome of our discussion was:
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 The SDK is the whole circus, with everything needed to run PowerShell just like |
d3cd1f7
into
PowerShell:master
JamesWTruher commentedMar 9, 2020
•
edited
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
.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.