Allow features with platform expressions #813
Conversation
…tead of using magic "core" feature. This also solves the "issue" that I can pass ImplicitDefault::No to Dependency::to_full_spec to get default_features = false even when Dependency comes from a parsed manifest file with "default-features": true.
# Conflicts: # src/vcpkg/install.cpp # src/vcpkg/sourceparagraph.cpp
# Conflicts: # src/vcpkg-test/manifests.cpp
# Conflicts: # src/vcpkg/sourceparagraph.cpp
# Conflicts: # include/vcpkg/paragraphparser.h # src/vcpkg/paragraphs.cpp
|
Quoth @ras0219-msft:
(That is to say, agreement with what you did here) |
include/vcpkg/packagespec.h
Outdated
| { | ||
| std::string name; | ||
| PlatformExpression::Expr platform; | ||
| Feature(std::string name) : Feature(std::move(name), PlatformExpression::Expr::Empty()) { } |
There was a problem hiding this comment.
I'm a bit nervous about this implicit conversion; too easy to accidentally chop the expression off.
There was a problem hiding this comment.
It is only relevant for the tests. It changes
{
Dependency{"semver", {{"x"}}},
},to
{
Dependency{"semver", {Dependency::Feature{"x"}}},
},I can change the 18 occurrences if you want.
include/vcpkg/paragraphparser.h
Outdated
| const std::string& str, | ||
| StringView origin = "<unknown>", | ||
| TextRowCol textrowcol = {}, | ||
| ImplicitDefault implicit_defaults = ImplicitDefault::YES); |
There was a problem hiding this comment.
I feel like this is a layering problem. Just parsing the file shouldn't be doing the semantic transform to figure out what defaults are. Ideally we would have two types: one which is just the parsed guts, and another which is the parsed guts with expressions evaluated and applied.
I understand this suggestion is kind of a large/structural change though :(. @ras0219-msft Would you characterize my statement here as consistent with the dependencies stuff or do you think it's out of left field? (I think it's worth asking autowantwort to go here if this situation is a problem he created but not if we are already filthy about this elsewhere)
There was a problem hiding this comment.
I have now removed the parameter.
just parsing the file shouldn't be doing the semantic transform to figure out what defaults are.
It is not about what defaults are. It is about if defaults are requested or not. So currently we always use ImplicitDefault::YES while parsing.
liba => default-features: true
liba[core] => default-features: false
If we add a ImplicitDefault implicit_defaults parameter the following would be possible if ImplicitDefault::NO is passed:
liba => default-features: false
But this is semantic currently is never needed (except in one test) so I removed the parameter.
We could also introduce a new data type for the return value of this function but I currently don't see a value in that. If you really want it I would suggest do not do that in this PR to keep this PR small.
# Conflicts: # include/vcpkg/base/messages.h # src/vcpkg/base/messages.cpp
# Conflicts: # include/vcpkg/packagespec.h # include/vcpkg/paragraphparser.h # src/vcpkg/packagespec.cpp
# Conflicts: # src/vcpkg/sourceparagraph.cpp
# Conflicts: # src/vcpkg-test/dependencies.cpp # src/vcpkg/binaryparagraph.cpp # src/vcpkg/commands.build.cpp # src/vcpkg/dependencies.cpp
# Conflicts: # src/vcpkg-test/dependencies.cpp # src/vcpkg/dependencies.cpp
# Conflicts: # src/vcpkg-test/dependencies.cpp
|
Resolving conflicts after #1014 was ... fun |
|
@ras0219-msft Anything that needs to be done here or is this PR only waiting for design approval? |
# Conflicts: # src/vcpkg-test/dependencies.cpp # src/vcpkg-test/spdx.cpp # src/vcpkg/dependencies.cpp
# Conflicts: # src/vcpkg-test/dependencies.cpp
@dan-shaw I have created MicrosoftDocs/vcpkg-docs#114 |
|
Thanks for the new feature! |
|
Whoop whoop 🥳 Thank you all for reviewing :) |
Allows
{ "dependencies": [{ "name": "spdlog", "features": [{"name": "wchar", "platform": "windows"}] }] }and
{ "default-features": [{ "name": "wchar", "platform": "windows" }] }