Update, simplify, and consolidate build scripts #1082

JamesWTruher · 2018-10-17T21:57:28Z

PR Summary

In order to actually build script analyzer, you have to run a couple of different scripts if you're going to also build documentation. This PR consolidates everything into a single script build.ps1 and a module build.psm1 which implements the actions. This is targeting the developer scenario rather than CI usage.

The documentation is updated to provide examples of how to use the script.
Additionally, I'm updating the sdk version to 2.1.400 to match PowerShell Core, so you can build both PowerShell Core and Script Analyzer with the same SDK.

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.

Require any 2.1 sdk for building, this way we can build with the same sdk as PowerShell core

To build for core, simply type ./build, to build all flavors and documentation type ./build -all. You can now run tests as well via ./build -Test.

because it's really about the version of PS, not the analyzer

bergmeister

By looking at the code it looks good overall, with one small mistake. Some odd (non-standard and inconsistent) whitespace at the beginning and/or end of a parenthesis.
I will try running and using this locally the next days and then approve.

bergmeister · 2018-10-17T22:36:13Z

tools/docker/ubuntu16/Dockerfile

+
+RUN apt update -qq && \
+    cd / && \
+    git clone https://github.com/JamesWTruher/PSScriptAnalyzer


Shouldn't this be?

Suggested change

git clone https://github.com/JamesWTruher/PSScriptAnalyzer

git clone https://github.com/PowerShell/PSScriptAnalyzer

indeed it should :/

rjmholt

LGTM

rjmholt · 2018-10-17T23:26:10Z

README.md

@@ -123,25 +123,33 @@ Note: the PSScriptAnalyzer Chocolatey package is provided and supported by the c
 * Building

    You can either build using the `Visual Studio` solution `PSScriptAnalyzer.sln` or build using `PowerShell` specifically for your platform as follows:
-    * Windows PowerShell version 5.0 and greater
+    * The default build is for PowerShell Core


Just for the sake of argument, should the default build be for the current platform the script is being run on? That's possibly an order of magnitude harder to accomplish, but thought I would bring it up.

+1 for that but I see it as an optional nice to have that could come after this PR

:) why would anyone be using anything other than core? :)

rjmholt · 2018-10-17T23:28:05Z

build.ps1

-            $testScriptFilePath = CreateTestRunnerScript $testPath
-            Start-Process powershell -ArgumentList "-NoExit","-File $testScriptFilePath" -Verb runas
-            # clean up the test file
+    $setName = $PSCmdlet.ParameterSetName


Since this is used above, maybe it's worth assigning above, below the Import-Module line?

rjmholt · 2018-10-17T23:30:40Z

build.ps1

+            Build-ScriptAnalyzer -Documentation
+        }
+        "BuildOne" {
+            $buildArgs = @{


Side note: I find I do this a lot in PowerShell as well (pass arguments down through commands). Perhaps it's an antipattern (Law of Demeter violation type of thing), but it would be nice to introduce a simpler way for commands to "inherit" all the common parameters between it and its caller (which haven't been overridden).

i often create a hash earlier in the script and then modify it based on conditions.

rjmholt · 2018-10-17T23:35:17Z

build.psm1

+        [string]$Framework = "core",
+
+        [Parameter(ParameterSetName="BuildOne")]
+        [ValidateSet("3","4","5")]


Should "6" be an option here? I feel like equating 5 with Core might prove confusing. We should probably have a comment to document it at least.

Yes, and we should then remove the $Framework parameter and handle that internally since we know which ps version is core and which is full

JamesWTruher

it looks like i've broken the CI - I'm working on a fix, but it's not ready yet

JamesWTruher · 2018-10-17T23:12:20Z

tools/docker/ubuntu16/Dockerfile

+
+RUN apt update -qq && \
+    cd / && \
+    git clone https://github.com/JamesWTruher/PSScriptAnalyzer


indeed it should :/

Add additional verbose statements for debugging

When simplifying to use only psversion, add back the 'PSV' prolog to configuration

This reverts commit b3018ea.

JamesWTruher · 2018-10-19T22:50:08Z

there's a couple of good suggestions here which I would like to put in a follow-on PR

JamesWTruher · 2018-10-19T22:54:10Z

@bergmeister - CI problems addressed

bergmeister

I added some minor comments to be addressed please. What would be good to know is why the pragmas (psv4) needed to be adapted (why was it working before?)
Otherwise it looks good, let;s get it merged after that :-)

bergmeister · 2018-10-21T21:31:16Z

build.psm1

+    $docsPath = Join-Path $projectRoot docs
+    $markdownDocsPath = Join-Path $docsPath markdown
+    $outputDocsPath = Join-Path $destinationDir en-US
+    $requiredVersionOfplatyPS = 0.9


We should update platyPs to 0.12.0 but this can be another PR after that
https://twitter.com/xvorsx/status/1053495983588265991?s=19

bergmeister · 2018-10-22T07:09:06Z

build.psm1

+    $outputDocsPath = Join-Path $destinationDir en-US
+    $requiredVersionOfplatyPS = 0.9
+    #$modInfo = new-object Microsoft.PowerShell.Commands.ModuleSpecification -ArgumentList @{ ModuleName = "platyps"; ModuleVersion = $requiredVersionOfplatyPS}
+    #if ( $null -eq (Get-Module -ListAvailable -FullyQualifiedName $modInfo))


please remove commented out code

bergmeister · 2018-10-22T07:10:15Z

build.psm1

+    {
+        Write-Verbose -verbose "platyPS not found, installing"
+        Install-Module -Force -Name platyPS -Scope CurrentUser
+        # throw "Cannot find required minimum version $requiredVersionOfplatyPS of platyPS. Install via 'Install-Module platyPS'"


please remove commented out code.

bergmeister · 2018-10-22T07:39:32Z

build.psm1

+        try {
+            $savedModulePath = $env:PSModulePath
+            $env:PSModulePath = "${testModulePath}{0}${env:PSModulePath}" -f [System.IO.Path]::PathSeparator
+            $scriptBlock = [scriptblock]::Create("Invoke-Pester -Path $testScripts -OutputFormat NUnitXml -OutputFile $testResultsFile -Show Describe")


I'd be OK with showing only the Describe output but then Pester does not show the summary any more, which we definitely need if we want to avoid scrolling through all test results when running locally. Can we therefore use the Describe,Summary option please

bergmeister

Some comments to be address please but looks good overall. Can you please comment on why the conditional compilation had to change for WMF4 (why was it working before but not now any more?)?

bergmeister · 2018-10-22T10:14:40Z

build.psm1

+    $outputDocsPath = Join-Path $destinationDir en-US
+    $requiredVersionOfplatyPS = 0.9
+    #$modInfo = new-object Microsoft.PowerShell.Commands.ModuleSpecification -ArgumentList @{ ModuleName = "platyps"; ModuleVersion = $requiredVersionOfplatyPS}
+    #if ( $null -eq (Get-Module -ListAvailable -FullyQualifiedName $modInfo))


please remove commented out line

bergmeister · 2018-10-22T10:15:00Z

build.psm1

+    $outputDocsPath = Join-Path $destinationDir en-US
+    $requiredVersionOfplatyPS = 0.9
+    #$modInfo = new-object Microsoft.PowerShell.Commands.ModuleSpecification -ArgumentList @{ ModuleName = "platyps"; ModuleVersion = $requiredVersionOfplatyPS}
+    #if ( $null -eq (Get-Module -ListAvailable -FullyQualifiedName $modInfo))


please remove commented out line

bergmeister · 2018-10-22T10:19:33Z

build.psm1

+    {
+        Write-Verbose -verbose "platyPS not found, installing"
+        Install-Module -Force -Name platyPS -Scope CurrentUser
+        # throw "Cannot find required minimum version $requiredVersionOfplatyPS of platyPS. Install via 'Install-Module platyPS'"


please remove commented out line

bergmeister · 2018-10-22T10:20:49Z

build.psm1

+        try {
+            $savedModulePath = $env:PSModulePath
+            $env:PSModulePath = "${testModulePath}{0}${env:PSModulePath}" -f [System.IO.Path]::PathSeparator
+            $scriptBlock = [scriptblock]::Create("Invoke-Pester -Path $testScripts -OutputFormat NUnitXml -OutputFile $testResultsFile -Show Describe")


Adding the Summary option to -Show would be useful

Suggested change

$scriptBlock = [scriptblock]::Create("Invoke-Pester -Path $testScripts -OutputFormat NUnitXml -OutputFile $testResultsFile -Show Describe")

$scriptBlock = [scriptblock]::Create("Invoke-Pester -Path $testScripts -OutputFormat NUnitXml -OutputFile $testResultsFile -Show Describe,Summary")

bergmeister · 2018-10-22T10:21:59Z

build.psm1

+        try {
+            $savedModulePath = $env:PSModulePath
+            $env:PSModulePath = "${testModulePath}{0}${env:PSModulePath}" -f [System.IO.Path]::PathSeparator
+            $scriptBlock = [scriptblock]::Create("Invoke-Pester -Path $testScripts -OutputFormat NUnitXml -OutputFile $testResultsFile -Show Describe")


Using the Describe,Summary option to -Show would be useful

Suggested change

$scriptBlock = [scriptblock]::Create("Invoke-Pester -Path $testScripts -OutputFormat NUnitXml -OutputFile $testResultsFile -Show Describe")

$scriptBlock = [scriptblock]::Create("Invoke-Pester -Path $testScripts -OutputFormat NUnitXml -OutputFile $testResultsFile -Show Describe,Summary")

bergmeister · 2018-10-22T10:22:24Z

build.psm1

+        try {
+            $savedModulePath = $env:PSModulePath
+            $env:PSModulePath = "${testModulePath}{0}${env:PSModulePath}" -f [System.IO.Path]::PathSeparator
+            $scriptBlock = [scriptblock]::Create("Invoke-Pester -Path $testScripts -OutputFormat NUnitXml -OutputFile $testResultsFile -Show Describe")


Using the Describe,Summary option to -Show would be useful

Suggested change

$scriptBlock = [scriptblock]::Create("Invoke-Pester -Path $testScripts -OutputFormat NUnitXml -OutputFile $testResultsFile -Show Describe")

$scriptBlock = [scriptblock]::Create("Invoke-Pester -Path $testScripts -OutputFormat NUnitXml -OutputFile $testResultsFile -Show Describe,Summary")

bergmeister

Some comments to be addressed please but looks good overall, GitHub had a glitch, therefore some comments are duplicated. Can you please comment on why the conditional compilation had to change for WMF4 (why was it working before but not now any more?)? After this PR we also need to update ReleaseMaker.psm1

…nalyzer into buildscripts use HEAD due to newer sdk version # Conflicts: # global.json

bergmeister · 2018-10-23T21:01:55Z

This is good enough to merge, we plan to have follow up PRs that tweak this and also adapt ReleaseMaker.psm1 for that. It's better to merge early to avoid conflicts

* consolidate build of script analyzer into module Require any 2.1 sdk for building, this way we can build with the same sdk as PowerShell core * Have git ignore vim swap files and the test results file * Only special case linux for the customized rule tests * Update to build scripts. To build for core, simply type ./build, to build all flavors and documentation type ./build -all. You can now run tests as well via ./build -Test. * Modify readme to match new build instructions * Add ubuntu16 docker file to build scripts * change to use psversion rather than analyzerversion because it's really about the version of PS, not the analyzer * Add copyright banners to script and module * Fix repo to point to the official repo * Update appveyor scripts to use new build script * Forcing installation of platyPS if not found when building documentation * Be sure to skip uses of TypeDefinitionAst on both PSV3 and PSV4 * Modified logic to not build full on non-Windows Add additional verbose statements for debugging * fix issue with incorrect build configuration When simplifying to use only psversion, add back the 'PSV' prolog to configuration * reduce build matrix to accellerate CI testing * Revert "reduce build matrix to accellerate CI testing" This reverts commit b3018ea.

JamesWTruher added 7 commits October 16, 2018 14:57

consolidate build of script analyzer into module

cab2708

Require any 2.1 sdk for building, this way we can build with the same sdk as PowerShell core

Have git ignore vim swap files and the test results file

8977733

Only special case linux for the customized rule tests

920a8ab

Update to build scripts.

7a3f1d2

To build for core, simply type ./build, to build all flavors and documentation type ./build -all. You can now run tests as well via ./build -Test.

Modify readme to match new build instructions

90e7957

Add ubuntu16 docker file to build scripts

3064367

change to use psversion rather than analyzerversion

f46abf3

because it's really about the version of PS, not the analyzer

JamesWTruher requested a review from bergmeister October 17, 2018 21:57

Add copyright banners to script and module

1b8386a

bergmeister reviewed Oct 17, 2018

View reviewed changes

Fix repo to point to the official repo

2978f78

rjmholt approved these changes Oct 17, 2018

View reviewed changes

JamesWTruher changed the title ~~update, simplify, and consolidate build scripts~~ WIP: update, simplify, and consolidate build scripts Oct 17, 2018

JamesWTruher commented Oct 17, 2018

View reviewed changes

JamesWTruher force-pushed the buildscripts branch 3 times, most recently from 75891d8 to 67ceb40 Compare October 18, 2018 19:58

Update appveyor scripts to use new build script

5c01978

JamesWTruher force-pushed the buildscripts branch from 67ceb40 to 5c01978 Compare October 18, 2018 21:26

JamesWTruher added 2 commits October 18, 2018 15:58

Forcing installation of platyPS if not found when building documentation

9ef4a9d

Be sure to skip uses of TypeDefinitionAst on both PSV3 and PSV4

e9c8175

JamesWTruher force-pushed the buildscripts branch 2 times, most recently from fcd5f2d to 14e0fdc Compare October 18, 2018 23:42

Modified logic to not build full on non-Windows

118e0b2

Add additional verbose statements for debugging

JamesWTruher force-pushed the buildscripts branch from 14e0fdc to 118e0b2 Compare October 19, 2018 00:03

JamesWTruher added 3 commits October 19, 2018 14:06

fix issue with incorrect build configuration

9a20e22

When simplifying to use only psversion, add back the 'PSV' prolog to configuration

reduce build matrix to accellerate CI testing

b3018ea

Revert "reduce build matrix to accellerate CI testing"

cb95da1

This reverts commit b3018ea.

JamesWTruher changed the title ~~WIP: update, simplify, and consolidate build scripts~~ Update, simplify, and consolidate build scripts Oct 19, 2018

bergmeister approved these changes Oct 22, 2018

View reviewed changes

wilmardo mentioned this pull request Oct 22, 2018

Remove pwsh 6.0.2 installation code during AppVeyor build boostrap that is not needed any more #1084

Merged

6 tasks

bergmeister mentioned this pull request Oct 22, 2018

Update SDK version in global.json #1085

Closed

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

a6a0070

…nalyzer into buildscripts use HEAD due to newer sdk version # Conflicts: # global.json

bergmeister merged commit 1eb9a01 into PowerShell:development Oct 23, 2018

bergmeister mentioned this pull request Oct 23, 2018

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

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update, simplify, and consolidate build scripts #1082

Update, simplify, and consolidate build scripts #1082

JamesWTruher commented Oct 17, 2018 •

edited

Loading

bergmeister left a comment

bergmeister Oct 17, 2018

JamesWTruher Oct 17, 2018

rjmholt left a comment

rjmholt Oct 17, 2018

bergmeister Oct 18, 2018 •

edited

Loading

JamesWTruher Oct 18, 2018

rjmholt Oct 17, 2018

rjmholt Oct 17, 2018

JamesWTruher Oct 18, 2018

rjmholt Oct 17, 2018

bergmeister Oct 18, 2018

JamesWTruher left a comment

JamesWTruher Oct 17, 2018

JamesWTruher commented Oct 19, 2018

JamesWTruher commented Oct 19, 2018

bergmeister left a comment •

edited

Loading

bergmeister Oct 21, 2018

bergmeister Oct 22, 2018

bergmeister Oct 22, 2018

bergmeister Oct 22, 2018

bergmeister left a comment

bergmeister Oct 22, 2018

bergmeister Oct 22, 2018

bergmeister Oct 22, 2018

bergmeister Oct 22, 2018

bergmeister Oct 22, 2018

bergmeister Oct 22, 2018

bergmeister left a comment •

edited

Loading

bergmeister commented Oct 23, 2018

	git clone https://github.com/JamesWTruher/PSScriptAnalyzer
	git clone https://github.com/PowerShell/PSScriptAnalyzer

	$scriptBlock = [scriptblock]::Create("Invoke-Pester -Path $testScripts -OutputFormat NUnitXml -OutputFile $testResultsFile -Show Describe")
	$scriptBlock = [scriptblock]::Create("Invoke-Pester -Path $testScripts -OutputFormat NUnitXml -OutputFile $testResultsFile -Show Describe,Summary")

Update, simplify, and consolidate build scripts #1082

Update, simplify, and consolidate build scripts #1082

Conversation

JamesWTruher commented Oct 17, 2018 • edited Loading

PR Summary

PR Checklist

bergmeister left a 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

Choose a reason for hiding this comment

bergmeister Oct 18, 2018 • 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

JamesWTruher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesWTruher commented Oct 19, 2018

JamesWTruher commented Oct 19, 2018

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

bergmeister left a 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

bergmeister left a comment • edited Loading

Choose a reason for hiding this comment

bergmeister commented Oct 23, 2018

JamesWTruher commented Oct 17, 2018 •

edited

Loading

bergmeister Oct 18, 2018 •

edited

Loading

bergmeister left a comment •

edited

Loading

bergmeister left a comment •

edited

Loading