[vcpkg] Address some CR comments from microsoft#13639

scripts/azure-pipelines/test-modified-ports.ps1

@@ -10,7 +10,10 @@ Runs the 'Test Modified Ports' part of the vcpkg CI system for all platforms.
 The triplet to test.
 
 .PARAMETER ArchivesRoot
-The location where the binary caching archives are stored. Shared across runs of this script.
+Equivalent to '-BinarySourceStub "files,$ArchivesRoot"'
+
+.PARAMETER BinarySourceStub
+The type and parameters of the binary source. Shared across runs of this script. Example: "files,W:\"
 
 .PARAMETER WorkingRoot
 The location used as scratch space for 'installed', 'packages', and 'buildtrees' vcpkg directories.
@@ -28,9 +31,12 @@ Param(
     [Parameter(Mandatory = $true)]
     [ValidateNotNullOrEmpty()]
     [string]$Triplet,
-    [Parameter(Mandatory = $true)]
+    [Parameter(Mandatory = $true, ParameterSetName='ArchivesRoot')]
     [ValidateNotNullOrEmpty()]
     $ArchivesRoot,
+    [Parameter(Mandatory = $true, ParameterSetName='BinarySourceStub')]
+    [ValidateNotNullOrEmpty()]
+    $BinarySourceStub,
     [Parameter(Mandatory = $true)]
     [ValidateNotNullOrEmpty()]
     $WorkingRoot,
@@ -69,8 +75,10 @@ else {
     Write-Host "Build reason was $BuildReason, using binary caching in write only mode."
     $binaryCachingMode = 'write'
 }
-
-#$commonArgs += @("--x-binarysource=clear;files,$ArchivesRoot,$binaryCachingMode")
+if ([string]::IsNullOrWhiteSpace($ArchivesRoot)) {
+    $BinarySourceStub = "files,$ArchivesRoot"
+}
+$commonArgs += @("--binarysource=clear;$BinarySourceStub,$binaryCachingMode")
 
 if ($Triplet -eq 'x64-linux') {
     $env:HOME = '/home/agent'

scripts/azure-pipelines/windows/azure-pipelines.yml

@@ -15,6 +15,8 @@ jobs:
   - name: VCPKG_DOWNLOADS
     value: D:\downloads
   - group: azblob-test-sas-group
+  - name: BINARY_SOURCE_STUB
+    value: "x-azblob,$(azblob-root-url),$(azblob-test-sas)"
 
   steps:
   - task: PowerShell@2
@@ -58,10 +60,8 @@ jobs:
     inputs:
       failOnStderr: true
       filePath: 'scripts/azure-pipelines/test-modified-ports.ps1'
-      arguments: '-Triplet ${{ parameters.triplet }} -BuildReason $(Build.Reason) -ArchivesRoot W:\ -WorkingRoot ${{ variables.WORKING_ROOT }} -ArtifactStagingDirectory $(Build.ArtifactStagingDirectory)'
+      arguments: '-Triplet ${{ parameters.triplet }} -BuildReason $(Build.Reason) -BinarySourceStub $(BINARY_SOURCE_STUB) -WorkingRoot ${{ variables.WORKING_ROOT }} -ArtifactStagingDirectory $(Build.ArtifactStagingDirectory)'
       pwsh: true
-    env:
-      VCPKG_BINARY_SOURCES: "clear;x-azblob,$(azblob-root-url),$(azblob-test-sas),readwrite"
   - task: PowerShell@2
     displayName: 'Report on Disk Space After Build'
     condition: always()

toolsrc/include/vcpkg/base/lockguarded.h

@@ -30,7 +30,7 @@ namespace vcpkg::Util
         LockGuardPtr(LockGuarded<T>& sync) : m_lock(sync.m_mutex), m_ptr(sync.m_t) { }
 
     private:
-        std::unique_lock<std::mutex> m_lock;
+        std::lock_guard<std::mutex> m_lock;
         T& m_ptr;
     };
 }

toolsrc/include/vcpkg/binarycaching.h

@@ -33,31 +33,32 @@ namespace vcpkg
     struct IBinaryProvider
     {
         virtual ~IBinaryProvider() = default;
-        /// Gives the BinaryProvider an opportunity to batch any downloading or server communication for executing
-        /// `plan`.
-        void prefetch(const VcpkgPaths& paths, const Dependencies::ActionPlan& plan);
+
         /// Attempts to restore the package referenced by `action` into the packages directory.
-        virtual RestoreResult try_restore(const VcpkgPaths& paths, const Dependencies::InstallPlanAction& action);
-        /// Called upon a successful build of `action`
-        virtual void push_success(const VcpkgPaths& paths, const Dependencies::InstallPlanAction& action);
-        /// Requests the result of `try_restore()` without actually downloading the package. Used by CI to determine
-        /// missing packages.
-        std::unordered_map<const Dependencies::InstallPlanAction*, RestoreResult> precheck(
-            const VcpkgPaths& paths, View<Dependencies::InstallPlanAction> actions);
+        virtual RestoreResult try_restore(const VcpkgPaths& paths, const Dependencies::InstallPlanAction& action) = 0;
 
-        friend struct MergeBinaryProviders;
+        /// Called upon a successful build of `action`
+        virtual void push_success(const VcpkgPaths& paths, const Dependencies::InstallPlanAction& action) = 0;
 
-    protected:
+        /// <summary>Gives the BinaryProvider an opportunity to batch any downloading or server communication for
+        /// executing `plan`.</summary>
+        /// <remarks>Must only be called once for a given binary provider instance</remarks>
+        /// <param name="actions">InOut vector of actions to be prefetched</param>
         virtual void prefetch(const VcpkgPaths& paths,
-                              const Dependencies::ActionPlan& plan,
-                              std::set<PackageSpec>* restored);
+                              std::vector<const Dependencies::InstallPlanAction*>& actions) = 0;
 
-        /// Requests the result of `try_restore()` without actually downloading the package. Used by CI to determine
-        /// missing packages.
-        virtual void precheck(const VcpkgPaths& paths,
-                              std::unordered_map<const Dependencies::InstallPlanAction*, RestoreResult>* results_map);
+        /// <summary>Requests the result of <c>try_restore()</c> without actually downloading the package. Used by CI to
+        /// determine missing packages.</summary>
+        /// <param name="results_map">InOut map to track the restored packages. Should be initialized to
+        /// <c>{&amp;action, RestoreResult::missing}</c> for all install actions</param>
+        virtual void precheck(
+            const VcpkgPaths& paths,
+            std::unordered_map<const Dependencies::InstallPlanAction*, RestoreResult>& results_map) = 0;
     };
 
+    std::unordered_map<const Dependencies::InstallPlanAction*, RestoreResult> binary_provider_precheck(
+        const VcpkgPaths& paths, const Dependencies::ActionPlan& plan, IBinaryProvider& provider);
+
     IBinaryProvider& null_binary_provider();
 
     ExpectedS<std::unique_ptr<IBinaryProvider>> create_binary_provider_from_configs(View<std::string> args);

toolsrc/src/vcpkg/base/downloads.cpp

@@ -127,8 +127,8 @@ namespace vcpkg::Downloads
             }
 
             // Use Windows 10 defaults on Windows 7
-            DWORD secure_protocols(WINHTTP_FLAG_SECURE_PROTOCOL_SSL3 | WINHTTP_FLAG_SECURE_PROTOCOL_TLS1 |
-                                   WINHTTP_FLAG_SECURE_PROTOCOL_TLS1_1 | WINHTTP_FLAG_SECURE_PROTOCOL_TLS1_2);
+            DWORD secure_protocols(WINHTTP_FLAG_SECURE_PROTOCOL_TLS1 | WINHTTP_FLAG_SECURE_PROTOCOL_TLS1_1 |
+                                   WINHTTP_FLAG_SECURE_PROTOCOL_TLS1_2);
             WinHttpSetOption(
                 m_hSession.get(), WINHTTP_OPTION_SECURE_PROTOCOLS, &secure_protocols, sizeof(secure_protocols));
         }
@@ -235,22 +235,6 @@ namespace vcpkg::Downloads
     {
         static constexpr size_t batch_size = 100;
 
-        // Checks::check_exit(VCPKG_LINE_INFO, Strings::starts_with(url, "https://"));
-        // auto url_no_proto = url.substr(8); // drop https://
-        // auto path_begin = Util::find(url_no_proto, '/');
-        // std::string hostname(url_no_proto.begin(), path_begin);
-        // std::string path(path_begin, url_no_proto.end());
-
-        // static WinHttpSession s_session;
-        // static Util::LockGuarded<Cache<std::string, WinHttpConnection>> s_conn_map;
-        // const auto& conn = [&hostname]() -> const WinHttpConnection& {
-        //    auto l = s_conn_map.lock();
-        //    return l->get_lazy(hostname,
-        //                       [&hostname] { return WinHttpConnection(s_session.m_hSession.get(), hostname); });
-        //}();
-        // WinHttpRequest req(conn.m_hConnect.get(), path);
-        // req.forall_data([&](Span<char> span) { System::print2(StringView(span.data(), span.size())); });
-
         std::vector<int> ret;
 
         size_t i = 0;
@@ -308,16 +292,11 @@ namespace vcpkg::Downloads
         static constexpr StringLiteral guid_marker = "9a1db05f-a65d-419b-aa72-037fb4d0672e";
 
         System::CmdLineBuilder cmd;
-        cmd.string_arg("curl")
-            .string_arg("-X")
-            .string_arg("PUT")
-            .string_arg("-w")
-            .string_arg(Strings::concat("\\n", guid_marker, "%{http_code}"))
-            .string_arg(url)
-            .string_arg("-T")
-            .path_arg(file)
-            .string_arg("-H")
-            .string_arg("x-ms-blob-type: BlockBlob");
+        cmd.string_arg("curl").string_arg("-X").string_arg("PUT");
+        cmd.string_arg("-w").string_arg(Strings::concat("\\n", guid_marker, "%{http_code}"));
+        cmd.string_arg(url);
+        cmd.string_arg("-T").path_arg(file);
+        cmd.string_arg("-H").string_arg("x-ms-blob-type: BlockBlob");
         int code = 0;
         auto res = System::cmd_execute_and_stream_lines(cmd, [&code](const std::string& line) {
             if (Strings::starts_with(line, guid_marker))