-
Notifications
You must be signed in to change notification settings - Fork 279
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
Conversation
0482764
to
4713bf9
Compare
4713bf9
to
3ba02ce
Compare
@BillyONeal and team, please have a look. |
@JavierMatosD This is the demo where even the top-level manifest isn't honored. (I prefer to refer to app manifest.) |
bb9b04d
to
c2a3e70
Compare
src/vcpkg-test/dependencies.cpp
Outdated
@@ -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"))); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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 fordefault-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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
vcpkg-tool/src/vcpkg-test/dependencies.cpp
Lines 159 to 163 in 9c1e418
static const PackageSpec& toplevel_spec() | |
{ | |
static const PackageSpec ret("toplevel-spec", Test::X86_WINDOWS); | |
return ret; | |
} |
c2a3e70
to
3c664f9
Compare
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 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. |
3c664f9
to
e1d04cf
Compare
d85bd8f
to
4285366
Compare
Did the team give up on default-features? |
@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? |
What is needed for this PR to be merged? |
Thanks for the fix! Sorry it took so long for me/us to convince myself/ourselves that the handling of nameless manifests was correct |
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 setno-default-features-1,no-default-features-3
with regard to the dependency onvcpkg-default-features-fail-require-other-feature
?