Move merging original_cwd into overlay directories into VcpkgPaths. (#1522)

This work is part of resolving https://github.com/microsoft/vcpkg/issues/30942 / https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1979597

When overlay directories from the config were added in https://github.com/microsoft/vcpkg-tool/pull/743 this added a condition where VcpkgPaths took responsibility to tack on the relative path, transforming those paths that come from the config into absolute paths. I did not realize at the time that this block was repeating machinery already present in IOverlayProvider family. The difference is that, there, it always assumed the prefix would be original_cwd.

As part of adding overlay-port-dirs, I needed to add the same kind of prefix handling as overlay-ports get, thus pointing this out to me. It doesn't make sense to keep two independent ways to do this, leaving two options:

* Move the prefix stapling into VcpkgPaths (as done in this PR)
* Move the prefix stapling related to configs down into OverlayPortProviderImpl

I chose to move into VcpkgPaths for several reasons:

* Plumbing the information about how to handle config paths correctly into OverlayPortProviderImpl would have added many many function parameters to large parts of the product that are only passing paths information around.
* The decision to chdir in VcpkgPaths is a consideration *it* makes, and OverlayPortProviderImpl would be just as happy if the relative paths were left alone. (And in fact I think the right behavior in the future would be to remove this harmful chdir but that is not proposed right now)
* Despite making VcpkgPaths do "more work" here, I actually argue that it makes the "VcpkgPaths big ball of mud" problem better, by removing parts of the code that need to consider original_cwd.

include/vcpkg/commands.find.h

@@ -1,5 +1,6 @@
 #pragma once
 
+#include <vcpkg/base/fwd/files.h>
 #include <vcpkg/base/fwd/optional.h>
 #include <vcpkg/base/fwd/stringview.h>
 
@@ -14,7 +15,7 @@ namespace vcpkg
                                     bool full_description,
                                     bool enable_json,
                                     Optional<StringView> filter,
-                                    View<std::string> overlay_ports);
+                                    View<Path> overlay_ports);
     extern const CommandMetadata CommandFindMetadata;
     void command_find_and_exit(const VcpkgCmdArguments& args, const VcpkgPaths& paths);
 }

include/vcpkg/portfileprovider.h

@@ -76,12 +76,9 @@ namespace vcpkg
 
     std::unique_ptr<IBaselineProvider> make_baseline_provider(const RegistrySet& registry_set);
     std::unique_ptr<IFullVersionedPortfileProvider> make_versioned_portfile_provider(const RegistrySet& registry_set);
-    std::unique_ptr<IFullOverlayProvider> make_overlay_provider(const ReadOnlyFilesystem& fs,
-                                                                const Path& original_cwd,
-                                                                View<std::string> overlay_ports);
+    std::unique_ptr<IFullOverlayProvider> make_overlay_provider(const ReadOnlyFilesystem& fs, View<Path> overlay_ports);
     std::unique_ptr<IOverlayProvider> make_manifest_provider(const ReadOnlyFilesystem& fs,
-                                                             const Path& original_cwd,
-                                                             View<std::string> overlay_ports,
+                                                             View<Path> overlay_ports,
                                                              const Path& manifest_path,
                                                              std::unique_ptr<SourceControlFile>&& manifest_scf);
 }

include/vcpkg/vcpkgpaths.h

@@ -98,10 +98,10 @@ namespace vcpkg
     private:
         const Path triplets;
         const Path community_triplets;
-        std::vector<std::string> overlay_triplets;
+        std::vector<Path> overlay_triplets;
 
     public:
-        std::vector<std::string> overlay_ports;
+        std::vector<Path> overlay_ports;
 
         std::string get_toolver_diagnostics() const;
 

src/vcpkg/commands.build-external.cpp

@@ -47,7 +47,7 @@ namespace vcpkg
 
         auto& fs = paths.get_filesystem();
         auto registry_set = paths.make_registry_set();
-        PathsPortFileProvider provider(*registry_set, make_overlay_provider(fs, paths.original_cwd, overlays));
+        PathsPortFileProvider provider(*registry_set, make_overlay_provider(fs, overlays));
         command_build_and_exit_ex(args, paths, host_triplet, build_options, spec, provider, null_build_logs_recorder());
     }
 }

src/vcpkg/commands.build.cpp

@@ -106,8 +106,7 @@ namespace vcpkg
 
         auto& fs = paths.get_filesystem();
         auto registry_set = paths.make_registry_set();
-        PathsPortFileProvider provider(*registry_set,
-                                       make_overlay_provider(fs, paths.original_cwd, paths.overlay_ports));
+        PathsPortFileProvider provider(*registry_set, make_overlay_provider(fs, paths.overlay_ports));
         Checks::exit_with_code(VCPKG_LINE_INFO,
                                command_build_ex(args,
                                                 paths,

src/vcpkg/commands.check-support.cpp

@@ -128,8 +128,7 @@ namespace vcpkg
 
         auto& fs = paths.get_filesystem();
         auto registry_set = paths.make_registry_set();
-        PathsPortFileProvider provider(*registry_set,
-                                       make_overlay_provider(fs, paths.original_cwd, paths.overlay_ports));
+        PathsPortFileProvider provider(*registry_set, make_overlay_provider(fs, paths.overlay_ports));
         auto cmake_vars = CMakeVars::make_triplet_cmake_var_provider(paths);
 
         // for each spec in the user-requested specs, check all dependencies

src/vcpkg/commands.ci.cpp

@@ -375,8 +375,7 @@ namespace vcpkg
             build_logs_recorder_storage ? *(build_logs_recorder_storage.get()) : null_build_logs_recorder();
 
         auto registry_set = paths.make_registry_set();
-        PathsPortFileProvider provider(*registry_set,
-                                       make_overlay_provider(filesystem, paths.original_cwd, paths.overlay_ports));
+        PathsPortFileProvider provider(*registry_set, make_overlay_provider(filesystem, paths.overlay_ports));
         auto var_provider_storage = CMakeVars::make_triplet_cmake_var_provider(paths);
         auto& var_provider = *var_provider_storage;
 

src/vcpkg/commands.depend-info.cpp

@@ -409,8 +409,7 @@ namespace vcpkg
 
         auto& fs = paths.get_filesystem();
         auto registry_set = paths.make_registry_set();
-        PathsPortFileProvider provider(*registry_set,
-                                       make_overlay_provider(fs, paths.original_cwd, paths.overlay_ports));
+        PathsPortFileProvider provider(*registry_set, make_overlay_provider(fs, paths.overlay_ports));
         auto var_provider_storage = CMakeVars::make_triplet_cmake_var_provider(paths);
         auto& var_provider = *var_provider_storage;
 

src/vcpkg/commands.env.cpp

@@ -80,8 +80,7 @@ namespace vcpkg
         const ParsedArguments options = args.parse_arguments(CommandEnvMetadata);
 
         auto registry_set = paths.make_registry_set();
-        PathsPortFileProvider provider(*registry_set,
-                                       make_overlay_provider(fs, paths.original_cwd, paths.overlay_ports));
+        PathsPortFileProvider provider(*registry_set, make_overlay_provider(fs, paths.overlay_ports));
         auto var_provider_storage = CMakeVars::make_triplet_cmake_var_provider(paths);
         auto& var_provider = *var_provider_storage;
 

src/vcpkg/commands.export.cpp

@@ -602,8 +602,7 @@ namespace vcpkg
         // Load ports from ports dirs
         auto& fs = paths.get_filesystem();
         auto registry_set = paths.make_registry_set();
-        PathsPortFileProvider provider(*registry_set,
-                                       make_overlay_provider(fs, paths.original_cwd, paths.overlay_ports));
+        PathsPortFileProvider provider(*registry_set, make_overlay_provider(fs, paths.overlay_ports));
 
         // create the plan
         std::vector<ExportPlanAction> export_plan = create_export_plan(opts.specs, status_db);

src/vcpkg/commands.find.cpp

@@ -133,12 +133,12 @@ namespace vcpkg
                                     bool full_description,
                                     bool enable_json,
                                     Optional<StringView> filter,
-                                    View<std::string> overlay_ports)
+                                    View<Path> overlay_ports)
     {
         Checks::check_exit(VCPKG_LINE_INFO, msg::default_output_stream == OutputStream::StdErr);
         auto& fs = paths.get_filesystem();
         auto registry_set = paths.make_registry_set();
-        PathsPortFileProvider provider(*registry_set, make_overlay_provider(fs, paths.original_cwd, overlay_ports));
+        PathsPortFileProvider provider(*registry_set, make_overlay_provider(fs, overlay_ports));
         auto source_paragraphs =
             Util::fmap(provider.load_all_control_files(),
                        [](auto&& port) -> const SourceControlFile* { return port->source_control_file.get(); });

src/vcpkg/commands.install.cpp

@@ -1238,16 +1238,16 @@ namespace vcpkg
             auto verprovider = make_versioned_portfile_provider(*registry_set);
             auto baseprovider = make_baseline_provider(*registry_set);
 
-            std::vector<std::string> extended_overlay_ports;
+            std::vector<Path> extended_overlay_ports;
             extended_overlay_ports.reserve(paths.overlay_ports.size() + add_builtin_ports_directory_as_overlay);
             extended_overlay_ports = paths.overlay_ports;
             if (add_builtin_ports_directory_as_overlay)
             {
-                extended_overlay_ports.emplace_back(paths.builtin_ports_directory().native());
+                extended_overlay_ports.emplace_back(paths.builtin_ports_directory());
             }
 
-            auto oprovider = make_manifest_provider(
-                fs, paths.original_cwd, extended_overlay_ports, manifest->path, std::move(manifest_scf));
+            auto oprovider =
+                make_manifest_provider(fs, extended_overlay_ports, manifest->path, std::move(manifest_scf));
             auto install_plan = create_versioned_install_plan(*verprovider,
                                                               *baseprovider,
                                                               *oprovider,
@@ -1277,8 +1277,7 @@ namespace vcpkg
         }
 
         auto registry_set = paths.make_registry_set();
-        PathsPortFileProvider provider(*registry_set,
-                                       make_overlay_provider(fs, paths.original_cwd, paths.overlay_ports));
+        PathsPortFileProvider provider(*registry_set, make_overlay_provider(fs, paths.overlay_ports));
 
         const std::vector<FullPackageSpec> specs = Util::fmap(options.command_arguments, [&](const std::string& arg) {
             return check_and_get_full_package_spec(arg, default_triplet, paths.get_triplet_db())

src/vcpkg/commands.package-info.cpp

@@ -102,8 +102,7 @@ namespace vcpkg
             Json::Object response;
             Json::Object results;
             auto registry_set = paths.make_registry_set();
-            PathsPortFileProvider provider(*registry_set,
-                                           make_overlay_provider(fs, paths.original_cwd, paths.overlay_ports));
+            PathsPortFileProvider provider(*registry_set, make_overlay_provider(fs, paths.overlay_ports));
 
             for (auto&& arg : options.command_arguments)
             {

src/vcpkg/commands.remove.cpp

@@ -196,8 +196,7 @@ namespace vcpkg
             // Load ports from ports dirs
             auto& fs = paths.get_filesystem();
             auto registry_set = paths.make_registry_set();
-            PathsPortFileProvider provider(*registry_set,
-                                           make_overlay_provider(fs, paths.original_cwd, paths.overlay_ports));
+            PathsPortFileProvider provider(*registry_set, make_overlay_provider(fs, paths.overlay_ports));
 
             specs =
                 Util::fmap(find_outdated_packages(provider, status_db), [](auto&& outdated) { return outdated.spec; });

src/vcpkg/commands.set-installed.cpp

@@ -315,8 +315,7 @@ namespace vcpkg
 
         auto& fs = paths.get_filesystem();
         auto registry_set = paths.make_registry_set();
-        PathsPortFileProvider provider(*registry_set,
-                                       make_overlay_provider(fs, paths.original_cwd, paths.overlay_ports));
+        PathsPortFileProvider provider(*registry_set, make_overlay_provider(fs, paths.overlay_ports));
         auto cmake_vars = CMakeVars::make_triplet_cmake_var_provider(paths);
 
         Optional<Path> pkgsconfig;

src/vcpkg/commands.update.cpp

@@ -68,8 +68,7 @@ namespace vcpkg
         const StatusParagraphs status_db = database_load_check(fs, paths.installed());
 
         auto registry_set = paths.make_registry_set();
-        PathsPortFileProvider provider(*registry_set,
-                                       make_overlay_provider(fs, paths.original_cwd, paths.overlay_ports));
+        PathsPortFileProvider provider(*registry_set, make_overlay_provider(fs, paths.overlay_ports));
 
         const auto outdated_packages = SortedVector<OutdatedPackage, decltype(&OutdatedPackage::compare_by_name)>(
             find_outdated_packages(provider, status_db), &OutdatedPackage::compare_by_name);

src/vcpkg/commands.upgrade.cpp

@@ -80,8 +80,7 @@ namespace vcpkg
         // Load ports from ports dirs
         auto& fs = paths.get_filesystem();
         auto registry_set = paths.make_registry_set();
-        PathsPortFileProvider provider(*registry_set,
-                                       make_overlay_provider(fs, paths.original_cwd, paths.overlay_ports));
+        PathsPortFileProvider provider(*registry_set, make_overlay_provider(fs, paths.overlay_ports));
         auto var_provider_storage = CMakeVars::make_triplet_cmake_var_provider(paths);
         auto& var_provider = *var_provider_storage;
 

src/vcpkg/portfileprovider.cpp

@@ -211,10 +211,8 @@ namespace vcpkg
 
         struct OverlayProviderImpl : IFullOverlayProvider
         {
-            OverlayProviderImpl(const ReadOnlyFilesystem& fs, const Path& original_cwd, View<std::string> overlay_ports)
-                : m_fs(fs), m_overlay_ports(Util::fmap(overlay_ports, [&original_cwd](const std::string& s) -> Path {
-                    return original_cwd / s;
-                }))
+            OverlayProviderImpl(const ReadOnlyFilesystem& fs, View<Path> overlay_ports)
+                : m_fs(fs), m_overlay_ports(overlay_ports.begin(), overlay_ports.end())
             {
                 for (auto&& overlay : m_overlay_ports)
                 {
@@ -364,11 +362,10 @@ namespace vcpkg
         struct ManifestProviderImpl : IFullOverlayProvider
         {
             ManifestProviderImpl(const ReadOnlyFilesystem& fs,
-                                 const Path& original_cwd,
-                                 View<std::string> overlay_ports,
+                                 View<Path> overlay_ports,
                                  const Path& manifest_path,
                                  std::unique_ptr<SourceControlFile>&& manifest_scf)
-                : m_overlay_ports{fs, original_cwd, overlay_ports}
+                : m_overlay_ports{fs, overlay_ports}
                 , m_manifest_scf_and_location{std::move(manifest_scf), manifest_path}
             {
             }
@@ -407,21 +404,17 @@ namespace vcpkg
         return std::make_unique<VersionedPortfileProviderImpl>(registry_set);
     }
 
-    std::unique_ptr<IFullOverlayProvider> make_overlay_provider(const ReadOnlyFilesystem& fs,
-                                                                const Path& original_cwd,
-                                                                View<std::string> overlay_ports)
+    std::unique_ptr<IFullOverlayProvider> make_overlay_provider(const ReadOnlyFilesystem& fs, View<Path> overlay_ports)
     {
-        return std::make_unique<OverlayProviderImpl>(fs, original_cwd, overlay_ports);
+        return std::make_unique<OverlayProviderImpl>(fs, overlay_ports);
     }
 
     std::unique_ptr<IOverlayProvider> make_manifest_provider(const ReadOnlyFilesystem& fs,
-                                                             const Path& original_cwd,
-                                                             View<std::string> overlay_ports,
+                                                             View<Path> overlay_ports,
                                                              const Path& manifest_path,
                                                              std::unique_ptr<SourceControlFile>&& manifest_scf)
     {
-        return std::make_unique<ManifestProviderImpl>(
-            fs, original_cwd, overlay_ports, manifest_path, std::move(manifest_scf));
+        return std::make_unique<ManifestProviderImpl>(fs, overlay_ports, manifest_path, std::move(manifest_scf));
     }
 
 } // namespace vcpkg

src/vcpkg/vcpkgpaths.cpp

@@ -98,16 +98,31 @@ namespace
         return nullopt;
     }
 
-    static std::vector<std::string> merge_overlays(const std::vector<std::string>& cli_overlays,
-                                                   const std::vector<std::string>& manifest_overlays,
-                                                   const std::vector<std::string>& env_overlays)
+    static void append_overlays(std::vector<Path>& result,
+                                const std::vector<std::string>& overlay_entries,
+                                const Path& relative_root)
     {
-        std::vector<std::string> ret = cli_overlays;
+        for (auto&& entry : overlay_entries)
+        {
+            result.push_back(relative_root / entry);
+        }
+    }
 
-        ret.insert(std::end(ret), std::begin(manifest_overlays), std::end(manifest_overlays));
-        ret.insert(std::end(ret), std::begin(env_overlays), std::end(env_overlays));
-
-        return ret;
+    // Merges overlay settings from the 3 major sources in the usual priority order, where command line wins first, then
+    // manifest, then environment. The parameter order is specifically chosen to group information that comes from the
+    // manifest together and make parameter order confusion less likely to compile.
+    static std::vector<Path> merge_overlays(const std::vector<std::string>& cli_overlays,
+                                            const std::vector<std::string>& env_overlays,
+                                            const Path& original_cwd,
+                                            const std::vector<std::string>& config_overlays,
+                                            const Path& config_directory)
+    {
+        std::vector<Path> result;
+        result.reserve(cli_overlays.size() + config_overlays.size() + env_overlays.size());
+        append_overlays(result, cli_overlays, original_cwd);
+        append_overlays(result, config_overlays, config_directory);
+        append_overlays(result, env_overlays, original_cwd);
+        return result;
     }
 
     ConfigurationAndSource merge_validate_configs(Optional<ManifestConfiguration>&& manifest_data,
@@ -658,43 +673,33 @@ namespace vcpkg
         m_pimpl->m_download_manager->asset_cache_configured() ? Debug::println("Asset caching is enabled.")
                                                               : Debug::println("Asset cache is not configured.");
 
-        {
-            const auto config_path = m_pimpl->m_config_dir / "vcpkg-configuration.json";
-            auto maybe_manifest_config = config_from_manifest(m_pimpl->m_manifest_doc);
-            auto maybe_json_config =
-                !filesystem.exists(config_path, IgnoreErrors{})
-                    ? nullopt
-                    : parse_configuration(filesystem.read_contents(config_path, IgnoreErrors{}), config_path, out_sink);
+        const auto config_path = m_pimpl->m_config_dir / "vcpkg-configuration.json";
+        auto maybe_manifest_config = config_from_manifest(m_pimpl->m_manifest_doc);
+        auto maybe_json_config =
+            filesystem.exists(config_path, IgnoreErrors{})
+                ? parse_configuration(filesystem.read_contents(config_path, IgnoreErrors{}), config_path, out_sink)
+                : nullopt;
 
-            m_pimpl->m_config = merge_validate_configs(std::move(maybe_manifest_config),
-                                                       m_pimpl->m_manifest_dir,
-                                                       std::move(maybe_json_config),
-                                                       m_pimpl->m_config_dir,
-                                                       *this);
-
-            auto resolve_relative_to_config = [&](const std::string& overlay_path) {
-                return (m_pimpl->m_config.directory / overlay_path).native();
-            };
-
-            std::vector<std::string> overlay_triplet_paths;
-            std::vector<std::string> overlay_port_paths;
-
-            if (!m_pimpl->m_config.directory.empty())
-            {
-                auto& config = m_pimpl->m_config.config;
-                overlay_triplet_paths = Util::fmap(config.overlay_triplets, resolve_relative_to_config);
-                overlay_port_paths = Util::fmap(config.overlay_ports, resolve_relative_to_config);
-            }
-
-            overlay_ports = merge_overlays(args.cli_overlay_ports, overlay_port_paths, args.env_overlay_ports);
-            overlay_triplets =
-                merge_overlays(args.cli_overlay_triplets, overlay_triplet_paths, args.env_overlay_triplets);
-        }
-
-        for (const std::string& triplet : this->overlay_triplets)
+        m_pimpl->m_config = merge_validate_configs(std::move(maybe_manifest_config),
+                                                   m_pimpl->m_manifest_dir,
+                                                   std::move(maybe_json_config),
+                                                   m_pimpl->m_config_dir,
+                                                   *this);
+        overlay_ports = merge_overlays(args.cli_overlay_ports,
+                                       args.env_overlay_ports,
+                                       original_cwd,
+                                       m_pimpl->m_config.config.overlay_ports,
+                                       m_pimpl->m_config.directory);
+        overlay_triplets = merge_overlays(args.cli_overlay_triplets,
+                                          args.env_overlay_triplets,
+                                          original_cwd,
+                                          m_pimpl->m_config.config.overlay_triplets,
+                                          m_pimpl->m_config.directory);
+        for (const auto& triplet : this->overlay_triplets)
         {
             m_pimpl->triplets_dirs.emplace_back(filesystem.almost_canonical(triplet, VCPKG_LINE_INFO));
         }
+
         m_pimpl->triplets_dirs.emplace_back(triplets);
         m_pimpl->triplets_dirs.emplace_back(community_triplets);
     }