-
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
Support creating tarball package for Linux and macOS #5085
Conversation
{ | ||
Copy-Item -Path $appImageFile.FullName -Destination $destination -force | ||
} | ||
"AppImage" { $extraPackages += @(Get-ChildItem -Path $location -Filter '*.AppImage') } |
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.
Instead of get and then copy, can't we just copy?
Copy-Item -Path "$location/*.AppImage", "$location/*.tar.gz" -Destination $destination -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.
Or even this:
Copy-Item -Path "$location/powershell*/*.deb","$location/powershell*/*.rpm","$location/*.AppImage", "$location/*.tar.gz" -Destination $destination -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.
Please make sure the code logs all copies. Remember this is copying between machines and we need to be able to diagnose where the failure occurred if a file doesn't show up somewhere.
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.
@TravisEz13 Is Write-Verbose -verbose
sufficient for the logging purpose?
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.
Thanks @adityapatwardhan, your comment is addressed.
@TravisEz13 I added Write-Verbose -Verbose
for each file to be copied.
|
||
if (Test-Path -Path $packagePath) { | ||
if ($Force -or $PSCmdlet.ShouldProcess("Overwrite existing package file")) { | ||
Write-Verbose "Overwrite existing package file at $packagePath" -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 guess we can remove -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.
Here I put -Verbose
explicitly to notify that the existing package is going to be removed.
} | ||
|
||
if($AppImage.IsPresent) | ||
$linuxPackages = Get-ChildItem "$location/powershell*" -Include *.deb,*.rpm,*.AppImage,*.tar.gz |
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.
In the previous commit, *.deb and .rpm packages are found under $location/powershell while *.AppImage and *.tar.gz are found under $location. With this change *.AppImage and *.tar.gz package won't be found.
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.
It's not under $location/powershell
, but $location/powershell*
(there is a wildcard * here). All packages start with powershell
.
@adityapatwardhan Your comments are all addressed/replied. Can you please take another look? |
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.
LGTM
Related to #5027
build.json
to build and publish tarball package.deb-arm
type inStart-PSPackage
for now because the underlyingNew-UnixPackage
doesn't support it.Documentation needs to be added about the dependencies of PowerShell Core, so the tarball package can be useful to users. This task is tracked in #3961