Skip to content

fix (decompress): Expand-7zipArchive only delete temp dir / $extractDir if it is empty#6092

Merged
niheaven merged 14 commits intoScoopInstaller:developfrom
o-l-a-v:fix-6011
Aug 9, 2024
Merged

fix (decompress): Expand-7zipArchive only delete temp dir / $extractDir if it is empty#6092
niheaven merged 14 commits intoScoopInstaller:developfrom
o-l-a-v:fix-6011

Conversation

@o-l-a-v
Copy link
Copy Markdown
Contributor

@o-l-a-v o-l-a-v commented Aug 6, 2024

Description

decompress.ps1 deletes $extractDir even if it's not empty, which can cause applications to not work after installation.

More context and how I got to this solution in following comment:

Motivation and Context

Closes #6011, ScoopInstaller/Extras#13324

How Has This Been

Tested locally with scoop install extras/tor-browser.

Checklist:

  • I have read the Contributing Guide.
  • I have ensured that I am targeting the develop branch.
  • I have updated the documentation accordingly.
  • I have updated the tests accordingly.
  • I have added an entry in the CHANGELOG.

@o-l-a-v
Copy link
Copy Markdown
Contributor Author

o-l-a-v commented Aug 6, 2024

If tests should be updated (tell me): Should be sufficient to add a .7z to https://github.com/ScoopInstaller/Scoop/blob/develop/test/fixtures/decompress/TestCases.zip with:

root
├─ $PLUGINSDIR
└─ keep
   └─ keep
      └─  file.txt

Then check that "$DestinationPath\keep" exists?

@o-l-a-v o-l-a-v changed the title Short term fix to #6011 fix (decompress): Short term fix to #6011 Aug 6, 2024
@niheaven
Copy link
Copy Markdown
Member

niheaven commented Aug 7, 2024

Could you please sign your commit? And the logic should be tweaked by only check if $ExtractDir is empty.

@o-l-a-v
Copy link
Copy Markdown
Contributor Author

o-l-a-v commented Aug 7, 2024

Could you please sign your commit?

I've never done that before, I'll see if I can figure how. 👍

And the logic should be tweaked by only check if $ExtractDir is empty.

Isn't that what I do? If no child items inside it => delete. It's also possible to use Test-Path -Path '<directory>\*', but I like using Get-ChildItem for readability.

I also kept $ExtractDir -replace '[\\/].*. I don't really see why it's neccessary, but I tried doing as little changes as possible.


I now see that the other Expand-*Archive functions suffers from the same bug. Or maybe they don't?

image

Comment thread lib/decompress.ps1 Outdated
@niheaven
Copy link
Copy Markdown
Member

niheaven commented Aug 7, 2024

@niheaven
Copy link
Copy Markdown
Member

niheaven commented Aug 7, 2024

Hmm, you should try to pass the tests now :)

@o-l-a-v
Copy link
Copy Markdown
Contributor Author

o-l-a-v commented Aug 7, 2024

I think I've fixed the test errors now @niheaven: Can't do Get-ChildItem if -Path does not exist.

Comment thread lib/decompress.ps1 Outdated
@niheaven niheaven changed the title fix (decompress): Short term fix to #6011 fix (decompress): Expand-7zipArchive Only delete temp dir / $extractDir if it is empty Aug 9, 2024
@niheaven niheaven changed the title fix (decompress): Expand-7zipArchive Only delete temp dir / $extractDir if it is empty fix (decompress): Expand-7zipArchive only delete temp dir / $extractDir if it is empty Aug 9, 2024
@niheaven niheaven merged commit 7f99c49 into ScoopInstaller:develop Aug 9, 2024
@o-l-a-v o-l-a-v deleted the fix-6011 branch August 24, 2024 15:17
HUMORCE added a commit that referenced this pull request Aug 11, 2025
* fix (decompress): `Expand-7zipArchive` only delete temp dir / `$extractDir` if it is empty (#6092)

Co-authored-by: Hsiao-nan Cheung <niheaven@gmail.com>

* refactor(download): Move download-related functions to 'download.ps1' (#6095)

* fix(download): Fallback to default downloader when aria2 fails (#4292)

* fix(commands): Handling broken aliases (#6141)

* fix(shim): properly check `wslpath`/`cygpath` command first (#6114)

Co-authored-by: Hsiao-nan Cheung <niheaven@gmail.com>

* fix(scoop-bucket): Add missing import for `no_junction` envs (#6181)

Signed-off-by: Chawye Hsu <su+git@chawyehsu.com>

* docs(chglog): Update to 0.5.3 (#6258)

* perf(shim): Update kiennq-shim to v3.1.2 (#6261)

* fix(decompress): Replace deprecated 7ZIPEXTRACT_USE_EXTERNAL config (#6327)

Co-authored-by: Hsiao-nan Cheung <niheaven@gmail.com>

* fix(scoop-uninstall): Fix uninstaller does not gain Global state (#6430)

* global arg

* changelog

* refactor(Get-Manifest): Select actual source for manifest (#6142)

* first step

* Revert "first step"

This reverts commit c5907c3.

* refactor(Get-Manifest): Select actual source for installed manifest

* rework sub-commands, `scoop-depends` is NOT working at this stage

* URI manifest

* opt

* deprecated manifest

* source of manifests

* source of manifest pt2

- Mark URI(path/URL/UNC/etc.) query as standalone manifest
- Drop `installed` and `available update` items for [query] and [installed] are different sources.

* remove variable preventing I forget it

* scoop-info: fix source of manifest on bucket

* fix `scoop-depends`

* Fix Standalone and Source detection

* fix global install

* Fix scoop-cat, scoop-home

- Query for remote manifest

* scoop-list: info +deprecated

* manifest: Fix first selected manifest

* gramma..

* Fix 61b3259

* length

* fix(scoop-depends-tests): Mocking `USE_EXTERNAL_7ZIP` as $false (#6431)

* fix(scoop-depends-tests): Mocking `USE_EXTERNAL_7ZIP` as $false to avoding error when it is $true

* CHANGELOG

* feat(autoupdate): GitHub predefined hashes support (#6416)

* feat(autoupdate): predefined hash case for GitHub

- Remove `sha256:` prefix in `format_hash()`
- Add GitHub support in `get_hash_for_app()`

Close #6381

* doc(chglog): GitHub auto hash update

* fix(autoupdate): remove prefix only

* docs(CHANGELOG): Update to 0.5.3 (#6432)

* docs(CHANGELOG): Update to 0.5.3

* 6416

---------

Signed-off-by: Chawye Hsu <su+git@chawyehsu.com>
Co-authored-by: Olav Rønnestad Birkeland <6450056+o-l-a-v@users.noreply.github.com>
Co-authored-by: kiennq <kien.n.quang@gmail.com>
Co-authored-by: HUMORCE <humorce@outlook.com>
Co-authored-by: Ryan <sitiom@proton.me>
Co-authored-by: Chawye Hsu <su+git@chawyehsu.com>
Co-authored-by: Bassel Rachid <101208715+basselworkforce@users.noreply.github.com>
Co-authored-by: Wordless Echo <wordless@echo.moe>
HUMORCE pushed a commit to HUMORCE/Scoop that referenced this pull request Aug 11, 2025
…ctDir` if it is empty (ScoopInstaller#6092)

Co-authored-by: Hsiao-nan Cheung <niheaven@gmail.com>
HUMORCE added a commit to HUMORCE/Scoop that referenced this pull request Aug 12, 2025
* chore(release): Bump to version 0.5.3 (ScoopInstaller#6257)

* fix (decompress): `Expand-7zipArchive` only delete temp dir / `$extractDir` if it is empty (ScoopInstaller#6092)

Co-authored-by: Hsiao-nan Cheung <niheaven@gmail.com>

* refactor(download): Move download-related functions to 'download.ps1' (ScoopInstaller#6095)

* fix(download): Fallback to default downloader when aria2 fails (ScoopInstaller#4292)

* fix(commands): Handling broken aliases (ScoopInstaller#6141)

* fix(shim): properly check `wslpath`/`cygpath` command first (ScoopInstaller#6114)

Co-authored-by: Hsiao-nan Cheung <niheaven@gmail.com>

* fix(scoop-bucket): Add missing import for `no_junction` envs (ScoopInstaller#6181)

Signed-off-by: Chawye Hsu <su+git@chawyehsu.com>

* docs(chglog): Update to 0.5.3 (ScoopInstaller#6258)

* perf(shim): Update kiennq-shim to v3.1.2 (ScoopInstaller#6261)

* fix(decompress): Replace deprecated 7ZIPEXTRACT_USE_EXTERNAL config (ScoopInstaller#6327)

Co-authored-by: Hsiao-nan Cheung <niheaven@gmail.com>

* fix(scoop-uninstall): Fix uninstaller does not gain Global state (ScoopInstaller#6430)

* global arg

* changelog

* refactor(Get-Manifest): Select actual source for manifest (ScoopInstaller#6142)

* first step

* Revert "first step"

This reverts commit c5907c3.

* refactor(Get-Manifest): Select actual source for installed manifest

* rework sub-commands, `scoop-depends` is NOT working at this stage

* URI manifest

* opt

* deprecated manifest

* source of manifests

* source of manifest pt2

- Mark URI(path/URL/UNC/etc.) query as standalone manifest
- Drop `installed` and `available update` items for [query] and [installed] are different sources.

* remove variable preventing I forget it

* scoop-info: fix source of manifest on bucket

* fix `scoop-depends`

* Fix Standalone and Source detection

* fix global install

* Fix scoop-cat, scoop-home

- Query for remote manifest

* scoop-list: info +deprecated

* manifest: Fix first selected manifest

* gramma..

* Fix 61b3259

* length

* fix(scoop-depends-tests): Mocking `USE_EXTERNAL_7ZIP` as $false (ScoopInstaller#6431)

* fix(scoop-depends-tests): Mocking `USE_EXTERNAL_7ZIP` as $false to avoding error when it is $true

* CHANGELOG

* feat(autoupdate): GitHub predefined hashes support (ScoopInstaller#6416)

* feat(autoupdate): predefined hash case for GitHub

- Remove `sha256:` prefix in `format_hash()`
- Add GitHub support in `get_hash_for_app()`

Close ScoopInstaller#6381

* doc(chglog): GitHub auto hash update

* fix(autoupdate): remove prefix only

* docs(CHANGELOG): Update to 0.5.3 (ScoopInstaller#6432)

* docs(CHANGELOG): Update to 0.5.3

* 6416

---------

Signed-off-by: Chawye Hsu <su+git@chawyehsu.com>
Co-authored-by: Olav Rønnestad Birkeland <6450056+o-l-a-v@users.noreply.github.com>
Co-authored-by: kiennq <kien.n.quang@gmail.com>
Co-authored-by: HUMORCE <humorce@outlook.com>
Co-authored-by: Ryan <sitiom@proton.me>
Co-authored-by: Chawye Hsu <su+git@chawyehsu.com>
Co-authored-by: Bassel Rachid <101208715+basselworkforce@users.noreply.github.com>
Co-authored-by: Wordless Echo <wordless@echo.moe>

* fix(autoupdate): Skiping github mode while hash.url already set

---------

Signed-off-by: Chawye Hsu <su+git@chawyehsu.com>
Co-authored-by: Hsiao-nan Cheung <niheaven@gmail.com>
Co-authored-by: Olav Rønnestad Birkeland <6450056+o-l-a-v@users.noreply.github.com>
Co-authored-by: kiennq <kien.n.quang@gmail.com>
Co-authored-by: Ryan <sitiom@proton.me>
Co-authored-by: Chawye Hsu <su+git@chawyehsu.com>
Co-authored-by: Bassel Rachid <101208715+basselworkforce@users.noreply.github.com>
Co-authored-by: Wordless Echo <wordless@echo.moe>
Comment thread lib/decompress.ps1
$ExtractDirTopPath = [string] "$DestinationPath\$($ExtractDir -replace '[\\/].*')"
if ((Get-ChildItem -Path $ExtractDirTopPath -Force -ErrorAction Ignore).Count -eq 0) {
Remove-Item -Path $ExtractDirTopPath -Recurse -Force -ErrorAction Ignore
}
Copy link
Copy Markdown
Member

@z-Fng z-Fng Oct 11, 2025

Choose a reason for hiding this comment

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

While testing manifests, I happened to run into an issue here.

When the $ExtractDir depth is greater than or equal to three, like the one below ($ExtractDir : keep\sub\sub-sub), the folder 'keep' will not be removed. The return value of Get-ChildItem for folder 'keep' is always greater than 0 as there exists a sub-folder.

└─ keep
   └─ sub
      └─  sub-sub
         └─  empty
   └─ othersub

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So one should look for files specifically? Add -File -Recurse to Get-ChildItem?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Add -File -Recurse to Get-ChildItem?

When extraction is done, we cannot tell whether the folder is newly created or already existed.
If the folder already exists before extraction, even if it is an empty folder, we should not remove it.

Perhaps we could check if the folder exists recursively before 7-Zip actually starts extracting?

keep
   └─ sub
      └─  sub-sub
  1. Check if the folder $DestinationPath\keep exists.
    • If it does not exist, we can safely remove $DestinationPath\keep.
    • If it does exist, proceed to step 2.
  2. Check if the folder $DestinationPath\keep\sub exists.
    • If it does not exist, we can safely remove $DestinationPath\keep\sub.
    • If it does exist, proceed to step 3.
  3. ...

Copy link
Copy Markdown
Contributor Author

@o-l-a-v o-l-a-v Oct 13, 2025

Choose a reason for hiding this comment

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

Can you create a new issue with an example manifest where this problem occurs?

There is also this feature request which would make this logic unnecessary:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you create a new issue with an example manifest where this problem occurs?

OK. I'm just about to do it.

There is also this feature request which would make this logic unnecessary

It would be even better if this new feature fixes the issue.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you create a new issue with an example manifest where this problem occurs?

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.

4 participants