From a3780d374bf8c27684a4c4cd6b5b5de125b68ceb Mon Sep 17 00:00:00 2001 From: sreadingMSFT <74242768+sreadingMSFT@users.noreply.github.com> Date: Fri, 3 Sep 2021 17:36:50 -0700 Subject: [PATCH 01/13] Sample change for multiple concurrent downloads. --- src/AppInstallerCLICore/Command.cpp | 5 ++ src/AppInstallerCLICore/Command.h | 5 +- .../Commands/COMInstallCommand.cpp | 21 ++++++- .../Commands/COMInstallCommand.h | 11 ++++ .../ContextOrchestrator.cpp | 61 +++++++++++++------ src/AppInstallerCLICore/ContextOrchestrator.h | 16 +++++ .../Workflows/InstallFlow.cpp | 18 ++++-- .../Workflows/InstallFlow.h | 6 ++ 8 files changed, 117 insertions(+), 26 deletions(-) diff --git a/src/AppInstallerCLICore/Command.cpp b/src/AppInstallerCLICore/Command.cpp index d0988c10aa..06111e8435 100644 --- a/src/AppInstallerCLICore/Command.cpp +++ b/src/AppInstallerCLICore/Command.cpp @@ -797,6 +797,11 @@ namespace AppInstaller::CLI } } + bool Command::IsCommandAllowedToRunNow(std::map&, UINT32) const + { + return false; + } + void Command::ValidateArgumentsInternal(Execution::Args&) const { // Do nothing by default. diff --git a/src/AppInstallerCLICore/Command.h b/src/AppInstallerCLICore/Command.h index 9225a0ca25..b4bd0f3b84 100644 --- a/src/AppInstallerCLICore/Command.h +++ b/src/AppInstallerCLICore/Command.h @@ -78,6 +78,7 @@ namespace AppInstaller::CLI constexpr static char ParentSplitChar = ':'; std::string_view Name() const { return m_name; } + const std::string& NameAsString() const { return m_name; } const std::string& FullName() const { return m_fullName; } Command::Visibility GetVisibility() const; Settings::ExperimentalFeature::Feature Feature() const { return m_feature; } @@ -104,12 +105,14 @@ namespace AppInstaller::CLI virtual void Execute(Execution::Context& context) const; + virtual bool IsCommandAllowedToRunNow(std::map& runningCommands, UINT32 runningCommandsOfCurrentType) const; + protected: virtual void ValidateArgumentsInternal(Execution::Args& execArgs) const; virtual void ExecuteInternal(Execution::Context& context) const; private: - std::string_view m_name; + std::string m_name; std::string m_fullName; Command::Visibility m_visibility; Settings::ExperimentalFeature::Feature m_feature; diff --git a/src/AppInstallerCLICore/Commands/COMInstallCommand.cpp b/src/AppInstallerCLICore/Commands/COMInstallCommand.cpp index 9235cbcb32..5122f2c319 100644 --- a/src/AppInstallerCLICore/Commands/COMInstallCommand.cpp +++ b/src/AppInstallerCLICore/Commands/COMInstallCommand.cpp @@ -13,12 +13,29 @@ using namespace AppInstaller::Utility::literals; namespace AppInstaller::CLI { // IMPORTANT: To use this command, the caller should have already retrieved the package manifest (GetManifest()) and added it to the Context Data - void COMInstallCommand::ExecuteInternal(Context& context) const + void COMDownloadCommand::ExecuteInternal(Context& context) const { context << Workflow::ReportExecutionStage(ExecutionStage::Discovery) << Workflow::SelectInstaller << Workflow::EnsureApplicableInstaller << - Workflow::InstallSinglePackage; + Workflow::DownloadPackageVersion; + } + + // IMPORTANT: To use this command, the caller should have already retrieved the package manifest (GetManifest()) and added it to the Context Data + void COMInstallCommand::ExecuteInternal(Context& context) const + { + context << + Workflow::InstallPackageInstaller; + } + + bool COMDownloadCommand::IsCommandAllowedToRunNow(std::map&, UINT32 runningCommandsOfCurrentType) const + { + return (runningCommandsOfCurrentType < 5); + } + + bool COMInstallCommand::IsCommandAllowedToRunNow(std::map&, UINT32 runningCommandsOfCurrentType) const + { + return (runningCommandsOfCurrentType == 0); } } diff --git a/src/AppInstallerCLICore/Commands/COMInstallCommand.h b/src/AppInstallerCLICore/Commands/COMInstallCommand.h index 46ad3498a2..ca105f8021 100644 --- a/src/AppInstallerCLICore/Commands/COMInstallCommand.h +++ b/src/AppInstallerCLICore/Commands/COMInstallCommand.h @@ -5,10 +5,21 @@ namespace AppInstaller::CLI { + // IMPORTANT: To use this command, the caller should have already retrieved the package manifest (GetManifest()) and added it to the Context Data + struct COMDownloadCommand final : public Command + { + COMDownloadCommand(std::string_view parent) : Command("download", parent) {} + bool IsCommandAllowedToRunNow(std::map& runningCommands, UINT32 runningCommandsOfCurrentType) const; + + protected: + void ExecuteInternal(Execution::Context& context) const override; + }; + // IMPORTANT: To use this command, the caller should have already retrieved the package manifest (GetManifest()) and added it to the Context Data struct COMInstallCommand final : public Command { COMInstallCommand(std::string_view parent) : Command("install", parent) {} + bool IsCommandAllowedToRunNow(std::map& runningCommands, UINT32 runningCommandsOfCurrentType) const; protected: void ExecuteInternal(Execution::Context& context) const override; diff --git a/src/AppInstallerCLICore/ContextOrchestrator.cpp b/src/AppInstallerCLICore/ContextOrchestrator.cpp index f9876d4bcb..1a3a1fcddf 100644 --- a/src/AppInstallerCLICore/ContextOrchestrator.cpp +++ b/src/AppInstallerCLICore/ContextOrchestrator.cpp @@ -59,6 +59,13 @@ namespace AppInstaller::CLI::Execution } } + void ContextOrchestrator::RequeueItem(OrchestratorQueueItem& item) + { + std::lock_guard lockQueue{ m_queueLock }; + + item.SetState(OrchestratorQueueItemState::Queued); + } + void ContextOrchestrator::EnqueueAndRunItem(std::shared_ptr item) { EnqueueItem(item); @@ -76,21 +83,31 @@ namespace AppInstaller::CLI::Execution return {}; } - std::shared_ptr item = m_queueItems.front(); - - // Check if item can be dequeued. - // Since only one item can be installed at a time currently the logic is very simple, - // and can just check if the first item is ready to run. This logic will need to become - // more complicated if multiple operation types (e.g. Download & Install) are added that can - // run simultaneously. - if (item->GetState() != OrchestratorQueueItemState::Queued) + for (auto itr = m_queueItems.cbegin(); itr != m_queueItems.cend(); ++itr) { - return {}; + UINT32 runningCommandsOfNextType = 0; + auto foundVal = m_runningCommandCountMap.find((*itr)->GetNextCommand().NameAsString()); + if (foundVal != m_runningCommandCountMap.cend()) + { + runningCommandsOfNextType = foundVal->second; + } + if ((*itr)->GetNextCommand().IsCommandAllowedToRunNow(m_runningCommandCountMap, runningCommandsOfNextType)) + { + if (foundVal != m_runningCommandCountMap.cend()) + { + foundVal->second = runningCommandsOfNextType + 1; + } + else + { + m_runningCommandCountMap.insert(std::pair((*itr)->GetNextCommand().NameAsString(), 1)); + } + // Running state must be set inside the queueLock so that multiple threads don't try to run the same item. + (*itr)->SetState(OrchestratorQueueItemState::Running); + return *itr; + } } - // Running state must be set inside the queueLock so that multiple threads don't try to run the same item. - item->SetState(OrchestratorQueueItemState::Running); - return item; + return {}; } void ContextOrchestrator::RunItems() @@ -101,11 +118,10 @@ namespace AppInstaller::CLI::Execution HRESULT terminationHR = S_OK; try { - ::AppInstaller::CLI::RootCommand rootCommand; + std::unique_ptr command = item->PopNextCommand(); std::unique_ptr setThreadGlobalsToPreviousState = item->GetContext().GetThreadGlobals().SetForCurrentThread(); - std::unique_ptr<::AppInstaller::CLI::Command> command = std::make_unique<::AppInstaller::CLI::COMInstallCommand>(rootCommand.Name()); item->GetContext().GetThreadGlobals().GetTelemetryLogger().LogCommand(command->FullName()); command->ValidateArguments(item->GetContext().Args); @@ -123,7 +139,14 @@ namespace AppInstaller::CLI::Execution item->GetContext().SetTerminationHR(terminationHR); } - RemoveItemInState(*item, OrchestratorQueueItemState::Running); + if (FAILED(terminationHR) || item->IsComplete()) + { + RemoveItemInState(*item, OrchestratorQueueItemState::Running); + } + else + { + RequeueItem(*item); + } item = GetNextItem(); } @@ -179,8 +202,12 @@ namespace AppInstaller::CLI::Execution } std::unique_ptr OrchestratorQueueItemFactory::CreateItemForInstall(std::wstring packageId, std::wstring sourceId, std::unique_ptr context) - { - return std::make_unique(OrchestratorQueueItemId(std::move(packageId), std::move(sourceId)), std::move(context)); + { + std::unique_ptr item = std::make_unique(OrchestratorQueueItemId(std::move(packageId), std::move(sourceId)), std::move(context)); + ::AppInstaller::CLI::RootCommand rootCommand; + item->AddCommand(std::make_unique<::AppInstaller::CLI::COMDownloadCommand>(rootCommand.Name())); + item->AddCommand(std::make_unique<::AppInstaller::CLI::COMInstallCommand>(rootCommand.Name())); + return item; } } diff --git a/src/AppInstallerCLICore/ContextOrchestrator.h b/src/AppInstallerCLICore/ContextOrchestrator.h index e45d8a2946..ca49fac301 100644 --- a/src/AppInstallerCLICore/ContextOrchestrator.h +++ b/src/AppInstallerCLICore/ContextOrchestrator.h @@ -7,6 +7,7 @@ #include "ExecutionContextData.h" #include "CompletionData.h" #include "COMContext.h" +#include "Commands/COMInstallCommand.h" #include @@ -40,11 +41,24 @@ namespace AppInstaller::CLI::Execution COMContext& GetContext() const { return *m_context; } const wil::unique_event& GetCompletedEvent() const { return m_completedEvent; } const OrchestratorQueueItemId& GetId() const { return m_id; } + void AddCommand(std::unique_ptr command) { m_commands.push_back(std::move(command)); } + const std::string& GetCurrentCommandName() const { return m_currentCommandName; } + Command& GetNextCommand() { return *m_commands.front(); } + std::unique_ptr PopNextCommand() + { + std::unique_ptr command = std::move(m_commands.front()); + m_currentCommandName = command->NameAsString(); + m_commands.pop_front(); + return command; + } + bool IsComplete() const { return m_commands.empty(); } private: OrchestratorQueueItemState m_state = OrchestratorQueueItemState::NotQueued; std::unique_ptr m_context; wil::unique_event m_completedEvent{ wil::EventOptions::ManualReset }; OrchestratorQueueItemId m_id; + std::deque> m_commands; + std::string m_currentCommandName; }; struct OrchestratorQueueItemFactory @@ -67,6 +81,7 @@ namespace AppInstaller::CLI::Execution void RunItems(); std::shared_ptr GetNextItem(); void EnqueueItem(std::shared_ptr item); + void RequeueItem(OrchestratorQueueItem& item); void RemoveItemInState(const OrchestratorQueueItem& item, OrchestratorQueueItemState state); _Requires_lock_held_(m_queueLock) @@ -76,5 +91,6 @@ namespace AppInstaller::CLI::Execution std::shared_ptr<::AppInstaller::Repository::IMutablePackageSource> m_installingWriteableSource = nullptr; std::deque> m_queueItems; + std::map m_runningCommandCountMap; }; } diff --git a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp index f4da58cab8..4bb3136268 100644 --- a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp @@ -607,11 +607,20 @@ namespace AppInstaller::CLI::Workflow Workflow::ShowInstallationDisclaimer; } - void InstallPackageInstaller(Execution::Context& context) + void DownloadPackageVersion(Execution::Context& context) { context << + Workflow::ReportIdentityAndInstallationDisclaimer << + Workflow::ShowPackageAgreements(/* ensureAcceptance */ true) << + Workflow::GetDependenciesFromInstaller << + Workflow::ReportDependencies(Resource::String::InstallAndUpgradeCommandsReportDependencies) << Workflow::ReportExecutionStage(ExecutionStage::Download) << - Workflow::DownloadInstaller << + Workflow::DownloadInstaller; + } + + void InstallPackageInstaller(Execution::Context& context) + { + context << Workflow::ReportExecutionStage(ExecutionStage::PreExecution) << Workflow::SnapshotARPEntries << Workflow::ReportExecutionStage(ExecutionStage::Execution) << @@ -624,10 +633,7 @@ namespace AppInstaller::CLI::Workflow void InstallSinglePackage(Execution::Context& context) { context << - Workflow::ReportIdentityAndInstallationDisclaimer << - Workflow::ShowPackageAgreements(/* ensureAcceptance */ true) << - Workflow::GetDependenciesFromInstaller << - Workflow::ReportDependencies(Resource::String::InstallAndUpgradeCommandsReportDependencies) << + Workflow::DownloadPackageVersion << Workflow::InstallPackageInstaller; } diff --git a/src/AppInstallerCLICore/Workflows/InstallFlow.h b/src/AppInstallerCLICore/Workflows/InstallFlow.h index 1fb2a5240b..aae98fd72d 100644 --- a/src/AppInstallerCLICore/Workflows/InstallFlow.h +++ b/src/AppInstallerCLICore/Workflows/InstallFlow.h @@ -147,6 +147,12 @@ namespace AppInstaller::CLI::Workflow // Outputs: None void ReportIdentityAndInstallationDisclaimer(Execution::Context& context); + // Downloads a specific package installer. + // Required Args: None + // Inputs: Manifest, Installer + // Outputs: None + void DownloadPackageVersion(Execution::Context& context); + // Installs a specific package installer. See also InstallSinglePackage & InstallMultiplePackages. // Required Args: None // Inputs: Manifest, Installer, PackageVersion, InstalledPackageVersion? From 2c120b52c5bb62b29382730bc74f6911cc3bf7a4 Mon Sep 17 00:00:00 2001 From: Flor Elisa Chacon Ochoa Date: Wed, 29 Sep 2021 11:38:34 -0700 Subject: [PATCH 02/13] Add hash re-check when file already exists --- src/AppInstallerCLICore/Command.cpp | 5 --- src/AppInstallerCLICore/Command.h | 5 +-- .../Commands/COMInstallCommand.cpp | 19 ++++---- .../Commands/COMInstallCommand.h | 2 - .../ContextOrchestrator.cpp | 36 ++++++--------- src/AppInstallerCLICore/ContextOrchestrator.h | 11 ++--- .../Workflows/InstallFlow.cpp | 44 +++++++++++++------ .../Workflows/InstallFlow.h | 7 +-- 8 files changed, 57 insertions(+), 72 deletions(-) diff --git a/src/AppInstallerCLICore/Command.cpp b/src/AppInstallerCLICore/Command.cpp index 06111e8435..d0988c10aa 100644 --- a/src/AppInstallerCLICore/Command.cpp +++ b/src/AppInstallerCLICore/Command.cpp @@ -797,11 +797,6 @@ namespace AppInstaller::CLI } } - bool Command::IsCommandAllowedToRunNow(std::map&, UINT32) const - { - return false; - } - void Command::ValidateArgumentsInternal(Execution::Args&) const { // Do nothing by default. diff --git a/src/AppInstallerCLICore/Command.h b/src/AppInstallerCLICore/Command.h index b4bd0f3b84..9225a0ca25 100644 --- a/src/AppInstallerCLICore/Command.h +++ b/src/AppInstallerCLICore/Command.h @@ -78,7 +78,6 @@ namespace AppInstaller::CLI constexpr static char ParentSplitChar = ':'; std::string_view Name() const { return m_name; } - const std::string& NameAsString() const { return m_name; } const std::string& FullName() const { return m_fullName; } Command::Visibility GetVisibility() const; Settings::ExperimentalFeature::Feature Feature() const { return m_feature; } @@ -105,14 +104,12 @@ namespace AppInstaller::CLI virtual void Execute(Execution::Context& context) const; - virtual bool IsCommandAllowedToRunNow(std::map& runningCommands, UINT32 runningCommandsOfCurrentType) const; - protected: virtual void ValidateArgumentsInternal(Execution::Args& execArgs) const; virtual void ExecuteInternal(Execution::Context& context) const; private: - std::string m_name; + std::string_view m_name; std::string m_fullName; Command::Visibility m_visibility; Settings::ExperimentalFeature::Feature m_feature; diff --git a/src/AppInstallerCLICore/Commands/COMInstallCommand.cpp b/src/AppInstallerCLICore/Commands/COMInstallCommand.cpp index 5122f2c319..ca8e027e0f 100644 --- a/src/AppInstallerCLICore/Commands/COMInstallCommand.cpp +++ b/src/AppInstallerCLICore/Commands/COMInstallCommand.cpp @@ -2,6 +2,7 @@ // Licensed under the MIT License. #include "pch.h" #include "COMInstallCommand.h" +#include "Workflows/DependenciesFlow.h" #include "Workflows/InstallFlow.h" #include "Workflows/WorkflowBase.h" @@ -15,11 +16,17 @@ namespace AppInstaller::CLI // IMPORTANT: To use this command, the caller should have already retrieved the package manifest (GetManifest()) and added it to the Context Data void COMDownloadCommand::ExecuteInternal(Context& context) const { + // TODO: Remove duplication with InstallFlow context << Workflow::ReportExecutionStage(ExecutionStage::Discovery) << Workflow::SelectInstaller << Workflow::EnsureApplicableInstaller << - Workflow::DownloadPackageVersion; + Workflow::ReportIdentityAndInstallationDisclaimer << + Workflow::ShowPackageAgreements(/* ensureAcceptance */ true) << + Workflow::GetDependenciesFromInstaller << + Workflow::ReportDependencies(Resource::String::InstallAndUpgradeCommandsReportDependencies) << + Workflow::ReportExecutionStage(ExecutionStage::Download) << + Workflow::DownloadInstaller; } // IMPORTANT: To use this command, the caller should have already retrieved the package manifest (GetManifest()) and added it to the Context Data @@ -28,14 +35,4 @@ namespace AppInstaller::CLI context << Workflow::InstallPackageInstaller; } - - bool COMDownloadCommand::IsCommandAllowedToRunNow(std::map&, UINT32 runningCommandsOfCurrentType) const - { - return (runningCommandsOfCurrentType < 5); - } - - bool COMInstallCommand::IsCommandAllowedToRunNow(std::map&, UINT32 runningCommandsOfCurrentType) const - { - return (runningCommandsOfCurrentType == 0); - } } diff --git a/src/AppInstallerCLICore/Commands/COMInstallCommand.h b/src/AppInstallerCLICore/Commands/COMInstallCommand.h index ca105f8021..f454f4e5b1 100644 --- a/src/AppInstallerCLICore/Commands/COMInstallCommand.h +++ b/src/AppInstallerCLICore/Commands/COMInstallCommand.h @@ -9,7 +9,6 @@ namespace AppInstaller::CLI struct COMDownloadCommand final : public Command { COMDownloadCommand(std::string_view parent) : Command("download", parent) {} - bool IsCommandAllowedToRunNow(std::map& runningCommands, UINT32 runningCommandsOfCurrentType) const; protected: void ExecuteInternal(Execution::Context& context) const override; @@ -19,7 +18,6 @@ namespace AppInstaller::CLI struct COMInstallCommand final : public Command { COMInstallCommand(std::string_view parent) : Command("install", parent) {} - bool IsCommandAllowedToRunNow(std::map& runningCommands, UINT32 runningCommandsOfCurrentType) const; protected: void ExecuteInternal(Execution::Context& context) const override; diff --git a/src/AppInstallerCLICore/ContextOrchestrator.cpp b/src/AppInstallerCLICore/ContextOrchestrator.cpp index 1a3a1fcddf..abf3f3cb35 100644 --- a/src/AppInstallerCLICore/ContextOrchestrator.cpp +++ b/src/AppInstallerCLICore/ContextOrchestrator.cpp @@ -83,31 +83,21 @@ namespace AppInstaller::CLI::Execution return {}; } - for (auto itr = m_queueItems.cbegin(); itr != m_queueItems.cend(); ++itr) + std::shared_ptr item = m_queueItems.front(); + + // Check if item can be dequeued. + // Since only one item can be installed at a time currently the logic is very simple, + // and can just check if the first item is ready to run. This logic will need to become + // more complicated if multiple operation types (e.g. Download & Install) are added that can + // run simultaneously. + if (item->GetState() != OrchestratorQueueItemState::Queued) { - UINT32 runningCommandsOfNextType = 0; - auto foundVal = m_runningCommandCountMap.find((*itr)->GetNextCommand().NameAsString()); - if (foundVal != m_runningCommandCountMap.cend()) - { - runningCommandsOfNextType = foundVal->second; - } - if ((*itr)->GetNextCommand().IsCommandAllowedToRunNow(m_runningCommandCountMap, runningCommandsOfNextType)) - { - if (foundVal != m_runningCommandCountMap.cend()) - { - foundVal->second = runningCommandsOfNextType + 1; - } - else - { - m_runningCommandCountMap.insert(std::pair((*itr)->GetNextCommand().NameAsString(), 1)); - } - // Running state must be set inside the queueLock so that multiple threads don't try to run the same item. - (*itr)->SetState(OrchestratorQueueItemState::Running); - return *itr; - } + return {}; } - return {}; + // Running state must be set inside the queueLock so that multiple threads don't try to run the same item. + item->SetState(OrchestratorQueueItemState::Running); + return item; } void ContextOrchestrator::RunItems() @@ -139,6 +129,8 @@ namespace AppInstaller::CLI::Execution item->GetContext().SetTerminationHR(terminationHR); } + item->GetContext().EnableCtrlHandler(false); + if (FAILED(terminationHR) || item->IsComplete()) { RemoveItemInState(*item, OrchestratorQueueItemState::Running); diff --git a/src/AppInstallerCLICore/ContextOrchestrator.h b/src/AppInstallerCLICore/ContextOrchestrator.h index ca49fac301..1123e10831 100644 --- a/src/AppInstallerCLICore/ContextOrchestrator.h +++ b/src/AppInstallerCLICore/ContextOrchestrator.h @@ -6,8 +6,8 @@ #include "ExecutionArgs.h" #include "ExecutionContextData.h" #include "CompletionData.h" +#include "Command.h" #include "COMContext.h" -#include "Commands/COMInstallCommand.h" #include @@ -42,12 +42,9 @@ namespace AppInstaller::CLI::Execution const wil::unique_event& GetCompletedEvent() const { return m_completedEvent; } const OrchestratorQueueItemId& GetId() const { return m_id; } void AddCommand(std::unique_ptr command) { m_commands.push_back(std::move(command)); } - const std::string& GetCurrentCommandName() const { return m_currentCommandName; } - Command& GetNextCommand() { return *m_commands.front(); } - std::unique_ptr PopNextCommand() - { + std::unique_ptr PopNextCommand() + { std::unique_ptr command = std::move(m_commands.front()); - m_currentCommandName = command->NameAsString(); m_commands.pop_front(); return command; } @@ -58,7 +55,6 @@ namespace AppInstaller::CLI::Execution wil::unique_event m_completedEvent{ wil::EventOptions::ManualReset }; OrchestratorQueueItemId m_id; std::deque> m_commands; - std::string m_currentCommandName; }; struct OrchestratorQueueItemFactory @@ -91,6 +87,5 @@ namespace AppInstaller::CLI::Execution std::shared_ptr<::AppInstaller::Repository::IMutablePackageSource> m_installingWriteableSource = nullptr; std::deque> m_queueItems; - std::map m_runningCommandCountMap; }; } diff --git a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp index 4bb3136268..d722fd9e6c 100644 --- a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp @@ -255,13 +255,35 @@ namespace AppInstaller::CLI::Workflow std::filesystem::path tempInstallerPath = Runtime::GetPathTo(Runtime::PathName::Temp); tempInstallerPath /= Utility::ConvertToUTF16(manifest.Id + '.' + manifest.Version); + AICLI_LOG(CLI, Info, << "Generated temp download path: " << tempInstallerPath); + + // Check if file was already downloaded. + // This may happen after a failed installation or if the download was done + // separately before, e.g. on COM scenarios. + // TODO: The installer gets renamed before executing it. This check could be extended + // to consider the renamed file. + if (std::filesystem::exists(tempInstallerPath)) + { + AICLI_LOG(CLI, Info, << "Installer already downloaded. Verifying hash."); + std::ifstream inStream{ tempInstallerPath, std::ifstream::binary }; + auto existingFileHash = SHA256::ComputeHash(inStream); + + if (std::equal(installer.Sha256.begin(), installer.Sha256.end(), existingFileHash.begin())) + { + AICLI_LOG(CLI, Info, << "Hash matches. Will use existing installer."); + context.Add(std::make_pair(installer.Sha256, existingFileHash)); + context.Add(std::move(tempInstallerPath)); + return; + } + + AICLI_LOG(CLI, Info, << "Hash mismatch. Will download installer."); + } + Utility::DownloadInfo downloadInfo{}; downloadInfo.DisplayName = Resource::GetFixedString(Resource::FixedString::ProductName); // Use the SHA256 hash of the installer as the identifier for the download downloadInfo.ContentId = SHA256::ConvertToString(installer.Sha256); - AICLI_LOG(CLI, Info, << "Generated temp download path: " << tempInstallerPath); - context.Reporter.Info() << "Downloading " << Execution::UrlEmphasis << installer.Url << std::endl; std::optional> hash; @@ -607,20 +629,11 @@ namespace AppInstaller::CLI::Workflow Workflow::ShowInstallationDisclaimer; } - void DownloadPackageVersion(Execution::Context& context) - { - context << - Workflow::ReportIdentityAndInstallationDisclaimer << - Workflow::ShowPackageAgreements(/* ensureAcceptance */ true) << - Workflow::GetDependenciesFromInstaller << - Workflow::ReportDependencies(Resource::String::InstallAndUpgradeCommandsReportDependencies) << - Workflow::ReportExecutionStage(ExecutionStage::Download) << - Workflow::DownloadInstaller; - } - void InstallPackageInstaller(Execution::Context& context) { context << + Workflow::ReportExecutionStage(ExecutionStage::Download) << + Workflow::DownloadInstaller << Workflow::ReportExecutionStage(ExecutionStage::PreExecution) << Workflow::SnapshotARPEntries << Workflow::ReportExecutionStage(ExecutionStage::Execution) << @@ -633,7 +646,10 @@ namespace AppInstaller::CLI::Workflow void InstallSinglePackage(Execution::Context& context) { context << - Workflow::DownloadPackageVersion << + Workflow::ReportIdentityAndInstallationDisclaimer << + Workflow::ShowPackageAgreements(/* ensureAcceptance */ true) << + Workflow::GetDependenciesFromInstaller << + Workflow::ReportDependencies(Resource::String::InstallAndUpgradeCommandsReportDependencies) << Workflow::InstallPackageInstaller; } diff --git a/src/AppInstallerCLICore/Workflows/InstallFlow.h b/src/AppInstallerCLICore/Workflows/InstallFlow.h index aae98fd72d..14692bbb59 100644 --- a/src/AppInstallerCLICore/Workflows/InstallFlow.h +++ b/src/AppInstallerCLICore/Workflows/InstallFlow.h @@ -67,6 +67,7 @@ namespace AppInstaller::CLI::Workflow void DownloadInstaller(Execution::Context& context); // Downloads the file referenced by the Installer. + // Do nothing if the file has already been downloaded. // Required Args: None // Inputs: Installer // Outputs: HashPair, InstallerPath @@ -147,12 +148,6 @@ namespace AppInstaller::CLI::Workflow // Outputs: None void ReportIdentityAndInstallationDisclaimer(Execution::Context& context); - // Downloads a specific package installer. - // Required Args: None - // Inputs: Manifest, Installer - // Outputs: None - void DownloadPackageVersion(Execution::Context& context); - // Installs a specific package installer. See also InstallSinglePackage & InstallMultiplePackages. // Required Args: None // Inputs: Manifest, Installer, PackageVersion, InstalledPackageVersion? From ef62a7f817588e3e5881cead56220a8d7850b58c Mon Sep 17 00:00:00 2001 From: Flor Elisa Chacon Ochoa Date: Wed, 29 Sep 2021 14:07:37 -0700 Subject: [PATCH 03/13] Skip removing motw if not present --- src/AppInstallerCommonCore/Downloader.cpp | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/AppInstallerCommonCore/Downloader.cpp b/src/AppInstallerCommonCore/Downloader.cpp index 4fc2e0e6d3..f923f81e1c 100644 --- a/src/AppInstallerCommonCore/Downloader.cpp +++ b/src/AppInstallerCommonCore/Downloader.cpp @@ -275,11 +275,20 @@ namespace AppInstaller::Utility Microsoft::WRL::ComPtr zoneIdentifier; THROW_IF_FAILED(CoCreateInstance(CLSID_PersistentZoneIdentifier, nullptr, CLSCTX_INPROC_SERVER, IID_PPV_ARGS(&zoneIdentifier))); - THROW_IF_FAILED(zoneIdentifier->Remove()); Microsoft::WRL::ComPtr persistFile; THROW_IF_FAILED(zoneIdentifier.As(&persistFile)); - THROW_IF_FAILED(persistFile->Save(filePath.c_str(), TRUE)); + + auto hr = persistFile->Load(filePath.c_str(), STGM_READ); + if (hr == HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND)) + { + AICLI_LOG(Core, Info, << "File does not contain motw. Skipped removing motw"); + return; + } + + THROW_IF_FAILED(hr); + THROW_IF_FAILED(zoneIdentifier->Remove()); + THROW_IF_FAILED(persistFile->Save(NULL, TRUE)); AICLI_LOG(Core, Info, << "Finished removing motw"); } From 1e9df7d1b38e95e206b0fa34a5776f3fec72814c Mon Sep 17 00:00:00 2001 From: Flor Elisa Chacon Ochoa Date: Wed, 29 Sep 2021 14:24:46 -0700 Subject: [PATCH 04/13] Generate installer path only once --- .../Workflows/InstallFlow.cpp | 35 +++++++++++++------ .../Workflows/InstallFlow.h | 9 ++++- 2 files changed, 32 insertions(+), 12 deletions(-) diff --git a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp index d722fd9e6c..a68d76d745 100644 --- a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp @@ -247,32 +247,46 @@ namespace AppInstaller::CLI::Workflow } } - void DownloadInstallerFile(Execution::Context& context) + void GetInstallerDownloadPath(Execution::Context& context) { - const auto& manifest = context.Get(); - const auto& installer = context.Get().value(); + if (!context.Contains(Execution::Data::InstallerPath)) + { + const auto& manifest = context.Get(); + + std::filesystem::path tempInstallerPath = Runtime::GetPathTo(Runtime::PathName::Temp); + tempInstallerPath /= Utility::ConvertToUTF16(manifest.Id + '.' + manifest.Version); + + AICLI_LOG(CLI, Info, << "Generated temp download path: " << tempInstallerPath); + context.Add(std::move(tempInstallerPath)); + } + } - std::filesystem::path tempInstallerPath = Runtime::GetPathTo(Runtime::PathName::Temp); - tempInstallerPath /= Utility::ConvertToUTF16(manifest.Id + '.' + manifest.Version); + void DownloadInstallerFile(Execution::Context& context) + { + context << GetInstallerDownloadPath; + if (context.IsTerminated()) + { + return; + } - AICLI_LOG(CLI, Info, << "Generated temp download path: " << tempInstallerPath); + const auto& installer = context.Get().value(); + const auto& installerPath = context.Get(); // Check if file was already downloaded. // This may happen after a failed installation or if the download was done // separately before, e.g. on COM scenarios. // TODO: The installer gets renamed before executing it. This check could be extended // to consider the renamed file. - if (std::filesystem::exists(tempInstallerPath)) + if (std::filesystem::exists(installerPath)) { AICLI_LOG(CLI, Info, << "Installer already downloaded. Verifying hash."); - std::ifstream inStream{ tempInstallerPath, std::ifstream::binary }; + std::ifstream inStream{ installerPath, std::ifstream::binary }; auto existingFileHash = SHA256::ComputeHash(inStream); if (std::equal(installer.Sha256.begin(), installer.Sha256.end(), existingFileHash.begin())) { AICLI_LOG(CLI, Info, << "Hash matches. Will use existing installer."); context.Add(std::make_pair(installer.Sha256, existingFileHash)); - context.Add(std::move(tempInstallerPath)); return; } @@ -296,7 +310,7 @@ namespace AppInstaller::CLI::Workflow { hash = context.Reporter.ExecuteWithProgress(std::bind(Utility::Download, installer.Url, - tempInstallerPath, + installerPath, Utility::DownloadType::Installer, std::placeholders::_1, true, @@ -330,7 +344,6 @@ namespace AppInstaller::CLI::Workflow } context.Add(std::make_pair(installer.Sha256, hash.value())); - context.Add(std::move(tempInstallerPath)); } void GetMsixSignatureHash(Execution::Context& context) diff --git a/src/AppInstallerCLICore/Workflows/InstallFlow.h b/src/AppInstallerCLICore/Workflows/InstallFlow.h index 14692bbb59..885820bc0c 100644 --- a/src/AppInstallerCLICore/Workflows/InstallFlow.h +++ b/src/AppInstallerCLICore/Workflows/InstallFlow.h @@ -66,10 +66,17 @@ namespace AppInstaller::CLI::Workflow // Outputs: None void DownloadInstaller(Execution::Context& context); + // Computes the download path for the installer file. + // Does nothing if already determined. + // Required Args: None + // Inputs: Manifest + // Outputs: InstallerPath + void GetInstallerDownloadPath(Execution::Context& context); + // Downloads the file referenced by the Installer. // Do nothing if the file has already been downloaded. // Required Args: None - // Inputs: Installer + // Inputs: Installer, Manifest // Outputs: HashPair, InstallerPath void DownloadInstallerFile(Execution::Context& context); From 07d6b754c0f147d64f5d388d7b77f4b1adb363a3 Mon Sep 17 00:00:00 2001 From: Flor Elisa Chacon Ochoa Date: Wed, 29 Sep 2021 14:44:06 -0700 Subject: [PATCH 05/13] Update comment --- src/AppInstallerCLICore/Commands/COMInstallCommand.cpp | 2 +- src/AppInstallerCLICore/ContextOrchestrator.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/AppInstallerCLICore/Commands/COMInstallCommand.cpp b/src/AppInstallerCLICore/Commands/COMInstallCommand.cpp index ca8e027e0f..f0df22bdc7 100644 --- a/src/AppInstallerCLICore/Commands/COMInstallCommand.cpp +++ b/src/AppInstallerCLICore/Commands/COMInstallCommand.cpp @@ -29,7 +29,7 @@ namespace AppInstaller::CLI Workflow::DownloadInstaller; } - // IMPORTANT: To use this command, the caller should have already retrieved the package manifest (GetManifest()) and added it to the Context Data + // IMPORTANT: To use this command, the caller should have already executed the COMDownloadCommand void COMInstallCommand::ExecuteInternal(Context& context) const { context << diff --git a/src/AppInstallerCLICore/ContextOrchestrator.cpp b/src/AppInstallerCLICore/ContextOrchestrator.cpp index abf3f3cb35..fb4a99a601 100644 --- a/src/AppInstallerCLICore/ContextOrchestrator.cpp +++ b/src/AppInstallerCLICore/ContextOrchestrator.cpp @@ -194,7 +194,7 @@ namespace AppInstaller::CLI::Execution } std::unique_ptr OrchestratorQueueItemFactory::CreateItemForInstall(std::wstring packageId, std::wstring sourceId, std::unique_ptr context) - { + { std::unique_ptr item = std::make_unique(OrchestratorQueueItemId(std::move(packageId), std::move(sourceId)), std::move(context)); ::AppInstaller::CLI::RootCommand rootCommand; item->AddCommand(std::make_unique<::AppInstaller::CLI::COMDownloadCommand>(rootCommand.Name())); From 8e7641a6577f13606916e02792a2fdb9ef4ef083 Mon Sep 17 00:00:00 2001 From: Flor Elisa Chacon Ochoa Date: Wed, 29 Sep 2021 17:20:29 -0700 Subject: [PATCH 06/13] Spelling --- .github/actions/spelling/allow.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/actions/spelling/allow.txt b/.github/actions/spelling/allow.txt index 79ba88574a..6163cccbff 100644 --- a/.github/actions/spelling/allow.txt +++ b/.github/actions/spelling/allow.txt @@ -385,6 +385,7 @@ regex regexp removemanifest repolibtest +requeue rescap resheader resmimetype From 5d93b690950f4146dd78a88fe3fa1a9752d0742b Mon Sep 17 00:00:00 2001 From: Flor Elisa Chacon Ochoa Date: Fri, 8 Oct 2021 15:34:19 -0700 Subject: [PATCH 07/13] Comments from PR --- .../AppInstallerCLICore.vcxproj | 2 + .../AppInstallerCLICore.vcxproj.filters | 6 + .../Commands/COMInstallCommand.cpp | 11 +- .../Commands/RootCommand.h | 4 +- .../ContextOrchestrator.cpp | 5 +- .../Workflows/DownloadFlow.cpp | 420 ++++++++++++++++++ .../Workflows/DownloadFlow.h | 59 +++ .../Workflows/InstallFlow.cpp | 296 +----------- .../Workflows/InstallFlow.h | 55 +-- .../ShellExecuteInstallerHandler.cpp | 87 ---- .../Workflows/ShellExecuteInstallerHandler.h | 8 - src/AppInstallerCLITests/WorkFlow.cpp | 9 +- src/AppInstallerCommonCore/Downloader.cpp | 1 - 13 files changed, 516 insertions(+), 447 deletions(-) create mode 100644 src/AppInstallerCLICore/Workflows/DownloadFlow.cpp create mode 100644 src/AppInstallerCLICore/Workflows/DownloadFlow.h diff --git a/src/AppInstallerCLICore/AppInstallerCLICore.vcxproj b/src/AppInstallerCLICore/AppInstallerCLICore.vcxproj index 6156f71782..8fb56be603 100644 --- a/src/AppInstallerCLICore/AppInstallerCLICore.vcxproj +++ b/src/AppInstallerCLICore/AppInstallerCLICore.vcxproj @@ -275,6 +275,7 @@ + @@ -324,6 +325,7 @@ + diff --git a/src/AppInstallerCLICore/AppInstallerCLICore.vcxproj.filters b/src/AppInstallerCLICore/AppInstallerCLICore.vcxproj.filters index 23bf4b6e8a..35e3bed028 100644 --- a/src/AppInstallerCLICore/AppInstallerCLICore.vcxproj.filters +++ b/src/AppInstallerCLICore/AppInstallerCLICore.vcxproj.filters @@ -170,6 +170,9 @@ Workflows + + Workflows + @@ -307,6 +310,9 @@ Workflows + + Workflows + diff --git a/src/AppInstallerCLICore/Commands/COMInstallCommand.cpp b/src/AppInstallerCLICore/Commands/COMInstallCommand.cpp index f0df22bdc7..e8ad55171a 100644 --- a/src/AppInstallerCLICore/Commands/COMInstallCommand.cpp +++ b/src/AppInstallerCLICore/Commands/COMInstallCommand.cpp @@ -2,7 +2,7 @@ // Licensed under the MIT License. #include "pch.h" #include "COMInstallCommand.h" -#include "Workflows/DependenciesFlow.h" +#include "Workflows/DownloadFlow.h" #include "Workflows/InstallFlow.h" #include "Workflows/WorkflowBase.h" @@ -21,18 +21,15 @@ namespace AppInstaller::CLI Workflow::ReportExecutionStage(ExecutionStage::Discovery) << Workflow::SelectInstaller << Workflow::EnsureApplicableInstaller << - Workflow::ReportIdentityAndInstallationDisclaimer << - Workflow::ShowPackageAgreements(/* ensureAcceptance */ true) << - Workflow::GetDependenciesFromInstaller << - Workflow::ReportDependencies(Resource::String::InstallAndUpgradeCommandsReportDependencies) << - Workflow::ReportExecutionStage(ExecutionStage::Download) << - Workflow::DownloadInstaller; + Workflow::DownloadSinglePackage; } // IMPORTANT: To use this command, the caller should have already executed the COMDownloadCommand void COMInstallCommand::ExecuteInternal(Context& context) const { context << + Workflow::GetInstallerHash << + Workflow::VerifyInstallerHash << Workflow::InstallPackageInstaller; } } diff --git a/src/AppInstallerCLICore/Commands/RootCommand.h b/src/AppInstallerCLICore/Commands/RootCommand.h index 2f7a38e1e9..87d051eee6 100644 --- a/src/AppInstallerCLICore/Commands/RootCommand.h +++ b/src/AppInstallerCLICore/Commands/RootCommand.h @@ -7,7 +7,9 @@ namespace AppInstaller::CLI { struct RootCommand final : public Command { - RootCommand() : Command("root", {}) {} + constexpr static std::string_view CommandName = "root"sv; + + RootCommand() : Command(CommandName, {}) {} std::vector> GetCommands() const override; std::vector GetArguments() const override; diff --git a/src/AppInstallerCLICore/ContextOrchestrator.cpp b/src/AppInstallerCLICore/ContextOrchestrator.cpp index fb4a99a601..b8f0c01216 100644 --- a/src/AppInstallerCLICore/ContextOrchestrator.cpp +++ b/src/AppInstallerCLICore/ContextOrchestrator.cpp @@ -196,9 +196,8 @@ namespace AppInstaller::CLI::Execution std::unique_ptr OrchestratorQueueItemFactory::CreateItemForInstall(std::wstring packageId, std::wstring sourceId, std::unique_ptr context) { std::unique_ptr item = std::make_unique(OrchestratorQueueItemId(std::move(packageId), std::move(sourceId)), std::move(context)); - ::AppInstaller::CLI::RootCommand rootCommand; - item->AddCommand(std::make_unique<::AppInstaller::CLI::COMDownloadCommand>(rootCommand.Name())); - item->AddCommand(std::make_unique<::AppInstaller::CLI::COMInstallCommand>(rootCommand.Name())); + item->AddCommand(std::make_unique<::AppInstaller::CLI::COMDownloadCommand>(RootCommand::CommandName)); + item->AddCommand(std::make_unique<::AppInstaller::CLI::COMInstallCommand>(RootCommand::CommandName)); return item; } diff --git a/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp b/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp new file mode 100644 index 0000000000..1a65983934 --- /dev/null +++ b/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp @@ -0,0 +1,420 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. +#include "pch.h" +#include "DownloadFlow.h" + +#include + +namespace AppInstaller::CLI::Workflow +{ + using namespace AppInstaller::Manifest; + using namespace AppInstaller::Repository; + using namespace AppInstaller::Utility; + + namespace + { + + // Complicated rename algorithm due to somewhat arbitrary failures. + // 1. First, try to rename. + // 2. Then, create an empty file for the target, and attempt to rename. + // 3. Then, try repeatedly for 500ms in case it is a timing thing. + // 4. Attempt to use a hard link if available. + // 5. Copy the file if nothing else has worked so far. + void RenameFile(const std::filesystem::path& from, const std::filesystem::path& to) + { + // 1. First, try to rename. + try + { + // std::filesystem::rename() handles motw correctly if applicable. + std::filesystem::rename(from, to); + return; + } + CATCH_LOG(); + + // 2. Then, create an empty file for the target, and attempt to rename. + // This seems to fix things in certain cases, so we do it. + try + { + { + std::ofstream targetFile{ to }; + } + std::filesystem::rename(from, to); + return; + } + CATCH_LOG(); + + // 3. Then, try repeatedly for 500ms in case it is a timing thing. + for (int i = 0; i < 5; ++i) + { + try + { + std::this_thread::sleep_for(100ms); + std::filesystem::rename(from, to); + return; + } + CATCH_LOG(); + } + + // 4. Attempt to use a hard link if available. + if (Runtime::SupportsHardLinks(from)) + { + try + { + // Create a hard link to the file; the installer will be left in the temp directory afterward + // but it is better to succeed the operation and leave a file around than to fail. + // First we have to remove the target file as the function will not overwrite. + std::filesystem::remove(to); + std::filesystem::create_hard_link(from, to); + return; + } + CATCH_LOG(); + } + + // 5. Copy the file if nothing else has worked so far. + // Create a copy of the file; the installer will be left in the temp directory afterward + // but it is better to succeed the operation and leave a file around than to fail. + std::filesystem::copy_file(from, to, std::filesystem::copy_options::overwrite_existing); + } + } + + void DownloadInstaller(Execution::Context& context) + { + context << ReportExecutionStage(ExecutionStage::Download); + if (context.IsTerminated()) + { + return; + } + + const auto& installer = context.Get().value(); + + switch (installer.InstallerType) + { + case InstallerTypeEnum::Exe: + case InstallerTypeEnum::Burn: + case InstallerTypeEnum::Inno: + case InstallerTypeEnum::Msi: + case InstallerTypeEnum::Nullsoft: + case InstallerTypeEnum::Wix: + context << DownloadInstallerFile << VerifyInstallerHash << UpdateInstallerFileMotwIfApplicable; + break; + case InstallerTypeEnum::Msix: + if (installer.SignatureSha256.empty()) + { + context << DownloadInstallerFile << VerifyInstallerHash << UpdateInstallerFileMotwIfApplicable; + } + else + { + // Signature hash provided. No download needed. Just verify signature hash. + context << GetMsixSignatureHash << VerifyInstallerHash << UpdateInstallerFileMotwIfApplicable; + } + break; + case InstallerTypeEnum::MSStore: + // Nothing to do here + break; + default: + THROW_HR(HRESULT_FROM_WIN32(ERROR_NOT_SUPPORTED)); + } + } + + void GetInstallerDownloadPath(Execution::Context& context) + { + if (!context.Contains(Execution::Data::InstallerPath)) + { + const auto& manifest = context.Get(); + const auto& installer = context.Get(); + + std::filesystem::path tempInstallerPath = Runtime::GetPathTo(Runtime::PathName::Temp); + tempInstallerPath /= Utility::ConvertToUTF16(manifest.Id + '.' + manifest.Version); + + switch (installer->InstallerType) + { + case InstallerTypeEnum::Burn: + case InstallerTypeEnum::Exe: + case InstallerTypeEnum::Inno: + case InstallerTypeEnum::Nullsoft: + tempInstallerPath += L".exe"; + break; + case InstallerTypeEnum::Msi: + case InstallerTypeEnum::Wix: + tempInstallerPath += L".msi"; + break; + case InstallerTypeEnum::Msix: + { + Msix::MsixInfo msixInfo(installer->Url); + tempInstallerPath += msixInfo.GetIsBundle() ? L".msixbundle" : L".msix"; + break; + } + case InstallerTypeEnum::Zip: + tempInstallerPath += L".zip"; + break; + } + + AICLI_LOG(CLI, Info, << "Generated temp download path: " << tempInstallerPath); + context.Add(std::move(tempInstallerPath)); + } + } + + void DownloadInstallerFile(Execution::Context& context) + { + context << GetInstallerDownloadPath; + if (context.IsTerminated()) + { + return; + } + + const auto& installer = context.Get().value(); + const auto& installerPath = context.Get(); + + // Check if file was already downloaded. + // This may happen after a failed installation or if the download was done + // separately before, e.g. on COM scenarios. + if (std::filesystem::exists(installerPath)) + { + AICLI_LOG(CLI, Info, << "Installer already downloaded. Verifying hash."); + std::ifstream inStream{ installerPath, std::ifstream::binary }; + auto existingFileHash = SHA256::ComputeHash(inStream); + + if (std::equal(installer.Sha256.begin(), installer.Sha256.end(), existingFileHash.begin())) + { + AICLI_LOG(CLI, Info, << "Hash matches. Will use existing installer."); + context.Add(std::make_pair(installer.Sha256, existingFileHash)); + return; + } + + AICLI_LOG(CLI, Info, << "Hash mismatch. Will download installer again. Removing existing installer file."); + context << RemoveInstaller; + if (context.IsTerminated()) + { + return; + } + } + + Utility::DownloadInfo downloadInfo{}; + downloadInfo.DisplayName = Resource::GetFixedString(Resource::FixedString::ProductName); + // Use the SHA256 hash of the installer as the identifier for the download + downloadInfo.ContentId = SHA256::ConvertToString(installer.Sha256); + + context.Reporter.Info() << "Downloading " << Execution::UrlEmphasis << installer.Url << std::endl; + + std::optional> hash; + + const int MaxRetryCount = 2; + for (int retryCount = 0; retryCount < MaxRetryCount; ++retryCount) + { + bool success = false; + try + { + hash = context.Reporter.ExecuteWithProgress(std::bind(Utility::Download, + installer.Url, + installerPath, + Utility::DownloadType::Installer, + std::placeholders::_1, + true, + downloadInfo)); + + success = true; + } + catch (...) + { + if (retryCount < MaxRetryCount - 1) + { + AICLI_LOG(CLI, Info, << "Failed to download, waiting a bit and retry. Url: " << installer.Url); + Sleep(500); + } + else + { + throw; + } + } + + if (success) + { + break; + } + } + + if (!hash) + { + context.Reporter.Info() << "Package download canceled." << std::endl; + AICLI_TERMINATE_CONTEXT(E_ABORT); + } + + context.Add(std::make_pair(installer.Sha256, hash.value())); + } + + void GetMsixSignatureHash(Execution::Context& context) + { + // We use this when the server won't support streaming install to swap to download. + bool downloadInstead = false; + + try + { + const auto& installer = context.Get().value(); + + Msix::MsixInfo msixInfo(installer.Url); + auto signature = msixInfo.GetSignature(); + + auto signatureHash = SHA256::ComputeHash(signature.data(), static_cast(signature.size())); + + context.Add(std::make_pair(installer.SignatureSha256, signatureHash)); + } + catch (const winrt::hresult_error& e) + { + if (static_cast(e.code()) == HRESULT_FROM_WIN32(ERROR_NO_RANGES_PROCESSED) || + HRESULT_FACILITY(e.code()) == FACILITY_HTTP) + { + // Failed to get signature hash through HttpStream, use download + downloadInstead = true; + } + else + { + throw; + } + } + + if (downloadInstead) + { + context << DownloadInstallerFile; + } + } + + void VerifyInstallerHash(Execution::Context& context) + { + const auto& hashPair = context.Get(); + + if (!std::equal( + hashPair.first.begin(), + hashPair.first.end(), + hashPair.second.begin())) + { + bool overrideHashMismatch = context.Args.Contains(Execution::Args::Type::HashOverride); + + const auto& manifest = context.Get(); + Logging::Telemetry().LogInstallerHashMismatch(manifest.Id, manifest.Version, manifest.Channel, hashPair.first, hashPair.second, overrideHashMismatch); + + // If running as admin, do not allow the user to override the hash failure. + if (Runtime::IsRunningAsAdmin()) + { + context.Reporter.Error() << Resource::String::InstallerHashMismatchAdminBlock << std::endl; + } + else if (overrideHashMismatch) + { + context.Reporter.Warn() << Resource::String::InstallerHashMismatchOverridden << std::endl; + return; + } + else if (Settings::GroupPolicies().IsEnabled(Settings::TogglePolicy::Policy::HashOverride)) + { + context.Reporter.Error() << Resource::String::InstallerHashMismatchOverrideRequired << std::endl; + } + else + { + context.Reporter.Error() << Resource::String::InstallerHashMismatchError << std::endl; + } + + AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_INSTALLER_HASH_MISMATCH); + } + else + { + AICLI_LOG(CLI, Info, << "Installer hash verified"); + context.Reporter.Info() << Resource::String::InstallerHashVerified << std::endl; + + context.SetFlags(Execution::ContextFlag::InstallerHashMatched); + + if (context.Contains(Execution::Data::PackageVersion) && + context.Get()->GetSource() != nullptr && + WI_IsFlagSet(context.Get()->GetSource()->GetDetails().TrustLevel, SourceTrustLevel::Trusted)) + { + context.SetFlags(Execution::ContextFlag::InstallerTrusted); + } + } + } + + void UpdateInstallerFileMotwIfApplicable(Execution::Context& context) + { + if (context.Contains(Execution::Data::InstallerPath)) + { + if (WI_IsFlagSet(context.GetFlags(), Execution::ContextFlag::InstallerTrusted)) + { + Utility::ApplyMotwIfApplicable(context.Get(), URLZONE_TRUSTED); + } + else if (WI_IsFlagSet(context.GetFlags(), Execution::ContextFlag::InstallerHashMatched)) + { + const auto& installer = context.Get(); + HRESULT hr = Utility::ApplyMotwUsingIAttachmentExecuteIfApplicable(context.Get(), installer.value().Url, URLZONE_INTERNET); + + // Not using SUCCEEDED(hr) to check since there are cases file is missing after a successful scan + if (hr != S_OK) + { + switch (hr) + { + case INET_E_SECURITY_PROBLEM: + context.Reporter.Error() << Resource::String::InstallerBlockedByPolicy << std::endl; + break; + case E_FAIL: + context.Reporter.Error() << Resource::String::InstallerFailedVirusScan << std::endl; + break; + default: + context.Reporter.Error() << Resource::String::InstallerFailedSecurityCheck << std::endl; + } + + AICLI_LOG(Fail, Error, << "Installer failed security check. Url: " << installer.value().Url << " Result: " << WINGET_OSTREAM_FORMAT_HRESULT(hr)); + AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_INSTALLER_SECURITY_CHECK_FAILED); + } + } + } + } + + void GetInstallerHash(Execution::Context& context) + { + const auto& installer = context.Get().value(); + + if (context.Contains(Execution::Data::InstallerPath)) + { + // Get the hash from the installer file + const auto& installerPath = context.Get(); + std::ifstream inStream{ installerPath, std::ifstream::binary }; + auto existingFileHash = SHA256::ComputeHash(inStream); + context.Add(std::make_pair(installer.Sha256, existingFileHash)); + } + else if (installer.InstallerType == InstallerTypeEnum::MSStore) + { + // No installer file in this case + return; + } + else if (installer.InstallerType == InstallerTypeEnum::Msix && !installer.SignatureSha256.empty()) + { + // We didn't download the installer file before. Just verify the signature hash again. + context << GetMsixSignatureHash; + } + else + { + // No installer downloaded + AICLI_LOG(CLI, Error, << "Installer file not found."); + AICLI_TERMINATE_CONTEXT(HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND)); + } + } + + void RemoveInstaller(Execution::Context& context) + { + // Path may not be present if installed from a URL for MSIX + if (context.Contains(Execution::Data::InstallerPath)) + { + const auto& path = context.Get(); + AICLI_LOG(CLI, Info, << "Removing installer: " << path); + + try + { + // best effort + std::filesystem::remove(path); + } + catch (const std::exception& e) + { + AICLI_LOG(CLI, Warning, << "Failed to remove installer file. Reason: " << e.what()); + } + catch (...) + { + AICLI_LOG(CLI, Warning, << "Failed to remove installer file. Reason unknown."); + } + } + } +} diff --git a/src/AppInstallerCLICore/Workflows/DownloadFlow.h b/src/AppInstallerCLICore/Workflows/DownloadFlow.h new file mode 100644 index 0000000000..832b25de49 --- /dev/null +++ b/src/AppInstallerCLICore/Workflows/DownloadFlow.h @@ -0,0 +1,59 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. +#pragma once +#include "ExecutionContext.h" + +namespace AppInstaller::CLI::Workflow +{ + // Composite flow that chooses what to do based on the installer type. + // Required Args: None + // Inputs: Manifest, Installer + // Outputs: None + void DownloadInstaller(Execution::Context& context); + + // Computes the download path for the installer file. Does nothing if already determined. + // This adds appropriate extension to the installer path, as ShellExecute uses file extension + // to launch the installer appropriately. + // Required Args: None + // Inputs: Installer, Manifest + // Outputs: InstallerPath + void GetInstallerDownloadPath(Execution::Context& context); + + // Downloads the file referenced by the Installer. + // Do nothing if the file has already been downloaded. + // Required Args: None + // Inputs: Installer, Manifest + // Outputs: HashPair, InstallerPath + void DownloadInstallerFile(Execution::Context& context); + + // Computes the hash of the MSIX signature file. + // Required Args: None + // Inputs: Installer + // Outputs: HashPair + void GetMsixSignatureHash(Execution::Context& context); + + // Gets the hash of the downloaded installer. + // Downloading already computes the hash, so this is only needed to re-verify the installer hash. + // Required Args: None + // Inputs: InstallerPath, Installer + // Outputs: HashPair + void GetInstallerHash(Execution::Context& context); + + // Verifies that the downloaded installer hash matches the hash in the manifest. + // Required Args: None + // Inputs: HashPair + // Outputs: None + void VerifyInstallerHash(Execution::Context& context); + + // Update Motw of the downloaded installer if applicable + // Required Args: None + // Inputs: HashPair, InstallerPath?, SourceId? + // Outputs: None + void UpdateInstallerFileMotwIfApplicable(Execution::Context& context); + + // Deletes the installer file. + // Required Args: None + // Inputs: InstallerPath + // Outputs: None + void RemoveInstaller(Execution::Context& context); +} diff --git a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp index a68d76d745..c5a9f4ad71 100644 --- a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp @@ -2,6 +2,7 @@ // Licensed under the MIT License. #include "pch.h" #include "InstallFlow.h" +#include "DownloadFlow.h" #include "UninstallFlow.h" #include "ShowFlow.h" #include "Resources.h" @@ -12,7 +13,6 @@ #include "Workflows/DependenciesFlow.h" #include -#include namespace AppInstaller::CLI::Workflow { @@ -20,7 +20,6 @@ namespace AppInstaller::CLI::Workflow using namespace winrt::Windows::Foundation; using namespace winrt::Windows::Foundation::Collections; using namespace winrt::Windows::Management::Deployment; - using namespace AppInstaller::Utility; using namespace AppInstaller::Manifest; using namespace AppInstaller::Repository; using namespace AppInstaller::Settings; @@ -214,260 +213,6 @@ namespace AppInstaller::CLI::Workflow } } - void DownloadInstaller(Execution::Context& context) - { - const auto& installer = context.Get().value(); - - switch (installer.InstallerType) - { - case InstallerTypeEnum::Exe: - case InstallerTypeEnum::Burn: - case InstallerTypeEnum::Inno: - case InstallerTypeEnum::Msi: - case InstallerTypeEnum::Nullsoft: - case InstallerTypeEnum::Wix: - context << DownloadInstallerFile << VerifyInstallerHash << UpdateInstallerFileMotwIfApplicable; - break; - case InstallerTypeEnum::Msix: - if (installer.SignatureSha256.empty()) - { - context << DownloadInstallerFile << VerifyInstallerHash << UpdateInstallerFileMotwIfApplicable; - } - else - { - // Signature hash provided. No download needed. Just verify signature hash. - context << GetMsixSignatureHash << VerifyInstallerHash << UpdateInstallerFileMotwIfApplicable; - } - break; - case InstallerTypeEnum::MSStore: - // Nothing to do here - break; - default: - THROW_HR(HRESULT_FROM_WIN32(ERROR_NOT_SUPPORTED)); - } - } - - void GetInstallerDownloadPath(Execution::Context& context) - { - if (!context.Contains(Execution::Data::InstallerPath)) - { - const auto& manifest = context.Get(); - - std::filesystem::path tempInstallerPath = Runtime::GetPathTo(Runtime::PathName::Temp); - tempInstallerPath /= Utility::ConvertToUTF16(manifest.Id + '.' + manifest.Version); - - AICLI_LOG(CLI, Info, << "Generated temp download path: " << tempInstallerPath); - context.Add(std::move(tempInstallerPath)); - } - } - - void DownloadInstallerFile(Execution::Context& context) - { - context << GetInstallerDownloadPath; - if (context.IsTerminated()) - { - return; - } - - const auto& installer = context.Get().value(); - const auto& installerPath = context.Get(); - - // Check if file was already downloaded. - // This may happen after a failed installation or if the download was done - // separately before, e.g. on COM scenarios. - // TODO: The installer gets renamed before executing it. This check could be extended - // to consider the renamed file. - if (std::filesystem::exists(installerPath)) - { - AICLI_LOG(CLI, Info, << "Installer already downloaded. Verifying hash."); - std::ifstream inStream{ installerPath, std::ifstream::binary }; - auto existingFileHash = SHA256::ComputeHash(inStream); - - if (std::equal(installer.Sha256.begin(), installer.Sha256.end(), existingFileHash.begin())) - { - AICLI_LOG(CLI, Info, << "Hash matches. Will use existing installer."); - context.Add(std::make_pair(installer.Sha256, existingFileHash)); - return; - } - - AICLI_LOG(CLI, Info, << "Hash mismatch. Will download installer."); - } - - Utility::DownloadInfo downloadInfo{}; - downloadInfo.DisplayName = Resource::GetFixedString(Resource::FixedString::ProductName); - // Use the SHA256 hash of the installer as the identifier for the download - downloadInfo.ContentId = SHA256::ConvertToString(installer.Sha256); - - context.Reporter.Info() << "Downloading " << Execution::UrlEmphasis << installer.Url << std::endl; - - std::optional> hash; - - const int MaxRetryCount = 2; - for (int retryCount = 0; retryCount < MaxRetryCount; ++retryCount) - { - bool success = false; - try - { - hash = context.Reporter.ExecuteWithProgress(std::bind(Utility::Download, - installer.Url, - installerPath, - Utility::DownloadType::Installer, - std::placeholders::_1, - true, - downloadInfo)); - - success = true; - } - catch (...) - { - if (retryCount < MaxRetryCount - 1) - { - AICLI_LOG(CLI, Info, << "Failed to download, waiting a bit and retry. Url: " << installer.Url); - Sleep(500); - } - else - { - throw; - } - } - - if (success) - { - break; - } - } - - if (!hash) - { - context.Reporter.Info() << "Package download canceled." << std::endl; - AICLI_TERMINATE_CONTEXT(E_ABORT); - } - - context.Add(std::make_pair(installer.Sha256, hash.value())); - } - - void GetMsixSignatureHash(Execution::Context& context) - { - // We use this when the server won't support streaming install to swap to download. - bool downloadInstead = false; - - try - { - const auto& installer = context.Get().value(); - - Msix::MsixInfo msixInfo(installer.Url); - auto signature = msixInfo.GetSignature(); - - auto signatureHash = SHA256::ComputeHash(signature.data(), static_cast(signature.size())); - - context.Add(std::make_pair(installer.SignatureSha256, signatureHash)); - } - catch (const winrt::hresult_error& e) - { - if (static_cast(e.code()) == HRESULT_FROM_WIN32(ERROR_NO_RANGES_PROCESSED) || - HRESULT_FACILITY(e.code()) == FACILITY_HTTP) - { - // Failed to get signature hash through HttpStream, use download - downloadInstead = true; - } - else - { - throw; - } - } - - if (downloadInstead) - { - context << DownloadInstallerFile; - } - } - - void VerifyInstallerHash(Execution::Context& context) - { - const auto& hashPair = context.Get(); - - if (!std::equal( - hashPair.first.begin(), - hashPair.first.end(), - hashPair.second.begin())) - { - bool overrideHashMismatch = context.Args.Contains(Execution::Args::Type::HashOverride); - - const auto& manifest = context.Get(); - Logging::Telemetry().LogInstallerHashMismatch(manifest.Id, manifest.Version, manifest.Channel, hashPair.first, hashPair.second, overrideHashMismatch); - - // If running as admin, do not allow the user to override the hash failure. - if (Runtime::IsRunningAsAdmin()) - { - context.Reporter.Error() << Resource::String::InstallerHashMismatchAdminBlock << std::endl; - } - else if (overrideHashMismatch) - { - context.Reporter.Warn() << Resource::String::InstallerHashMismatchOverridden << std::endl; - return; - } - else if (Settings::GroupPolicies().IsEnabled(Settings::TogglePolicy::Policy::HashOverride)) - { - context.Reporter.Error() << Resource::String::InstallerHashMismatchOverrideRequired << std::endl; - } - else - { - context.Reporter.Error() << Resource::String::InstallerHashMismatchError << std::endl; - } - - AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_INSTALLER_HASH_MISMATCH); - } - else - { - AICLI_LOG(CLI, Info, << "Installer hash verified"); - context.Reporter.Info() << Resource::String::InstallerHashVerified << std::endl; - - context.SetFlags(Execution::ContextFlag::InstallerHashMatched); - - if (context.Contains(Execution::Data::PackageVersion) && - context.Get()->GetSource() != nullptr && - WI_IsFlagSet(context.Get()->GetSource()->GetDetails().TrustLevel, SourceTrustLevel::Trusted)) - { - context.SetFlags(Execution::ContextFlag::InstallerTrusted); - } - } - } - - void UpdateInstallerFileMotwIfApplicable(Execution::Context& context) - { - if (context.Contains(Execution::Data::InstallerPath)) - { - if (WI_IsFlagSet(context.GetFlags(), Execution::ContextFlag::InstallerTrusted)) - { - Utility::ApplyMotwIfApplicable(context.Get(), URLZONE_TRUSTED); - } - else if (WI_IsFlagSet(context.GetFlags(), Execution::ContextFlag::InstallerHashMatched)) - { - const auto& installer = context.Get(); - HRESULT hr = Utility::ApplyMotwUsingIAttachmentExecuteIfApplicable(context.Get(), installer.value().Url, URLZONE_INTERNET); - - // Not using SUCCEEDED(hr) to check since there are cases file is missing after a successful scan - if (hr != S_OK) - { - switch (hr) - { - case INET_E_SECURITY_PROBLEM: - context.Reporter.Error() << Resource::String::InstallerBlockedByPolicy << std::endl; - break; - case E_FAIL: - context.Reporter.Error() << Resource::String::InstallerFailedVirusScan << std::endl; - break; - default: - context.Reporter.Error() << Resource::String::InstallerFailedSecurityCheck << std::endl; - } - - AICLI_LOG(Fail, Error, << "Installer failed security check. Url: " << installer.value().Url << " Result: " << WINGET_OSTREAM_FORMAT_HRESULT(hr)); - AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_INSTALLER_SECURITY_CHECK_FAILED); - } - } - } - } - void ExecuteInstaller(Execution::Context& context) { const auto& installer = context.Get().value(); @@ -515,7 +260,6 @@ namespace AppInstaller::CLI::Workflow { context << GetInstallerArgs << - RenameDownloadedInstaller << ShellExecuteInstallImpl << ReportInstallerResult("ShellExecute"sv, APPINSTALLER_CLI_ERROR_SHELLEXEC_INSTALL_FAILED); } @@ -524,7 +268,6 @@ namespace AppInstaller::CLI::Workflow { context << GetInstallerArgs << - RenameDownloadedInstaller << DirectMSIInstallImpl << ReportInstallerResult("MsiInstallProduct"sv, APPINSTALLER_CLI_ERROR_MSI_INSTALL_FAILED); } @@ -611,30 +354,6 @@ namespace AppInstaller::CLI::Workflow } } - void RemoveInstaller(Execution::Context& context) - { - // Path may not be present if installed from a URL for MSIX - if (context.Contains(Execution::Data::InstallerPath)) - { - const auto& path = context.Get(); - AICLI_LOG(CLI, Info, << "Removing installer: " << path); - - try - { - // best effort - std::filesystem::remove(path); - } - catch (const std::exception& e) - { - AICLI_LOG(CLI, Warning, << "Failed to remove installer file after execution. Reason: " << e.what()); - } - catch (...) - { - AICLI_LOG(CLI, Warning, << "Failed to remove installer file after execution. Reason unknown."); - } - } - } - void ReportIdentityAndInstallationDisclaimer(Execution::Context& context) { context << @@ -645,8 +364,6 @@ namespace AppInstaller::CLI::Workflow void InstallPackageInstaller(Execution::Context& context) { context << - Workflow::ReportExecutionStage(ExecutionStage::Download) << - Workflow::DownloadInstaller << Workflow::ReportExecutionStage(ExecutionStage::PreExecution) << Workflow::SnapshotARPEntries << Workflow::ReportExecutionStage(ExecutionStage::Execution) << @@ -656,13 +373,20 @@ namespace AppInstaller::CLI::Workflow Workflow::RemoveInstaller; } - void InstallSinglePackage(Execution::Context& context) + void DownloadSinglePackage(Execution::Context& context) { context << Workflow::ReportIdentityAndInstallationDisclaimer << Workflow::ShowPackageAgreements(/* ensureAcceptance */ true) << Workflow::GetDependenciesFromInstaller << Workflow::ReportDependencies(Resource::String::InstallAndUpgradeCommandsReportDependencies) << + Workflow::DownloadInstaller; + } + + void InstallSinglePackage(Execution::Context& context) + { + context << + Workflow::DownloadSinglePackage << Workflow::InstallPackageInstaller; } @@ -703,8 +427,10 @@ namespace AppInstaller::CLI::Workflow installContext.Add(package.InstalledPackageVersion); installContext.Add(package.Installer); + // TODO: We could move installContext << Workflow::ReportIdentityAndInstallationDisclaimer << + Workflow::DownloadInstaller << Workflow::InstallPackageInstaller; installContext.Reporter.Info() << std::endl; diff --git a/src/AppInstallerCLICore/Workflows/InstallFlow.h b/src/AppInstallerCLICore/Workflows/InstallFlow.h index 885820bc0c..40ccf60af8 100644 --- a/src/AppInstallerCLICore/Workflows/InstallFlow.h +++ b/src/AppInstallerCLICore/Workflows/InstallFlow.h @@ -60,44 +60,6 @@ namespace AppInstaller::CLI::Workflow // Outputs: None void EnsurePackageAgreementsAcceptanceForMultipleInstallers(Execution::Context& context); - // Composite flow that chooses what to do based on the installer type. - // Required Args: None - // Inputs: Manifest, Installer - // Outputs: None - void DownloadInstaller(Execution::Context& context); - - // Computes the download path for the installer file. - // Does nothing if already determined. - // Required Args: None - // Inputs: Manifest - // Outputs: InstallerPath - void GetInstallerDownloadPath(Execution::Context& context); - - // Downloads the file referenced by the Installer. - // Do nothing if the file has already been downloaded. - // Required Args: None - // Inputs: Installer, Manifest - // Outputs: HashPair, InstallerPath - void DownloadInstallerFile(Execution::Context& context); - - // Computes the hash of the MSIX signature file. - // Required Args: None - // Inputs: Installer - // Outputs: HashPair - void GetMsixSignatureHash(Execution::Context& context); - - // Gets the source list, filtering it if SourceName is present. - // Required Args: None - // Inputs: HashPair - // Outputs: SourceList - void VerifyInstallerHash(Execution::Context& context); - - // Update Motw of the downloaded installer if applicable - // Required Args: None - // Inputs: HashPair, InstallerPath?, SourceId? - // Outputs: None - void UpdateInstallerFileMotwIfApplicable(Execution::Context& context); - // Composite flow that chooses what to do based on the installer type. // Required Args: None // Inputs: Installer, InstallerPath @@ -142,13 +104,6 @@ namespace AppInstaller::CLI::Workflow bool m_isHResult; }; - - // Deletes the installer file. - // Required Args: None - // Inputs: InstallerPath - // Outputs: None - void RemoveInstaller(Execution::Context& context); - // Reports manifest identity and shows installation disclaimer // Required Args: None // Inputs: Manifest @@ -157,11 +112,17 @@ namespace AppInstaller::CLI::Workflow // Installs a specific package installer. See also InstallSinglePackage & InstallMultiplePackages. // Required Args: None - // Inputs: Manifest, Installer, PackageVersion, InstalledPackageVersion? + // Inputs: InstallerPath, Manifest, Installer, PackageVersion, InstalledPackageVersion? // Outputs: None void InstallPackageInstaller(Execution::Context& context); - // Installs a single package. This also does the reporting and user interaction + // Downloads the installer for a single package. This also does all the reporting and user interaction needed. + // Required Args: None + // Inputs: Manifest, Installer + // Outputs: InstallerPath + void DownloadSinglePackage(Execution::Context& context); + + // Installs a single package. This also does the reporting, user interaction, and installer download // for single-package installation. // RequiredArgs: None // Inputs: Manifest, Installer, PackageVersion, InstalledPackageVersion? diff --git a/src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.cpp b/src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.cpp index 7611af7195..83ca60a863 100644 --- a/src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.cpp +++ b/src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.cpp @@ -181,68 +181,6 @@ namespace AppInstaller::CLI::Workflow return args; } - - // Complicated rename algorithm due to somewhat arbitrary failures. - // 1. First, try to rename. - // 2. Then, create an empty file for the target, and attempt to rename. - // 3. Then, try repeatedly for 500ms in case it is a timing thing. - // 4. Attempt to use a hard link if available. - // 5. Copy the file if nothing else has worked so far. - void RenameFile(const std::filesystem::path& from, const std::filesystem::path& to) - { - // 1. First, try to rename. - try - { - // std::filesystem::rename() handles motw correctly if applicable. - std::filesystem::rename(from, to); - return; - } - CATCH_LOG(); - - // 2. Then, create an empty file for the target, and attempt to rename. - // This seems to fix things in certain cases, so we do it. - try - { - { - std::ofstream targetFile{ to }; - } - std::filesystem::rename(from, to); - return; - } - CATCH_LOG(); - - // 3. Then, try repeatedly for 500ms in case it is a timing thing. - for (int i = 0; i < 5; ++i) - { - try - { - std::this_thread::sleep_for(100ms); - std::filesystem::rename(from, to); - return; - } - CATCH_LOG(); - } - - // 4. Attempt to use a hard link if available. - if (Runtime::SupportsHardLinks(from)) - { - try - { - // Create a hard link to the file; the installer will be left in the temp directory afterward - // but it is better to succeed the operation and leave a file around than to fail. - // First we have to remove the target file as the function will not overwrite. - std::filesystem::remove(to); - std::filesystem::create_hard_link(from, to); - return; - } - CATCH_LOG(); - } - - // 5. Copy the file if nothing else has worked so far. - // Create a copy of the file; the installer will be left in the temp directory afterward - // but it is better to succeed the operation and leave a file around than to fail. - std::filesystem::copy_file(from, to, std::filesystem::copy_options::overwrite_existing); - } } void ShellExecuteInstallImpl(Execution::Context& context) @@ -285,31 +223,6 @@ namespace AppInstaller::CLI::Workflow context.Add(std::move(installerArgs)); } - void RenameDownloadedInstaller(Execution::Context& context) - { - auto& installerPath = context.Get(); - std::filesystem::path renamedDownloadedInstaller(installerPath); - - switch(context.Get()->InstallerType) - { - case InstallerTypeEnum::Burn: - case InstallerTypeEnum::Exe: - case InstallerTypeEnum::Inno: - case InstallerTypeEnum::Nullsoft: - renamedDownloadedInstaller += L".exe"; - break; - case InstallerTypeEnum::Msi: - case InstallerTypeEnum::Wix: - renamedDownloadedInstaller += L".msi"; - break; - } - - RenameFile(installerPath, renamedDownloadedInstaller); - - installerPath.assign(renamedDownloadedInstaller); - AICLI_LOG(CLI, Info, << "Successfully renamed downloaded installer. Path: " << installerPath); - } - void ShellExecuteUninstallImpl(Execution::Context& context) { context.Reporter.Info() << Resource::String::UninstallFlowStartingPackageUninstall << std::endl; diff --git a/src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.h b/src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.h index ee2142e27e..ff5721e3d4 100644 --- a/src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.h +++ b/src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.h @@ -34,12 +34,4 @@ namespace AppInstaller::CLI::Workflow // Inputs: Manifest?, Installer, InstallerPath // Outputs: InstallerArgs void GetInstallerArgs(Execution::Context& context); - - // This method appends appropriate extension to the downloaded installer. - // ShellExecute uses file extension to launch the installer appropriately. - // Required Args: None - // Inputs: Installer, InstallerPath - // Modifies: InstallerPath - // Outputs: None - void RenameDownloadedInstaller(Execution::Context& context); } \ No newline at end of file diff --git a/src/AppInstallerCLITests/WorkFlow.cpp b/src/AppInstallerCLITests/WorkFlow.cpp index f76630a481..f81c1e1daa 100644 --- a/src/AppInstallerCLITests/WorkFlow.cpp +++ b/src/AppInstallerCLITests/WorkFlow.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -407,10 +408,6 @@ void OverrideForShellExecute(TestContext& context) context.Add(TestDataFile("AppInstallerTestExeInstaller.exe")); } }); - context.Override({ RenameDownloadedInstaller, [](TestContext&) - { - } }); - OverrideForUpdateInstallerMotw(context); } @@ -423,10 +420,6 @@ void OverrideForDirectMsi(TestContext& context) context.Add(TestDataFile("AppInstallerTestExeInstaller.exe")); } }); - context.Override({ RenameDownloadedInstaller, [](TestContext&) - { - } }); - OverrideForUpdateInstallerMotw(context); context.Override({ DirectMSIInstallImpl, [](TestContext& context) diff --git a/src/AppInstallerCommonCore/Downloader.cpp b/src/AppInstallerCommonCore/Downloader.cpp index f923f81e1c..491f33a4f8 100644 --- a/src/AppInstallerCommonCore/Downloader.cpp +++ b/src/AppInstallerCommonCore/Downloader.cpp @@ -286,7 +286,6 @@ namespace AppInstaller::Utility return; } - THROW_IF_FAILED(hr); THROW_IF_FAILED(zoneIdentifier->Remove()); THROW_IF_FAILED(persistFile->Save(NULL, TRUE)); From 3f92091c79caa98c39080bd03ed6fc3b8bc63256 Mon Sep 17 00:00:00 2001 From: Flor Elisa Chacon Ochoa Date: Fri, 8 Oct 2021 16:56:17 -0700 Subject: [PATCH 08/13] Remove outdated comment --- src/AppInstallerCLICore/Commands/COMInstallCommand.cpp | 1 - src/AppInstallerCLICore/Workflows/InstallFlow.cpp | 1 - 2 files changed, 2 deletions(-) diff --git a/src/AppInstallerCLICore/Commands/COMInstallCommand.cpp b/src/AppInstallerCLICore/Commands/COMInstallCommand.cpp index e8ad55171a..2ebc86d3d4 100644 --- a/src/AppInstallerCLICore/Commands/COMInstallCommand.cpp +++ b/src/AppInstallerCLICore/Commands/COMInstallCommand.cpp @@ -16,7 +16,6 @@ namespace AppInstaller::CLI // IMPORTANT: To use this command, the caller should have already retrieved the package manifest (GetManifest()) and added it to the Context Data void COMDownloadCommand::ExecuteInternal(Context& context) const { - // TODO: Remove duplication with InstallFlow context << Workflow::ReportExecutionStage(ExecutionStage::Discovery) << Workflow::SelectInstaller << diff --git a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp index 9cfa497879..40a2bee916 100644 --- a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp @@ -427,7 +427,6 @@ namespace AppInstaller::CLI::Workflow installContext.Add(package.InstalledPackageVersion); installContext.Add(package.Installer); - // TODO: We could move installContext << Workflow::ReportIdentityAndInstallationDisclaimer << Workflow::DownloadInstaller << From 92822a6cafdc9dae1510c004bd83cbfd06f402e6 Mon Sep 17 00:00:00 2001 From: Flor Elisa Chacon Ochoa Date: Mon, 11 Oct 2021 12:59:33 -0700 Subject: [PATCH 09/13] Check if file exists when no motw found --- src/AppInstallerCommonCore/Downloader.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/AppInstallerCommonCore/Downloader.cpp b/src/AppInstallerCommonCore/Downloader.cpp index 491f33a4f8..af5970e406 100644 --- a/src/AppInstallerCommonCore/Downloader.cpp +++ b/src/AppInstallerCommonCore/Downloader.cpp @@ -282,6 +282,10 @@ namespace AppInstaller::Utility auto hr = persistFile->Load(filePath.c_str(), STGM_READ); if (hr == HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND)) { + // IPersistFile::Load returns same error for "file not found" and "motw not found". + // Check if the file exists to be sure we are on the "motw not found" case. + THROW_HR_IF(HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND), !std::filesystem::exists(filePath)); + AICLI_LOG(Core, Info, << "File does not contain motw. Skipped removing motw"); return; } From b4a3ab4b029a04708aac17e7fb22c2701a2b1425 Mon Sep 17 00:00:00 2001 From: Flor Elisa Chacon Ochoa Date: Mon, 11 Oct 2021 15:16:57 -0700 Subject: [PATCH 10/13] Add back rename downloaded file --- .../Workflows/DownloadFlow.cpp | 259 ++++++++++++------ .../Workflows/DownloadFlow.h | 19 +- src/AppInstallerCLITests/WorkFlow.cpp | 16 ++ 3 files changed, 200 insertions(+), 94 deletions(-) diff --git a/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp b/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp index 1a65983934..0a448220ad 100644 --- a/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp @@ -10,9 +10,88 @@ namespace AppInstaller::CLI::Workflow using namespace AppInstaller::Manifest; using namespace AppInstaller::Repository; using namespace AppInstaller::Utility; + using namespace std::string_view_literals; namespace { + // Get the base download path for the installer path. + // This path does not include the file extension, which will be added + // after verifying the file hash to prevent it from being ShellExecute-d + std::filesystem::path GetInstallerBaseDownloadPath(Execution::Context& context) + { + const auto& manifest = context.Get(); + std::filesystem::path tempInstallerPath = Runtime::GetPathTo(Runtime::PathName::Temp); + tempInstallerPath /= Utility::ConvertToUTF16(manifest.Id + '.' + manifest.Version); + return tempInstallerPath; + } + + // Get the file extension to be used for the installer file. + std::wstring_view GetInstallerFileExtension(Execution::Context& context) + { + const auto& installer = context.Get(); + switch (installer->InstallerType) + { + case InstallerTypeEnum::Burn: + case InstallerTypeEnum::Exe: + case InstallerTypeEnum::Inno: + case InstallerTypeEnum::Nullsoft: + return L".exe"sv; + case InstallerTypeEnum::Msi: + case InstallerTypeEnum::Wix: + return L".msi"sv; + case InstallerTypeEnum::Msix: + { + Msix::MsixInfo msixInfo(installer->Url); + return msixInfo.GetIsBundle() ? L".msixbundle"sv : L".msix"sv; + break; + } + case InstallerTypeEnum::Zip: + return L".zip"sv; + default: + THROW_HR(HRESULT_FROM_WIN32(ERROR_NOT_SUPPORTED)); + } + } + + // Try to remove the installer file, ignoring any errors. + void RemoveInstallerFile(const std::filesystem::path& path) + { + try + { + std::filesystem::remove(path); + } + catch (const std::exception& e) + { + AICLI_LOG(CLI, Warning, << "Failed to remove installer file. Reason: " << e.what()); + } + catch (...) + { + AICLI_LOG(CLI, Warning, << "Failed to remove installer file. Reason unknown."); + } + + } + + // Checks the file hash for an existing installer file. + // Returns true if the file exists and its hash matches, false otherwise. + // If the hash does not match, deletes the file. + bool ExistingInstallerFileHasHashMatch(const SHA256::HashBuffer& expectedHash, const std::filesystem::path& filePath, SHA256::HashBuffer& fileHash) + { + if (std::filesystem::exists(filePath)) + { + AICLI_LOG(CLI, Info, << "Found existing installer file at '" << filePath << "'. Verifying file hash."); + std::ifstream inStream{ filePath, std::ifstream::binary }; + fileHash = SHA256::ComputeHash(inStream); + + if (std::equal(expectedHash.begin(), expectedHash.end(), fileHash.begin())) + { + return true; + } + + AICLI_LOG(CLI, Info, << "Hash does not match. Removing existing installer file " << filePath); + RemoveInstallerFile(filePath); + } + + return false; + } // Complicated rename algorithm due to somewhat arbitrary failures. // 1. First, try to rename. @@ -79,76 +158,88 @@ namespace AppInstaller::CLI::Workflow void DownloadInstaller(Execution::Context& context) { - context << ReportExecutionStage(ExecutionStage::Download); + // Check if file was already downloaded. + // This may happen after a failed installation or if the download was done + // separately before, e.g. on COM scenarios. + context << + ReportExecutionStage(ExecutionStage::Download) << + CheckForExistingInstaller; if (context.IsTerminated()) { return; } - const auto& installer = context.Get().value(); - - switch (installer.InstallerType) - { - case InstallerTypeEnum::Exe: - case InstallerTypeEnum::Burn: - case InstallerTypeEnum::Inno: - case InstallerTypeEnum::Msi: - case InstallerTypeEnum::Nullsoft: - case InstallerTypeEnum::Wix: - context << DownloadInstallerFile << VerifyInstallerHash << UpdateInstallerFileMotwIfApplicable; - break; - case InstallerTypeEnum::Msix: - if (installer.SignatureSha256.empty()) - { - context << DownloadInstallerFile << VerifyInstallerHash << UpdateInstallerFileMotwIfApplicable; - } - else - { - // Signature hash provided. No download needed. Just verify signature hash. - context << GetMsixSignatureHash << VerifyInstallerHash << UpdateInstallerFileMotwIfApplicable; - } - break; - case InstallerTypeEnum::MSStore: - // Nothing to do here - break; - default: - THROW_HR(HRESULT_FROM_WIN32(ERROR_NOT_SUPPORTED)); - } - } - - void GetInstallerDownloadPath(Execution::Context& context) - { + // CheckForExistingInstaller will set the InstallerPath if found if (!context.Contains(Execution::Data::InstallerPath)) { - const auto& manifest = context.Get(); - const auto& installer = context.Get(); - - std::filesystem::path tempInstallerPath = Runtime::GetPathTo(Runtime::PathName::Temp); - tempInstallerPath /= Utility::ConvertToUTF16(manifest.Id + '.' + manifest.Version); - - switch (installer->InstallerType) + const auto& installer = context.Get().value(); + switch (installer.InstallerType) { - case InstallerTypeEnum::Burn: case InstallerTypeEnum::Exe: + case InstallerTypeEnum::Burn: case InstallerTypeEnum::Inno: - case InstallerTypeEnum::Nullsoft: - tempInstallerPath += L".exe"; - break; case InstallerTypeEnum::Msi: + case InstallerTypeEnum::Nullsoft: case InstallerTypeEnum::Wix: - tempInstallerPath += L".msi"; + context << DownloadInstallerFile; break; case InstallerTypeEnum::Msix: - { - Msix::MsixInfo msixInfo(installer->Url); - tempInstallerPath += msixInfo.GetIsBundle() ? L".msixbundle" : L".msix"; + if (installer.SignatureSha256.empty()) + { + context << DownloadInstallerFile; + } + else + { + // Signature hash provided. No download needed. Just verify signature hash. + context << GetMsixSignatureHash; + } break; + case InstallerTypeEnum::MSStore: + // Nothing to do here + return; + default: + THROW_HR(HRESULT_FROM_WIN32(ERROR_NOT_SUPPORTED)); } - case InstallerTypeEnum::Zip: - tempInstallerPath += L".zip"; - break; + } + + context << + VerifyInstallerHash << + UpdateInstallerFileMotwIfApplicable << + RenameDownloadedInstaller; + } + + void CheckForExistingInstaller(Execution::Context& context) + { + const auto& installer = context.Get().value(); + if (installer.InstallerType == InstallerTypeEnum::MSStore) + { + // No installer is downloaded in this case + return; + } + + // Try looking for the file with and without extension. + auto installerPath = GetInstallerBaseDownloadPath(context); + SHA256::HashBuffer fileHash; + if (!ExistingInstallerFileHasHashMatch(installer.Sha256, installerPath, fileHash)) + { + installerPath += GetInstallerFileExtension(context); + if (!ExistingInstallerFileHasHashMatch(installer.Sha256, installerPath, fileHash)) + { + // No match + return; } + } + + AICLI_LOG(CLI, Info, << "Existing installer file hash matches. Will use existing installer."); + context.Add(installerPath); + context.Add(std::make_pair(installer.Sha256, fileHash)); + } + void GetInstallerDownloadPath(Execution::Context& context) + { + if (!context.Contains(Execution::Data::InstallerPath)) + { + auto tempInstallerPath = GetInstallerBaseDownloadPath(context); AICLI_LOG(CLI, Info, << "Generated temp download path: " << tempInstallerPath); context.Add(std::move(tempInstallerPath)); } @@ -165,30 +256,6 @@ namespace AppInstaller::CLI::Workflow const auto& installer = context.Get().value(); const auto& installerPath = context.Get(); - // Check if file was already downloaded. - // This may happen after a failed installation or if the download was done - // separately before, e.g. on COM scenarios. - if (std::filesystem::exists(installerPath)) - { - AICLI_LOG(CLI, Info, << "Installer already downloaded. Verifying hash."); - std::ifstream inStream{ installerPath, std::ifstream::binary }; - auto existingFileHash = SHA256::ComputeHash(inStream); - - if (std::equal(installer.Sha256.begin(), installer.Sha256.end(), existingFileHash.begin())) - { - AICLI_LOG(CLI, Info, << "Hash matches. Will use existing installer."); - context.Add(std::make_pair(installer.Sha256, existingFileHash)); - return; - } - - AICLI_LOG(CLI, Info, << "Hash mismatch. Will download installer again. Removing existing installer file."); - context << RemoveInstaller; - if (context.IsTerminated()) - { - return; - } - } - Utility::DownloadInfo downloadInfo{}; downloadInfo.DisplayName = Resource::GetFixedString(Resource::FixedString::ProductName); // Use the SHA256 hash of the installer as the identifier for the download @@ -394,6 +461,31 @@ namespace AppInstaller::CLI::Workflow } } + void RenameDownloadedInstaller(Execution::Context& context) + { + if (!context.Contains(Execution::Data::InstallerPath)) + { + // No installer downloaded, no need to rename anything. + return; + } + + auto& installerPath = context.Get(); + auto installerExtension = GetInstallerFileExtension(context); + if (installerPath.extension() == installerExtension) + { + // Installer file already has expected extension. + return; + } + + std::filesystem::path renamedDownloadedInstaller(installerPath); + renamedDownloadedInstaller += installerExtension; + + RenameFile(installerPath, renamedDownloadedInstaller); + + installerPath.assign(renamedDownloadedInstaller); + AICLI_LOG(CLI, Info, << "Successfully renamed downloaded installer. Path: " << installerPath); + } + void RemoveInstaller(Execution::Context& context) { // Path may not be present if installed from a URL for MSIX @@ -401,20 +493,7 @@ namespace AppInstaller::CLI::Workflow { const auto& path = context.Get(); AICLI_LOG(CLI, Info, << "Removing installer: " << path); - - try - { - // best effort - std::filesystem::remove(path); - } - catch (const std::exception& e) - { - AICLI_LOG(CLI, Warning, << "Failed to remove installer file. Reason: " << e.what()); - } - catch (...) - { - AICLI_LOG(CLI, Warning, << "Failed to remove installer file. Reason unknown."); - } + RemoveInstallerFile(path); } } } diff --git a/src/AppInstallerCLICore/Workflows/DownloadFlow.h b/src/AppInstallerCLICore/Workflows/DownloadFlow.h index 832b25de49..744de69467 100644 --- a/src/AppInstallerCLICore/Workflows/DownloadFlow.h +++ b/src/AppInstallerCLICore/Workflows/DownloadFlow.h @@ -11,16 +11,19 @@ namespace AppInstaller::CLI::Workflow // Outputs: None void DownloadInstaller(Execution::Context& context); - // Computes the download path for the installer file. Does nothing if already determined. - // This adds appropriate extension to the installer path, as ShellExecute uses file extension - // to launch the installer appropriately. + // Check if the desired installer has already been downloaded. + // Required Args: None + // Inputs: Manifest, Installer + // Outputs: HashPair, InstallerPath (only if found) + void CheckForExistingInstaller(Execution::Context& context); + + // Computes the download path for the installer file. Does nothing if already determined // Required Args: None // Inputs: Installer, Manifest // Outputs: InstallerPath void GetInstallerDownloadPath(Execution::Context& context); // Downloads the file referenced by the Installer. - // Do nothing if the file has already been downloaded. // Required Args: None // Inputs: Installer, Manifest // Outputs: HashPair, InstallerPath @@ -51,6 +54,14 @@ namespace AppInstaller::CLI::Workflow // Outputs: None void UpdateInstallerFileMotwIfApplicable(Execution::Context& context); + // This method appends appropriate extension to the downloaded installer. + // ShellExecute uses file extension to launch the installer appropriately. + // Required Args: None + // Inputs: Installer, InstallerPath + // Modifies: InstallerPath + // Outputs: None + void RenameDownloadedInstaller(Execution::Context& context); + // Deletes the installer file. // Required Args: None // Inputs: InstallerPath diff --git a/src/AppInstallerCLITests/WorkFlow.cpp b/src/AppInstallerCLITests/WorkFlow.cpp index c7c42c0210..b0bd4ba4f0 100644 --- a/src/AppInstallerCLITests/WorkFlow.cpp +++ b/src/AppInstallerCLITests/WorkFlow.cpp @@ -403,17 +403,29 @@ void OverrideForUpdateInstallerMotw(TestContext& context) void OverrideForShellExecute(TestContext& context) { + context.Override({ CheckForExistingInstaller, [](TestContext&) + { + } }); + context.Override({ DownloadInstallerFile, [](TestContext& context) { context.Add({ {}, {} }); context.Add(TestDataFile("AppInstallerTestExeInstaller.exe")); } }); + context.Override({ RenameDownloadedInstaller, [](TestContext&) + { + } }); + OverrideForUpdateInstallerMotw(context); } void OverrideForDirectMsi(TestContext& context) { + context.Override({ CheckForExistingInstaller, [](TestContext&) + { + } }); + context.Override({ DownloadInstallerFile, [](TestContext& context) { context.Add({ {}, {} }); @@ -421,6 +433,10 @@ void OverrideForDirectMsi(TestContext& context) context.Add(TestDataFile("AppInstallerTestExeInstaller.exe")); } }); + context.Override({ RenameDownloadedInstaller, [](TestContext&) + { + } }); + OverrideForUpdateInstallerMotw(context); context.Override({ DirectMSIInstallImpl, [](TestContext& context) From 6acf52f94105adaed3a37c7c41be9e069cec9371 Mon Sep 17 00:00:00 2001 From: Flor Elisa Chacon Ochoa Date: Mon, 11 Oct 2021 15:35:43 -0700 Subject: [PATCH 11/13] Fix failing test --- src/AppInstallerCLITests/WorkFlow.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/AppInstallerCLITests/WorkFlow.cpp b/src/AppInstallerCLITests/WorkFlow.cpp index b0bd4ba4f0..334698a23c 100644 --- a/src/AppInstallerCLITests/WorkFlow.cpp +++ b/src/AppInstallerCLITests/WorkFlow.cpp @@ -467,6 +467,10 @@ void OverrideForExeUninstall(TestContext& context) void OverrideForMSIX(TestContext& context) { + context.Override({ CheckForExistingInstaller, [](TestContext&) + { + } }); + context.Override({ MsixInstall, [](TestContext& context) { std::filesystem::path temp = std::filesystem::temp_directory_path(); From 918e4f2356fa131cfdd02eb1730424075ee5af19 Mon Sep 17 00:00:00 2001 From: Flor Elisa Chacon Ochoa Date: Tue, 12 Oct 2021 12:37:29 -0700 Subject: [PATCH 12/13] Fix tests --- src/AppInstallerCLITests/WorkFlow.cpp | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/AppInstallerCLITests/WorkFlow.cpp b/src/AppInstallerCLITests/WorkFlow.cpp index 334698a23c..b61e1fc43e 100644 --- a/src/AppInstallerCLITests/WorkFlow.cpp +++ b/src/AppInstallerCLITests/WorkFlow.cpp @@ -401,11 +401,16 @@ void OverrideForUpdateInstallerMotw(TestContext& context) } }); } -void OverrideForShellExecute(TestContext& context) +void OverrideForCheckExistingInstaller(TestContext& context) { context.Override({ CheckForExistingInstaller, [](TestContext&) { } }); +} + +void OverrideForShellExecute(TestContext& context) +{ + OverrideForCheckExistingInstaller(context); context.Override({ DownloadInstallerFile, [](TestContext& context) { @@ -422,9 +427,7 @@ void OverrideForShellExecute(TestContext& context) void OverrideForDirectMsi(TestContext& context) { - context.Override({ CheckForExistingInstaller, [](TestContext&) - { - } }); + OverrideForCheckExistingInstaller(context); context.Override({ DownloadInstallerFile, [](TestContext& context) { @@ -467,10 +470,6 @@ void OverrideForExeUninstall(TestContext& context) void OverrideForMSIX(TestContext& context) { - context.Override({ CheckForExistingInstaller, [](TestContext&) - { - } }); - context.Override({ MsixInstall, [](TestContext& context) { std::filesystem::path temp = std::filesystem::temp_directory_path(); @@ -697,6 +696,7 @@ TEST_CASE("MsixInstallFlow_StreamingFlow", "[InstallFlow][workflow]") std::ostringstream installOutput; TestContext context{ installOutput, std::cin }; OverrideForMSIX(context); + OverrideForCheckExistingInstaller(context); // Todo: point to files from our repo when the repo goes public context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_Msix_StreamingFlow.yaml").GetPath().u8string()); From 4f0886eb7c599ac444d3f1a4a242837a0d0ba1ab Mon Sep 17 00:00:00 2001 From: Flor Elisa Chacon Ochoa Date: Wed, 13 Oct 2021 15:06:37 -0700 Subject: [PATCH 13/13] PR comments --- src/AppInstallerCLICore/Workflows/DownloadFlow.cpp | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp b/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp index 0a448220ad..031e810cc9 100644 --- a/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp @@ -40,11 +40,8 @@ namespace AppInstaller::CLI::Workflow case InstallerTypeEnum::Wix: return L".msi"sv; case InstallerTypeEnum::Msix: - { - Msix::MsixInfo msixInfo(installer->Url); - return msixInfo.GetIsBundle() ? L".msixbundle"sv : L".msix"sv; - break; - } + // Note: We may need to distinguish between .msix and .msixbundle in the future. + return L".msix"sv; case InstallerTypeEnum::Zip: return L".zip"sv; default: @@ -81,7 +78,7 @@ namespace AppInstaller::CLI::Workflow std::ifstream inStream{ filePath, std::ifstream::binary }; fileHash = SHA256::ComputeHash(inStream); - if (std::equal(expectedHash.begin(), expectedHash.end(), fileHash.begin())) + if (SHA256::AreEqual(expectedHash, fileHash)) { return true; }