-
Notifications
You must be signed in to change notification settings - Fork 286
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 crash while parsing malformed manifest files and make errors prettier #1219
Fix crash while parsing malformed manifest files and make errors prettier #1219
Conversation
Alternative to microsoft#1211 Fixes microsoft/vcpkg#33973 I'm not entirely happy with this because it emits extra 'mismatched type' warnings like $.dependencies[0].features[0]: mismatched type: expected a feature in a dependency .
@@ -84,27 +84,42 @@ namespace vcpkg | |||
static LocalizedString from_raw(std::basic_string<T>&& s) noexcept; | |||
static LocalizedString from_raw(StringView s); | |||
|
|||
LocalizedString& append_raw(char c); | |||
LocalizedString& append_raw(StringView s); | |||
LocalizedString& append_raw(char c) &; |
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 just stole this out of #1210 :)
PrintErrors::No); | ||
REQUIRE(!m_pgh.has_value()); | ||
REQUIRE(m_pgh.error()->to_string() == "error: while loading <test manifest>:\n" | ||
"$.default-features[0].name (a default feature): the feature \"core\" turns " |
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.
If an error doesn't contain more than a sentence I left everything lowercase with no period.
PrintErrors::No); | ||
REQUIRE(!m_pgh.has_value()); | ||
REQUIRE(m_pgh.error()->to_string() == "error: while loading <test manifest>:\n" | ||
"$.default-features[0] (a feature name): \"\" is not a valid feature name. " |
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.
If an error contains multiple sentences, I ended the first with a period and capitalize to start the next one.
@@ -994,6 +1007,19 @@ DECLARE_MESSAGE(DeleteVcpkgConfigFromManifest, | |||
(msg::path), | |||
"", | |||
"-- Or remove \"vcpkg-configuration\" from the manifest file {path}.") | |||
DECLARE_MESSAGE(DependencyFeatureCore, | |||
(), | |||
"The word \"core\" is an on-disk name that must not be localized. The 'default-features' part is JSON " |
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 word \"core\" is an on-disk name that must not be localized. The 'default-features' part is JSON " | |
"The word \"core\" is an on-disk name that must not be localized. The \"default-features\" part is JSON " |
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.
Or they should both use '
, but it should be made consistent either way.
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.
Heh I didn't fix all the localizer notes
src/vcpkg-test/dependencies.cpp
Outdated
@@ -1619,7 +1619,7 @@ TEST_CASE ("version install default features", "[versionplan]") | |||
|
|||
auto a_x = make_fpgh("x"); | |||
auto& a_scf = vp.emplace("a", {"1", 0}, VersionScheme::Relaxed).source_control_file; | |||
a_scf->core_paragraph->default_features.emplace_back("x"); | |||
a_scf->core_paragraph->default_features.push_back(DependencyRequestedFeature{"x"}); |
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.
a_scf->core_paragraph->default_features.push_back(DependencyRequestedFeature{"x"}); | |
a_scf->core_paragraph->default_features.push_back({"x"}); |
This seems verbose
CHECK(errors[1] == | ||
"$.registries[0].packages[2] (a package pattern): \"a*a\" is not a valid package pattern. Package patterns " | ||
"must use only one wildcard character (*) and it must be the last character in the pattern (see " | ||
"https://learn.microsoft.com/vcpkg/users/registries for more information)"); | ||
"https://learn.microsoft.com/vcpkg/users/registries for more information)."); | ||
CHECK(errors[2] == | ||
"$.registries[0].packages[3] (a package pattern): \"*a\" is not a valid package pattern. Package patterns " |
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.
(future work) It would be great to break up these extremely long lines into something like:
/in/a/file:10:5: error: "*a" is not a valid package pattern.
info: Package patterns must use only one wildcard character (*) and it must be the last character in the pattern.
see-also: https://learn.microsoft.com/vcpkg/users/registries
return DefaultFeatureNameDeserializer::instance.visit_string(r, sv).map( | ||
[](std::string&& name) { return DependencyRequestedFeature{std::move(name)}; }); |
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.
return DefaultFeatureNameDeserializer::instance.visit_string(r, sv).map( | |
[](std::string&& name) { return DependencyRequestedFeature{std::move(name)}; }); | |
return DefaultFeatureNameDeserializer::instance.visit_string(r, sv); |
& make OptionalStorage(Optional<U>)
use new (&m_t) T{std::move(*p)};
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 really don't want to do that because it will accidentally engage initializer_list ctors :/
Thank you! |
Alternative to #1211 ; this version preserves the JSON deserializer invariant that the deserialized type generally can't have semantic constraints. The intended use is that the results are only semantically valid if there were no logged errors.
Fixes microsoft/vcpkg#33973
I highly recommend looking at src/vcpkg-test/manifests.cpp first as most of these changes are nitpick fixes across parsers to get the output to look like.