Make --only-downloads not affect ABI (#1363)

While reviewing changes to #1339 , I observed that it was strange that the abi hash depends on whether download_only is selected. After discussion with @ras0219-msft , he agreed that the ABI hash shouldn't be affected by this setting.

However, given that in download mode, we intentionally build things which do not have all their dependencies present, it would not be safe to attempt to cache or otherwise install anything in that condition.

This change removes download_only from the ABI calculation, and changes installation to only attempt to install built bits whose dependencies are all satisfied.

azure-pipelines/e2e-ports/vcpkg-e2e-test-fails-in-download-only-transitive/portfile.cmake

@@ -0,0 +1,4 @@
+set(VCPKG_POLICY_EMPTY_PACKAGE enabled)
+file(MAKE_DIRECTORY "${CURRENT_PACKAGES_DIR}/share/${PORT}")
+file(WRITE "${CURRENT_PACKAGES_DIR}/share/${PORT}/copyright" "copyright message")
+file(WRITE "${CURRENT_PACKAGES_DIR}/share/${PORT}/installed.txt" "${PORT} installed")

azure-pipelines/e2e-ports/vcpkg-e2e-test-fails-in-download-only-transitive/vcpkg.json

@@ -0,0 +1,5 @@
+{
+	"name": "vcpkg-e2e-test-fails-in-download-only-transitive",
+	"version": "1.0.0",
+	"dependencies": [ "vcpkg-e2e-test-fails-in-download-only" ]
+}

azure-pipelines/e2e-ports/vcpkg-e2e-test-fails-in-download-only/portfile.cmake

@@ -0,0 +1,8 @@
+if(DEFINED VCPKG_DOWNLOAD_MODE)
+	message(FATAL_ERROR "This port does not compile in download mode")
+endif()
+
+set(VCPKG_POLICY_EMPTY_PACKAGE enabled)
+file(MAKE_DIRECTORY "${CURRENT_PACKAGES_DIR}/share/${PORT}")
+file(WRITE "${CURRENT_PACKAGES_DIR}/share/${PORT}/copyright" "copyright message")
+file(WRITE "${CURRENT_PACKAGES_DIR}/share/${PORT}/installed.txt" "${PORT} installed")

azure-pipelines/e2e-ports/vcpkg-e2e-test-fails-in-download-only/vcpkg.json

@@ -0,0 +1,5 @@
+{
+	"name": "vcpkg-e2e-test-fails-in-download-only",
+	"version": "1.0.0",
+	"dependencies": ["vcpkg-internal-e2e-test-port"]
+}

azure-pipelines/e2e-ports/vcpkg-internal-e2e-test-port/portfile.cmake

@@ -1 +1,4 @@
 set(VCPKG_POLICY_EMPTY_PACKAGE enabled)
+file(MAKE_DIRECTORY "${CURRENT_PACKAGES_DIR}/share/${PORT}")
+file(WRITE "${CURRENT_PACKAGES_DIR}/share/${PORT}/copyright" "copyright message")
+file(WRITE "${CURRENT_PACKAGES_DIR}/share/${PORT}/installed.txt" "${PORT} installed")

azure-pipelines/end-to-end-tests-dir/only-downloads.ps1

@@ -11,3 +11,18 @@ Refresh-TestRoot
 Run-Vcpkg @commonArgs install --only-downloads vcpkg-uses-touch-missing-dependency
 Throw-IfFailed
 Require-FileNotExists "$installRoot/$Triplet/include/vcpkg-uses-touch.h"
+
+Refresh-TestRoot
+
+Run-Vcpkg @commonArgs install "--overlay-ports=$PSScriptRoot/../e2e-ports" vcpkg-e2e-test-fails-in-download-only-transitive --only-downloads
+Throw-IfFailed
+if (Test-Path "$installRoot/$Triplet/share/vcpkg-e2e-test-fails-in-download-only-transitive/installed.txt") {
+	throw "--only-downloads installed a port with a missing transitive dependency"
+}
+
+
+Run-Vcpkg @commonArgs install "--overlay-ports=$PSScriptRoot/../e2e-ports" vcpkg-e2e-test-fails-in-download-only-transitive
+Throw-IfFailed
+if (-Not (Test-Path "$installRoot/$Triplet/share/vcpkg-e2e-test-fails-in-download-only-transitive/installed.txt")) {
+	throw "The --only-downloads transitive test port did not succeed (likely test bug)"
+}

include/vcpkg/base/message-data.inc.h

@@ -898,7 +898,13 @@ DECLARE_MESSAGE(ControlAndManifestFilesPresent,
 DECLARE_MESSAGE(ControlCharacterInString, (), "", "Control character in string")
 DECLARE_MESSAGE(ControlSupportsMustBeAPlatformExpression, (), "", "\"Supports\" must be a platform expression")
 DECLARE_MESSAGE(CopyrightIsDir, (msg::path), "", "`{path}` being a directory is deprecated.")
-DECLARE_MESSAGE(CorruptedDatabase, (), "", "Database corrupted.")
+DECLARE_MESSAGE(CorruptedDatabase,
+                (),
+                "",
+                "vcpkg's installation database corrupted. This is either a bug in vcpkg or something else has modified "
+                "the contents of the 'installed' directory in an unexpected way. You may be able to fix this by "
+                "deleting the 'installed' directory and reinstalling what you want to use. If this problem happens "
+                "consistently, please file a bug at https://github.com/microsoft/vcpkg .")
 DECLARE_MESSAGE(CorruptedInstallTree, (), "", "Your vcpkg 'installed' tree is corrupted.")
 DECLARE_MESSAGE(CouldNotDeduceNugetIdAndVersion,
                 (msg::path),

locales/messages.json

@@ -532,7 +532,7 @@
   "ControlSupportsMustBeAPlatformExpression": "\"Supports\" must be a platform expression",
   "CopyrightIsDir": "`{path}` being a directory is deprecated.",
   "_CopyrightIsDir.comment": "An example of {path} is /foo/bar.",
-  "CorruptedDatabase": "Database corrupted.",
+  "CorruptedDatabase": "vcpkg's installation database corrupted. This is either a bug in vcpkg or something else has modified the contents of the 'installed' directory in an unexpected way. You may be able to fix this by deleting the 'installed' directory and reinstalling what you want to use. If this problem happens consistently, please file a bug at https://github.com/microsoft/vcpkg .",
   "CorruptedInstallTree": "Your vcpkg 'installed' tree is corrupted.",
   "CouldNotDeduceNugetIdAndVersion": "Could not deduce nuget id and version from filename: {path}",
   "_CouldNotDeduceNugetIdAndVersion.comment": "An example of {path} is /foo/bar.",

src/vcpkg/commands.build.cpp

@@ -156,37 +156,42 @@ namespace vcpkg
         const ElapsedTimer build_timer;
         const auto result = build_package(args, paths, *action, build_logs_recorder, status_db);
         msg::print(msgElapsedForPackage, msg::spec = spec, msg::elapsed = build_timer);
-        if (result.code == BuildResult::CascadedDueToMissingDependencies)
+        switch (result.code)
         {
-            LocalizedString errorMsg = msg::format_error(msgBuildDependenciesMissing);
-            for (const auto& p : result.unmet_dependencies)
+            case BuildResult::Succeeded: binary_cache.push_success(*action); return 0;
+            case BuildResult::CascadedDueToMissingDependencies:
             {
-                errorMsg.append_raw('\n').append_indent().append_raw(p.to_string());
-            }
-
-            Checks::msg_exit_with_message(VCPKG_LINE_INFO, errorMsg);
-        }
-
-        Checks::check_exit(VCPKG_LINE_INFO, result.code != BuildResult::Excluded);
+                LocalizedString errorMsg = msg::format_error(msgBuildDependenciesMissing);
+                for (const auto& p : result.unmet_dependencies)
+                {
+                    errorMsg.append_raw('\n').append_indent().append_raw(p.to_string());
+                }
 
-        if (result.code != BuildResult::Succeeded)
-        {
-            LocalizedString warnings;
-            for (auto&& msg : action->build_failure_messages)
-            {
-                warnings.append(msg).append_raw('\n');
+                Checks::msg_exit_with_message(VCPKG_LINE_INFO, errorMsg);
             }
-            if (!warnings.data().empty())
+            case BuildResult::BuildFailed:
+            case BuildResult::PostBuildChecksFailed:
+            case BuildResult::FileConflicts:
+            case BuildResult::CacheMissing:
+            case BuildResult::Downloaded:
+            case BuildResult::Removed:
             {
-                msg::print(Color::warning, warnings);
+                LocalizedString warnings;
+                for (auto&& msg : action->build_failure_messages)
+                {
+                    warnings.append(msg).append_raw('\n');
+                }
+                if (!warnings.data().empty())
+                {
+                    msg::print(Color::warning, warnings);
+                }
+                msg::println_error(create_error_message(result, spec));
+                msg::print(create_user_troubleshooting_message(*action, paths, nullopt));
+                return 1;
             }
-            msg::println_error(create_error_message(result, spec));
-            msg::print(create_user_troubleshooting_message(*action, paths, nullopt));
-            return 1;
+            case BuildResult::Excluded:
+            default: Checks::unreachable(VCPKG_LINE_INFO);
         }
-        binary_cache.push_success(*action);
-
-        return 0;
     }
 
     static std::remove_const_t<decltype(ALL_POLICIES)> generate_all_policies()
@@ -1274,31 +1279,28 @@ namespace vcpkg
             if (action.abi_info.has_value()) continue;
 
             std::vector<AbiEntry> dependency_abis;
-            if (!Util::Enum::to_bool(action.build_options.only_downloads))
+            for (auto&& pspec : action.package_dependencies)
             {
-                for (auto&& pspec : action.package_dependencies)
-                {
-                    if (pspec == action.spec) continue;
+                if (pspec == action.spec) continue;
 
-                    auto pred = [&](const InstallPlanAction& ipa) { return ipa.spec == pspec; };
-                    auto it2 = std::find_if(action_plan.install_actions.begin(), it, pred);
-                    if (it2 == it)
-                    {
-                        // Finally, look in current installed
-                        auto status_it = status_db.find(pspec);
-                        if (status_it == status_db.end())
-                        {
-                            Checks::unreachable(
-                                VCPKG_LINE_INFO,
-                                fmt::format("Failed to find dependency abi for {} -> {}", action.spec, pspec));
-                        }
-
-                        dependency_abis.emplace_back(pspec.name(), status_it->get()->package.abi);
-                    }
-                    else
+                auto pred = [&](const InstallPlanAction& ipa) { return ipa.spec == pspec; };
+                auto it2 = std::find_if(action_plan.install_actions.begin(), it, pred);
+                if (it2 == it)
+                {
+                    // Finally, look in current installed
+                    auto status_it = status_db.find(pspec);
+                    if (status_it == status_db.end())
                     {
-                        dependency_abis.emplace_back(pspec.name(), it2->public_abi());
+                        Checks::unreachable(
+                            VCPKG_LINE_INFO,
+                            fmt::format("Failed to find dependency abi for {} -> {}", action.spec, pspec));
                     }
+
+                    dependency_abis.emplace_back(pspec.name(), status_it->get()->package.abi);
+                }
+                else
+                {
+                    dependency_abis.emplace_back(pspec.name(), it2->public_abi());
                 }
             }
 
@@ -1336,21 +1338,25 @@ namespace vcpkg
         }
 
         const bool all_dependencies_satisfied = missing_fspecs.empty();
-        if (!all_dependencies_satisfied && !Util::Enum::to_bool(action.build_options.only_downloads))
-        {
-            return {BuildResult::CascadedDueToMissingDependencies, std::move(missing_fspecs)};
-        }
-
         if (action.build_options.only_downloads == OnlyDownloads::No)
         {
+            if (!all_dependencies_satisfied)
+            {
+                return {BuildResult::CascadedDueToMissingDependencies, std::move(missing_fspecs)};
+            }
+
+            // assert that all_dependencies_satisfied is accurate above by checking that they're all installed
             for (auto&& pspec : action.package_dependencies)
             {
                 if (pspec == spec)
                 {
                     continue;
                 }
-                const auto status_it = status_db.find_installed(pspec);
-                Checks::check_exit(VCPKG_LINE_INFO, status_it != status_db.end());
+
+                if (status_db.find_installed(pspec) == status_db.end())
+                {
+                    Checks::msg_exit_with_error(VCPKG_LINE_INFO, msgCorruptedDatabase);
+                }
             }
         }
 

src/vcpkg/commands.install.cpp

@@ -339,6 +339,7 @@ namespace vcpkg
             return ExtendedBuildResult{BuildResult::Succeeded};
         }
 
+        bool all_dependencies_satisfied;
         if (plan_type == InstallPlanType::BUILD_AND_INSTALL)
         {
             std::unique_ptr<BinaryControlFile> bcf;
@@ -347,6 +348,7 @@ namespace vcpkg
                 auto maybe_bcf = Paragraphs::try_load_cached_package(
                     fs, action.package_dir.value_or_exit(VCPKG_LINE_INFO), action.spec);
                 bcf = std::make_unique<BinaryControlFile>(std::move(maybe_bcf).value_or_exit(VCPKG_LINE_INFO));
+                all_dependencies_satisfied = true;
             }
             else if (action.build_options.build_missing == BuildMissing::No)
             {
@@ -365,6 +367,7 @@ namespace vcpkg
                     return result;
                 }
 
+                all_dependencies_satisfied = result.unmet_dependencies.empty();
                 if (result.code != BuildResult::Succeeded)
                 {
                     LocalizedString warnings;
@@ -386,16 +389,23 @@ namespace vcpkg
             }
             // Build or restore succeeded and `bcf` is populated with the control file.
             Checks::check_exit(VCPKG_LINE_INFO, bcf != nullptr);
-
-            const auto install_result = install_package(paths, *bcf, &status_db);
             BuildResult code;
-            switch (install_result)
+            if (all_dependencies_satisfied)
             {
-                case InstallResult::SUCCESS: code = BuildResult::Succeeded; break;
-                case InstallResult::FILE_CONFLICTS: code = BuildResult::FileConflicts; break;
-                default: Checks::unreachable(VCPKG_LINE_INFO);
+                const auto install_result = install_package(paths, *bcf, &status_db);
+                switch (install_result)
+                {
+                    case InstallResult::SUCCESS: code = BuildResult::Succeeded; break;
+                    case InstallResult::FILE_CONFLICTS: code = BuildResult::FileConflicts; break;
+                    default: Checks::unreachable(VCPKG_LINE_INFO);
+                }
+                binary_cache.push_success(action);
+            }
+            else
+            {
+                Checks::check_exit(VCPKG_LINE_INFO, action.build_options.only_downloads == OnlyDownloads::Yes);
+                code = BuildResult::Downloaded;
             }
-            binary_cache.push_success(action);
 
             if (action.build_options.clean_downloads == CleanDownloads::Yes)
             {