Allow packaging from a zip package to allow for signing #5418

TravisEz13 · 2017-11-11T01:00:13Z

Include a serialized version of PSOptions in an includesymbols zip
Add a function which will create a zip package from the expanded includesymbols zip and a folder of signed files
add a function to restore an includesymbols zip as a build and populated PSOptions with the options
update related files

add ability to set the PSOptions

TravisEz13 · 2017-11-11T20:32:05Z

rebased due to merge conflict

daxian-dbw · 2017-11-11T19:38:52Z

build.psm1

@@ -742,6 +753,13 @@ function Get-PSOptions {
    return $script:Options
 }

+function Set-PSOptions {
+    param(
+        $Options


So the argument passed in could be a PSObject, right?

daxian-dbw · 2017-11-11T19:56:45Z

tools/packaging/packaging.psm1

@@ -180,6 +180,7 @@ function Start-PSPackage {
                # Zip symbols.zip to the root package
                $zipSource = Join-Path $symbolsSource -ChildPath '*'
                $zipPath = Join-Path -Path $Source -ChildPath 'symbols.zip'
+                $Script:Options | ConvertTo-Json -Depth 3 | Out-File -Encoding utf8 -FilePath (Join-Path -Path $source -ChildPath 'psoptions.json')


Be careful about the entries PSModuleRestore and CrossGen when convert to/from json. Currnetly the values of these 2 are SwitchParameter values, which works fined with the comparison to bool. However, after convert from json, you will get a synthetic property named 'IsPresent' with a bool value, and the comparison to bool would behave differently. See the example:

> $p = New-PSOptions > $s= $p | ConvertTo-Json | ConvertFrom-Json > if (-not $p.PSModuleRestore) { echo "ff" } ff > if (-not $s.PSModuleRestore) { echo "ff" } ## print nothing.

I suggest to not use the SwitchParameter value in the first place. Instead, use $CrossGen.IsPresent and $PSModuleRestore.IsPresent when setting the initial $options

updated to suggested implementation

daxian-dbw · 2017-11-12T04:42:27Z

tools/packaging/packaging.psm1

+    $windowsExecutablePath = (Join-Path $buildPath -ChildPath 'pwsh.exe')
+    $linuxExecutablePath = (Join-Path $buildPath -ChildPath 'pwsh')
+
+    Restore-PSModuleToBuild -PublishPath $buildPath


Can you please explain again why we need to restore modules again here? After restoring the modules, they only exist in the publish folder. We do signing and other compliance check using the files from the parent folder of publish so those restored modules won't affect any of that ...

This is the first time modules are being restored. Restoring them during the initial build would add size to the files we are carrying around. It also ensures 100% that we do not sign 3rd party code, which not signing 3rd party code is a requirement.

Restoring them during the initial build would add size to the files we are carrying around.

This makes sense. My worry is that the package management cmdlet may fail when running Restore-PSModuleToBuild when working with myget. I see this failure often enough in our CI runs. Before this change, the build can fail fast if this happens, but after this change, the signing effort would be in vain in case this happens.

But this is not a blocking comment. Let's do this and see how it works.

daxian-dbw · 2017-11-12T04:43:42Z

tools/packaging/packaging.psm1

+    remove-item -Path (Join-Path -Path $buildPath -ChildPath '*.zip') -Recurse
+
+    $windowsExecutablePath = (Join-Path $buildPath -ChildPath 'pwsh.exe')
+    $linuxExecutablePath = (Join-Path $buildPath -ChildPath 'pwsh')


Why checking for Linux? We don't sign individual files for Linux so this function should never be used in a Linux build, right?

daxian-dbw · 2017-11-12T04:45:14Z

tools/releaseBuild/Images/microsoft_powershell_windowsservercore/PowerShellPackage.ps1

    [string] $destination = "$env:WORKSPACE",
-    [ValidateSet("win7-x64", "win81-x64", "win10-x64", "win7-x86")]    
+
+    [ValidateSet("win7-x64", "win81-x64", "win10-x64", "win7-x86")]
    [string]$Runtime = 'win10-x64',


Minor: I guess we should only accept win7-x64 and win7-x86 now.

daxian-dbw · 2017-11-12T04:52:03Z

tools/releaseBuild/build.json

+        "BuildCommand": "C:\\PowerShellPackage.ps1 -BuildZip _RepoDestinationPath_\\_BuildPackageName_ -location _RepoDestinationPath_ -destination _DockerVolume_ -Runtime win7-x64 -ReleaseTag _ReleaseTag_",
+        "BuildDockerOptions": [
+            "-m",
+            "3968m"


Just curious, why do we want to limit the memory for the container?

This is increasing the memory in the container on many machines. I cannot add comments in JSON. but the packaging script verifies that we have increased the memory to at least 2GB.

I'd say a minimum of this value if I could but they don't have this option.

Got it. Thanks!

daxian-dbw · 2017-11-12T04:59:24Z

tools/releaseBuild/vstsbuild.ps1

@@ -1,7 +1,18 @@
+[cmdletbinding(DefaultParameterSetName='default')]


There are no common parameters that can be used for the default set. It seems to me we should use Build as the default parameter set.

daxian-dbw · 2017-11-12T05:07:35Z

tools/releaseBuild/vstsbuild.ps1

@@ -74,7 +108,12 @@ End {
        Import-Module "$location/dockerBasedBuild" -Force
        Clear-VstsTaskState

-        Invoke-Build -RepoPath $resolvedRepoRoot  -BuildJsonPath './tools/releaseBuild/build.json' -Name $Name -Parameters $PSBoundParameters
+        $buildParameters = @{
+            ReleaseTag = $ReleaseTag


Previously, you pass in $PSBoundParameters, so if -ReleaseTag is not specified, it won't be passed to Invoke-Build.
Now you always pass in ReleaseTag, and the value will be $null in case -ReleaseTag is not specified when calling this script. Will this cause any problems?

The new hashtable I have has ReleaseTag

No, this won't cause any issues. I've verified this previously.

TravisEz13 · 2017-11-12T20:02:34Z

started a build at: https://mscodehub.visualstudio.com/PowerShellCore/_build/index?buildId=5708&_a=summary to verify
Passed

TravisEz13 requested review from adityapatwardhan and daxian-dbw as code owners November 11, 2017 01:00

SteveL-MSFT added this to the 6.0.0-RC milestone Nov 11, 2017

TravisEz13 assigned daxian-dbw Nov 11, 2017

TravisEz13 added 7 commits November 11, 2017 12:29

ignore the expanded signed build

7d7321e

Refactor PSModuleRestore so it can be run for a signed build

bba7d86

add ability to set the PSOptions

update to support packaging signed build

0c1e2a3

Add ability to expand signed build for packaging

2bf41fb

add signed build definition

b3df229

update to support building a signed build

bf2340b

export New-PSSignedBuildZip

94ca4c3

TravisEz13 force-pushed the codesigning2 branch from 04ab9c7 to 94ca4c3 Compare November 11, 2017 20:31

daxian-dbw reviewed Nov 12, 2017

View reviewed changes

address PR feedback

63ea065

daxian-dbw approved these changes Nov 13, 2017

View reviewed changes

daxian-dbw merged commit 36fac11 into PowerShell:master Nov 13, 2017

TravisEz13 deleted the codesigning2 branch November 13, 2017 18:25

		@@ -1,7 +1,18 @@
		[cmdletbinding(DefaultParameterSetName='default')]

Allow packaging from a zip package to allow for signing #5418

Allow packaging from a zip package to allow for signing #5418

Uh oh!

Conversation

TravisEz13 commented Nov 11, 2017

Uh oh!

TravisEz13 commented Nov 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TravisEz13 commented Nov 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

TravisEz13 commented Nov 12, 2017 •

edited

Loading