Ensure spdx_location is not dropped when loading ports. #1237

BillyONeal · 2023-10-18T04:19:01Z

The next extraction from #1210

Change try_load_port and friends to accept the spdx location and return SourceControlFileAndLocation rather than requiring all callers to staple that on.
PathAndLocation => PortLocation
Fix confusion in SourceControlFileAndLocation due to some parts of the codebase using source_location as the port_directory.
Rename all "registry_location"s to "spdx_location" to avoid it sounding like this is the spdx location of the registry.
Require origin to be provided in all the places.

1. Change try_load_port and friends to accept the spdx location and return SourceControlFileAndLocation rather than requiring all callers to staple that on. 2. PathAndLocation => PortLocation 3. Fix confusion in SourceControlFileAndLocation due to some parts of the codebase using source_location as the port_directory. 4. Rename all "registry_location"s to "spdx_location" to avoid it sounding like this is the spdx location of the registry. 5. Require origin to be provided in all the places.

src/vcpkg/paragraphs.cpp

include/vcpkg/sourceparagraph.h

include/vcpkg/paragraphs.h

src/vcpkg/paragraphs.cpp

# Conflicts: # src/vcpkg/portfileprovider.cpp # src/vcpkg/spdx.cpp

ras0219-msft · 2023-10-31T23:52:34Z

src/vcpkg/commands.add-version.cpp

-
-            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(


Suggested change

auto maybe_scf = Paragraphs::try_load_port_required(

auto maybe_scfl = Paragraphs::try_load_port_required(

scf == SourceControlFile

ras0219-msft · 2023-11-02T00:24:43Z

src/vcpkg/commands.add-version.cpp

                    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);


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

ras0219-msft · 2023-11-02T00:27:12Z

src/vcpkg/commands.build.cpp

        {
-            msg::println(msgInstallingFromLocation, msg::path = scfl.source_location);
+            msg::println(msgInstallingFromLocation, msg::path = scfl.control_path);


Should this be port_directory() or control_path?

I did intend this change but I guess technically it's also referring to patches and stuff in there.

src/vcpkg/commands.ci-verify-versions.cpp

ras0219-msft · 2023-11-02T00:32:37Z

src/vcpkg/paragraphs.cpp

+        const auto manifest_path = port_location.port_directory / "vcpkg.json";
+        const auto control_path = port_location.port_directory / "CONTROL";


Should these be non-const so they can be moved into the return?

ras0219-msft · 2023-11-08T20:52:15Z

src/vcpkg/paragraphs.cpp

@@ -549,20 +537,16 @@ namespace vcpkg::Paragraphs
            auto maybe_port_location = (*port_entry)->get_version(*baseline_version);
            const auto port_location = maybe_port_location.get();


remove const so this can be moved into try_load_port_required?

This is a pointer so there's no benefit to be had for moving it. Also, try_load_port_required accepts by const& because the data in question is normally owned by the registry...

ras0219-msft · 2023-11-08T20:56:11Z

src/vcpkg/portfileprovider.cpp

                        {
-                            auto scf_vspec = scf->get()->to_version_spec();
+                            auto scf_vspec = scfl->source_control_file.get()->to_version_spec();


Suggested change

auto scf_vspec = scfl->source_control_file.get()->to_version_spec();

auto scf_vspec = scfl->source_control_file->to_version_spec();

Additionally, consider lifting to_version_spec() up to SourceControlFileAndLocation.

ras0219-msft · 2023-11-08T20:59:41Z

src/vcpkg/portfileprovider.cpp

+                    auto port_name = scfl.source_control_file->core_paragraph->name;
+                    auto it = m_control_cache.emplace(VersionSpec{port_name, scfl.to_version()}, std::move(scfl)).first;
+                    out.emplace(std::move(port_name), &it->second.value_or_exit(VCPKG_LINE_INFO));


This is calling scfl.to_version() while also moving from scfl before the next sequence point.

Suggested change

auto port_name = scfl.source_control_file->core_paragraph->name;

auto it = m_control_cache.emplace(VersionSpec{port_name, scfl.to_version()}, std::move(scfl)).first;

out.emplace(std::move(port_name), &it->second.value_or_exit(VCPKG_LINE_INFO));

auto vspec = scfl.source_control_file->to_version_spec();

auto it = m_control_cache.emplace(std::move(vspec), std::move(scfl)).first;

out.emplace(it->first.port_name, &it->second.value_or_exit(VCPKG_LINE_INFO));

ras0219-msft · 2023-11-08T21:02:02Z

src/vcpkg/portfileprovider.cpp

                        {
-                            auto& scf = *scfp;
-                            if (scf->core_paragraph->name == port_name)
+                            if (scfl->source_control_file->core_paragraph->name == port_name)


It would be very nice to expose .name() on SCF/SCFL

ras0219-msft · 2023-11-08T21:03:34Z

src/vcpkg/portfileprovider.cpp

                    if (Paragraphs::is_port_directory(m_fs, ports_spec))
                    {
-                        auto found_scf = Paragraphs::try_load_port_required(m_fs, port_name, ports_spec);
-                        if (auto scfp = found_scf.get())
+                        auto found_scfl = Paragraphs::try_load_port_required(m_fs, port_name, PortLocation{ports_spec});


Is there some duplicate work between is_port_directory and try_load_port_required that can be removed without much contortion?

Yes, but not in this extraction :)

BillyONeal commented Oct 18, 2023

View reviewed changes

src/vcpkg/paragraphs.cpp Outdated Show resolved Hide resolved

BillyONeal added 2 commits October 26, 2023 13:55

Merge remote-tracking branch 'origin/main' into spdx_location

dc0110e

Persist the correct path.

a18997b

BillyONeal commented Oct 26, 2023

View reviewed changes

include/vcpkg/sourceparagraph.h Outdated Show resolved Hide resolved

BillyONeal commented Oct 26, 2023

View reviewed changes

include/vcpkg/paragraphs.h Outdated Show resolved Hide resolved

BillyONeal commented Oct 26, 2023

View reviewed changes

src/vcpkg/paragraphs.cpp Outdated Show resolved Hide resolved

BillyONeal added 3 commits October 27, 2023 19:44

Code review feedback from 2023-10-26.

6e794ec

Merge remote-tracking branch 'origin/main' into spdx_location

a922e76

Merge remote-tracking branch 'origin/main' into spdx_location

4434e81

# Conflicts: # src/vcpkg/portfileprovider.cpp # src/vcpkg/spdx.cpp

ras0219-msft approved these changes Nov 8, 2023

View reviewed changes

BillyONeal added 2 commits November 9, 2023 16:52

Merge remote-tracking branch 'origin/main' into spdx_location

a869a44

CR feedback from Robert

51150c8

BillyONeal merged commit 365d419 into microsoft:main Nov 10, 2023
5 checks passed

BillyONeal deleted the spdx_location branch November 10, 2023 23:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure spdx_location is not dropped when loading ports. #1237

Ensure spdx_location is not dropped when loading ports. #1237

BillyONeal commented Oct 18, 2023

ras0219-msft Oct 31, 2023

ras0219-msft Nov 2, 2023

ras0219-msft Nov 2, 2023

BillyONeal Nov 10, 2023

ras0219-msft Nov 2, 2023

ras0219-msft Nov 8, 2023

BillyONeal Nov 10, 2023

ras0219-msft Nov 8, 2023

ras0219-msft Nov 8, 2023

ras0219-msft Nov 8, 2023

ras0219-msft Nov 8, 2023

BillyONeal Nov 8, 2023

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

		const auto manifest_path = port_location.port_directory / "vcpkg.json";
		const auto control_path = port_location.port_directory / "CONTROL";

		@@ -549,20 +537,16 @@ namespace vcpkg::Paragraphs
		auto maybe_port_location = (port_entry)->get_version(baseline_version);
		const auto port_location = maybe_port_location.get();

	auto scf_vspec = scfl->source_control_file.get()->to_version_spec();
	auto scf_vspec = scfl->source_control_file->to_version_spec();

Ensure spdx_location is not dropped when loading ports. #1237

Ensure spdx_location is not dropped when loading ports. #1237

Conversation

BillyONeal commented Oct 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment