commit 0b3ad7fd283ceb22969a60eea8c84692cad72bff parent 2fdaadc21d7bce84dbaa7e4618c81c9cf5ea24a8 Author: yao-msft <50888816+yao-msft@users.noreply.github.com> Date: Tue, 10 Nov 2020 02:26:25 -0800 Improve upgrade output experience (#634) Diffstat:
18 files changed, 403 insertions(+), 218 deletions(-)
diff --git a/src/AppInstallerCLICore/Commands/ListCommand.cpp b/src/AppInstallerCLICore/Commands/ListCommand.cpp @@ -72,7 +72,7 @@ namespace AppInstaller::CLI Workflow::OpenSource << Workflow::OpenCompositeSource(Repository::PredefinedSource::Installed) << Workflow::SearchSourceForMany << - Workflow::EnsureMatchesFromSearchResult << + Workflow::EnsureMatchesFromSearchResult(true) << Workflow::ReportListResult(); } } diff --git a/src/AppInstallerCLICore/Commands/SearchCommand.cpp b/src/AppInstallerCLICore/Commands/SearchCommand.cpp @@ -69,7 +69,7 @@ namespace AppInstaller::CLI context << Workflow::OpenSource << Workflow::SearchSourceForMany << - Workflow::EnsureMatchesFromSearchResult << + Workflow::EnsureMatchesFromSearchResult(false) << Workflow::ReportSearchResult; } } diff --git a/src/AppInstallerCLICore/Commands/ShowCommand.cpp b/src/AppInstallerCLICore/Commands/ShowCommand.cpp @@ -62,8 +62,8 @@ namespace AppInstaller::CLI context << Workflow::OpenSource << Workflow::SearchSourceForSingle << - Workflow::EnsureOneMatchFromSearchResult << - Workflow::ReportSearchResultIdentity << + Workflow::EnsureOneMatchFromSearchResult(false) << + Workflow::ReportPackageIdentity << Workflow::ShowAppVersions; } } diff --git a/src/AppInstallerCLICore/Commands/UpgradeCommand.cpp b/src/AppInstallerCLICore/Commands/UpgradeCommand.cpp @@ -14,6 +14,15 @@ using namespace AppInstaller::CLI::Workflow; namespace AppInstaller::CLI { + namespace + { + bool ShouldListUpgrade(Context& context) + { + return context.Args.Empty() || + (context.Args.GetArgsCount() == 1 && context.Args.Contains(Execution::Args::Type::Source)); + } + } + std::vector<Argument> UpgradeCommand::GetArguments() const { return { @@ -120,12 +129,12 @@ namespace AppInstaller::CLI OpenSource << OpenCompositeSource(Repository::PredefinedSource::Installed); - if (context.Args.Empty()) + if (ShouldListUpgrade(context)) { // Upgrade with no args list packages with updates available context << Workflow::SearchSourceForMany << - Workflow::EnsureMatchesFromSearchResult << + Workflow::EnsureMatchesFromSearchResult(true) << Workflow::ReportListResult(true); } else if (context.Args.Contains(Execution::Args::Type::All)) @@ -133,7 +142,7 @@ namespace AppInstaller::CLI // --all switch updates all packages found context << SearchSourceForMany << - EnsureMatchesFromSearchResult << + EnsureMatchesFromSearchResult(true) << UpdateAllApplicable; } else if (context.Args.Contains(Execution::Args::Type::Manifest)) @@ -143,7 +152,7 @@ namespace AppInstaller::CLI GetManifestFromArg << ReportManifestIdentity << SearchSourceUsingManifest << - EnsureOneMatchFromSearchResult << + EnsureOneMatchFromSearchResult(true) << GetInstalledPackageVersion << EnsureUpdateVersionApplicable << EnsureMinOSVersion << @@ -162,15 +171,15 @@ namespace AppInstaller::CLI // The remaining case: search for single installed package to update context << SearchSourceForSingle << - EnsureOneMatchFromSearchResult << - ReportSearchResultIdentity << + EnsureOneMatchFromSearchResult(true) << + ReportPackageIdentity << GetInstalledPackageVersion; if (context.Args.Contains(Execution::Args::Type::Version)) { // If version specified, use the version and verify applicability context << - GetManifestFromSearchResult << + GetManifestFromPackage << EnsureUpdateVersionApplicable << EnsureMinOSVersion << SelectInstaller << @@ -180,8 +189,7 @@ namespace AppInstaller::CLI { // iterate through available versions to find latest applicable update // This step also populates Manifest and Installer in context data - context << - SelectLatestApplicableUpdate(*(context.Get<Execution::Data::SearchResult>().Matches.at(0).Package)); + context << SelectLatestApplicableUpdate(true); } context << diff --git a/src/AppInstallerCLICore/ExecutionArgs.h b/src/AppInstallerCLICore/ExecutionArgs.h @@ -116,6 +116,11 @@ namespace AppInstaller::CLI::Execution return m_parsedArgs.empty(); } + size_t GetArgsCount() + { + return m_parsedArgs.size(); + } + private: std::map<Type, std::vector<std::string>> m_parsedArgs; }; diff --git a/src/AppInstallerCLICore/ExecutionContext.cpp b/src/AppInstallerCLICore/ExecutionContext.cpp @@ -116,14 +116,12 @@ namespace AppInstaller::CLI::Execution // Unless we want to spin a separate thread for all work, we have to just exit here. if (m_CtrlSignalCount >= 2) { + Logging::Telemetry().LogCommandTermination(hr, file, line); std::exit(hr); } } - if (!file.empty() && line) - { - Logging::Telemetry().LogCommandTermination(hr, file, line); - } + Logging::Telemetry().LogCommandTermination(hr, file, line); m_isTerminated = true; m_terminationHR = hr; diff --git a/src/AppInstallerCLICore/ExecutionContext.h b/src/AppInstallerCLICore/ExecutionContext.h @@ -46,6 +46,7 @@ namespace AppInstaller::CLI::Execution Source, SearchResult, SourceList, + Package, Manifest, PackageVersion, Installer, @@ -97,6 +98,12 @@ namespace AppInstaller::CLI::Execution }; template <> + struct DataMapping<Data::Package> + { + using value_t = std::shared_ptr<Repository::IPackage>; + }; + + template <> struct DataMapping<Data::Manifest> { using value_t = Manifest::Manifest; diff --git a/src/AppInstallerCLICore/TableOutput.h b/src/AppInstallerCLICore/TableOutput.h @@ -50,6 +50,8 @@ namespace AppInstaller::CLI::Execution void OutputLine(line_t&& line) { + m_empty = false; + if (m_buffer.size() < m_sizingBuffer) { m_buffer.emplace_back(std::move(line)); @@ -63,7 +65,15 @@ namespace AppInstaller::CLI::Execution void Complete() { - EvaluateAndFlushBuffer(); + if (!m_empty) + { + EvaluateAndFlushBuffer(); + } + } + + bool IsEmpty() + { + return m_empty; } private: @@ -81,6 +91,7 @@ namespace AppInstaller::CLI::Execution size_t m_sizingBuffer; std::vector<line_t> m_buffer; bool m_bufferEvaluated = false; + bool m_empty = true; void EvaluateAndFlushBuffer() { diff --git a/src/AppInstallerCLICore/Workflows/CompletionFlow.cpp b/src/AppInstallerCLICore/Workflows/CompletionFlow.cpp @@ -71,7 +71,7 @@ namespace AppInstaller::CLI::Workflow const std::string& word = context.Get<Data::CompletionData>().Word(); auto stream = context.Reporter.Completion(); - for (const auto& vc : context.Get<Execution::Data::SearchResult>().Matches[0].Package->GetAvailableVersionKeys()) + for (const auto& vc : context.Get<Execution::Data::Package>()->GetAvailableVersionKeys()) { if (word.empty() || Utility::ICUCaseInsensitiveStartsWith(vc.Version, word)) { @@ -87,7 +87,7 @@ namespace AppInstaller::CLI::Workflow std::vector<std::string> channels; - for (const auto& vc : context.Get<Execution::Data::SearchResult>().Matches[0].Package->GetAvailableVersionKeys()) + for (const auto& vc : context.Get<Execution::Data::Package>()->GetAvailableVersionKeys()) { if ((word.empty() || Utility::ICUCaseInsensitiveStartsWith(vc.Channel, word)) && std::find(channels.begin(), channels.end(), vc.Channel) == channels.end()) @@ -164,14 +164,14 @@ namespace AppInstaller::CLI::Workflow // Here we require that the standard search finds a single entry, and we list those versions. context << Workflow::SearchSourceForSingle << - Workflow::EnsureOneMatchFromSearchResult << + Workflow::EnsureOneMatchFromSearchResult(false) << Workflow::CompleteWithSearchResultVersions; break; case Execution::Args::Type::Channel: // Here we require that the standard search finds a single entry, and we list those channels. context << Workflow::SearchSourceForSingle << - Workflow::EnsureOneMatchFromSearchResult << + Workflow::EnsureOneMatchFromSearchResult(false) << Workflow::CompleteWithSearchResultChannels; break; case Execution::Args::Type::Source: diff --git a/src/AppInstallerCLICore/Workflows/MSStoreInstallerHandler.cpp b/src/AppInstallerCLICore/Workflows/MSStoreInstallerHandler.cpp @@ -69,26 +69,22 @@ namespace AppInstaller::CLI::Workflow return errorCode; } - } - void MSStoreInstall(Execution::Context& context) - { - auto productId = Utility::ConvertToUTF16(context.Get<Execution::Data::Installer>()->ProductId); + bool GetFreeUserEntitlement(Execution::Context& context, const std::wstring& productId) + { + AppInstallManager installManager; - AppInstallManager installManager; + // Verifying/Acquiring product ownership + context.Reporter.Info() << Resource::String::MSStoreInstallTryGetEntitlement << std::endl; + GetEntitlementResult enr = installManager.GetFreeUserEntitlementAsync(productId, winrt::hstring(), winrt::hstring()).get(); - // Verifying/Acquiring product ownership - context.Reporter.Info() << Resource::String::MSStoreInstallTryGetEntitlement << std::endl; - GetEntitlementResult enr = installManager.GetFreeUserEntitlementAsync(productId, winrt::hstring(), winrt::hstring()).get(); + if (enr.Status() == GetEntitlementStatus::Succeeded) + { + context.Reporter.Info() << Resource::String::MSStoreInstallGetEntitlementSuccess << std::endl; + AICLI_LOG(CLI, Error, << "Get entitlement succeeded."); - if (enr.Status() == GetEntitlementStatus::Succeeded) - { - context.Reporter.Info() << Resource::String::MSStoreInstallGetEntitlementSuccess << std::endl; - AICLI_LOG(CLI, Error, << "Get entitlement succeeded."); - } - else - { - if (enr.Status() == GetEntitlementStatus::NoStoreAccount) + } + else if (enr.Status() == GetEntitlementStatus::NoStoreAccount) { context.Reporter.Info() << Resource::String::MSStoreInstallGetEntitlementNoStoreAccount << std::endl; AICLI_LOG(CLI, Error, << "Get entitlement failed. No Store account."); @@ -104,6 +100,19 @@ namespace AppInstaller::CLI::Workflow AICLI_LOG(CLI, Error, << "Get entitlement succeeded. Server error. ProductId: " << Utility::ConvertToUTF8(productId)); } + return enr.Status() == GetEntitlementStatus::Succeeded; + } + } + + void MSStoreInstall(Execution::Context& context) + { + auto productId = Utility::ConvertToUTF16(context.Get<Execution::Data::Installer>()->ProductId); + + AppInstallManager installManager; + + // Verifying/Acquiring product ownership + if (!GetFreeUserEntitlement(context, productId)) + { AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_MSSTORE_INSTALL_FAILED); } @@ -139,6 +148,12 @@ namespace AppInstaller::CLI::Workflow AppInstallManager installManager; + // Verifying/Acquiring product ownership + if (!GetFreeUserEntitlement(context, productId)) + { + AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_MSSTORE_INSTALL_FAILED); + } + context.Reporter.Info() << Resource::String::InstallFlowStartingPackageInstall << std::endl; // SearchForUpdateAsync will automatically trigger update if found. diff --git a/src/AppInstallerCLICore/Workflows/ShowFlow.cpp b/src/AppInstallerCLICore/Workflows/ShowFlow.cpp @@ -84,7 +84,7 @@ namespace AppInstaller::CLI::Workflow void ShowAppVersions(Execution::Context& context) { - auto versions = context.Get<Execution::Data::SearchResult>().Matches.at(0).Package->GetAvailableVersionKeys(); + auto versions = context.Get<Execution::Data::Package>()->GetAvailableVersionKeys(); Execution::TableOutput<2> table(context.Reporter, { Resource::String::ShowVersion, Resource::String::ShowChannel }); for (const auto& version : versions) diff --git a/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp b/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp @@ -21,19 +21,20 @@ namespace AppInstaller::CLI::Workflow void SelectLatestApplicableUpdate::operator()(Execution::Context& context) const { + auto package = context.Get<Execution::Data::Package>(); auto installedPackage = context.Get<Execution::Data::InstalledPackageVersion>(); Utility::Version installedVersion = Utility::Version(installedPackage->GetProperty(PackageVersionProperty::Version)); ManifestComparator manifestComparator(context.Args, installedPackage->GetMetadata()); bool updateFound = false; // The version keys should have already been sorted by version - const auto& versionKeys = m_package.GetAvailableVersionKeys(); + const auto& versionKeys = package->GetAvailableVersionKeys(); for (const auto& key : versionKeys) { // Check Update Version if (IsUpdateVersionApplicable(installedVersion, Utility::Version(key.Version))) { - auto packageVersion = m_package.GetAvailableVersion(key); + auto packageVersion = package->GetAvailableVersion(key); auto manifest = packageVersion->GetManifest(); // Check MinOSVersion @@ -66,7 +67,10 @@ namespace AppInstaller::CLI::Workflow if (!updateFound) { - context.Reporter.Info() << Resource::String::UpdateNotApplicable << std::endl; + if (m_reportUpdateNotFound) + { + context.Reporter.Info() << Resource::String::UpdateNotApplicable << std::endl; + } AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_UPDATE_NOT_APPLICABLE); } } @@ -88,6 +92,8 @@ namespace AppInstaller::CLI::Workflow { const auto& matches = context.Get<Execution::Data::SearchResult>().Matches; bool updateAllHasFailure = false; + bool updateAllFoundUpdate = false; + for (const auto& match : matches) { Logging::SubExecutionTelemetryScope subExecution; @@ -95,13 +101,23 @@ namespace AppInstaller::CLI::Workflow // We want to do best effort to update all applicable updates regardless on previous update failure auto updateContextPtr = context.Clone(); Execution::Context& updateContext = *updateContextPtr; - updateContext.Reporter.Info() << std::endl; - updateContext.Add<Execution::Data::InstalledPackageVersion>(match.Package->GetInstalledVersion()); + updateContext.Add<Execution::Data::Package>(match.Package); updateContext << + Workflow::GetInstalledPackageVersion << Workflow::ReportExecutionStage(ExecutionStage::Discovery) << - SelectLatestApplicableUpdate(*(match.Package)) << + SelectLatestApplicableUpdate(false); + + if (updateContext.GetTerminationHR() == APPINSTALLER_CLI_ERROR_UPDATE_NOT_APPLICABLE) + { + continue; + } + + updateAllFoundUpdate = true; + + updateContext << + ReportManifestIdentity << ShowInstallationDisclaimer << Workflow::ReportExecutionStage(ExecutionStage::Download) << DownloadInstaller << @@ -110,6 +126,9 @@ namespace AppInstaller::CLI::Workflow Workflow::ReportExecutionStage(ExecutionStage::PostExecution) << RemoveInstaller; + updateContext.Reporter.Info() << std::endl; + + // msstore update might still terminate with APPINSTALLER_CLI_ERROR_UPDATE_NOT_APPLICABLE if (updateContext.GetTerminationHR() != S_OK && updateContext.GetTerminationHR() != APPINSTALLER_CLI_ERROR_UPDATE_NOT_APPLICABLE) { @@ -117,6 +136,11 @@ namespace AppInstaller::CLI::Workflow } } + if (!updateAllFoundUpdate) + { + context.Reporter.Info() << Resource::String::UpdateNotApplicable << std::endl; + } + if (updateAllHasFailure) { AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_UPDATE_ALL_HAS_FAILURE); diff --git a/src/AppInstallerCLICore/Workflows/UpdateFlow.h b/src/AppInstallerCLICore/Workflows/UpdateFlow.h @@ -7,18 +7,18 @@ namespace AppInstaller::CLI::Workflow { // Iterates through all available versions from a package and find latest applicable update - // Required Args: the package - // Inputs: InstalledPackageVersion + // Required Args: bool indicating whether to report update not found + // Inputs: InstalledPackageVersion, Package // Outputs: Manifest?, Installer? struct SelectLatestApplicableUpdate : public WorkflowTask { - SelectLatestApplicableUpdate(const AppInstaller::Repository::IPackage& package) : - WorkflowTask("SelectLatestApplicableUpdate"), m_package(package) {} + SelectLatestApplicableUpdate(bool reportUpdateNotFound) : + WorkflowTask("SelectLatestApplicableUpdate"), m_reportUpdateNotFound(reportUpdateNotFound) {} void operator()(Execution::Context& context) const override; private: - const AppInstaller::Repository::IPackage& m_package; + bool m_reportUpdateNotFound; }; // Ensures the update package has higher version than installed diff --git a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp @@ -325,6 +325,34 @@ namespace AppInstaller::CLI::Workflow } } + void ReportMultiplePackageFoundResult(Execution::Context& context) + { + auto& searchResult = context.Get<Execution::Data::SearchResult>(); + + Execution::TableOutput<5> table(context.Reporter, + { + Resource::String::SearchName, + Resource::String::SearchId + }); + + for (size_t i = 0; i < searchResult.Matches.size(); ++i) + { + auto package = searchResult.Matches[i].Package; + + table.OutputLine({ + package->GetProperty(PackageProperty::Name), + package->GetProperty(PackageProperty::Id) + }); + } + + table.Complete(); + + if (searchResult.Truncated) + { + context.Reporter.Info() << '<' << Resource::String::SearchTruncated << '>' << std::endl; + } + } + void ReportListResult::operator()(Execution::Context& context) const { auto& searchResult = context.Get<Execution::Data::SearchResult>(); @@ -371,13 +399,18 @@ namespace AppInstaller::CLI::Workflow table.Complete(); + if (table.IsEmpty()) + { + context.Reporter.Info() << Resource::String::NoInstalledPackageFound << std::endl; + } + if (searchResult.Truncated) { context.Reporter.Info() << '<' << Resource::String::SearchTruncated << '>' << std::endl; } } - void EnsureMatchesFromSearchResult(Execution::Context& context) + void EnsureMatchesFromSearchResult::operator()(Execution::Context& context) const { auto& searchResult = context.Get<Execution::Data::SearchResult>(); @@ -385,7 +418,7 @@ namespace AppInstaller::CLI::Workflow { Logging::Telemetry().LogNoAppMatch(); - if (WI_IsFlagSet(context.GetFlags(), Execution::ContextFlag::InstallerExecutionUseUpdate)) + if (m_isFromInstalledSource) { context.Reporter.Info() << Resource::String::NoInstalledPackageFound << std::endl; } @@ -398,11 +431,12 @@ namespace AppInstaller::CLI::Workflow } } - void EnsureOneMatchFromSearchResult(Execution::Context& context) + void EnsureOneMatchFromSearchResult::operator()(Execution::Context& context) const { context << - EnsureMatchesFromSearchResult << - [](Execution::Context& context) + EnsureMatchesFromSearchResult(m_isFromInstalledSource); + + if (!context.IsTerminated()) { auto& searchResult = context.Get<Execution::Data::SearchResult>(); @@ -410,7 +444,7 @@ namespace AppInstaller::CLI::Workflow { Logging::Telemetry().LogMultiAppMatch(); - if (WI_IsFlagSet(context.GetFlags(), Execution::ContextFlag::InstallerExecutionUseUpdate)) + if (m_isFromInstalledSource) { context.Reporter.Warn() << Resource::String::MultipleInstalledPackagesFound << std::endl; } @@ -419,22 +453,25 @@ namespace AppInstaller::CLI::Workflow context.Reporter.Warn() << Resource::String::MultiplePackagesFound << std::endl; } - context << ReportSearchResult; + context << ReportMultiplePackageFoundResult; + AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_MULTIPLE_APPLICATIONS_FOUND); } - auto latestVersion = searchResult.Matches.at(0).Package->GetLatestAvailableVersion(); - Logging::Telemetry().LogAppFound(latestVersion->GetProperty(PackageVersionProperty::Name), latestVersion->GetProperty(PackageVersionProperty::Id)); + std::shared_ptr<IPackage> package = searchResult.Matches.at(0).Package; + Logging::Telemetry().LogAppFound(package->GetProperty(PackageProperty::Name), package->GetProperty(PackageProperty::Id)); + + context.Add<Execution::Data::Package>(std::move(package)); }; } - void GetManifestFromSearchResult(Execution::Context& context) + void GetManifestFromPackage(Execution::Context& context) { std::string_view version = context.Args.GetArg(Execution::Args::Type::Version); std::string_view channel = context.Args.GetArg(Execution::Args::Type::Channel); PackageVersionKey key("", version, channel); - auto requestedVersion = context.Get<Execution::Data::SearchResult>().Matches.at(0).Package->GetAvailableVersion(key); + auto requestedVersion = context.Get<Execution::Data::Package>()->GetAvailableVersion(key); std::optional<Manifest::Manifest> manifest; if (requestedVersion) @@ -494,10 +531,10 @@ namespace AppInstaller::CLI::Workflow }; } - void ReportSearchResultIdentity(Execution::Context& context) + void ReportPackageIdentity(Execution::Context& context) { - auto latestVersion = context.Get<Execution::Data::SearchResult>().Matches.at(0).Package->GetLatestAvailableVersion(); - ReportIdentity(context, latestVersion->GetProperty(PackageVersionProperty::Name), latestVersion->GetProperty(PackageVersionProperty::Id)); + auto package = context.Get<Execution::Data::Package>(); + ReportIdentity(context, package->GetProperty(PackageProperty::Name), package->GetProperty(PackageProperty::Id)); } void ReportManifestIdentity(Execution::Context& context) @@ -519,9 +556,9 @@ namespace AppInstaller::CLI::Workflow context << OpenSource << SearchSourceForSingle << - EnsureOneMatchFromSearchResult << - ReportSearchResultIdentity << - GetManifestFromSearchResult; + EnsureOneMatchFromSearchResult(false) << + ReportPackageIdentity << + GetManifestFromPackage; } } @@ -600,8 +637,7 @@ namespace AppInstaller::CLI::Workflow void GetInstalledPackageVersion(Execution::Context& context) { - const auto& searchResult = context.Get<Execution::Data::SearchResult>(); - context.Add<Execution::Data::InstalledPackageVersion>(searchResult.Matches.at(0).Package->GetInstalledVersion()); + context.Add<Execution::Data::InstalledPackageVersion>(context.Get<Execution::Data::Package>()->GetInstalledVersion()); } void ReportExecutionStage::operator()(Execution::Context& context) const diff --git a/src/AppInstallerCLICore/Workflows/WorkflowBase.h b/src/AppInstallerCLICore/Workflows/WorkflowBase.h @@ -148,23 +148,47 @@ namespace AppInstaller::CLI::Workflow bool m_onlyShowUpgrades; }; - // Ensures that there is at least one result in the search. + // Outputs the search results when multiple packages found but only one expected. // Required Args: None // Inputs: SearchResult // Outputs: None - void EnsureMatchesFromSearchResult(Execution::Context& context); + void ReportMultiplePackageFoundResult(Execution::Context& context); + + // Ensures that there is at least one result in the search. + // Required Args: bool indicating id the search result is from installed source + // Inputs: SearchResult + // Outputs: None + struct EnsureMatchesFromSearchResult : public WorkflowTask + { + EnsureMatchesFromSearchResult(bool isFromInstalledSource) : + WorkflowTask("EnsureMatchesFromSearchResult"), m_isFromInstalledSource(isFromInstalledSource) {} + + void operator()(Execution::Context& context) const override; + + private: + bool m_isFromInstalledSource; + }; // Ensures that there is only one result in the search. - // Required Args: None + // Required Args: bool indicating id the search result is from installed source // Inputs: SearchResult // Outputs: None - void EnsureOneMatchFromSearchResult(Execution::Context& context); + struct EnsureOneMatchFromSearchResult : public WorkflowTask + { + EnsureOneMatchFromSearchResult(bool isFromInstalledSource) : + WorkflowTask("EnsureOneMatchFromSearchResult"), m_isFromInstalledSource(isFromInstalledSource) {} + + void operator()(Execution::Context& context) const override; + + private: + bool m_isFromInstalledSource; + }; - // Gets the manifest from a search result. + // Gets the manifest from package. // Required Args: None - // Inputs: SearchResult - // Outputs: Manifest - void GetManifestFromSearchResult(Execution::Context& context); + // Inputs: Package + // Outputs: Manifest, PackageVersion + void GetManifestFromPackage(Execution::Context& context); // Ensures the the file exists and is not a directory. // Required Args: the one given @@ -186,11 +210,11 @@ namespace AppInstaller::CLI::Workflow // Outputs: Manifest void GetManifestFromArg(Execution::Context& context); - // Reports the search result's identity. + // Reports the search result's package identity. // Required Args: None - // Inputs: SearchResult (only 1) + // Inputs: Package // Outputs: None - void ReportSearchResultIdentity(Execution::Context& context); + void ReportPackageIdentity(Execution::Context& context); // Reports the manifest's identity. // Required Args: None @@ -238,7 +262,7 @@ namespace AppInstaller::CLI::Workflow // Gets the installed package version // Required Args: None - // Inputs: SearchResult + // Inputs: Package // Outputs: InstalledPackageVersion void GetInstalledPackageVersion(Execution::Context& context); diff --git a/src/AppInstallerCLITests/Sources.cpp b/src/AppInstallerCLITests/Sources.cpp @@ -535,7 +535,7 @@ TEST_CASE("RepoSources_DropSourceByName", "[sources]") DropSource("testName"); sources = GetSources(); - REQUIRE(sources.size() == 3); + REQUIRE(sources.size() == 2); const char* suffix[2] = { "2", "3" }; @@ -549,8 +549,6 @@ TEST_CASE("RepoSources_DropSourceByName", "[sources]") REQUIRE(sources[i].LastUpdateTime == ConvertUnixEpochToSystemClock(i + 1)); REQUIRE(sources[i].Origin == SourceOrigin::User); } - - REQUIRE(sources[2].Origin == SourceOrigin::Default); } TEST_CASE("RepoSources_DropAllSources", "[sources]") diff --git a/src/AppInstallerCommonCore/Manifest/ManifestInstaller.cpp b/src/AppInstallerCommonCore/Manifest/ManifestInstaller.cpp @@ -12,6 +12,7 @@ namespace AppInstaller::Manifest None, Exe, Msi, + Msix, }; CompatibilitySet GetCompatibilitySet(ManifestInstaller::InstallerTypeEnum type) @@ -26,6 +27,9 @@ namespace AppInstaller::Manifest case ManifestInstaller::InstallerTypeEnum::Wix: case ManifestInstaller::InstallerTypeEnum::Msi: return CompatibilitySet::Msi; + case ManifestInstaller::InstallerTypeEnum::Msix: + case ManifestInstaller::InstallerTypeEnum::MSStore: + return CompatibilitySet::Msix; default: return CompatibilitySet::None; } diff --git a/src/AppInstallerRepositoryCore/RepositorySource.cpp b/src/AppInstallerRepositoryCore/RepositorySource.cpp @@ -43,12 +43,6 @@ namespace AppInstaller::Repository bool IsTombstone = false; }; - // Finds a source from the given vector by its name. - auto FindSourceByName(std::vector<SourceDetailsInternal>& sources, std::string_view name) - { - return std::find_if(sources.begin(), sources.end(), [&name](const SourceDetailsInternal& sd) { return Utility::ICUCaseInsensitiveEquals(sd.Name, name); }); - } - // Attempts to read a single scalar value from the node. template<typename Value> bool TryReadScalar(std::string_view settingName, const std::string& settingValue, const YAML::Node& sourceNode, std::string_view name, Value& value, bool required = true) @@ -235,56 +229,6 @@ namespace AppInstaller::Repository return result; } - // Gets the internal view of the sources. - std::vector<SourceDetailsInternal> GetSourcesInternal() - { - std::vector<SourceDetailsInternal> result; - - for (SourceOrigin origin : { SourceOrigin::User, SourceOrigin::Default }) - { - auto forOrigin = GetSourcesByOrigin(origin); - - for (auto&& source : forOrigin) - { - auto itr = FindSourceByName(result, source.Name); - if (itr == result.end()) - { - // Name not already defined, add it - result.emplace_back(std::move(source)); - } - else - { - AICLI_LOG(Repo, Info, << "Source named '" << itr->Name << "' is already defined at origin " << ToString(itr->Origin) << - ". The source from origin " << ToString(origin) << " is dropped."); - } - } - } - - // Remove all tombstones, walking backwards. - for (size_t j = result.size(); j > 0; --j) - { - size_t i = j - 1; - - if (result[i].IsTombstone) - { - AICLI_LOG(Repo, Info, << "Source named '" << result[i].Name << "' from origin " << ToString(result[i].Origin) << " is a tombstone and is dropped."); - result.erase(result.begin() + i); - } - } - - auto metadata = GetMetadata(); - for (const auto& metaSource : metadata) - { - auto itr = FindSourceByName(result, metaSource.Name); - if (itr != result.end()) - { - itr->LastUpdateTime = metaSource.LastUpdateTime; - } - } - - return result; - } - // Sets the sources for a particular setting, from a particular origin. void SetSourcesToSettingWithFilter(const Settings::StreamDefinition& setting, SourceOrigin origin, const std::vector<SourceDetailsInternal>& sources) { @@ -446,6 +390,153 @@ namespace AppInstaller::Repository return false; } + + // Struct containing internal implementation of source list + // This contains all sources including tombstoned sources + struct SourceListInternal + { + SourceListInternal(); + + // Get a list of current sources references which can be used to update the contents in place. + // e.g. update the LastTimeUpdated value of sources. + std::vector<std::reference_wrapper<SourceDetailsInternal>> GetCurrentSourceRefs(); + + // Current source means source that's not in tombstone + SourceDetailsInternal* GetCurrentSource(std::string_view name); + + // Source includes ones in tombstone + SourceDetailsInternal* GetSource(std::string_view name); + + // Add/remove a current source + void AddSource(const SourceDetailsInternal& source); + void RemoveSource(const SourceDetailsInternal& source); + + // Save source metadata. Currently only LastTimeUpdated is used. + void SaveMetadata() const; + + private: + std::vector<SourceDetailsInternal> m_sourceList; + + // calls std::find_if and return the iterator. + auto FindSource(std::string_view name, bool includeTombstone = false); + }; + + SourceListInternal::SourceListInternal() + { + for (SourceOrigin origin : { SourceOrigin::User, SourceOrigin::Default }) + { + auto forOrigin = GetSourcesByOrigin(origin); + + for (auto&& source : forOrigin) + { + auto foundSource = GetSource(source.Name); + if (!foundSource) + { + // Name not already defined, add it + m_sourceList.emplace_back(std::move(source)); + } + else + { + AICLI_LOG(Repo, Info, << "Source named '" << foundSource->Name << "' is already defined at origin " << ToString(foundSource->Origin) << + ". The source from origin " << ToString(origin) << " is dropped."); + } + } + } + + auto metadata = GetMetadata(); + for (const auto& metaSource : metadata) + { + auto source = GetSource(metaSource.Name); + if (source) + { + source->LastUpdateTime = metaSource.LastUpdateTime; + } + } + } + + std::vector<std::reference_wrapper<SourceDetailsInternal>> SourceListInternal::GetCurrentSourceRefs() + { + std::vector<std::reference_wrapper<SourceDetailsInternal>> result; + + for (auto& s : m_sourceList) + { + if (!s.IsTombstone) + { + result.emplace_back(std::ref(s)); + } + else + { + AICLI_LOG(Repo, Info, << "GetCurrentSourceRefs: Source named '" << s.Name << "' from origin " << ToString(s.Origin) << " is a tombstone and is dropped."); + } + } + + return result; + } + + auto SourceListInternal::FindSource(std::string_view name, bool includeTombstone) + { + return std::find_if(m_sourceList.begin(), m_sourceList.end(), + [name, includeTombstone](const SourceDetailsInternal& sd) + { + return Utility::ICUCaseInsensitiveEquals(sd.Name, name) && + (!sd.IsTombstone || includeTombstone); + }); + } + + SourceDetailsInternal* SourceListInternal::GetCurrentSource(std::string_view name) + { + auto itr = FindSource(name); + return itr == m_sourceList.end() ? nullptr : &(*itr); + } + + SourceDetailsInternal* SourceListInternal::GetSource(std::string_view name) + { + auto itr = FindSource(name, true); + return itr == m_sourceList.end() ? nullptr : &(*itr); + } + + void SourceListInternal::AddSource(const SourceDetailsInternal& details) + { + // Erase the source's tombstone entry if applicable + auto itr = FindSource(details.Name, true); + if (itr != m_sourceList.end()) + { + THROW_HR_IF(HRESULT_FROM_WIN32(ERROR_INVALID_STATE), !itr->IsTombstone); + m_sourceList.erase(itr); + } + + m_sourceList.emplace_back(details); + + SetSourcesByOrigin(SourceOrigin::User, m_sourceList); + } + + void SourceListInternal::RemoveSource(const SourceDetailsInternal& source) + { + switch (source.Origin) + { + case SourceOrigin::Default: + { + SourceDetailsInternal tombstone; + tombstone.Name = source.Name; + tombstone.IsTombstone = true; + tombstone.Origin = SourceOrigin::User; + m_sourceList.emplace_back(std::move(tombstone)); + } + break; + case SourceOrigin::User: + m_sourceList.erase(FindSource(source.Name)); + break; + default: + THROW_HR(E_UNEXPECTED); + } + + SetSourcesByOrigin(SourceOrigin::User, m_sourceList); + } + + void SourceListInternal::SaveMetadata() const + { + SetMetadata(m_sourceList); + } } std::string_view ToString(SourceOrigin origin) @@ -463,10 +554,10 @@ namespace AppInstaller::Repository std::vector<SourceDetails> GetSources() { - auto internalResult = GetSourcesInternal(); + SourceListInternal sourceList; std::vector<SourceDetails> result; - for (auto&& source : internalResult) + for (auto&& source : sourceList.GetCurrentSourceRefs()) { result.emplace_back(std::move(source)); } @@ -477,16 +568,16 @@ namespace AppInstaller::Repository std::optional<SourceDetails> GetSource(std::string_view name) { // Check all sources for the given name. - auto currentSources = GetSourcesInternal(); + SourceListInternal sourceList; - auto itr = FindSourceByName(currentSources, name); - if (itr == currentSources.end()) + auto source = sourceList.GetCurrentSource(name); + if (!source) { return {}; } else { - return *itr; + return *source; } } @@ -497,10 +588,10 @@ namespace AppInstaller::Repository AICLI_LOG(Repo, Info, << "Adding source: Name[" << name << "], Type[" << type << "], Arg[" << arg << "]"); // Check all sources for the given name. - auto currentSources = GetSourcesInternal(); + SourceListInternal sourceList; - auto itr = FindSourceByName(currentSources, name); - THROW_HR_IF(APPINSTALLER_CLI_ERROR_SOURCE_NAME_ALREADY_EXISTS, itr != currentSources.end()); + auto source = sourceList.GetCurrentSource(name); + THROW_HR_IF(APPINSTALLER_CLI_ERROR_SOURCE_NAME_ALREADY_EXISTS, source != nullptr); SourceDetailsInternal details; details.Name = name; @@ -512,14 +603,14 @@ namespace AppInstaller::Repository AddSourceFromDetails(details, progress); AICLI_LOG(Repo, Info, << "Source created with extra data: " << details.Data); - currentSources.emplace_back(details); - SetSourcesByOrigin(SourceOrigin::User, currentSources); + sourceList.AddSource(details); } std::shared_ptr<ISource> OpenSource(std::string_view name, IProgressCallback& progress) { - auto currentSources = GetSourcesInternal(); + SourceListInternal sourceList; + auto currentSources = sourceList.GetCurrentSourceRefs(); if (name.empty()) { @@ -530,8 +621,8 @@ namespace AppInstaller::Repository } else if(currentSources.size() == 1) { - AICLI_LOG(Repo, Info, << "Default source requested, only 1 source available, using the only source: " << currentSources[0].Name); - return OpenSource(currentSources[0].Name, progress); + AICLI_LOG(Repo, Info, << "Default source requested, only 1 source available, using the only source: " << currentSources[0].get().Name); + return OpenSource(currentSources[0].get().Name, progress); } else { @@ -541,7 +632,7 @@ namespace AppInstaller::Repository bool sourceUpdated = false; for (auto& source : currentSources) { - AICLI_LOG(Repo, Info, << "Adding to aggregated source: " << source.Name); + AICLI_LOG(Repo, Info, << "Adding to aggregated source: " << source.get().Name); if (ShouldUpdateBeforeOpen(source)) { @@ -555,7 +646,7 @@ namespace AppInstaller::Repository if (sourceUpdated) { - SetMetadata(currentSources); + sourceList.SaveMetadata(); } return aggregatedSource; @@ -563,22 +654,22 @@ namespace AppInstaller::Repository } else { - auto itr = FindSourceByName(currentSources, name); - - if (itr == currentSources.end()) + auto source = sourceList.GetCurrentSource(name); + if (!source) { AICLI_LOG(Repo, Info, << "Named source requested, but not found: " << name); return {}; } else { - AICLI_LOG(Repo, Info, << "Named source requested, found: " << itr->Name); - if (ShouldUpdateBeforeOpen(*itr)) + AICLI_LOG(Repo, Info, << "Named source requested, found: " << source->Name); + + if (ShouldUpdateBeforeOpen(*source)) { - UpdateSourceFromDetails(*itr, progress); - SetMetadata(currentSources); + UpdateSourceFromDetails(*source, progress); + sourceList.SaveMetadata(); } - return CreateSourceFromDetails(*itr, progress); + return CreateSourceFromDetails(*source, progress); } } } @@ -630,20 +721,21 @@ namespace AppInstaller::Repository { THROW_HR_IF(E_INVALIDARG, name.empty()); - auto currentSources = GetSourcesInternal(); - auto itr = FindSourceByName(currentSources, name); + SourceListInternal sourceList; - if (itr == currentSources.end()) + auto source = sourceList.GetCurrentSource(name); + if (!source) { AICLI_LOG(Repo, Info, << "Named source to be updated, but not found: " << name); return false; } else { - AICLI_LOG(Repo, Info, << "Named source to be updated, found: " << itr->Name); - UpdateSourceFromDetails(*itr, progress); + AICLI_LOG(Repo, Info, << "Named source to be updated, found: " << source->Name); + + UpdateSourceFromDetails(*source, progress); - SetMetadata(currentSources); + sourceList.SaveMetadata(); return true; } } @@ -652,52 +744,20 @@ namespace AppInstaller::Repository { THROW_HR_IF(E_INVALIDARG, name.empty()); - auto currentSources = GetSourcesInternal(); - auto itr = FindSourceByName(currentSources, name); + SourceListInternal sourceList; - if (itr == currentSources.end()) + auto source = sourceList.GetCurrentSource(name); + if (!source) { AICLI_LOG(Repo, Info, << "Named source to be removed, but not found: " << name); return false; } else { - AICLI_LOG(Repo, Info, << "Named source to be removed, found: " << itr->Name << " [" << ToString(itr->Origin) << ']'); - RemoveSourceFromDetails(*itr, progress); - - switch (itr->Origin) - { - case SourceOrigin::Default: - { - SourceDetailsInternal tombstone; - tombstone.Name = name; - tombstone.IsTombstone = true; - tombstone.Origin = SourceOrigin::User; - currentSources.emplace_back(std::move(tombstone)); - } - break; - case SourceOrigin::User: - currentSources.erase(itr); - break; - default: - THROW_HR(E_UNEXPECTED); - } + AICLI_LOG(Repo, Info, << "Named source to be removed, found: " << source->Name << " [" << ToString(source->Origin) << ']'); + RemoveSourceFromDetails(*source, progress); - // Add back tombstoned default sources, otherwise the info will be lost by SetSourcesByOrigin - auto defaultSources = GetSourcesByOrigin(SourceOrigin::Default); - for (const auto& defaultSource : defaultSources) - { - if (FindSourceByName(currentSources, defaultSource.Name) == currentSources.end()) - { - SourceDetailsInternal tombstone; - tombstone.Name = defaultSource.Name; - tombstone.IsTombstone = true; - tombstone.Origin = SourceOrigin::User; - currentSources.emplace_back(std::move(tombstone)); - } - } - - SetSourcesByOrigin(SourceOrigin::User, currentSources); + sourceList.RemoveSource(*source); return true; } @@ -713,24 +773,19 @@ namespace AppInstaller::Repository } else { - auto currentSources = GetSourcesInternal(); - auto itr = FindSourceByName(currentSources, name); + SourceListInternal sourceList; - if (itr == currentSources.end()) + auto source = sourceList.GetCurrentSource(name); + if (!source) { AICLI_LOG(Repo, Info, << "Named source to be dropped, but not found: " << name); return false; } else { - AICLI_LOG(Repo, Info, << "Named source to be dropped, found: " << itr->Name); - - currentSources.erase(itr); + AICLI_LOG(Repo, Info, << "Named source to be dropped, found: " << source->Name); - // Since this only writes the user setting, it can't actually drop non-user sources. - // But since it also implicitly sets all metadata, it will drop the metadata and allow - // somewhat of a clean slate. - SetSourcesByOrigin(SourceOrigin::User, currentSources); + sourceList.RemoveSource(*source); return true; }