Simplify build scripts even more and upgrade platyPS in Appveyor #1088

bergmeister · 2018-10-23T21:57:10Z

PR Summary

This is a follow up of PR #1082 that improves it by further simplifying and tidying up the build scripts.
Especially: removing the $framework parameter as we can figure it out ourselves, upgrade platyPS version installed in AppVeyor for security (see here)

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.

rjmholt

LGTM! Left a couple of comments, but nothing blocking.

rjmholt · 2018-11-05T22:01:13Z

build.psm1

-    #$modInfo = new-object Microsoft.PowerShell.Commands.ModuleSpecification -ArgumentList @{ ModuleName = "platyps"; ModuleVersion = $requiredVersionOfplatyPS}
-    #if ( $null -eq (Get-Module -ListAvailable -FullyQualifiedName $modInfo))
-    if ( $null -eq (Get-Module -ListAvailable platyPS))
+    if ( $null -eq (Get-Module -ListAvailable platyPS) -or ((Get-Module -ListAvailable platyPS | Select-Object -First 1).Version -lt [version]0.12))


Style: Got a space on the left of the $null but no space on the right of the 0.12.

gmo -List is being called twice here and only looks at the first module (gmo -List returns module in essentially random order, so that might not be the check we want).

Maybe something like:

$platyPS = Get-Module -ListAvailable @{ ModuleName = 'platyPS'; ModuleVersion = '0.12' } ` | Sort-Object Version ` | Select-Object -First 1 if (-not $platyPS) { Write-Verbose -verbose "platyPS module not found or below required version of 0.12, installing the latest version." Install-Module -Force -Name platyPS -Scope CurrentUser }

Refactored that slightly to use a FullyQualifiedName

Sorry, but Get-Module -ListAvailable @{ ModuleName = 'platyPS'; ModuleVersion = '0.12.0' } does not work on my machine using either WPS or PSCore, I will refactor it to call gmo only once though and sort by version. This shows that gmo could benefit from -MinimumVersion parameter. P.S. Sort-Object Version also needs the Descending switch to get the latest version, otherwise it'd be the earliest.

rjmholt · 2018-11-05T22:02:49Z

build.psm1

-            $destinationDirBinaries = "$destinationDir\coreclr"
-        }
-        elseif ($PSVersion -eq '3') {
+        if ($PSVersion -eq '3') {


$PSVersion above is compared to an [int] rather than a [string] -- I know they work out the same though :)

This if/elseif/else might be a good candidate for a switch maybe:

switch ($PSVersion) { 3 { } 4 { } 5 { } default { } }

Thanks, point taken. I'll change it to a switch for 3,4,5 and 6 and throw otherwise

rjmholt · 2018-11-05T22:05:52Z

build.psm1

        # The Rules project has a dependency on the Engine therefore just building the Rules project is enough
        $config = "PSV${PSVersion}${Configuration}"
        try {
            Push-Location $projectRoot/Rules
-            Write-Progress "Building ScriptAnalyzer '$framework' version '${PSVersion}' configuration '${Configuration}'"
-            $buildOutput = dotnet build Rules.csproj --framework $frameworkName --configuration "${config}"
+            Write-Progress "Building ScriptAnalyzer for PSVersion '$PSVersion' using framework '$framework' and configuration '$config'"


I don't see a change in variables $config or $Configuration in the script -- just want to check you don't mean $Configuration here.

$Configuration is the high level Debug\Release variable that the end user uses when calling the build script, internally, the actual configuration passed to msbuild is $config, which can be different and is e.g. PSV3Release for WMF3. I thought logging the actual low level variable was better, but looking at it now, I am probably wrong and will change it back to $Configuration

rjmholt · 2018-11-05T22:06:21Z

build.psm1

-            Write-Progress "Building ScriptAnalyzer '$framework' version '${PSVersion}' configuration '${Configuration}'"
-            $buildOutput = dotnet build Rules.csproj --framework $frameworkName --configuration "${config}"
+            Write-Progress "Building ScriptAnalyzer for PSVersion '$PSVersion' using framework '$framework' and configuration '$config'"
+            $buildOutput = dotnet build --framework $framework --configuration "${config}"


Might be worth taking the ${config} out of brackets here

Yes, this was a James-ism ;-)

rjmholt · 2018-11-05T22:07:56Z

@JamesWTruher gentle ping on the review request

JamesWTruher

rob makes a couple good points (esp wrt multiple calls to get-module), so it would be good to address those.
thanks for doing this, I'll abandon my similar changes :)

…nalyzer into upgradePlatyPS

bergmeister · 2018-12-19T21:40:58Z

Ok, I addressed all points and added one more improvement, which is to build docs automatically the first time if the doc file does not exist and then do not always build docs, the behaviour of the Documentation parameter it therefore similar to the -ResGen parameter on Start-PSBuild. In another PR, I'd also like to get rid of the parameter sets in build.ps1 to allow combining switches like e.g. Clean or Test.
Merge is now only being blocked by the build failure on Linux due to the AppVeyor image update. PR #1107 has the best possible workaround for that atm, can you please review this one @rjmholt while James is out of office?

… save time on rebuild and get rid of unnecessary low level parameter sets

…nalyzer into upgradePlatyPS

…nalyzer into upgradePlatyPS # Conflicts: # Utils/ReleaseMaker.psm1 # build.psm1

…nalyzer into upgradePlatyPS

…erShell#1088) * simplify build scripts even more and upgrade platyPS in Appveyor * add check for minimum version of platyps * fix ci and remove other leftovers * fix appveyor core build * fix wmf4 build by including the newtonsoft dll * fix syntax * trigger ci * trigger ci * address PR comments * build docs automatically the first time to simplify build process and save time on rebuild and get rid of unnecessary low level parameter sets * revert accidentally checked in file * fix merge/integration problem when merging from upstream * Fix another merge problem due to variable rename

bergmeister added 8 commits October 23, 2018 22:55

simplify build scripts even more and upgrade platyPS in Appveyor

6f97d1a

add check for minimum version of platyps

749138b

fix ci and remove other leftovers

863b48b

fix appveyor core build

95b4042

fix wmf4 build by including the newtonsoft dll

2c0326b

fix syntax

7d78b1a

trigger ci

3be79c7

trigger ci

43113ac

bergmeister requested review from JamesWTruher and rjmholt October 24, 2018 08:04

rjmholt approved these changes Nov 5, 2018

View reviewed changes

JamesWTruher approved these changes Dec 6, 2018

View reviewed changes

bergmeister added 2 commits December 19, 2018 20:57

Merge branch 'development' of https://github.com/PowerShell/PSScriptA…

2c93636

…nalyzer into upgradePlatyPS

address PR comments

7517b5a

bergmeister added 2 commits December 19, 2018 22:09

build docs automatically the first time to simplify build process and…

c595fcc

… save time on rebuild and get rid of unnecessary low level parameter sets

Merge branch 'development' of https://github.com/PowerShell/PSScriptA…

9de6986

…nalyzer into upgradePlatyPS

bergmeister added the Area - Build label Jan 9, 2019

bergmeister added 5 commits January 15, 2019 07:26

Merge branch 'development' of https://github.com/PowerShell/PSScriptA…

0ef7eff

…nalyzer into upgradePlatyPS # Conflicts: # Utils/ReleaseMaker.psm1 # build.psm1

revert accidentally checked in file

3e53231

Merge branch 'development' of https://github.com/PowerShell/PSScriptA…

8a44577

…nalyzer into upgradePlatyPS

fix merge/integration problem when merging from upstream

ac11bf5

Fix another merge problem due to variable rename

fa1c18a

bergmeister merged commit 7a9d45d into PowerShell:development Jan 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify build scripts even more and upgrade platyPS in Appveyor #1088

Simplify build scripts even more and upgrade platyPS in Appveyor #1088

bergmeister commented Oct 23, 2018 •

edited

Loading

rjmholt left a comment

rjmholt Nov 5, 2018 •

edited

Loading

rjmholt Dec 6, 2018

bergmeister Dec 19, 2018 •

edited

Loading

rjmholt Nov 5, 2018

rjmholt Nov 5, 2018

bergmeister Dec 19, 2018

rjmholt Nov 5, 2018

bergmeister Dec 19, 2018 •

edited

Loading

rjmholt Nov 5, 2018

bergmeister Dec 19, 2018

rjmholt commented Nov 5, 2018

JamesWTruher left a comment

bergmeister commented Dec 19, 2018 •

edited

Loading

Simplify build scripts even more and upgrade platyPS in Appveyor #1088

Simplify build scripts even more and upgrade platyPS in Appveyor #1088

Conversation

bergmeister commented Oct 23, 2018 • edited Loading

PR Summary

PR Checklist

rjmholt left a comment

Choose a reason for hiding this comment

rjmholt Nov 5, 2018 • edited Loading

Choose a reason for hiding this comment

rjmholt Dec 6, 2018

Choose a reason for hiding this comment

bergmeister Dec 19, 2018 • edited Loading

Choose a reason for hiding this comment

rjmholt Nov 5, 2018

Choose a reason for hiding this comment

rjmholt Nov 5, 2018

Choose a reason for hiding this comment

bergmeister Dec 19, 2018

Choose a reason for hiding this comment

rjmholt Nov 5, 2018

Choose a reason for hiding this comment

bergmeister Dec 19, 2018 • edited Loading

Choose a reason for hiding this comment

rjmholt Nov 5, 2018

Choose a reason for hiding this comment

bergmeister Dec 19, 2018

Choose a reason for hiding this comment

rjmholt commented Nov 5, 2018

JamesWTruher left a comment

Choose a reason for hiding this comment

bergmeister commented Dec 19, 2018 • edited Loading

bergmeister commented Oct 23, 2018 •

edited

Loading

rjmholt Nov 5, 2018 •

edited

Loading

bergmeister Dec 19, 2018 •

edited

Loading

bergmeister Dec 19, 2018 •

edited

Loading

bergmeister commented Dec 19, 2018 •

edited

Loading