From 41e70acb3c1f24a54742c088dbe7a3949f7ffdb0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Flor=20Chac=C3=B3n?= <14323496+florelis@users.noreply.github.com> Date: Tue, 6 Jun 2023 11:22:38 -0700 Subject: [PATCH] Make pinning a stable feature (#3315) Co-authored-by: Ryan Fu --- doc/Settings.md | 11 --------- .../JSON/settings/settings.schema.0.2.json | 5 ---- src/AppInstallerCLICore/Commands/PinCommand.h | 2 +- .../Workflows/WorkflowBase.cpp | 4 ++-- src/AppInstallerCLIE2ETests/Pinning.cs | 9 -------- src/AppInstallerCLITests/CompositeSource.cpp | 10 +------- .../ExperimentalFeature.cpp | 4 ---- .../Public/winget/ExperimentalFeature.h | 5 ++-- .../Public/winget/UserSettings.h | 2 -- src/AppInstallerCommonCore/UserSettings.cpp | 1 - .../CompositeSource.cpp | 23 ++++++++----------- 11 files changed, 16 insertions(+), 60 deletions(-) diff --git a/doc/Settings.md b/doc/Settings.md index cd86044f17..2019f7fee6 100644 --- a/doc/Settings.md +++ b/doc/Settings.md @@ -252,17 +252,6 @@ Experimental feature with the aim of managing dependencies, as of now it only sh }, ``` -### pinning - -This feature enables the ability to pin packages to prevent the Windows Package Manager from updating them. -You can enable the feature as shown below. - -```json - "experimentalFeatures": { - "pinning": true - }, -``` - ### configuration This feature enables the configuration commands. These commands allow configuring the system into a desired state. diff --git a/schemas/JSON/settings/settings.schema.0.2.json b/schemas/JSON/settings/settings.schema.0.2.json index 2a8dcfc6b5..838ed5ffd9 100644 --- a/schemas/JSON/settings/settings.schema.0.2.json +++ b/schemas/JSON/settings/settings.schema.0.2.json @@ -209,11 +209,6 @@ "type": "boolean", "default": false }, - "pinning": { - "description": "Enable support for package pinning", - "type": "boolean", - "default": false - }, "configuration": { "description": "Enable support for configuration", "type": "boolean", diff --git a/src/AppInstallerCLICore/Commands/PinCommand.h b/src/AppInstallerCLICore/Commands/PinCommand.h index d2eb3184d1..285e66eaa7 100644 --- a/src/AppInstallerCLICore/Commands/PinCommand.h +++ b/src/AppInstallerCLICore/Commands/PinCommand.h @@ -8,7 +8,7 @@ namespace AppInstaller::CLI { struct PinCommand final : public Command { - PinCommand(std::string_view parent) : Command("pin", {} /* aliases */, parent, Settings::ExperimentalFeature::Feature::Pinning) {} + PinCommand(std::string_view parent) : Command("pin", {} /* aliases */, parent) {} std::vector> GetCommands() const override; diff --git a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp index 319d866ea1..2390bec7fd 100644 --- a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp +++ b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp @@ -770,7 +770,7 @@ namespace AppInstaller::CLI::Workflow continue; } - if (m_onlyShowUpgrades && !updateAvailable && ExperimentalFeature::IsEnabled(ExperimentalFeature::Feature::Pinning)) + if (m_onlyShowUpgrades && !updateAvailable) { bool updateAvailableWithoutPins = match.Package->IsUpdateAvailable(PinBehavior::IgnorePins); if (updateAvailableWithoutPins) @@ -959,7 +959,7 @@ namespace AppInstaller::CLI::Workflow std::shared_ptr package = context.Get(); std::shared_ptr requestedVersion; - if (m_considerPins && ExperimentalFeature::IsEnabled(ExperimentalFeature::Feature::Pinning)) + if (m_considerPins) { bool isPinned = false; diff --git a/src/AppInstallerCLIE2ETests/Pinning.cs b/src/AppInstallerCLIE2ETests/Pinning.cs index ae65a4a942..5fb22a2244 100644 --- a/src/AppInstallerCLIE2ETests/Pinning.cs +++ b/src/AppInstallerCLIE2ETests/Pinning.cs @@ -15,15 +15,6 @@ namespace AppInstallerCLIE2ETests /// public class Pinning : BaseCommand { - /// - /// Setup done once before all the tests here. - /// - [OneTimeSetUp] - public void OneTimeSetup() - { - WinGetSettingsHelper.ConfigureFeature("pinning", true); - } - /// /// Set up for all tests. /// diff --git a/src/AppInstallerCLITests/CompositeSource.cpp b/src/AppInstallerCLITests/CompositeSource.cpp index e3eebbca9f..2d92ceb4bb 100644 --- a/src/AppInstallerCLITests/CompositeSource.cpp +++ b/src/AppInstallerCLITests/CompositeSource.cpp @@ -589,7 +589,6 @@ TEST_CASE("CompositePackage_AvailableVersions_NoChannelFilteredOut", "[Composite TEST_CASE("CompositeSource_MultipleAvailableSources_MatchAll", "[CompositeSource]") { TestCommon::TestUserSettings testSettings; - testSettings.Set(true); std::string pfn = "sortof_apfn"; std::string firstName = "Name1"; @@ -1102,8 +1101,6 @@ TEST_CASE("CompositeSource_Pinning_AvailableVersionPinned", "[CompositeSource][P TestHook::SetPinningIndex_Override pinningIndexOverride(indexFile.GetPath()); TestUserSettings userSettings; - userSettings.Set(true); - CompositeTestSetup setup; auto installedPackage = TestPackage::Make(MakeDefaultManifest("1.0.1"sv), TestPackage::MetadataMap{}); @@ -1215,8 +1212,6 @@ TEST_CASE("CompositeSource_Pinning_OneSourcePinned", "[CompositeSource][PinFlow] TestHook::SetPinningIndex_Override pinningIndexOverride(indexFile.GetPath()); TestUserSettings userSettings; - userSettings.Set(true); - CompositeTestSetup setup; auto installedPackage = TestPackage::Make(MakeDefaultManifest("1.0"sv), TestPackage::MetadataMap{}); @@ -1273,8 +1268,6 @@ TEST_CASE("CompositeSource_Pinning_OneSourceGated", "[CompositeSource][PinFlow]" TestHook::SetPinningIndex_Override pinningIndexOverride(indexFile.GetPath()); TestUserSettings userSettings; - userSettings.Set(true); - CompositeTestSetup setup; auto installedPackage = TestPackage::Make(MakeDefaultManifest("1.0"sv), TestPackage::MetadataMap{}); @@ -1338,8 +1331,7 @@ TEST_CASE("CompositeSource_Pinning_MultipleInstalled", "[CompositeSource][PinFlo TestHook::SetPinningIndex_Override pinningIndexOverride(indexFile.GetPath()); TestUserSettings userSettings; - userSettings.Set(true); - + std::string packageId = "packageId"; std::string productCode1 = "product-code1"; std::string productCode2 = "product-code2"; diff --git a/src/AppInstallerCommonCore/ExperimentalFeature.cpp b/src/AppInstallerCommonCore/ExperimentalFeature.cpp index eec1520223..259c0d0114 100644 --- a/src/AppInstallerCommonCore/ExperimentalFeature.cpp +++ b/src/AppInstallerCommonCore/ExperimentalFeature.cpp @@ -42,8 +42,6 @@ namespace AppInstaller::Settings return userSettings.Get(); case ExperimentalFeature::Feature::DirectMSI: return userSettings.Get(); - case ExperimentalFeature::Feature::Pinning: - return userSettings.Get(); case ExperimentalFeature::Feature::Configuration: return userSettings.Get(); case ExperimentalFeature::Feature::WindowsFeature: @@ -79,8 +77,6 @@ namespace AppInstaller::Settings return ExperimentalFeature{ "Show Dependencies Information", "dependencies", "https://aka.ms/winget-settings", Feature::Dependencies }; case Feature::DirectMSI: return ExperimentalFeature{ "Direct MSI Installation", "directMSI", "https://aka.ms/winget-settings", Feature::DirectMSI }; - case Feature::Pinning: - return ExperimentalFeature{ "Package Pinning", "pinning", "https://aka.ms/winget-settings", Feature::Pinning}; case Feature::Configuration: return ExperimentalFeature{ "Configuration", "configuration", "https://aka.ms/winget-settings#configuration", Feature::Configuration }; case Feature::WindowsFeature: diff --git a/src/AppInstallerCommonCore/Public/winget/ExperimentalFeature.h b/src/AppInstallerCommonCore/Public/winget/ExperimentalFeature.h index 40fbacd7b4..4f551984ae 100644 --- a/src/AppInstallerCommonCore/Public/winget/ExperimentalFeature.h +++ b/src/AppInstallerCommonCore/Public/winget/ExperimentalFeature.h @@ -24,9 +24,8 @@ namespace AppInstaller::Settings Dependencies = 0x1, // Before making DirectMSI non-experimental, it should be part of manifest validation. DirectMSI = 0x2, - Pinning = 0x4, - Configuration = 0x8, - WindowsFeature = 0x10, + Configuration = 0x4, + WindowsFeature = 0x8, Max, // This MUST always be after all experimental features // Features listed after Max will not be shown with the features command diff --git a/src/AppInstallerCommonCore/Public/winget/UserSettings.h b/src/AppInstallerCommonCore/Public/winget/UserSettings.h index 4fe7ad7490..b4acf742cf 100644 --- a/src/AppInstallerCommonCore/Public/winget/UserSettings.h +++ b/src/AppInstallerCommonCore/Public/winget/UserSettings.h @@ -71,7 +71,6 @@ namespace AppInstaller::Settings EFExperimentalArg, EFDependencies, EFDirectMSI, - EFPinning, EFConfiguration, EFWindowsFeature, // Telemetry @@ -143,7 +142,6 @@ namespace AppInstaller::Settings SETTINGMAPPING_SPECIALIZATION(Setting::EFExperimentalArg, bool, bool, false, ".experimentalFeatures.experimentalArg"sv); SETTINGMAPPING_SPECIALIZATION(Setting::EFDependencies, bool, bool, false, ".experimentalFeatures.dependencies"sv); SETTINGMAPPING_SPECIALIZATION(Setting::EFDirectMSI, bool, bool, false, ".experimentalFeatures.directMSI"sv); - SETTINGMAPPING_SPECIALIZATION(Setting::EFPinning, bool, bool, false, ".experimentalFeatures.pinning"sv); SETTINGMAPPING_SPECIALIZATION(Setting::EFConfiguration, bool, bool, false, ".experimentalFeatures.configuration"sv); SETTINGMAPPING_SPECIALIZATION(Setting::EFWindowsFeature, bool, bool, false, ".experimentalFeatures.windowsFeature"sv); // Telemetry diff --git a/src/AppInstallerCommonCore/UserSettings.cpp b/src/AppInstallerCommonCore/UserSettings.cpp index dccb783f35..1d08b5bd4c 100644 --- a/src/AppInstallerCommonCore/UserSettings.cpp +++ b/src/AppInstallerCommonCore/UserSettings.cpp @@ -260,7 +260,6 @@ namespace AppInstaller::Settings WINGET_VALIDATE_PASS_THROUGH(EFExperimentalArg) WINGET_VALIDATE_PASS_THROUGH(EFDependencies) WINGET_VALIDATE_PASS_THROUGH(EFDirectMSI) - WINGET_VALIDATE_PASS_THROUGH(EFPinning) WINGET_VALIDATE_PASS_THROUGH(EFConfiguration) WINGET_VALIDATE_PASS_THROUGH(EFWindowsFeature) WINGET_VALIDATE_PASS_THROUGH(AnonymizePathForDisplay) diff --git a/src/AppInstallerRepositoryCore/CompositeSource.cpp b/src/AppInstallerRepositoryCore/CompositeSource.cpp index 23bcad6439..07a894a8db 100644 --- a/src/AppInstallerRepositoryCore/CompositeSource.cpp +++ b/src/AppInstallerRepositoryCore/CompositeSource.cpp @@ -466,7 +466,7 @@ namespace AppInstaller::Repository std::vector GetAvailableVersionKeys(PinBehavior pinBehavior) const override { auto result = m_package->GetAvailableVersionKeys(); - if (ExperimentalFeature::IsEnabled(ExperimentalFeature::Feature::Pinning) && m_pin.has_value()) + if (m_pin.has_value()) { // Add pin information to all version keys for (auto& pvk : result) @@ -499,7 +499,7 @@ namespace AppInstaller::Repository { Pinning::PinType pinType = Pinning::PinType::Unknown; - if (ExperimentalFeature::IsEnabled(ExperimentalFeature::Feature::Pinning) && m_pin.has_value()) + if (m_pin.has_value()) { pinType = GetPinnedStateForVersion(versionKey, m_pin.value(), PinBehavior::ConsiderPins); } @@ -770,6 +770,12 @@ namespace AppInstaller::Repository { for (auto& availablePackage : m_availablePackages) { + // Safeguard in case a package with no available sneaks in as we intentionally do in tests + if (availablePackage.GetPackage()->GetAvailableVersionKeys().empty()) + { + continue; + } + Pinning::PinKey pinKey = GetPinKeyForAvailable(availablePackage.GetPackage().get()); auto pin = pinningIndex.GetPin(pinKey); @@ -813,9 +819,7 @@ namespace AppInstaller::Repository std::optional GetInstalledPin() const { - return (ExperimentalFeature::IsEnabled(ExperimentalFeature::Feature::Pinning) && m_installedPackage) - ? m_installedPackage->GetPin() - : std::nullopt; + return m_installedPackage ? m_installedPackage->GetPin() : std::nullopt; } std::optional m_installedPackage; @@ -1208,7 +1212,7 @@ namespace AppInstaller::Repository // Adds all the pin information to the results from a search to a CompositeSource. void AddPinInfoToCompositeSearchResult(CompositeResult& result) { - if (ExperimentalFeature::IsEnabled(ExperimentalFeature::Feature::Pinning) && !result.Matches.empty()) + if (!result.Matches.empty()) { // Look up any pins for the packages found auto pinningIndex = PinningIndex::OpenOrCreateDefault(); @@ -1391,13 +1395,6 @@ namespace AppInstaller::Repository // Search sources and add to result for (const auto& source : m_availableSources) { - if (addedAvailablePackage && !ExperimentalFeature::IsEnabled(ExperimentalFeature::Feature::Pinning)) - { - // Having multiple available packages is a new behavior introduced for package pinning, - // so we gate it with the same feature in case it causes problems. - break; - } - // Do not attempt to correlate local packages against this source if (!source.GetDetails().SupportInstalledSearchCorrelation) {