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 default features control by top level manifest #1331

Merged
merged 6 commits into from
Apr 20, 2024

Conversation

dg0yt
Copy link
Contributor

@dg0yt dg0yt commented Jan 29, 2024

For microsoft/vcpkg#35694 (comment):

Given that the app (aka top level) manifest defines the user's desired installation, should installing feature set no-default-features-3 really behave different from feature set no-default-features-1,no-default-features-3 with regard to the dependency on vcpkg-default-features-fail-require-other-feature?

... test manifest features: no-default-features, features = [no-default-features-1,no-default-features-3]
... (success)
... test manifest features: no-default-features, features = [no-default-features-3]
... (failure)

@dg0yt dg0yt force-pushed the minimal-features branch 2 times, most recently from 0482764 to 4713bf9 Compare January 29, 2024 20:13
@dg0yt dg0yt marked this pull request as ready for review January 29, 2024 20:32
@dg0yt
Copy link
Contributor Author

dg0yt commented Jan 30, 2024

@BillyONeal and team, please have a look.

@dg0yt
Copy link
Contributor Author

dg0yt commented Feb 2, 2024

@JavierMatosD This is the demo where even the top-level manifest isn't honored. (I prefer to refer to app manifest.)
microsoft/vcpkg-docs#255

@dg0yt dg0yt force-pushed the minimal-features branch 3 times, most recently from bb9b04d to c2a3e70 Compare February 5, 2024 06:49
@dg0yt dg0yt changed the title Test transitive features within app manifest Fix default features control by top level manifest Feb 5, 2024
@@ -1618,7 +1620,7 @@ TEST_CASE ("version install default features", "[versionplan]")
bp.v["a"] = {"1", 0};

WITH_EXPECTED(install_plan,
create_versioned_install_plan(vp, bp, var_provider, {Dependency{"a"}}, {}, toplevel_spec()));
create_versioned_install_plan(vp, bp, var_provider, {Dependency{"a"}}, {}, toplevel_spec("a")));
Copy link
Contributor Author

@dg0yt dg0yt Feb 5, 2024

Choose a reason for hiding this comment

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

Fixing a test mismatch:

  • In these tests, the default top level name is "toplevel-spec", but no dependencies passed to create_versioned_install_plan are using that name (before this change).
  • In regular manifest mode operation, the top level spec is the app spec, i.e. it carries the name of the app, and all dependencies passed to create_versioned_install_plan are using that name.

The observed bug cannot be fixed and tested without aligning top level spec name and dependencies.
This PR does the change only in two cases.
But probably this should be aligned for all calls to create_versioned_install_plan which deal with (default) features.

Copy link
Member

Choose a reason for hiding this comment

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

The top level manifest is allowed to have no name though, hmmm...

Copy link
Contributor Author

@dg0yt dg0yt Feb 6, 2024

Choose a reason for hiding this comment

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

I can imagine why (overlap with registry), but:

  • This adds to the problem of having two classes of manifests.
    (Known for default-features and for version constraints.)
  • This adds to the problem of the tests mismatching actual manifest mode invocations.
    Unless "top-level" is the implicitly forced name - this might make sense for UX in error message, but then it needs to be protected from other uses.

Copy link
Contributor Author

@dg0yt dg0yt Feb 6, 2024

Choose a reason for hiding this comment

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

And this PR makes no assumptions about the actual name of the top level manifest - it can be the empty string.

EDIT: But behavior changes for the dependencies of a port which has the same name as the app.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The top level manifest is allowed to have no name though, hmmm...

For clarification, this is what the tests use:

static const PackageSpec& toplevel_spec()
{
static const PackageSpec ret("toplevel-spec", Test::X86_WINDOWS);
return ret;
}

@BillyONeal
Copy link
Member

It is true that the case where this matters can only happen when there is a name, since you need to be able to form the dependencies block, which requires a "name". The issue is that the top level name is not required to exist, and this removes a lot of test coverage of that feature.

I think the minimum to proceed is to not remove that test coverage.

I think a separate feature where a vcpkg.json is able to depend on itself without specifying a name, like:

{
    "features": {
      "a": {
        "dependencies": [
            {
                "$comment": "No name at all means 'self'",
                "features": ["b"]
            }
        ]
      },
      "b":{}
    }
  }

or

{
    "features": {
      "a": {
        "dependencies": [
            {
                "self": true,
                "features": ["b"]
            }
        ]
      },
      "b":{}
    }
  }

or

{
    "features": {
      "a": {
        "feature-dependencies": ["b"]
      },
      "b":{}
    }
  }

would be nice, but I don't think that bike shed needs to be painted to land this.

@dg0yt
Copy link
Contributor Author

dg0yt commented Mar 20, 2024

Did the team give up on default-features?
You don't engage much in issues and discussion, you don't fix clear bugs quickly, and now you also turn off project defaults because "Default features are complicated to disable" (microsoft/vcpkg#36982 (comment)).

@Thomas1664
Copy link
Contributor

Did the team give up on default-features? You don't engage much in issues and discussion, you don't fix clear bugs quickly, and now you also turn off project defaults because "Default features are complicated to disable" (microsoft/vcpkg#36982 (comment)).

@dg0yt Given that this is an issue that affects many users, I think this would be a great candidate for merging into open-vcpkg/vcpkg-tool?

@dg0yt
Copy link
Contributor Author

dg0yt commented Apr 4, 2024

What is needed for this PR to be merged?

@BillyONeal BillyONeal enabled auto-merge (squash) April 20, 2024 00:51
@BillyONeal
Copy link
Member

Thanks for the fix! Sorry it took so long for me/us to convince myself/ourselves that the handling of nameless manifests was correct

@BillyONeal BillyONeal merged commit 87adc3f into microsoft:main Apr 20, 2024
5 checks passed
@dg0yt dg0yt deleted the minimal-features branch April 20, 2024 02:07
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