Make pinning a stable feature (#3315)

Co-authored-by: Ryan Fu <[email protected]>
microsoft · Jun 6, 2023 · 41e70ac · 41e70ac
1 parent 8282645
commit 41e70ac
Show file tree

Hide file tree

Showing 11 changed files with 16 additions and 60 deletions.
diff --git 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
@@ -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
@@ -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<std::unique_ptr<Command>> GetCommands() const override;
 

diff --git 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<IPackage> package = context.Get<Execution::Data::Package>();
         std::shared_ptr<IPackageVersion> 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
@@ -15,15 +15,6 @@ namespace AppInstallerCLIE2ETests
     /// </summary>
     public class Pinning : BaseCommand
     {
-        /// <summary>
-        /// Setup done once before all the tests here.
-        /// </summary>
-        [OneTimeSetUp]
-        public void OneTimeSetup()
-        {
-            WinGetSettingsHelper.ConfigureFeature("pinning", true);
-        }
-
         /// <summary>
         /// Set up for all tests.
         /// </summary>

diff --git 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<Settings::Setting::EFPinning>(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<Settings::Setting::EFPinning>(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<Settings::Setting::EFPinning>(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<Settings::Setting::EFPinning>(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<Settings::Setting::EFPinning>(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
@@ -42,8 +42,6 @@ namespace AppInstaller::Settings
                 return userSettings.Get<Setting::EFDependencies>();
             case ExperimentalFeature::Feature::DirectMSI:
                 return userSettings.Get<Setting::EFDirectMSI>();
-            case ExperimentalFeature::Feature::Pinning:
-                return userSettings.Get<Setting::EFPinning>();
             case ExperimentalFeature::Feature::Configuration:
                 return userSettings.Get<Setting::EFConfiguration>();
             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
@@ -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
@@ -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
@@ -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
@@ -466,7 +466,7 @@ namespace AppInstaller::Repository
             std::vector<PackageVersionKey> 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<Pinning::Pin> 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<PinnablePackage> 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)
                         {