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

builds(schema): Set manifest schema to be stricter #5093

Merged
merged 10 commits into from
Aug 21, 2022

Conversation

chawyehsu
Copy link
Member

@chawyehsu chawyehsu commented Aug 10, 2022

Description

Set manifest schema to be stricter.

Context

The schema was updated to support multiple URLs autoupdate in #3518 , which played well on its primary purpose. However, it introduced many other meaningless fields for autoupdate, such as psmodule and persist fields in the autoupdateArch, following license field added in #4528, causing unexpected manifests could be elaborately constructed to be like this:

meaningless.json

{
    "homepage": "https://github.com/boyter/scc",
    "license": "ISC",
    "version": "1.0.0",
    "architecture": {
        "64bit": {
            "url": "https://github.com/boyter/scc/releases/download/v3.0.0/scc-3.0.0-x86_64-pc-windows.zip",
            "hash": "f3972acf03c09ff836071d1d173cb49281c8bc0f9682217118565ca62c5559b8"
        }
    },
    "psmodule": {
        "name": "module1"
    },
    "persist": "real",
    "checkver": "github",
    "autoupdate": {
        "architecture": {
            "64bit": {
                "url": "https://github.com/boyter/scc/releases/download/v$version/scc-$version-x86_64-pc-windows.zip",
                "psmodule": {
                    "name": "module2"
                },
                "persist": "fake",
                "license": "GPL"
            }
        }
    }
}

This passes the current manifest schema without any errors, but it does not work as you expected of updating psmodule, persist, and license fields since they are not valid fields for the architecture.

I'm pretty sure @niheaven was going to reuse the autoupdateArch definition to achieve the goal of supporting multiple URLs, persistence, license, etc. autoupdate. But he failed and misused it, causing the possibility of creating invalid manifests.

Important notes

I'm going to make it much stricter such as removing shortcuts, bin fields from autoupdate. But @niheaven commented #3518 (comment) with the opinion of providing every possible property to be auto-updatable which I'm not a fan of it. Hence I choose not to implement it in this PR and do an RFC. I wasn't involved in #3518 therefore I didn't notice those changes in the schema. I would've challenged it if I did notice.

RFC
I do not think they are necessary to achieve the goal of updating shortcuts and binaries while updating the package version, though they have been used in some manifests such as Versions/python-alpha and Extras/linqpad. It's unnecessary and not a good practice to let shortcuts be tied with version-specific binaries, renaming the bin to a version-less name is always a better practice. You never want to shim a version-specific binary making the shim name change every time it updates to a new version.

How Has This Been Tested?

Test the crafted meaningless.json manifest with the patched schema.json and it will fail.
Test all manifests from known buckets plus your own bucket if available with the patched schema.json. All should be good.

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.

Signed-off-by: Chawye Hsu <chawyehsu@hotmail.com>
Signed-off-by: Chawye Hsu <chawyehsu@hotmail.com>
rasa
rasa previously approved these changes Aug 10, 2022
@rasa
Copy link
Member

rasa commented Aug 10, 2022

renaming the bin to a version-less name is always a better practice.

Agreed!

@niheaven
Copy link
Member

niheaven commented Aug 11, 2022

It's the manifest's failure, not the schema and update mechanism's. The following manifest will be updated well:

{
    "homepage": "https://github.com/boyter/scc",
    "license": "ISC",
    "version": "1.0.0",
    "architecture": {
        "64bit": {
            "url": "https://github.com/boyter/scc/releases/download/v3.0.0/scc-3.0.0-x86_64-pc-windows.zip",
            "hash": "f3972acf03c09ff836071d1d173cb49281c8bc0f9682217118565ca62c5559b8"
        }
    },
    "psmodule": {
        "name": "module1"
    },
    "persist": "real",
    "checkver": "github",
    "autoupdate": {
        "architecture": {
            "64bit": {
                "url": "https://github.com/boyter/scc/releases/download/v$version/scc-$version-x86_64-pc-windows.zip"
            }
        },
        "psmodule": {
            "name": "module2"
        },
        "persist": "fake",
        "license": "GPL"
    }
}

That means, items under autoupdate.arch only update the same items under arch, not the items outside it. The exception is, items under autoupdate will update all item under arch if there're no arch-sepc autoupdate items.

And yes I agree that not all entries in autoupdate might be under autoupdate.architecture, so separating them and making the schema stricter are more clear and accurate.

For the RFC, using autoupdate to update bins and shortcuts is just a choice, and is better understood. Of cause, maintainers chould change the names version-less, that's up to them.

The last thing I would like to asked is should persist, env_add_path and env_set be updated during autoupdating? I think there should be some use cases, but it's not been found now.

noarch url field is required if there is no
architecture-specific url field.

Signed-off-by: Chawye Hsu <chawyehsu@hotmail.com>
Signed-off-by: Chawye Hsu <chawyehsu@hotmail.com>
Signed-off-by: Chawye Hsu <chawyehsu@hotmail.com>
@chawyehsu
Copy link
Member Author

It's the manifest's failure, not the schema and update mechanism's.

Then we should not give it an opportunity to create such manifest.

The last thing I would like to asked is should persist, env_add_path and env_set be updated during autoupdating? I think there should be some use cases, but it's not been found now.

Though I've brought them back to the autoupdate field, I tend to not have them to be able to autoupdate. I do not think there is any use case in doing these type of updates. Autoupdate cares about and updates version-related placeholders in general, have you ever used any software having nested versioned folders that are needed to be added to the PATH and updated on each version upgrade?

niheaven
niheaven previously approved these changes Aug 21, 2022
niheaven
niheaven previously approved these changes Aug 21, 2022
@niheaven niheaven changed the title fix: set schema stricter builds(schema): Set manifest schema to be stricter Aug 21, 2022
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.

3 participants