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

fix(decompress): Match extract_dir/extract_to and archives #5983

Merged
merged 2 commits into from
May 26, 2024

Conversation

lewis-yeung
Copy link
Contributor

Description

This PR is to fix a regression introduced in the refactoring of #5951 which broke the match between entries of extract_dir/extract_to lists and files that require unpacking:

Scoop/lib/decompress.ps1

Lines 22 to 26 in a5bd229

$fnArgs = @{
Path = Join-Path $Path $Name[$i]
DestinationPath = Join-Path $Path $extractTo[$i]
ExtractDir = $extractDir[$i]
}

The directories specified by extract_dir/extract_to fields in an app manifest are only meaningful for download files that require unpacking, if a download URL of an unpacking-free file is placed before those of archive files, the latter will not be unpacked from/to the specified paths.

How Has This Been Tested?

Tested by running:

scoop install 'https://raw.githubusercontent.com/lewis-yeung/scoop-bucket/a9b9df6f918995d3c6f1323a6133b20a0d7b4c6d/bucket/oh-my-posh.json'

It works properly after the fix.

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.

@niheaven
Copy link
Member

The optimal approach is to move these files to the end, or prefix them with a . in extract_dir/extract_to.

In this PR, extracted is unused and should be removed.

@lewis-yeung
Copy link
Contributor Author

@niheaven The code must work without modifying any existing app manifests. Right?

The optimal approach is to move these files to the end,

Which kind of files are you referring to? Files that do not need to be unpacked?

or prefix them with a . in extract_dir/extract_to.

Do you mean filtering out non-archive files and adding placeholders for them in the $extractDir/$extractTo list? This approach is not that straightforward compared to the previous code.

In this PR, extracted is unused and should be removed.

Is it? I mean, it counts unpackable files here and ensures that they are correctly paired with entries of extract_dir/extract_to:

$extracted++

which is exactly what the previous code did.

@niheaven
Copy link
Member

Scoop/lib/install.ps1

Lines 590 to 595 in 5819b5a

foreach ($url in $urls) {
$fname = url_filename $url
$extract_dir = $extract_dirs[$extracted]
$extract_to = $extract_tos[$extracted]

The term extracted was previously used to identify extract_dir/extract_to because the old method employed a foreach routine. However, since I've switched to a for loop, the enumerator now serves to identify each item.

@lewis-yeung
Copy link
Contributor Author

@niheaven

The term extracted was previously used to identify extract_dir/extract_to because the old method employed a foreach routine. However, since I've switched to a for loop, the enumerator now serves to identify each item.

The fact is that extract_dir/extract_to entries are for archive files only and do not always align with URLs. The for loop here broke the previously working cases. Without further filtering logic, the $extracted counter works and is pretty straightforward. Why not?

@niheaven
Copy link
Member

You're right, I misunderstood what you meant.

@niheaven niheaven merged commit dec4232 into ScoopInstaller:develop May 26, 2024
2 checks passed
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.

None yet

2 participants