Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make pinning a stable feature #3315

Merged
merged 2 commits into from
Jun 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 0 additions & 11 deletions doc/Settings.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
5 changes: 0 additions & 5 deletions schemas/JSON/settings/settings.schema.0.2.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
2 changes: 1 addition & 1 deletion src/AppInstallerCLICore/Commands/PinCommand.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
4 changes: 2 additions & 2 deletions src/AppInstallerCLICore/Workflows/WorkflowBase.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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;

Expand Down
9 changes: 0 additions & 9 deletions src/AppInstallerCLIE2ETests/Pinning.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
10 changes: 1 addition & 9 deletions src/AppInstallerCLITests/CompositeSource.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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{});
Expand Down Expand Up @@ -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{});
Expand Down Expand Up @@ -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{});
Expand Down Expand Up @@ -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";
Expand Down
4 changes: 0 additions & 4 deletions src/AppInstallerCommonCore/ExperimentalFeature.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 0 additions & 2 deletions src/AppInstallerCommonCore/Public/winget/UserSettings.h
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,6 @@ namespace AppInstaller::Settings
EFExperimentalArg,
EFDependencies,
EFDirectMSI,
EFPinning,
EFConfiguration,
EFWindowsFeature,
// Telemetry
Expand Down Expand Up @@ -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
Expand Down
1 change: 0 additions & 1 deletion src/AppInstallerCommonCore/UserSettings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
23 changes: 10 additions & 13 deletions src/AppInstallerRepositoryCore/CompositeSource.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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)
{
Expand Down