Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Split functions to avoid 'is_manifest' control coupling. #1229

Merged
merged 2 commits into from
Oct 17, 2023
Merged

Split functions to avoid 'is_manifest' control coupling. #1229

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c0376eb
Split functions to avoid 'is_manifest' control coupling.
BillyONeal Oct 11, 2023
1bd12c7
Flatten control flow in try_load_port
BillyONeal Oct 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions include/vcpkg/base/message-data.inc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2500,10 +2500,10 @@ DECLARE_MESSAGE(PortNotInBaseline,
DECLARE_MESSAGE(PortsAdded, (msg::count), "", "The following {count} ports were added:")
DECLARE_MESSAGE(PortsDiffHelp, (), "", "The argument should be a branch/tag/hash to checkout.")
DECLARE_MESSAGE(PortDoesNotExist, (msg::package_name), "", "{package_name} does not exist")
DECLARE_MESSAGE(PortMissingManifest,
(msg::package_name, msg::path),
DECLARE_MESSAGE(PortMissingManifest2,
(msg::package_name),
"",
"{package_name} has no vcpkg.json or CONTROL file in {path}")
"{package_name} port manifest missing (no vcpkg.json or CONTROL file)")
DECLARE_MESSAGE(PortsNoDiff, (), "", "There were no changes in the ports between the two commits.")
DECLARE_MESSAGE(PortsRemoved, (msg::count), "", "The following {count} ports were removed:")
DECLARE_MESSAGE(PortsUpdated, (msg::count), "", "The following {count} ports were updated:")
Expand Down
11 changes: 7 additions & 4 deletions include/vcpkg/paragraphs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,13 @@ namespace vcpkg::Paragraphs
ExpectedL<std::unique_ptr<SourceControlFile>> try_load_port_required(const ReadOnlyFilesystem& fs,
StringView port_name,
const Path& port_directory);
ExpectedL<std::unique_ptr<SourceControlFile>> try_load_port_text(const std::string& text,
StringView origin,
bool is_manifest,
MessageSink& warning_sink);
ExpectedL<std::unique_ptr<SourceControlFile>> try_load_project_manifest_text(StringView text,
StringView origin,
MessageSink& warning_sink);
ExpectedL<std::unique_ptr<SourceControlFile>> try_load_port_manifest_text(StringView text,
StringView origin,
MessageSink& warning_sink);
ExpectedL<std::unique_ptr<SourceControlFile>> try_load_control_file_text(StringView text, StringView origin);

ExpectedL<BinaryControlFile> try_load_cached_package(const ReadOnlyFilesystem& fs,
const Path& package_dir,
Expand Down
4 changes: 2 additions & 2 deletions locales/messages.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1388,8 +1388,8 @@
"_PortDependencyConflict.comment": "An example of {package_name} is zlib.",
"PortDoesNotExist": "{package_name} does not exist",
"_PortDoesNotExist.comment": "An example of {package_name} is zlib.",
"PortMissingManifest": "{package_name} has no vcpkg.json or CONTROL file in {path}",
"_PortMissingManifest.comment": "An example of {package_name} is zlib. An example of {path} is /foo/bar.",
"PortMissingManifest2": "{package_name} port manifest missing (no vcpkg.json or CONTROL file)",
"_PortMissingManifest2.comment": "An example of {package_name} is zlib.",
"PortNotInBaseline": "the baseline does not contain an entry for port {package_name}",
"_PortNotInBaseline.comment": "An example of {package_name} is zlib.",
"PortSupportsField": "(supports: \"{supports_expression}\")",
Expand Down
11 changes: 7 additions & 4 deletions src/vcpkg/commands.ci-verify-versions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,9 @@ namespace
if (!maybe_file) continue;

const auto& file = maybe_file.value_or_exit(VCPKG_LINE_INFO);
auto maybe_scf =
Paragraphs::try_load_port_text(file, treeish, control_file == "vcpkg.json", stdout_sink);
auto maybe_scf = control_file == "vcpkg.json"
? Paragraphs::try_load_port_manifest_text(file, treeish, stdout_sink)
: Paragraphs::try_load_control_file_text(file, treeish);
auto scf = maybe_scf.get();
if (!scf)
{
Expand Down Expand Up @@ -306,8 +307,10 @@ namespace vcpkg
if (!manifest_exists && !control_exists)
{
msg::write_unlocalized_text_to_stdout(Color::error, fmt::format("FAIL: {}\n", port_name));
errors.emplace(
msg::format(msgPortMissingManifest, msg::package_name = port_name, msg::path = port_path));
errors.emplace(LocalizedString::from_raw(port_path)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this pattern will be common enough to warrant a

LocalizedString LocalizedString::error_in_path(StringView path) {
    return LocalizedString::from_raw(path).append_raw(": ").append(msgErrorMessage);
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be worth it but I'd rather do it as its own PR rather than expanding the scope of this one

.append_raw(": ")
.append(msgErrorMessage)
.append(msgPortMissingManifest2, msg::package_name = port_name));
continue;
}

Expand Down
119 changes: 65 additions & 54 deletions src/vcpkg/paragraphs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@

#include <tuple>

using namespace vcpkg;

static std::atomic<uint64_t> g_load_ports_stats(0);

namespace vcpkg
Expand Down Expand Up @@ -379,46 +381,45 @@ namespace vcpkg::Paragraphs
return fs.exists(maybe_directory / "CONTROL", IgnoreErrors{}) ||
fs.exists(maybe_directory / "vcpkg.json", IgnoreErrors{});
}
} // namespace vcpkg::Paragraphs

static ExpectedL<std::unique_ptr<SourceControlFile>> try_load_manifest_text(const std::string& text,
StringView origin,
MessageSink& warning_sink)
namespace
{
ExpectedL<std::unique_ptr<SourceControlFile>> try_load_any_manifest_text(
StringView text,
StringView origin,
MessageSink& warning_sink,
ParseExpected<SourceControlFile> (*do_parse)(StringView, const Json::Object&, MessageSink&))
{
auto res = Json::parse(text, origin);
if (auto val = res.get())
{
if (val->value.is_object())
{
return SourceControlFile::parse_port_manifest_object(
origin, val->value.object(VCPKG_LINE_INFO), warning_sink)
.map_error(ToLocalizedString);
}
StatsTimer timer(g_load_ports_stats);
return Json::parse_object(text, origin).then([&](Json::Object&& object) {
return do_parse(origin, std::move(object), warning_sink).map_error(ToLocalizedString);
});
}
}

return msg::format(msgJsonValueNotObject);
}
namespace vcpkg::Paragraphs
{
ExpectedL<std::unique_ptr<SourceControlFile>> try_load_project_manifest_text(StringView text,
StringView origin,
MessageSink& warning_sink)
{
return try_load_any_manifest_text(text, origin, warning_sink, SourceControlFile::parse_project_manifest_object);
}

return LocalizedString::from_raw(res.error()->to_string());
ExpectedL<std::unique_ptr<SourceControlFile>> try_load_port_manifest_text(StringView text,
StringView origin,
MessageSink& warning_sink)
{
return try_load_any_manifest_text(text, origin, warning_sink, SourceControlFile::parse_port_manifest_object);
}

ExpectedL<std::unique_ptr<SourceControlFile>> try_load_port_text(const std::string& text,
StringView origin,
bool is_manifest,
MessageSink& warning_sink)
ExpectedL<std::unique_ptr<SourceControlFile>> try_load_control_file_text(StringView text, StringView origin)
{
StatsTimer timer(g_load_ports_stats);

if (is_manifest)
{
return try_load_manifest_text(text, origin, warning_sink);
}

auto pghs = parse_paragraphs(StringView{text}, origin);
if (auto vector_pghs = pghs.get())
{
return SourceControlFile::parse_control_file(origin, std::move(*vector_pghs)).map_error(ToLocalizedString);
}

return std::move(pghs).error();
return parse_paragraphs(text, origin).then([&](std::vector<Paragraph>&& vector_pghs) {
return SourceControlFile::parse_control_file(origin, std::move(vector_pghs)).map_error(ToLocalizedString);
});
}

ExpectedL<std::unique_ptr<SourceControlFile>> try_load_port(const ReadOnlyFilesystem& fs,
Expand All @@ -431,39 +432,49 @@ namespace vcpkg::Paragraphs
const auto control_path = port_directory / "CONTROL";
std::error_code ec;
auto manifest_contents = fs.read_contents(manifest_path, ec);
Copy link
Contributor

@ras0219-msft ras0219-msft Oct 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this function should be able to be written with less nesting, perhaps along the lines of:

std::error_code ec_manifest, ec_control;
auto content_manifest = fs.read_contents(path_manifest, ec_manifest);
const auto exists_manifest = !ec_manifest;
const auto failed_manifest = ec_manifest && ec_manifest != std::errc::no_such_file_or_directory;
auto content_control = fs.read_contents(path_control, ec_control);
const auto exists_control = !ec_control;
const auto failed_control = ec_control && ec_control != std::errc::no_such_file_or_directory;
if (failed_manifest) {
  // msgFailedToAccessManifest(path_manifest)
} else if (failed_control) {
  // msgFailedToAccessManifest(path_control)
} else if (exists_manifest && exists_control) {
  // msgManifestConflict(port_directory)
} else if (exists_manifest) {
  // success on manifest
} else if (exists_control) {
  // success on control
} else {
  // msgPortMissingManifest2(port_directory)
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this transformation as it does a read_contents rather than an exists check on the CONTROL in the case we know loading the json succeeded. I inverted the ec tests and that flattened the result substantially, does that resolve your hearburn?

if (ec)
if (!ec)
{
const auto exists = ec != std::errc::no_such_file_or_directory;
if (exists)
{
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{}))
{
return get_paragraphs(fs, control_path).then([&](std::vector<Paragraph>&& vector_pghs) {
return SourceControlFile::parse_control_file(control_path, std::move(vector_pghs))
.map_error(ToLocalizedString);
});
return msg::format_error(msgManifestConflict, msg::path = port_directory);
}

if (fs.exists(port_directory, IgnoreErrors{}))
{
return msg::format_error(
msgPortMissingManifest, msg::package_name = port_name, msg::path = port_directory);
}
return try_load_port_manifest_text(manifest_contents, manifest_path, stdout_sink);
}

auto manifest_exists = ec != std::errc::no_such_file_or_directory;
if (manifest_exists)
{
return msg::format_error(msgFailedToParseManifest, msg::path = manifest_path)
.append_raw("\n")
.append(format_filesystem_call_error(ec, "read_contents", {manifest_path}));
}

return std::unique_ptr<SourceControlFile>();
auto control_contents = fs.read_contents(control_path, ec);
if (!ec)
{
return try_load_control_file_text(control_contents, control_path);
}

if (ec != std::errc::no_such_file_or_directory)
{
return LocalizedString::from_raw(port_directory)
.append_raw(": ")
.append(format_filesystem_call_error(ec, "read_contents", {control_path}));
}

if (fs.exists(control_path, IgnoreErrors{}))
if (fs.exists(port_directory, IgnoreErrors{}))
{
return msg::format_error(msgManifestConflict, msg::path = port_directory);
return LocalizedString::from_raw(port_directory)
.append_raw(": ")
.append(msgErrorMessage)
.append(msgPortMissingManifest2, msg::package_name = port_name);
}

return try_load_manifest_text(manifest_contents, manifest_path, stdout_sink);
return LocalizedString::from_raw(port_directory)
.append_raw(": ")
.append(msgErrorMessage)
.append(msgPortDoesNotExist, msg::package_name = port_name);
}

ExpectedL<std::unique_ptr<SourceControlFile>> try_load_port_required(const ReadOnlyFilesystem& fs,
Expand Down