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.

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

Ensure spdx_location is not dropped when loading ports. #1237

Merged
merged 8 commits into from
Nov 10, 2023
7 changes: 7 additions & 0 deletions include/vcpkg/fwd/sourceparagraph.h
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,13 @@
namespace vcpkg
{
struct ManifestAndPath;
struct DependencyConstraint;
struct DependencyRequestedFeature;
struct Dependency;
struct DependencyOverride;
struct FeatureParagraph;
struct SourceParagraph;
struct PortLocation;
struct SourceControlFile;
struct SourceControlFileAndLocation;
}
23 changes: 10 additions & 13 deletions include/vcpkg/paragraphs.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,22 +27,19 @@ namespace vcpkg::Paragraphs
bool is_port_directory(const ReadOnlyFilesystem& fs, const Path& maybe_directory);

// If an error occurs, the Expected will be in the error state.
// Otherwise, if the port is known, the unique_ptr contains the loaded port information.
// Otherwise, the unique_ptr is nullptr.
ExpectedL<std::unique_ptr<SourceControlFile>> try_load_port(const ReadOnlyFilesystem& fs,
StringView port_name,
const Path& port_directory);
// Otherwise, if the port is known, result->source_control_file contains the loaded port information.
// Otherwise, result->source_control_file is nullptr.
ExpectedL<SourceControlFileAndLocation> try_load_port(const ReadOnlyFilesystem& fs,
StringView port_name,
const PortLocation& port_location);
// Identical to try_load_port, but the port unknown condition is mapped to an error.
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_project_manifest_text(StringView text,
StringView origin,
MessageSink& warning_sink);
ExpectedL<SourceControlFileAndLocation> try_load_port_required(const ReadOnlyFilesystem& fs,
StringView port_name,
const PortLocation& port_location);
ExpectedL<std::unique_ptr<SourceControlFile>> try_load_port_manifest_text(StringView text,
StringView origin,
StringView control_path,
MessageSink& warning_sink);
ExpectedL<std::unique_ptr<SourceControlFile>> try_load_control_file_text(StringView text, StringView origin);
ExpectedL<std::unique_ptr<SourceControlFile>> try_load_control_file_text(StringView text, StringView control_path);

ExpectedL<BinaryControlFile> try_load_cached_package(const ReadOnlyFilesystem& fs,
const Path& package_dir,
Expand Down
13 changes: 2 additions & 11 deletions include/vcpkg/registries.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

#include <vcpkg/fwd/configuration.h>
#include <vcpkg/fwd/registries.h>
#include <vcpkg/fwd/sourceparagraph.h>
#include <vcpkg/fwd/vcpkgpaths.h>

#include <vcpkg/base/expected.h>
Expand All @@ -18,7 +19,6 @@
#include <map>
#include <memory>
#include <string>
#include <system_error>
#include <vector>

namespace vcpkg
Expand Down Expand Up @@ -54,20 +54,11 @@ namespace vcpkg
bool modified = false;
};

struct PathAndLocation
{
Path path;

/// Should model SPDX PackageDownloadLocation. Empty implies NOASSERTION.
/// See https://spdx.github.io/spdx-spec/package-information/#77-package-download-location-field
std::string location;
};

struct RegistryEntry
{
virtual ExpectedL<View<Version>> get_port_versions() const = 0;

virtual ExpectedL<PathAndLocation> get_version(const Version& version) const = 0;
virtual ExpectedL<PortLocation> get_version(const Version& version) const = 0;

virtual ~RegistryEntry() = default;
};
Expand Down
17 changes: 14 additions & 3 deletions include/vcpkg/sourceparagraph.h
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,15 @@ namespace vcpkg
friend bool operator!=(const SourceParagraph& lhs, const SourceParagraph& rhs) { return !(lhs == rhs); }
};

struct PortLocation
{
Path port_directory;

/// Should model SPDX PackageDownloadLocation. Empty implies NOASSERTION.
/// See https://spdx.github.io/spdx-spec/package-information/#77-package-download-location-field
std::string spdx_location;
};

/// <summary>
/// Full metadata of a package: core and other features.
/// </summary>
Expand Down Expand Up @@ -197,12 +206,14 @@ namespace vcpkg
Version to_version() const { return source_control_file->to_version(); }
VersionScheme scheme() const { return source_control_file->core_paragraph->version_scheme; }
SchemedVersion schemed_version() const { return {scheme(), to_version()}; }
Path port_directory() const { return control_path.parent_path(); }

std::unique_ptr<SourceControlFile> source_control_file;
Path source_location;
Path control_path;

/// Should model SPDX PackageDownloadLocation. Empty implies NOASSERTION.
/// See https://spdx.github.io/spdx-spec/package-information/#77-package-download-location-field
std::string registry_location;
std::string spdx_location;
};

void print_error_message(const LocalizedString& message);
Expand All @@ -212,6 +223,6 @@ namespace vcpkg

// Exposed for testing
ExpectedL<std::vector<Dependency>> parse_dependencies_list(const std::string& str,
StringView origin = "<unknown>",
StringView origin,
TextRowCol textrowcol = {});
}
2 changes: 1 addition & 1 deletion src/vcpkg-test/spdx.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ TEST_CASE ("spdx maximum serialization", "[spdx]")
{
PackageSpec spec{"zlib", Test::ARM_UWP};
SourceControlFileAndLocation scfl;
scfl.registry_location = "git://some-vcs-url";
scfl.spdx_location = "git://some-vcs-url";
auto& scf = *(scfl.source_control_file = std::make_unique<SourceControlFile>());
auto& cpgh = *(scf.core_paragraph = std::make_unique<SourceParagraph>());
cpgh.name = "zlib";
Expand Down
7 changes: 4 additions & 3 deletions src/vcpkg-test/versionplan.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ using namespace vcpkg;

TEST_CASE ("parse depends", "[dependencies]")
{
auto w = parse_dependencies_list("liba (windows)");
auto w = parse_dependencies_list("liba (windows)", "<test>");
REQUIRE(w);
auto& v = *w.get();
REQUIRE(v.size() == 1);
Expand All @@ -31,7 +31,7 @@ TEST_CASE ("filter depends", "[dependencies]")

const std::unordered_map<std::string, std::string> arm_uwp_cmake_vars{{"VCPKG_TARGET_ARCHITECTURE", "arm"},
{"VCPKG_CMAKE_SYSTEM_NAME", "WindowsStore"}};
auto deps_ = parse_dependencies_list("liba (!uwp), libb, libc (uwp)");
auto deps_ = parse_dependencies_list("liba (!uwp), libb, libc (uwp)", "<test>");
REQUIRE(deps_);
auto& deps = *deps_.get();
SECTION ("x64-windows")
Expand All @@ -58,7 +58,8 @@ TEST_CASE ("filter depends", "[dependencies]")
TEST_CASE ("parse feature depends", "[dependencies]")
{
auto u_ = parse_dependencies_list("libwebp[anim, gif2webp, img2webp, info, mux, nearlossless, "
"simd, cwebp, dwebp], libwebp[vwebp-sdl, extras] (!osx)");
"simd, cwebp, dwebp], libwebp[vwebp-sdl, extras] (!osx)",
"<test>");
REQUIRE(u_);
auto& v = *u_.get();
REQUIRE(v.size() == 2);
Expand Down
2 changes: 1 addition & 1 deletion src/vcpkg/cmakevars.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -367,7 +367,7 @@ endfunction()
for (const auto& install_action : action_plan.install_actions)
{
auto& scfl = install_action.source_control_file_and_location.value_or_exit(VCPKG_LINE_INFO);
const auto override_path = scfl.source_location / "vcpkg-abi-settings.cmake";
const auto override_path = scfl.port_directory() / "vcpkg-abi-settings.cmake";
spec_abi_settings.emplace_back(FullPackageSpec{install_action.spec, install_action.feature_list},
override_path.generic_u8string());
}
Expand Down
26 changes: 9 additions & 17 deletions src/vcpkg/commands.add-version.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,7 @@ namespace vcpkg
auto maybe_git_tree_map = paths.git_get_local_port_treeish_map();
auto& git_tree_map = maybe_git_tree_map.value_or_exit(VCPKG_LINE_INFO);

// Find ports with uncommited changes
// Find ports with uncommitted changes
std::set<std::string> changed_ports;
auto git_config = paths.git_builtin_config();
auto maybe_changes = git_ports_with_uncommitted_changes(git_config);
Expand All @@ -405,15 +405,8 @@ namespace vcpkg
{
auto port_dir = paths.builtin_ports_directory() / port_name;

if (!fs.exists(port_dir, IgnoreErrors{}))
{
msg::println_error(msgPortDoesNotExist, msg::package_name = port_name);
Checks::check_exit(VCPKG_LINE_INFO, !add_all);
continue;
}

auto maybe_scf =
Paragraphs::try_load_port_required(fs, port_name, paths.builtin_ports_directory() / port_name);
auto maybe_scf = Paragraphs::try_load_port_required(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
auto maybe_scf = Paragraphs::try_load_port_required(
auto maybe_scfl = Paragraphs::try_load_port_required(

scf == SourceControlFile

fs, port_name, PortLocation{paths.builtin_ports_directory() / port_name});
auto scf = maybe_scf.get();
if (!scf)
{
Expand All @@ -426,15 +419,15 @@ namespace vcpkg
if (!skip_formatting_check)
{
// check if manifest file is property formatted
const auto path_to_manifest = paths.builtin_ports_directory() / port_name / "vcpkg.json";
if (fs.exists(path_to_manifest, IgnoreErrors{}))

if (scf->control_path.filename() == "vcpkg.json")
{
const auto current_file_content = fs.read_contents(path_to_manifest, VCPKG_LINE_INFO);
const auto json = serialize_manifest(**scf);
const auto current_file_content = fs.read_contents(scf->control_path, VCPKG_LINE_INFO);
const auto json = serialize_manifest(*scf->source_control_file);
const auto formatted_content = Json::stringify(json);
if (current_file_content != formatted_content)
{
auto command_line = fmt::format("vcpkg format-manifest ports/{}/vcpkg.json", port_name);
auto command_line = fmt::format("vcpkg format-manifest {}", scf->control_path);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be shell escaped? For example, if the user has a space in their path.

msg::println_error(
msg::format(msgAddVersionPortHasImproperFormat, msg::package_name = port_name)
.append_raw('\n')
Expand All @@ -454,8 +447,7 @@ namespace vcpkg
msg::println_warning(msgAddVersionUncommittedChanges, msg::package_name = port_name);
}

const auto& schemed_version = (*scf)->to_schemed_version();

auto schemed_version = scf->source_control_file->to_schemed_version();
auto git_tree_it = git_tree_map.find(port_name);
if (git_tree_it == git_tree_map.end())
{
Expand Down
2 changes: 1 addition & 1 deletion src/vcpkg/commands.autocomplete.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ namespace vcpkg
StringView triplet_prefix{colon + 1, last_arg.end()};
// TODO: Support autocomplete for ports in --overlay-ports
auto maybe_port = Paragraphs::try_load_port_required(
paths.get_filesystem(), port_name, paths.builtin_ports_directory() / port_name);
paths.get_filesystem(), port_name, PortLocation{paths.builtin_ports_directory() / port_name});
BillyONeal marked this conversation as resolved.
Show resolved Hide resolved
if (!maybe_port)
{
Checks::exit_success(VCPKG_LINE_INFO);
Expand Down
14 changes: 7 additions & 7 deletions src/vcpkg/commands.build.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -760,7 +760,7 @@ namespace vcpkg

std::vector<CMakeVariable> variables{
{"ALL_FEATURES", all_features},
{"CURRENT_PORT_DIR", scfl.source_location},
{"CURRENT_PORT_DIR", scfl.port_directory()},
{"_HOST_TRIPLET", action.host_triplet.canonical_name()},
{"FEATURES", Strings::join(";", action.feature_list)},
{"PORT", scf.core_paragraph->name},
Expand Down Expand Up @@ -951,9 +951,9 @@ namespace vcpkg
msg::println(msgLoadingOverlayTriplet, msg::path = triplet_file_path);
}

if (!Strings::starts_with(scfl.source_location, paths.builtin_ports_directory()))
if (!Strings::starts_with(scfl.control_path, paths.builtin_ports_directory()))
{
msg::println(msgInstallingFromLocation, msg::path = scfl.source_location);
msg::println(msgInstallingFromLocation, msg::path = scfl.control_path);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be port_directory() or control_path?

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 did intend this change but I guess technically it's also referring to patches and stuff in there.

}

const ElapsedTimer timer;
Expand Down Expand Up @@ -1030,7 +1030,7 @@ namespace vcpkg
FileSink file_sink{fs, stdoutlog, Append::YES};
CombiningSink combo_sink{stdout_sink, file_sink};
error_count = perform_post_build_lint_checks(
action.spec, paths, pre_build_info, build_info, scfl.source_location, combo_sink);
action.spec, paths, pre_build_info, build_info, scfl.port_directory(), combo_sink);
};
if (error_count != 0 && action.build_options.backcompat_features == BackcompatFeatures::PROHIBIT)
{
Expand Down Expand Up @@ -1174,7 +1174,8 @@ namespace vcpkg
constexpr int max_port_file_count = 100;

std::string portfile_cmake_contents;
auto&& port_dir = action.source_control_file_and_location.value_or_exit(VCPKG_LINE_INFO).source_location;
auto&& scfl = action.source_control_file_and_location.value_or_exit(VCPKG_LINE_INFO);
auto port_dir = scfl.port_directory();
auto raw_files = fs.get_regular_files_recursive_lexically_proximate(port_dir, VCPKG_LINE_INFO);
if (raw_files.size() > max_port_file_count)
{
Expand Down Expand Up @@ -1273,8 +1274,7 @@ namespace vcpkg
abi_file_path /= triplet_canonical_name + ".vcpkg_abi_info.txt";
fs.write_contents(abi_file_path, full_abi_info, VCPKG_LINE_INFO);

auto& scf = action.source_control_file_and_location.value_or_exit(VCPKG_LINE_INFO).source_control_file;

auto& scf = scfl.source_control_file;
abi_info.package_abi = Hash::get_string_sha256(full_abi_info);
abi_info.abi_tag_file.emplace(std::move(abi_file_path));
abi_info.relative_port_files = std::move(files);
Expand Down
13 changes: 7 additions & 6 deletions src/vcpkg/commands.ci-verify-versions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ namespace
expected_right_tag};
}

auto&& git_tree_version = (*scf)->to_schemed_version();
auto&& git_tree_version = (**scf).to_schemed_version();
if (version_entry.version.version != git_tree_version.version)
{
return {
Expand Down Expand Up @@ -124,17 +124,18 @@ namespace
}
}

auto maybe_scf = Paragraphs::try_load_port_required(paths.get_filesystem(), port_name, port_path);
auto scf = maybe_scf.get();
if (!scf)
auto maybe_scfl =
Paragraphs::try_load_port_required(paths.get_filesystem(), port_name, PortLocation{port_path});
auto scfl = maybe_scfl.get();
if (!scfl)
{
return {msg::format_error(msgWhileLoadingLocalPort, msg::package_name = port_name)
BillyONeal marked this conversation as resolved.
Show resolved Hide resolved
.append_raw('\n')
.append(maybe_scf.error()),
.append(maybe_scfl.error()),
expected_right_tag};
}

const auto local_port_version = (*scf)->to_schemed_version();
const auto local_port_version = scfl->source_control_file->to_schemed_version();

auto versions_end = versions->end();
auto it = std::find_if(versions->begin(), versions_end, [&](const GitVersionDbEntry& entry) {
Expand Down
4 changes: 2 additions & 2 deletions src/vcpkg/dependencies.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -445,9 +445,9 @@ namespace vcpkg
if (auto scfl = action.source_control_file_and_location.get())
{
if (!builtin_ports_dir.empty() &&
!Strings::case_insensitive_ascii_starts_with(scfl->source_location, builtin_ports_dir))
!Strings::case_insensitive_ascii_starts_with(scfl->control_path, builtin_ports_dir))
{
out.append_raw(" -- ").append_raw(scfl->source_location);
out.append_raw(" -- ").append_raw(scfl->control_path);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be control_path or port_directory()?

}
}
}
Expand Down
Loading