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

Add multiple URL support for autoupdate #2803

Closed

Conversation

rasa
Copy link
Member

@rasa rasa commented Nov 23, 2018

Fixes (?) #2757. See also ScoopInstaller/Extras#1459

lib/autoupdate.ps1 Outdated Show resolved Hide resolved
@rasa rasa changed the title Add multiple URL support for autoupdate [WIP] Add multiple URL support for autoupdate Nov 25, 2018
@rasa rasa force-pushed the rasa/multiple-urls-in-autoupdate branch from 002c4cd to 9ca051c Compare March 9, 2019 07:55
@rasa rasa changed the title [WIP] Add multiple URL support for autoupdate Add multiple URL support for autoupdate Mar 9, 2019
@rasa
Copy link
Member Author

rasa commented Mar 9, 2019

Tested and working with
ScoopInstaller/Extras#1836
Ready for further testing or merging/

@rasa rasa added enhancement review-needed Asks for review of these changes labels Mar 9, 2019
@Ash258
Copy link
Contributor

Ash258 commented Mar 9, 2019

I will look into it in today and tommorow and give some feedback

@niheaven
Copy link
Member

niheaven commented Mar 9, 2019

A hashExtractionOrArrayOfHashExtrasctions should be added in dshema.json that looks like:

"hashExtractionOrArrayOfHashExtrasctions": {
    "anyOf": [
        {
            "$ref": "#/definitions/hashExtraction"
        },
        {
            "items": {
                "$ref": "#/definitions/hashExtraction"
            },
            "minItems": 1,
            "type": "array",
            "uniqueItems": true
        }
    ]
},

and change hash's $ref to hashExtractionOrArrayOfHashExtrasctions in autoupdate so as to enable multiple hash extraction in au.

There might be some tweaks in hash extraction handling for multiple URL autoupdate, e.g. using same hash extraction formula for multi-files, or some other variations. Let's think about it~

@rasa
Copy link
Member Author

rasa commented Mar 9, 2019

@niheaven Good catch! I'll add it now.

@rasa rasa changed the title Add multiple URL support for autoupdate [WIP] Add multiple URL support for autoupdate Mar 9, 2019
@rasa rasa changed the title [WIP] Add multiple URL support for autoupdate Add multiple URL support for autoupdate Mar 9, 2019
@rasa
Copy link
Member Author

rasa commented Mar 9, 2019

To get all the other .jsons to validate, we need to change

        "uriOrArrayOfUris": {
            "anyOf": [
                {
                    "format": "uri",
                    "not": {
                        "pattern": "(\\$)"
                    },

to

        "uriOrArrayOfUris": {
            "anyOf": [
                {
                    "format": "uri",

or

        "uriOrArrayOfUris": {
            "anyOf": [
                {
                    "format": "uri",
                    "not": {
                        "pattern": "(^\\$)"
                    },

Is this pattern here to avoid empty URLs? If so, then doesn't ^\\$ make more sense?

@r15ch13
Copy link
Member

r15ch13 commented Mar 9, 2019

No that pattern disallows URLs containing $, so the autoupdater can't slip an unresolved variable in 😄

@rasa
Copy link
Member Author

rasa commented Mar 9, 2019

@r15ch13 Two points:

  1. Autoupdate URLs have dollar signs
  2. Dollar signs are allowed in URLs, per here

I've addressed 1. in the schema. Let's ignore 2. for now.

@rasa rasa force-pushed the rasa/multiple-urls-in-autoupdate branch from e805766 to ae350e0 Compare March 9, 2019 22:10
lib/autoupdate.ps1 Outdated Show resolved Hide resolved
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.

Small tweak by static review. There is some code duplicacy, which I think could be removed and simplified, but i need to think about it little bit more.

@niheaven
Copy link
Member

Now the codes are applied to noarch, and they should be copied to arch-32 and arch-64 too.

BTW, the hash handling part is insufficiency, I thought, and a specialized function to handle multiple hash extracting methods may help. I'm trying to make one, but maybe later.

Enhance lets_do_autoupdates to return $has_changes and $has_errors

Co-Authored-By: rasa <[email protected]>
Copy link
Member

@niheaven niheaven left a comment

Choose a reason for hiding this comment

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

Suggestion about reusing hash extraction expression and applying lets_do_autoupdates to au.arch.

$hash = get_hash_for_app $app $json.autoupdate.hash $version $url $substitutions
if ($json.autoupdate.hash.length -gt $i) {
$h = $json.autoupdate.hash[$i]
} else {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
} else {
} elseif ($json.autoupdate.hash.length -ne 0) {
$h = $json.autoupdate.hash[-1]
} else {

If hash's length is less than url, the last hash expression is reused for remaining urls.

So we could use one hash expression for all these urls if hash extraction url and regex are the same, e.g., $sha256.*?$basename, different hashes for different $basenames.

$has_changes = $true
update_manifest_with_new_version $json $version $url $hash
if ($json.url) {
if ($json.autoupdate.url -is [System.Array]) {
Copy link
Member

Choose a reason for hiding this comment

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

This if...else... should be applied to $json.architecture's ForEach-Object script too.

@r15ch13 r15ch13 changed the base branch from master to develop May 3, 2019 18:39
@niheaven
Copy link
Member

@rasa Could I keep on working with multi-urls and hashes autoupdate support based on this PR?

@rasa
Copy link
Member Author

rasa commented Jun 18, 2019

@rasa Could I keep on working with multi-urls and hashes autoupdate support based on this PR?

Sure! Sorry I dropped the ball on implementing your suggestions.

@rasa
Copy link
Member Author

rasa commented Jun 26, 2019

Moved to #3518

@rasa rasa closed this Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement review-needed Asks for review of these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants