ScoopInstaller / Main

📦 The default bucket for Scoop.
https://scoop.sh
The Unlicense
1.56k stars 931 forks source link

zstd: Add 32-bit architecture #5661

Closed redactedscribe closed 3 months ago

redactedscribe commented 3 months ago

There may be a simpler method, but this fix extracts the zip within the zip and then moves its contents to $dir. Tested locally.

Closes #5658

github-actions[bot] commented 3 months ago

All changes look good.

Wait for review from human collaborators.

zstd

redactedscribe commented 3 months ago

Tests fail with the following which I'm not sure how to fix (the manifest installs locally just fine):

      [*] Error: Property 'extract_to' has not been defined and the schema does not allow additional properties.
        [^] Line: D:\a\Main\Main\my_bucket\bucket\zstd.json:10:25
Kosette commented 3 months ago

If you wish to temporarily bypass this issue, you can refer to this manifest, which has been tested successfully. However, personally, I am cautious about whether a fix is needed for the next time.

redactedscribe commented 3 months ago

@Kosette The manifest you link also uses extract_to like this PR. Won't it just trigger the test to fail again?

Kosette commented 3 months ago

@Kosette The manifest you link also uses extract_to like this PR. Won't it just trigger the test to fail again?

[^] Line: D:\a\Main\Main\my_bucket\bucket\zstd.json:10:25

test log tell you where problem lays,refer to the manifest schema.

https://github.com/ScoopInstaller/Scoop/wiki/App-Manifests

Kosette commented 3 months ago

extract_to and architecture in the same level, not sub-property.

redactedscribe commented 3 months ago

@pratikpc Should be good to go now.

pratikpc commented 3 months ago

https://github.com/ScoopInstaller/Main/blob/master/bucket/zstd.json

extract_dir would still be needed

Would recommend switching to v1.5.6 and then adding the 32 bit support to the same

redactedscribe commented 3 months ago

Why is extract_dir needed? The manifest auto-updates and installs correctly locally. This PR avoids the need to hard-code "extract_dir": "zstd-v1.5.6-win64" and "extract_dir": "zstd-v1.5.6-win32" for the root architecture ($version cannot be used).

The extract_dir present for architecture under autoupdate at the bottom also seems unnecessary as far as I can tell.

pratikpc commented 3 months ago

@redactedscribe its working on account of the extract_to and pre_install scripts

Both of them are not needed.

To make it easier to understand how it works, I would recommend looking at NodeJS for example

redactedscribe commented 3 months ago

@pratikpc Thank you for your help. I see now that autoupdate re-writes the extract_dir at the top.

I've compared my changes to the current main/zstd.json and this final iteration should only add the 32-bit architecture and via the conventional extract_dir.

niheaven commented 3 months ago

/verify

github-actions[bot] commented 3 months ago

All changes look good.

Wait for review from human collaborators.

zstd