-
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
Make registries distinguish between 'baseline is broken' and 'baseline missing'. #1203
Make registries distinguish between 'baseline is broken' and 'baseline missing'. #1203
Conversation
97651a5
to
bfdd29e
Compare
…s-error # Conflicts: # src/vcpkg/commands.ci-verify-versions.cpp # src/vcpkg/registries.cpp
src/vcpkg/paragraphs.cpp
Outdated
ExpectedL<std::vector<Paragraph>> pghs = parse_paragraphs(StringView{text}, origin); | ||
if (auto vector_pghs = pghs.get()) | ||
{ | ||
return SourceControlFile::parse_control_file(origin, std::move(*vector_pghs)); | ||
return map_parse_expected_to_localized_string( | ||
SourceControlFile::parse_control_file(origin, std::move(*vector_pghs))); | ||
} | ||
|
||
return ParseControlErrorInfo::from_error(origin, std::move(pghs).error()); | ||
return std::move(pghs).error(); |
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.
suggestion:
return parse_paragraphs(StringView{text}, origin)
.then([origin] (std::vector<Paragraph>&& p)
{
return map_parse_expected_to_localized_string(
SourceControlFile::parse_control_file(origin, std::move(p)));
});
See also my comment at the top about a more "functional" approach to map_parse_expected_to_localized_string
, which would result in:
return parse_paragraphs(StringView{text}, origin)
.then([origin] (std::vector<Paragraph>&& p)
{
return SourceControlFile::parse_control_file(origin, std::move(p))
.map_error(ToLocalizedString);
});
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.
Hmmmm I'm not a huge fan of nested transforms like that (map_error
inside then
) but this one might work.
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 did the map_error
but I'm opposed to replacing 1 if
with double nested lambdas.
include/vcpkg/paragraphparser.h
Outdated
@@ -49,11 +49,11 @@ namespace vcpkg | |||
using ParseExpected = vcpkg::ExpectedT<std::unique_ptr<P>, std::unique_ptr<ParseControlErrorInfo>>; | |||
|
|||
template<class 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.
suggestion:
static constexpr struct ToLocalizedString_t
{
LocalizedString operator()(std::unique_ptr<ParseControlErrorInfo> p) const;
} ToLocalizedString;
Then, call sites can use this as
return maybe_parse_thing().map_error(ToLocalizedString);
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.
Done!
src/vcpkg/paragraphs.cpp
Outdated
return ParseControlErrorInfo::from_error(port_name, std::move(formatted)); | ||
return msg::format_error(msgFailedToParseManifest, msg::path = manifest_path) | ||
.append_raw("\n") | ||
.append(format_filesystem_call_error(ec, "read_contents", {manifest_path})); | ||
} | ||
|
||
if (fs.exists(control_path, IgnoreErrors{})) | ||
{ | ||
ExpectedL<std::vector<Paragraph>> pghs = get_paragraphs(fs, control_path); |
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.
Similar to other suggestions, this could be:
return get_paragraphs(fs, control_path)
.then([&control_path] (std::vector<Paragraph>&& p)
{
return SourceControlFile::parse_control_file(control_path, std::move(p))
.map_error(ToLocalizedString);
});
auto version = maybe_version->get(); | ||
if (!version) | ||
{ | ||
return msg::format_error(msgPortNotInBaseline, msg::package_name = port_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.
Should this also record the registry used for the lookup?
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 looked at doing this but I'm not sure what we would print; registries don't have names and we don't currently have a way in console output to refer to a particular one. I think it would be good to emit the error attributed to the JSON document declaring the registry someday but I think that's outside the scope of this PR.
src/vcpkg/registries.cpp
Outdated
|
||
private: | ||
const ReadOnlyFilesystem& m_fs; | ||
|
||
Path m_path; | ||
std::string m_baseline_identifier; | ||
DelayedInit<Baseline> m_baseline; | ||
DelayedInit<ExpectedL<Optional<Baseline>>> m_baseline; |
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.
Shouldn't this be just ExpectedL<Baseline>
?
src/vcpkg/registries.cpp
Outdated
auto b = load_baseline_versions(m_paths.get_filesystem(), *maybe_path.get()).value_or_exit(VCPKG_LINE_INFO); | ||
if (auto p = b.get()) | ||
|
||
auto maybe_maybe_baseline = load_baseline_versions(m_paths.get_filesystem(), *path); |
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.
Are there any users that care about the ExpectedL<>(nullopt)
case? This seems ripe for simplification to just ExpectedL<Baseline>
.
I think consumers like BuiltinGitRegistry
should be adding their context information (m_baseline_identifier
) always -- why should "file not found" be special compared to "parse error"?
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.
ExpectedL<>(nullopt)
means 'that port is not in the baseline': https://github.com/microsoft/vcpkg-tool/pull/1203/files/38f738d8100a9119d08bf640fca4d5dceaef15ee#diff-9439a305925c63597b93bb0d49db1794b5b6120f7ca5456d3e99b403c862dbf5R93-R96
For instance, load_all_control_files
ignores ports not in the baseline, but emits an error if there are errors:
Both 'file not found' and 'parse failure' are errors. But 'parse success and the port you asked for is not in there' is not an error.
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.
edit: I realized that my comment is in a poor location -- I'm talking about load_baseline_versions
(called on this line), not get_baseline_version
(the encapsulating function).
By my reading, this call is not "Get the baseline for a port" -- this is "Load the baseline file".
ExpectedT<Optional<Baseline>> load_baseline_versions(fs, path);
Agreed that for ExpectedT<Optional<Version>>
, nullopt
means "not in baseline". But for this function (load_baseline_versions
), as far as I can tell nullopt
means "The baseline.json didn't exist at all" -- which seems the same as a parse error.
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.
Ah, I see. Thanks GH for making a totally reasonable comment opaque.
It looks like this was here because some callers emitted different error messages for not exist vs. parse error. I think, in general, sometimes this can make sense; I certainly used it for load_git_versions_file
in x-ci-verify-versions
to do something different for 'the versions file is corrupted' vs. 'this port does not have a versions file yet'.
However, I can't think of obvious reasons that would want to happen for baseline things, so I've ripped this one out.
auto pghs = parse_paragraphs(StringView{text}, origin); | ||
if (auto vector_pghs = pghs.get()) | ||
{ | ||
return SourceControlFile::parse_control_file(origin, std::move(*vector_pghs)); | ||
return SourceControlFile::parse_control_file(origin, std::move(*vector_pghs)).map_error(ToLocalizedString); | ||
} | ||
|
||
return ParseControlErrorInfo::from_error(origin, std::move(pghs).error()); | ||
return std::move(pghs).error(); |
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.
auto pghs = parse_paragraphs(StringView{text}, origin); | |
if (auto vector_pghs = pghs.get()) | |
{ | |
return SourceControlFile::parse_control_file(origin, std::move(*vector_pghs)); | |
return SourceControlFile::parse_control_file(origin, std::move(*vector_pghs)).map_error(ToLocalizedString); | |
} | |
return ParseControlErrorInfo::from_error(origin, std::move(pghs).error()); | |
return std::move(pghs).error(); | |
return parse_paragraphs(text, origin).then([origin] (auto&& p) { | |
return SourceControlFile::parse_control_file(origin, std::move(p)).map_error(ToLocalizedString); | |
}); |
.map_error([&](LocalizedString&& error) { | ||
return std::move(error).append(msgWhileCheckingOutBaseline, | ||
msg::commit_sha = m_baseline_identifier); | ||
}); | ||
}); | ||
|
||
auto baseline = maybe_baseline.get(); |
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.
Below this line, in return maybe_baseline.error();
, we should probably add at least some context of "while looking up port ". That at least will let users figure out the problematic registry themselves by matching patterns.
|
||
return Optional<Version>(); | ||
return Optional<Version>(); | ||
}); |
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.
}); | |
}).map_error([port_name](LocalizedString&& err) { | |
return std::move(err).append("while finding a baseline for port {port_name}", port_name); | |
}); |
(approximately; I think reversing might be better)
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 don't think map/then is clearer when processing needs to happen to 'both arms'. I did extract a common function though rather than repeating.
(Still happy with the merge of this PR, these three were just nits that I had queued up) |
Next in the series containing #1153