From 134326df9abe168e3c0bf76c63fb438c375212dc Mon Sep 17 00:00:00 2001 From: yao-msft <50888816+yao-msft@users.noreply.github.com> Date: Thu, 4 Nov 2021 11:25:30 -0700 Subject: [PATCH] Prevent duplicate installations in winget upgrade all (#1652) --- .../Workflows/UpdateFlow.cpp | 17 +++- src/AppInstallerCLITests/WorkFlow.cpp | 87 ++++++++++++++++--- 2 files changed, 90 insertions(+), 14 deletions(-) diff --git a/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp b/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp index c214bc4971..4197e7f298 100644 --- a/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp @@ -18,6 +18,21 @@ namespace AppInstaller::CLI::Workflow { return (installedVersion < updateVersion || updateVersion.IsLatest()); } + + void AddToPackagesToInstallIfNotPresent(std::vector& packagesToInstall, Execution::PackageToInstall&& package) + { + for (auto const& existing : packagesToInstall) + { + if (existing.Manifest.Id == package.Manifest.Id && + existing.Manifest.Version == package.Manifest.Version && + existing.PackageVersion->GetProperty(PackageVersionProperty::SourceIdentifier) == package.PackageVersion->GetProperty(PackageVersionProperty::SourceIdentifier)) + { + return; + } + } + + packagesToInstall.emplace_back(std::move(package)); + } } void SelectLatestApplicableUpdate::operator()(Execution::Context& context) const @@ -119,7 +134,7 @@ namespace AppInstaller::CLI::Workflow std::move(updateContext.Get().value()) }; package.PackageSubExecutionId = subExecution.GetCurrentSubExecutionId(); - packagesToInstall.emplace_back(std::move(package)); + AddToPackagesToInstallIfNotPresent(packagesToInstall, std::move(package)); } if (!updateAllFoundUpdate) diff --git a/src/AppInstallerCLITests/WorkFlow.cpp b/src/AppInstallerCLITests/WorkFlow.cpp index c3cbb72d35..4779e918ad 100644 --- a/src/AppInstallerCLITests/WorkFlow.cpp +++ b/src/AppInstallerCLITests/WorkFlow.cpp @@ -269,6 +269,41 @@ namespace PackageMatchFilter(PackageMatchField::Id, MatchType::Exact, "TestInstallerWithLicenseAgreement"))); } + if (input == "TestUpgradeAllWithDuplicateUpgradeItems") + { + auto manifest = YamlParser::CreateFromPath(TestDataFile("InstallFlowTest_Exe.yaml")); + auto manifest2 = YamlParser::CreateFromPath(TestDataFile("UpdateFlowTest_Exe.yaml")); + auto manifest3 = YamlParser::CreateFromPath(TestDataFile(m_upgradeUsesLicenses ? "UpdateFlowTest_Exe_2_LicenseAgreement.yaml" : "UpdateFlowTest_Exe_2.yaml")); + result.Matches.emplace_back( + ResultMatch( + TestPackage::Make( + manifest, + TestPackage::MetadataMap + { + { PackageVersionMetadata::InstalledType, "Exe" }, + { PackageVersionMetadata::StandardUninstallCommand, "C:\\uninstall.exe" }, + { PackageVersionMetadata::SilentUninstallCommand, "C:\\uninstall.exe /silence" }, + }, + std::vector{ manifest3, manifest2, manifest }, + shared_from_this() + ), + PackageMatchFilter(PackageMatchField::Id, MatchType::Exact, "AppInstallerCliTest.TestExeInstaller"))); + result.Matches.emplace_back( + ResultMatch( + TestPackage::Make( + manifest2, + TestPackage::MetadataMap + { + { PackageVersionMetadata::InstalledType, "Exe" }, + { PackageVersionMetadata::StandardUninstallCommand, "C:\\uninstall.exe" }, + { PackageVersionMetadata::SilentUninstallCommand, "C:\\uninstall.exe /silence" }, + }, + std::vector{ manifest3, manifest2, manifest }, + shared_from_this() + ), + PackageMatchFilter(PackageMatchField::Id, MatchType::Exact, "AppInstallerCliTest.TestExeInstaller"))); + } + return result; } @@ -280,16 +315,18 @@ namespace struct WorkflowTaskOverride { - WorkflowTaskOverride(WorkflowTask::Func f, const std::function& o) : - Target(f), Override(o) {} + WorkflowTaskOverride(WorkflowTask::Func f, const std::function& o, int expectedUseCount = -1) : + Target(f), Override(o), ExpectedUseCount(expectedUseCount) {} - WorkflowTaskOverride(std::string_view n, const std::function& o) : - Target(n), Override(o) {} + WorkflowTaskOverride(std::string_view n, const std::function& o, int expectedUseCount = -1) : + Target(n), Override(o), ExpectedUseCount(expectedUseCount) {} - WorkflowTaskOverride(const WorkflowTask& t, const std::function& o) : - Target(t), Override(o) {} + WorkflowTaskOverride(const WorkflowTask& t, const std::function& o, int expectedUseCount = -1) : + Target(t), Override(o), ExpectedUseCount(expectedUseCount) {} - bool Used = false; + // -1 means no check on actual use count, as long as it's used. + int ExpectedUseCount = -1; + int UseCount = 0; WorkflowTask Target; std::function Override; }; @@ -306,7 +343,7 @@ namespace } }; // Mark this one as used so that it doesn't anger the destructor. - wto.Used = true; + wto.UseCount++; Override(wto); } @@ -324,7 +361,7 @@ namespace } else { - itr->Used = true; + itr->UseCount++; itr->Override(*this); return false; } @@ -337,10 +374,14 @@ namespace { for (const auto& wto : *m_overrides) { - if (!wto.Used) + if (wto.UseCount == 0) { FAIL_CHECK("Unused override " + wto.Target.GetName()); } + else if (wto.ExpectedUseCount != -1 && wto.ExpectedUseCount != wto.UseCount) + { + FAIL_CHECK("Used override count does not match expected " + wto.Target.GetName()); + } } } } @@ -434,7 +475,7 @@ void OverrideForCheckExistingInstaller(TestContext& context) } }); } -void OverrideForShellExecute(TestContext& context) +void OverrideForShellExecute(TestContext& context, int expectedUseCount = -1) { OverrideForCheckExistingInstaller(context); @@ -442,11 +483,11 @@ void OverrideForShellExecute(TestContext& context) { context.Add({ {}, {} }); context.Add(TestDataFile("AppInstallerTestExeInstaller.exe")); - } }); + }, expectedUseCount }); context.Override({ RenameDownloadedInstaller, [](TestContext&) { - } }); + }, expectedUseCount }); OverrideForUpdateInstallerMotw(context); } @@ -1461,6 +1502,26 @@ TEST_CASE("UpdateFlow_UpdateAllApplicable", "[UpdateFlow][workflow]") REQUIRE(std::filesystem::exists(updateMSStoreResultPath.GetPath())); } +TEST_CASE("UpdateFlow_UpgradeWithDuplicateUpgradeItemsFound", "[UpdateFlow][workflow]") +{ + TestCommon::TempFile updateExeResultPath("TestExeInstalled.txt"); + + std::ostringstream updateOutput; + TestContext context{ updateOutput, std::cin }; + OverrideForCompositeInstalledSource(context); + // Installer should only be run once since the 2 upgrade items are same. + OverrideForShellExecute(context, 1); + context.Args.AddArg(Execution::Args::Type::Query, "TestUpgradeAllWithDuplicateUpgradeItems"sv); + context.Args.AddArg(Execution::Args::Type::All); + + UpgradeCommand update({}); + update.Execute(context); + INFO(updateOutput.str()); + + // Verify installers are called. + REQUIRE(std::filesystem::exists(updateExeResultPath.GetPath())); +} + TEST_CASE("UpdateFlow_Dependencies", "[UpdateFlow][workflow][dependencies]") { TestCommon::TempFile updateResultPath("TestExeInstalled.txt");