Test/fix internal dependencies in top level manifest. #1890
Conversation
| if (it != m_graph.end()) | ||
| { | ||
| it->second.origins.insert(origin); | ||
| it->second.default_features &= default_features_mask; |
There was a problem hiding this comment.
This reverts a change from #1331 and is the fix for the livelock: Do not reset default_features to false.
- The toplevel request (
ConstraintFrame) could include multiple dependencies pointing to the toplevel manifest's default features. resolve_stack()callsrequire_port_defaults()for each of these dependencies.- The
default_featuresflag of the solution data node (it->second) is what prevents the creation of multiple newresolve_stackitems (ConstraintFrame).
| it->second.default_features = default_features_mask; | ||
| // Implicit defaults are disabled if spec is requested from top-level spec. | ||
| // Note that if top-level doesn't also mark that reference as `[core]`, defaults will be re-engaged. | ||
| it->second.default_features = origin != m_toplevel.name(); |
There was a problem hiding this comment.
This is not a revert but retains the actual fix from #1331: Fixing the initialization of the flag.
There are three places which engage the actual default features:
- For the input to the
vcpkg installcommand, default features are initialy unrolled incommands.install.cpp, subject to command line switch. - For dependencies requested from the toplevel manifest (name), the value is
false. Actual default features can be engaged byrequire_port_defaults(), subject to explicit dependencies in theresolve_stackitems (ConstraintFrame). - For dependencies requested from other manifests, , the value is
true. The actual default features are engaged byrequire_scfl()called three lines later.
There was a problem hiding this comment.
Q: Why isn't this |=?
A: Because we know it was already false/unset because if we had not inserted a new node, the if (it != m_graph.end()) branch would have been entered.
No change requested.
|
Now it should be the right fix, rectifying changes from #1331. I removed cleanup and optimization from this PR. I would submit these usefull improvements after this PR. In addition, I think that |
| "@ | ||
| Throw-IfNonContains -Expected $expected -Actual $output | ||
|
|
||
| # This e2e tests is mirred in the dependencies.cpp test. |
There was a problem hiding this comment.
FWIW, if we can test it with a unit test I don't think an e2e test is strictly needed but I'm not going to say no to more tests :)
There was a problem hiding this comment.
Without the e2e test, you miss one the "three places which engage the actual default features", command.install.cpp.
| it->second.default_features = default_features_mask; | ||
| // Implicit defaults are disabled if spec is requested from top-level spec. | ||
| // Note that if top-level doesn't also mark that reference as `[core]`, defaults will be re-engaged. | ||
| it->second.default_features = origin != m_toplevel.name(); |
There was a problem hiding this comment.
Q: Why isn't this |=?
A: Because we know it was already false/unset because if we had not inserted a new node, the if (it != m_graph.end()) branch would have been entered.
No change requested.
3 participants
Fixes microsoft/vcpkg#48892.
Implements microsoft/vcpkg#48892 (comment)
This seems to be the first test that actually covers details of handling the top-level manifest, i.e. the request is installing the given top level manifest, not unrelated ports from the registry.
Edit: The difference is how the "source control file" is passed to
create_versioned_install_plan.The PR now includes the e2e test from #1889 because it helps to understand the construction of the test in
dependencies.cpp.