Skip to content
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 zstd for extraction #4137

Closed
wants to merge 13 commits into from
Closed

Support zstd for extraction #4137

wants to merge 13 commits into from

Conversation

wstein
Copy link

@wstein wstein commented Oct 13, 2020

I have defined a new 7zip-zstd helper, which is used in case of *.zst compressed files. 7zip-zstd will be automatically installed, if necessary.

I have also added a new testcase for .tar.zst file.

This patch is needed for PR ScoopInstaller/Main#1494 to fix ScoopInstaller/Main#1492

Closes #3990

@@ -18,6 +18,11 @@ if (!(Test-HelperInstalled -Helper 7zip)) {
$issues++
}

if (!(Test-HelperInstalled -Helper 7zip-zstd)) {
error "'7zip-zstd' is not installed! It's required for unpacking most programs. Please Run 'scoop install 7zip-zstd'."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is lie. It is definitely not needed for "most programs"

@@ -104,6 +127,68 @@ function Expand-7zipArchive {
}
}

function Expand-7zip-zstdArchive {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Why did you duplicate whole Expand-7zipArchive function?
    • Just add -Zstd parameter to Expand-7zipArchive.
  • Expand-7zip-zstdArchive is horrible name. Never use hyphens in powershell noun part of command.

@@ -20,6 +20,29 @@ function Test-7zipRequirement {
}
}

function Test-7zip-zstdRequirement {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not use hyphen in name

Copy link
Contributor

@Ash258 Ash258 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

7zip-zstd is not in main bucket. It is nonsense to make "critical" dependency to versions bucket, which is not added always and should not be added always.

Also there is no reason to believe that zstd fork of 7zip will be synced with master and always switching to it is not acceptable

Rework this to handle extraction with clean zstd

@wstein
Copy link
Author

wstein commented Oct 13, 2020

Thank for your review! I am changing the implementation.

@wstein
Copy link
Author

wstein commented Oct 13, 2020

I have finished rewritten the code.

@@ -16,7 +16,25 @@ function Test-7zipRequirement {
return ($URL | Where-Object { Test-7zipRequirement -File $_ }).Count -gt 0
}
} else {
return $File -match '\.((gz)|(tar)|(tgz)|(lzma)|(bz)|(bz2)|(7z)|(rar)|(iso)|(xz)|(lzh)|(nupkg))$'
return $File -match '\.((gz)|(tar)|(tgz)|(lzma)|(bz)|(bz2)|(7z)|(rar)|(iso)|(xz)|(lzh)|(nupkg)|(tar.zst))$'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you handle this in 7zip when it is not supported?

In general do not touch anything related to 7zip extraction

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

zstd can not handle tar zrchives. should I add tar as dependency?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So your current flow is this:

  1. Install manifest which has tar.zst as extension
  2. Add ONLY clean 7zip as dependency as it comfort Test-7zipRequirement
  3. Call Expand-ZipArchive (which is not aware that zstd dependency is missing in the most cases)
  4. Throw error to users' face that he is trying to bind null as parameter to Get-HelperPath as he is missing dependency to zstd.
  5. Abort

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me it looks not bad:

>scoop install openssh
Installing 'zstd' (1.4.5) [64bit]
Loading zstd-v1.4.5-win64.zip from cache
Checking hash of zstd-v1.4.5-win64.zip ... ok.
Extracting zstd-v1.4.5-win64.zip ... done.
Linking D:\scoop\apps\zstd\current => D:\scoop\apps\zstd\1.4.5
Creating shim for 'zstd'.
'zstd' (1.4.5) was installed successfully!
Installing 'openssh' (8.3p1-1) [64bit]
Loading bash-4.4.023-2-x86_64.pkg.tar.xz from cache
Checking hash of bash-4.4.023-2-x86_64.pkg.tar.xz ... ok.
Loading gcc-libs-9.3.0-1-x86_64.pkg.tar.xz from cache
Checking hash of gcc-libs-9.3.0-1-x86_64.pkg.tar.xz ... ok.
Loading heimdal-7.7.0-2-x86_64.pkg.tar.zst from cache
Checking hash of heimdal-7.7.0-2-x86_64.pkg.tar.zst ... ok.
Loading heimdal-libs-7.7.0-2-x86_64.pkg.tar.zst from cache
Checking hash of heimdal-libs-7.7.0-2-x86_64.pkg.tar.zst ... ok.
Loading libcrypt-2.1-2-x86_64.pkg.tar.xz from cache
Checking hash of libcrypt-2.1-2-x86_64.pkg.tar.xz ... ok.
Loading libdb-5.3.28-3-x86_64.pkg.tar.zst from cache
Checking hash of libdb-5.3.28-3-x86_64.pkg.tar.zst ... ok.
Loading libedit-20191231_3.1-1-x86_64.pkg.tar.xz from cache
Checking hash of libedit-20191231_3.1-1-x86_64.pkg.tar.xz ... ok.
Loading libopenssl-1.1.1.g-3-x86_64.pkg.tar.zst from cache
Checking hash of libopenssl-1.1.1.g-3-x86_64.pkg.tar.zst ... ok.
Loading libreadline-8.0.004-1-x86_64.pkg.tar.xz from cache
Checking hash of libreadline-8.0.004-1-x86_64.pkg.tar.xz ... ok.
Loading libsqlite-3.33.0-2-x86_64.pkg.tar.zst from cache
Checking hash of libsqlite-3.33.0-2-x86_64.pkg.tar.zst ... ok.
Loading msys2-runtime-3.1.7-2-x86_64.pkg.tar.xz from cache
Checking hash of msys2-runtime-3.1.7-2-x86_64.pkg.tar.xz ... ok.
Loading ncurses-6.2-1-x86_64.pkg.tar.xz from cache
Checking hash of ncurses-6.2-1-x86_64.pkg.tar.xz ... ok.
Loading openssh-8.3p1-1-x86_64.pkg.tar.zst from cache
Checking hash of openssh-8.3p1-1-x86_64.pkg.tar.zst ... ok.
Loading openssl-1.1.1.g-3-x86_64.pkg.tar.zst from cache
Checking hash of openssl-1.1.1.g-3-x86_64.pkg.tar.zst ... ok.
Loading tcl-8.6.10-1-x86_64.pkg.tar.xz from cache
Checking hash of tcl-8.6.10-1-x86_64.pkg.tar.xz ... ok.
Loading zlib-1.2.11-1-x86_64.pkg.tar.xz from cache
Checking hash of zlib-1.2.11-1-x86_64.pkg.tar.xz ... ok.
Extracting bash-4.4.023-2-x86_64.pkg.tar.xz ... done.
Extracting gcc-libs-9.3.0-1-x86_64.pkg.tar.xz ... done.
Extracting heimdal-7.7.0-2-x86_64.pkg.tar.zst ... done.
Extracting heimdal-libs-7.7.0-2-x86_64.pkg.tar.zst ... done.
Extracting libcrypt-2.1-2-x86_64.pkg.tar.xz ... done.
Extracting libdb-5.3.28-3-x86_64.pkg.tar.zst ... done.
Extracting libedit-20191231_3.1-1-x86_64.pkg.tar.xz ... done.
Extracting libopenssl-1.1.1.g-3-x86_64.pkg.tar.zst ... done.
Extracting libreadline-8.0.004-1-x86_64.pkg.tar.xz ... done.
Extracting libsqlite-3.33.0-2-x86_64.pkg.tar.zst ... done.
Extracting msys2-runtime-3.1.7-2-x86_64.pkg.tar.xz ... done.
Extracting ncurses-6.2-1-x86_64.pkg.tar.xz ... done.
Extracting openssh-8.3p1-1-x86_64.pkg.tar.zst ... done.
Extracting openssl-1.1.1.g-3-x86_64.pkg.tar.zst ... done.
Extracting tcl-8.6.10-1-x86_64.pkg.tar.xz ... done.
Extracting zlib-1.2.11-1-x86_64.pkg.tar.xz ... done.
Running pre-install script...


    Directory: D:\scoop\apps\openssh\8.3p1-1


Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
d-----        10/14/2020   5:31 PM                tmp
d-----        10/14/2020   5:31 PM                home


    Directory: D:\scoop\apps\openssh\8.3p1-1\home


Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
d-----        10/14/2020   5:31 PM                Werner


    Directory: C:\Users\Werner


Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
d-----         10/3/2020  10:25 PM                .ssh
Junction created for D:\scoop\apps\openssh\8.3p1-1\home\Werner\.ssh <<===>> C:\Users\Werner\.ssh
Linking D:\scoop\apps\openssh\current => D:\scoop\apps\openssh\8.3p1-1
Creating shim for 'findssl.sh'.
Creating shim for 'scp'.
Creating shim for 'sftp'.
Creating shim for 'ssh'.
Creating shim for 'ssh-add'.
Creating shim for 'ssh-agent'.
Creating shim for 'ssh-copy-id'.
Creating shim for 'ssh-keygen'.
Creating shim for 'ssh-keyscan'.
Creating shim for 'sshd'.
'openssh' (8.3p1-1) was installed successfully!

lib/depends.ps1 Outdated
@@ -61,6 +61,9 @@ function script_deps($script) {
if($script -like '*Expand-7zipArchive *' -or $script -like '*extract_7zip *') {
$deps += '7zip'
}
if($script -like '*Expand-ZstdArchive *' -or $script -like '*extract_zstd *') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no function extract_zstd and no reason to add it

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

}
switch ($Overwrite) {
"All" { $ArgList += "-f" }
"Skip" { $ArgList += "-q" }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-q Only suppress warnings. Nothing about skip overwriting files.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I will remove this option

"Skip" { $ArgList += "-q" }
}

$Status = Invoke-ExternalCommand $ZstdPath $ArgList -LogPath $LogPath
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use try catch and abort in case of [System.Management.Automation.ParameterBindingException]

https://github.com/Ash258/Scoop-Core/blob/96f1407439d2199a244d67b7a02cdf6d7cb36cf6/lib/decompress.ps1#L89-L93

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, will do that

abort "Failed to extract files from $Path.`nLog file:`n $(friendly_path $LogPath)`n$(new_issue_msg $app $bucket 'decompress error')"
}

if (Test-Path $LogPath) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inline whole if

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let me check

lib/install.ps1 Outdated
@@ -555,6 +555,8 @@ function dl_urls($app, $version, $manifest, $bucket, $architecture, $dir, $use_c
}
} elseif(Test-7zipRequirement -File $fname) { # 7zip
$extract_fn = 'Expand-7zipArchive'
} elseif($fname -match '\.zst$') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you create function when you do not use it then?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you create function when you do not use it then?

it is used inside function install_deps($manifest, $arch) {

Should I remove the the function? What should I implement inside install_deps.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are you talking about?

You have function Test-ZstdRequirement exactly for this reason

@@ -68,6 +69,13 @@ Describe 'Decompression function' -Tag 'Scoop', 'Decompress' {
(Get-ChildItem $to).Count | Should -Be 1
}

It "extract nested compressed file with different inner name" -Skip:$isUnix {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test does not make sense for this PR

@wstein
Copy link
Author

wstein commented Oct 14, 2020

Thanks again for reviewing. I think I got all comments addressed.

Ash258 added a commit to Ash258/Scoop-Core that referenced this pull request Oct 26, 2020
TODO: Vanilla 7zip cannot handle the tar.zst

- ScoopInstaller#4137
@sebma
Copy link

sebma commented Dec 7, 2020

@wstein At your comment : discussion_r504775387, what did you do for scoop to point to openssh 8.3p1-1, mine is pointing to openssh 8.2p1-1 (which btw, fails to install ) ?

@wstein
Copy link
Author

wstein commented Dec 8, 2020

@wstein At your comment : discussion_r504775387, what did you do for scoop to point to openssh 8.3p1-1, mine is pointing to openssh 8.2p1-1 (which btw, fails to install ) ?

@sebma Sorry for the late response, you also need to patch the main bucket: ScoopInstaller/Main@b249cd4

@neal2018
Copy link

neal2018 commented May 3, 2021

Any progress on this? I believe we need zst support for gcc and many other programs

@niheaven
Copy link
Member

Thanks for your job! Zstandard support has been added in #4372.

@niheaven niheaven closed this Nov 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

openssh fails to install, heimdall fails to download Support zstd for extraction
5 participants