-
Notifications
You must be signed in to change notification settings - Fork 7.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable install-powershell.ps1 to work from powershell-daily folder #5429
Conversation
enable running from installed daily by using temp folder
…ng files as they are in use and copy over the new ones
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still reviewing.
tools/install-powershell.ps1
Outdated
@@ -50,16 +55,31 @@ if (-not $Destination) { | |||
$Destination = "${Destination}-daily" | |||
} | |||
} | |||
Write-Verbose "Destination: $Destination" -Verbose |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will update
tools/install-powershell.ps1
Outdated
@@ -121,15 +148,19 @@ try { | |||
$packagePath = Join-Path -Path $tempDir -ChildPath $packageName | |||
Invoke-WebRequest -Uri $downloadURL -OutFile $packagePath | |||
|
|||
New-Item -ItemType Directory -Path "$Destination.new" > $null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will update
tools/install-powershell.ps1
Outdated
} | ||
|
||
Remove-Destination $Destination | ||
Move-Item -Path "$Destination.new" -Destination $Destination |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When installing a release package here, the Destination
could also be $PSHOME
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's true. Will fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tried install-powershell.ps1 -Destination $PSHOME
on Windows, and new files are not moved to $PSHOME after the run.
if (Test-Path -Path "$Destination.old") { | ||
Remove-Item "$Destination.old" -Recurse -Force | ||
} | ||
if ($IsWinEnv -and ($Destination -eq $PSHome)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
tools/install-powershell.ps1
Outdated
} | ||
|
||
Remove-Destination $Destination | ||
Move-Item -Path "$Destination.new" -Destination $Destination |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tried install-powershell.ps1 -Destination $PSHOME
on Windows, and new files are not moved to $PSHOME after the run.
tools/install-powershell.ps1
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't specify -ErrorAction Ignore
, if it fails to copy a file then the installation fails. And maybe add -Force
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The failure is due to it copying folders, I can change this to filter to just files and letting it fail
tools/install-powershell.ps1
Outdated
|
||
$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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move-Item is just updating a single directory pointer so shouldn't take any time
restarted macOS ci |
Still testing, don't merge yet |
Working for me. I think it's ready to go. |
To make it easier to selfhost daily:
This doesn't have any negative impact as the $Destination parameter will have default value if null or empty.