Insert "--head" defenses into the binary cache and block "default" when calculating feature ABIs. #336
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
<expected.h>: Add the missing T& overloads used elsewhere. <binarycaching.h>: * Remove VcpkgPaths from the IBinaryProvider interface; add it as a member of the concrete implementations. * Hoist BinaryConfigParserState (formerly State in binarycaching.cpp) here. It would be nice to make this more shiny, but hiding the definition of this struct wasn't buying us too much, and being unable to test stuff was a worse evil. * create_binary_providers_from_configs_pure now returns ExpectedS<BinaryConfigParserState>; this lets the tests of the parser machinery continue to be unchanged while still moving paths in as a member. binarycaching.cpp: * Push vcpkgpaths as a member into the concrete implementations. * Change/audit all IBinaryProvider::prefetch to support nullptr. We pass in the underlying action plan vector here, and preparing a vector with the ABI-less packages removed would require extra indirection or flattening of the data we don't want to do in a bugfix. * Change/audit all IBinaryProvider::precheck to not support nullptr. That is used only by CI and everything there should have a package ABI. * Change BinaryCache::prefetch to explicitly document that nullptr package_abi is acceptable. * Change BinaryCache::precheck to explicitly error out on nullptr package_abi. vcpkg-test/binarycaching.cpp: * Add tests for nullptr package ABIs passed to BinaryCache.
…res have not yet been resolved.
BillyONeal
commented
Jan 27, 2022
| std::vector<std::string> sorted_feature_list = action.feature_list; | ||
| Util::sort(sorted_feature_list); | ||
| InternalFeatureSet sorted_feature_list = action.feature_list; | ||
| // Check that no "default" feature is present. Default features must be resolved before attempting to calculate |
There was a problem hiding this comment.
Note that unlike #336 this does not insert "core" and rejects "default".
There was a problem hiding this comment.
The current documentation string for InternalFeatureSet indicates that default is valid:
/// In an internal feature set, "default" represents default features and missing "core" has no semanticHow do we know that there won't be pseudo-features here?
There was a problem hiding this comment.
@ras0219-msft We could introduce InternalResolvedFeatureSet or something like that if you wanted, but pseudo-features getting into an ABI calculation is a bug (since it would result in the same input having different ABIs depending on whether the pseudo-feature was resolved first).
BillyONeal
commented
Jan 27, 2022
| @@ -16,27 +17,6 @@ | |||
|
|
|||
| namespace vcpkg | |||
| { | |||
| enum class RestoreResult | |||
There was a problem hiding this comment.
These got put into the fwd header
ras0219-msft
approved these changes
Jan 27, 2022
| std::vector<std::string> sorted_feature_list = action.feature_list; | ||
| Util::sort(sorted_feature_list); | ||
| InternalFeatureSet sorted_feature_list = action.feature_list; | ||
| // Check that no "default" feature is present. Default features must be resolved before attempting to calculate |
There was a problem hiding this comment.
The current documentation string for InternalFeatureSet indicates that default is valid:
/// In an internal feature set, "default" represents default features and missing "core" has no semanticHow do we know that there won't be pseudo-features here?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is fallout from code review / investigation of #331
--head, and adds tests.VcpkgPathsis sucked out of a parameter passed through the binary cache and is made a member of the providers if necessary.<expected.h>: Add the missing T& overloads used elsewhere.
<binarycaching.h>:
binarycaching.cpp:
vcpkg-test/binarycaching.cpp: