Enable install-powershell.ps1 to work from powershell-daily folder #5429

SteveL-MSFT · 2017-11-13T06:45:44Z

To make it easier to selfhost daily:

removed [validate] attribute from parameter so this script can be invoked directly from web.
This doesn't have any negative impact as the $Destination parameter will have default value if null or empty.
moved removal of destination folder later as installing the package requires packagemanagement module and archive module (on Windows)
on Windows, because files have open handles when run from existing powershell-daily install, I rename the existing files and copy over the new ones. User needs to exit and restart pwsh to take effect (similar to macOS/Linux where you have to exit and restart anyways)

…oke-expression

enable running from installed daily by using temp folder

…ng files as they are in use and copy over the new ones

daxian-dbw

Still reviewing.

daxian-dbw · 2017-11-13T16:43:04Z

tools/install-powershell.ps1

        $Destination = "${Destination}-daily"
    }
 }
+Write-Verbose "Destination: $Destination" -Verbose


Should get the unresolved absolute provider path for $Destination. Otherwise, the $Destination -eq $PSHome check below may fail when it should succeed.
You can use

$Destination = $PSCmdlet.SessionState.Path.GetUnresolvedProviderPathFromPSPath($Destination)

Will update

daxian-dbw · 2017-11-13T17:00:22Z

tools/install-powershell.ps1

        $packagePath = Join-Path -Path $tempDir -ChildPath $packageName
        Invoke-WebRequest -Uri $downloadURL -OutFile $packagePath

+        New-Item -ItemType Directory -Path "$Destination.new" > $null


Please use the $tempDir for anything that is temporary, it's cleaner and also prevent the corner case that $Destination.new already exists.
For example

$packageContent = Join-Path -Path $tempDir -ChildPath content New-Item -ItemType Directory -Path $packageContent > $null ... if ($IsWinEnv) { Expand-Archive ... -DestinationPath $packageContent } else { tar zxf ... -C $packageContent }

Will update

daxian-dbw · 2017-11-13T17:02:29Z

tools/install-powershell.ps1

        }
+
+        Remove-Destination $Destination
+        Move-Item -Path "$Destination.new" -Destination $Destination


If you can use Move-Item here, I think you can do the same in the daily code path without having to differentiate between $Destination exists or not (line 127).

The differentiation is needed when $Destination is in use (because install-powershell was run from $Destination) so I can't use the Move-Item optimization.

When installing a release package here, the Destination could also be $PSHOME, right?

Yes, that's true. Will fix.

I just tried install-powershell.ps1 -Destination $PSHOME on Windows, and new files are not moved to $PSHOME after the run.

daxian-dbw · 2017-11-13T18:54:27Z

tools/install-powershell.ps1

+        if (Test-Path -Path "$Destination.old") {
+            Remove-Item "$Destination.old" -Recurse -Force
+        }
+        if ($IsWinEnv -and ($Destination -eq $PSHome)) {


I didn't know that on Linux you can delete $PSHome from a running powershell.
Can you please add some comment in Remove-Destination about the different scenarios of the remove operation?

daxian-dbw · 2017-11-13T19:32:04Z

tools/install-powershell.ps1

        }
+
+        Remove-Destination $Destination
+        Move-Item -Path "$Destination.new" -Destination $Destination


I just tried install-powershell.ps1 -Destination $PSHOME on Windows, and new files are not moved to $PSHOME after the run.

daxian-dbw · 2017-11-13T19:36:22Z

tools/install-powershell.ps1

-        Copy-Item -Path $contentPath\* -Destination $Destination -Recurse -Force
+        if (Test-Path $Destination) {
+            Write-Verbose "Copying files" -Verbose
+            Copy-Item -Recurse -Path "$contentPath\*" -Destination $Destination -ErrorAction Ignore


We shouldn't specify -ErrorAction Ignore, if it fails to copy a file then the installation fails. And maybe add -Force.

The failure is due to it copying folders, I can change this to filter to just files and letting it fail

daxian-dbw · 2017-11-13T19:43:34Z

tools/install-powershell.ps1

        $contentPath = [System.IO.Path]::Combine($tempDir, $packageName, "content")
-        Copy-Item -Path $contentPath\* -Destination $Destination -Recurse -Force
+        if (Test-Path $Destination) {
+            Write-Verbose "Copying files" -Verbose


I think this log message should be moved before if (Test-Path $Destination) since both copy-item and move-item would take time. Maybe change the message to Deploying files ...

Move-Item is just updating a single directory pointer so shouldn't take any time

TravisEz13 · 2017-11-13T22:45:28Z

restarted macOS ci

SteveL-MSFT · 2017-11-14T04:18:55Z

Still testing, don't merge yet

SteveL-MSFT · 2017-11-14T05:08:50Z

Working for me. I think it's ready to go.

SteveL-MSFT added 3 commits November 11, 2017 21:15

remove [validate] attribute so script can be called directly from inv…

e58ec0e

…oke-expression

enable installing daily build using $env:PSCoreDaily

402eb03

enable running from installed daily by using temp folder

if using from same folder as being installed for daily, rename existi…

5c1c8e2

…ng files as they are in use and copy over the new ones

SteveL-MSFT assigned daxian-dbw Nov 13, 2017

SteveL-MSFT requested a review from daxian-dbw November 13, 2017 06:45

SteveL-MSFT requested review from TravisEz13 and adityapatwardhan as code owners November 13, 2017 06:45

remove $env:pscoredaily usage, can use: iex "& { $(irm 'url') } -daily"

391ac68

daxian-dbw reviewed Nov 13, 2017

View reviewed changes

adityapatwardhan mentioned this pull request Nov 13, 2017

Try fixing the 'brew update' issue in bootstrap script #5400

Merged

address PR feedback

117427b

daxian-dbw approved these changes Nov 14, 2017

View reviewed changes

TravisEz13 approved these changes Nov 14, 2017

View reviewed changes

fix copying of files to correct destination folder

c1dc92f

daxian-dbw merged commit 83e1d64 into PowerShell:master Nov 14, 2017

SteveL-MSFT deleted the install-powershell branch November 15, 2017 01:08

Enable install-powershell.ps1 to work from powershell-daily folder #5429

Enable install-powershell.ps1 to work from powershell-daily folder #5429

Uh oh!

Conversation

SteveL-MSFT commented Nov 13, 2017

Uh oh!

daxian-dbw left a comment

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!

daxian-dbw Nov 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 13, 2017

Uh oh!

SteveL-MSFT commented Nov 14, 2017

Uh oh!

SteveL-MSFT commented Nov 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

daxian-dbw Nov 13, 2017 •

edited

Loading