Accept arbitrary version formats in overrides. #1329
Conversation
We accidentally implemented the overrides deserializer to reject non-relaxed-versions in "overrides", despite the original intent that overrides are scheme-less. Note how the original schema: microsoft#1209 , and original documentation: microsoft/vcpkg-docs#159 said everything should be accepted here. Also fix our example printing when we're asking users to add overrides to ensure it is valid JSON.
| @@ -150,7 +165,7 @@ namespace vcpkg | |||
| static constexpr StringLiteral DESCRIPTION = "Description"; | |||
| static constexpr StringLiteral FEATURE = "Feature"; | |||
| static constexpr StringLiteral MAINTAINERS = "Maintainer"; | |||
| static constexpr StringLiteral NAME = "Source"; | |||
There was a problem hiding this comment.
This took me an embarrassing amount of time to realize that NAME was not always "name".
| "type": "string", | ||
| "pattern": "^(0|[1-9]\\d*)\\.(0|[1-9]\\d*)\\.(0|[1-9]\\d*)(?:-((?:0|[1-9]\\d*|\\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\\.(?:0|[1-9]\\d*|\\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\\+([0-9a-zA-Z-]+(?:\\.[0-9a-zA-Z-]+)*))?(#\\d+)?$" | ||
| }, | ||
| "port-version": { | ||
| "description": "Deprecated in favor of being a part of 'version' in overrides. Specifies the port-version portion of the version. Overrides the value in one of the other version fields.", |
There was a problem hiding this comment.
Agreed with deprecation -- this should be the same as version>=.
|
|
||
| Json::Array example_array; | ||
| serialize_dependency_override(example_array, DependencyOverride{on.name(), baseline.version}); | ||
| doc.append_raw(Json::stringify_object_member(OVERRIDES, example_array, Json::JsonStyle::with_spaces(2), 1)); |
There was a problem hiding this comment.
Small smell that the structure of the manifest is shared between dependencies.cpp and sourceparagraph.cpp. Consider levelling up serialize_dependency_override to instead express something like "Create manifest snippet to add this override" so dependencies.cpp doesn't need to speak json at all.
This would hide OVERRIDES again.
There was a problem hiding this comment.
I'm not convinced I'm going to do a better job at abstracting this right now. I've raised the level of abstraction to at least json quote strings correctly and I think that's an improvement over the status quo.
| @@ -573,7 +587,6 @@ namespace vcpkg | |||
| { | |||
| LocalizedString type_name() const override { return msg::format(msgADependencyFeature); } | |||
|
|
|||
| constexpr static StringLiteral NAME = "name"; | |||
| constexpr static StringLiteral PLATFORM = "platform"; | |||
There was a problem hiding this comment.
Should this PLATFORM also be consolidated up?
There was a problem hiding this comment.
In the interest of making this PR be minimal I would rather do that in a separate PR. I filed #1355
…hese constants being separately declared variables is to avoid typos. By that standard, they should not be declared wherever they are used, they should be declared in one place and reused. This change does that. See also microsoft#1329 (comment)
…hese constants being separately declared variables is to avoid typos. By that standard, they should not be declared wherever they are used, they should be declared in one place and reused. This change does that. See also microsoft#1329 (comment)
…hese constants being separately declared variables is to avoid typos. By that standard, they should not be declared wherever they are used, they should be declared in one place and reused. This change does that. See also microsoft#1329 (comment) Note that the json constants in the recursive environment blob were renamed to be consistent with other JSON identifiers, but because this is our exe communicating with the same exe this is OK.
|
|
||
| r.required_object_field(type_name(), obj, m_version_field, version, version_deserializer); | ||
| r.optional_object_field(obj, PORT_VERSION, port_version, Json::NaturalNumberDeserializer::instance); | ||
| r.required_object_field(type_name(), obj, BASELINE, target.text, version_deserializer); |
There was a problem hiding this comment.
With BASELINE hardcoded here, I'd expect the type to be named more specifically than GenericVersionDeserializer. Maybe VersionTagDeserializer?
Partially reverts microsoft#159 following the tool being changed to match the *original* docs in microsoft/vcpkg-tool#1329
…282) Partially reverts #159 following the tool being changed to match the *original* docs in microsoft/vcpkg-tool#1329
We accidentally implemented the overrides deserializer to reject non-relaxed-versions in "overrides", despite the original intent that overrides are scheme-less. Note how the original schema:
#1209
, and original documentation:
microsoft/vcpkg-docs#159
said everything should be accepted here.
Also fix our example printing when we're asking users to add overrides to ensure it is valid JSON.