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

Manifests: Remove support for declaring features as lists #178

Merged

Conversation

autoantwort
Copy link
Contributor

@autoantwort autoantwort commented Aug 29, 2021

It is not used and not described in the docs. I used vcpkg format-manifest --all with the new version and it doesn't reported a wrong file.

It is not used and not described in the docs
@autoantwort
Copy link
Contributor Author

Only rebased

@strega-nil-ms
Copy link
Contributor

I'm okay with this; it's mostly a back-compat feature. I would appreciate a diagnostic, though.

@autoantwort
Copy link
Contributor Author

I don't know what you mean exactly with a diagnostic, but it seems that it is not used in the vcpkg repo since microsoft/vcpkg@0fec134

@strega-nil-ms
Copy link
Contributor

@autoantwort oh given that it's from before registries, I don't have a problem removing it personally; needs a discussion anyways tho
By diagnostic, I mean checking for arrays specifically and printing a special error message about it.

@autoantwort
Copy link
Contributor Author

Ok, then I will wait for the result of your internal discussion.

@strega-nil-ms strega-nil-ms added the requires:discussion This PR requires discussion of the correct way forward label Sep 22, 2021
@BillyONeal
Copy link
Member

We want a second opinion from @ras0219 but we see no problems with merging this.

Copy link
Contributor

@ras0219-msft ras0219-msft left a comment

Choose a reason for hiding this comment

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

This would break the following port versions:

  • abseil version 2020-03-03#7
  • blend2d versions beta_2020-07-31 and beta_2020-07-09

it seems that the other packages actually never had their port-version bumped while they had the unstable features syntax. Because this features syntax was not GA, I think it's acceptable to assume no concern for ports outside the curated repo.

To set precedent: I think breaking 3 specific versions of packages that do not have other special significance (last supported release of some major version of a major package like openssl or boost) is an acceptable cost in pursuit of removing an experimental feature. This does not establish the true upper limit of cost that would be acceptable, only that this case falls below it.

Script used to find them:

#!/bin/bash

for a in versions/*/*.json
do
    echo $a
    versions=$(jq -r '.versions[]."git-tree"' $a)
    for ver in $versions
    do
        features=$(git show $ver:vcpkg.json 2>/dev/null | jq ".features")
        if [ $? -ne 0 ]; then continue; fi
        if [ "${features:0:1}" == "[" ]
        then
            echo "$ver will be broken:"
            git show $ver:vcpkg.json | jq '{
            name: .name,
            "version-string": ."version-string",
            "version-semver": ."version-semver",
            "version-date": ."version-date",
            version: .version,
            "port-version": ."port-version",
            features: .features
            }'
        fi
    done
done

@strega-nil-ms
Copy link
Contributor

Given that we have three approvals, merging ahead! Thanks robert

@strega-nil-ms strega-nil-ms merged commit c0485c3 into microsoft:main Sep 23, 2021
@autoantwort autoantwort deleted the remove-feature-list-support branch September 23, 2023 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
requires:discussion This PR requires discussion of the correct way forward
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants