Make SxS stable (#4563)

## Change
Makes the side by side work stable and fixes a few tests that failed
after. These tests simply needed to be updated; no product code was
changed to resolve their issues.

.github/actions/spelling/patterns.txt

@@ -1,6 +1,7 @@
 # See https://github.com/check-spelling/check-spelling/wiki/Configuration-Examples:-patterns
 
 WinGetDevCLI_8wekyb3d8bbwe
+8wekyb3d8bbwe
 _tisf_sqliteReturnValue
 microsoft\.com/[^])">]*
 S[Hh][Aa]256: [0-9A-Fa-f]{64}

doc/Settings.md

@@ -312,17 +312,6 @@ You can enable the feature as shown below.
    },
 ```
 
-### sideBySide
-
-This feature enables experimental improvements for supporting multiple instances of a package being installed on a system.
-You can enable the feature as shown below.
-
-```json
-   "experimentalFeatures": {
-       "sideBySide": true
-   },
-```
-
 ### configureSelfElevate
 
 This feature enables configure commands to request elevation as needed.

schemas/JSON/settings/settings.schema.0.2.json

@@ -271,11 +271,6 @@
           "type": "boolean",
           "default": false
         },
-        "sideBySide": {
-          "description": "Enable support for improved side-by-side handling",
-          "type": "boolean",
-          "default": false
-        },
         "configureSelfElevate": {
           "description": "Enable configure commands request elevation as needed",
           "type": "boolean",

src/AppInstallerCLICore/Argument.cpp

@@ -415,7 +415,7 @@ namespace AppInstaller::CLI
         case Args::Type::IgnoreResumeLimit:
             return Argument{ type, Resource::String::IgnoreResumeLimitArgumentDescription, ArgumentType::Flag, ExperimentalFeature::Feature::Resume };
         case Args::Type::AllVersions:
-            return Argument{ type, Resource::String::UninstallAllVersionsArgumentDescription, ArgumentType::Flag, Argument::Visibility::Help, ExperimentalFeature::Feature::SideBySide };
+            return Argument{ type, Resource::String::UninstallAllVersionsArgumentDescription, ArgumentType::Flag, Argument::Visibility::Help };
         case Args::Type::TargetVersion:
             return Argument{ type, Resource::String::TargetVersionArgumentDescription, ArgumentType::Standard };
         case Args::Type::Proxy:

src/AppInstallerCLICore/Workflows/RepairFlow.cpp

@@ -398,6 +398,7 @@ namespace AppInstaller::CLI::Workflow
             context <<
                 SetPackageFamilyNamesInContext;
         }
+        break;
         case InstallerTypeEnum::MSStore:
             break;
         case InstallerTypeEnum::Portable:

src/AppInstallerCLICore/Workflows/WorkflowBase.cpp

@@ -1383,41 +1383,34 @@ namespace AppInstaller::CLI::Workflow
 
     void GetInstalledPackageVersion(Execution::Context& context)
     {
-        if (ExperimentalFeature::IsEnabled(ExperimentalFeature::Feature::SideBySide))
-        {
-            std::shared_ptr<IPackage> installed = context.Get<Execution::Data::Package>()->GetInstalled();
+        std::shared_ptr<IPackage> installed = context.Get<Execution::Data::Package>()->GetInstalled();
 
-            if (installed)
+        if (installed)
+        {
+            // TODO: This may need to be expanded dramatically to enable targeting across a variety of dimensions (architecture, etc.)
+            //       Alternatively, if we make it easier to see the fully unique package identifiers, we may avoid that need.
+            if (context.Args.Contains(Execution::Args::Type::TargetVersion))
             {
-                // TODO: This may need to be expanded dramatically to enable targeting across a variety of dimensions (architecture, etc.)
-                //       Alternatively, if we make it easier to see the fully unique package identifiers, we may avoid that need.
-                if (context.Args.Contains(Execution::Args::Type::TargetVersion))
-                {
-                    Repository::PackageVersionKey versionKey{ "", context.Args.GetArg(Execution::Args::Type::TargetVersion) , "" };
-                    std::shared_ptr<IPackageVersion> installedVersion = installed->GetVersion(versionKey);
+                Repository::PackageVersionKey versionKey{ "", context.Args.GetArg(Execution::Args::Type::TargetVersion) , "" };
+                std::shared_ptr<IPackageVersion> installedVersion = installed->GetVersion(versionKey);
 
-                    if (!installedVersion)
-                    {
-                        context.Reporter.Error() << Resource::String::GetManifestResultVersionNotFound(Utility::LocIndView{ versionKey.Version }) << std::endl;
-                        // This error maintains consistency with passing an available version to commands
-                        AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_NO_MANIFEST_FOUND);
-                    }
-
-                    context.Add<Execution::Data::InstalledPackageVersion>(std::move(installedVersion));
-                }
-                else
+                if (!installedVersion)
                 {
-                    context.Add<Execution::Data::InstalledPackageVersion>(installed->GetLatestVersion());
+                    context.Reporter.Error() << Resource::String::GetManifestResultVersionNotFound(Utility::LocIndView{ versionKey.Version }) << std::endl;
+                    // This error maintains consistency with passing an available version to commands
+                    AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_NO_MANIFEST_FOUND);
                 }
+
+                context.Add<Execution::Data::InstalledPackageVersion>(std::move(installedVersion));
             }
             else
             {
-                context.Add<Execution::Data::InstalledPackageVersion>(nullptr);
+                context.Add<Execution::Data::InstalledPackageVersion>(installed->GetLatestVersion());
             }
         }
         else
         {
-            context.Add<Execution::Data::InstalledPackageVersion>(GetInstalledVersion(context.Get<Execution::Data::Package>()));
+            context.Add<Execution::Data::InstalledPackageVersion>(nullptr);
         }
     }
 

src/AppInstallerCLIE2ETests/Constants.cs

@@ -50,6 +50,7 @@ public class Constants
 
         public const string AICLIPackageFamilyName = "WinGetDevCLI_8wekyb3d8bbwe";
         public const string AICLIPackageName = "WinGetDevCLI";
+        public const string AICLIPackagePublisherHash = "8wekyb3d8bbwe";
         public const string AICLIAppId = "WinGetDev";
 
         public const string TestPackage = "TëstPackage.msix";

src/AppInstallerCLIE2ETests/ListCommand.cs

@@ -23,7 +23,8 @@ public void ListSelf()
         {
             var result = TestCommon.RunAICLICommand("list", Constants.AICLIPackageFamilyName);
             Assert.AreEqual(Constants.ErrorCode.S_OK, result.ExitCode);
-            Assert.True(result.StdOut.Contains(Constants.AICLIPackageFamilyName));
+            Assert.True(result.StdOut.Contains(Constants.AICLIPackageName));
+            Assert.True(result.StdOut.Contains(Constants.AICLIPackagePublisherHash));
         }
 
         /// <summary>

src/AppInstallerCLITests/CompositeSource.cpp

@@ -423,17 +423,18 @@ TEST_CASE("CompositeSource_OnlyAvailableFoundByRootSearch", "[CompositeSource]")
         RequireSearchRequestIncludes(request.Inclusions, PackageMatchField::PackageFamilyName, MatchType::Exact, pfn);
 
         SearchResult result;
-        result.Matches.emplace_back(setup.MakeInstalled().WithPFN(pfn), Criteria());
+        result.Matches.emplace_back(setup.MakeInstalled().WithPFN(pfn), Criteria(PackageMatchField::PackageFamilyName));
         return result;
     };
 
-    setup.Available->Everything.Matches.emplace_back(setup.MakeAvailable().WithPFN(pfn), Criteria());
+    std::shared_ptr<TestCompositePackage> availablePackage = setup.MakeAvailable().WithPFN(pfn).ToPackage();
+    setup.Available->Everything.Matches.emplace_back(availablePackage, Criteria());
     setup.Available->SearchFunction = [&](const SearchRequest& request)
     {
         RequireSearchRequestIncludes(request.Inclusions, PackageMatchField::PackageFamilyName, MatchType::Exact, pfn);
 
         SearchResult result;
-        result.Matches.emplace_back(setup.MakeAvailable().WithPFN(pfn), Criteria());
+        result.Matches.emplace_back(availablePackage, Criteria(PackageMatchField::PackageFamilyName));
         return result;
     };
 
@@ -717,7 +718,7 @@ TEST_CASE("CompositeSource_MultipleAvailableSources_ReverseMatchBoth", "[Composi
         RequireSearchRequestIncludes(request.Inclusions, PackageMatchField::PackageFamilyName, MatchType::Exact, pfn);
 
         SearchResult result;
-        result.Matches.emplace_back(installedPackage, Criteria());
+        result.Matches.emplace_back(installedPackage, Criteria(PackageMatchField::PackageFamilyName));
         return result;
     };
 
@@ -728,8 +729,9 @@ TEST_CASE("CompositeSource_MultipleAvailableSources_ReverseMatchBoth", "[Composi
 
     REQUIRE(result.Matches.size() == 1);
     REQUIRE(GetInstalledVersion(result.Matches[0].Package));
-    REQUIRE(result.Matches[0].Package->GetAvailable().size() == 1);
+    REQUIRE(result.Matches[0].Package->GetAvailable().size() == 2);
     REQUIRE(result.Matches[0].Package->GetAvailable()[0]->GetVersionKeys().size() == 1);
+    REQUIRE(result.Matches[0].Package->GetAvailable()[1]->GetVersionKeys().size() == 1);
 }
 
 TEST_CASE("CompositeSource_IsSame", "[CompositeSource]")
@@ -1031,7 +1033,7 @@ TEST_CASE("CompositeSource_TrackingFound_AvailablePath", "[CompositeSource]")
         RequireSearchRequestIncludes(request.Inclusions, PackageMatchField::PackageFamilyName, MatchType::Exact, pfn);
 
         SearchResult result;
-        result.Matches.emplace_back(installedPackage, Criteria());
+        result.Matches.emplace_back(installedPackage, Criteria(PackageMatchField::PackageFamilyName));
         return result;
     };
 
@@ -1120,17 +1122,22 @@ struct ExpectedResultsForPinning
     std::vector<ExpectedPackageVersionKey> AvailableVersions;
 };
 
-void RequireExpectedResultsWithPin(std::shared_ptr<ICompositePackage> package, const ExpectedResultsForPinning& expectedResult)
+void RequireExpectedResultsWithPin(std::shared_ptr<ICompositePackage> package, const ExpectedResultsForPinning& expectedResult, std::shared_ptr<IPackageVersion> packageVersion = {})
 {
     PinningData pinningData{ PinningData::Disposition::ReadOnly };
     auto availableVersions = GetAvailableVersionsForInstalledVersion(package);
 
+    if (!packageVersion)
+    {
+        packageVersion = GetInstalledVersion(package);
+    }
+
     for (const auto& entry : expectedResult.ResultsForPinBehavior)
     {
         auto pinBehavior = entry.first;
         const auto& result = entry.second;
 
-        auto evaluator = pinningData.CreatePinStateEvaluator(pinBehavior, GetInstalledVersion(package));
+        auto evaluator = pinningData.CreatePinStateEvaluator(pinBehavior, packageVersion);
         auto latestAvailable = evaluator.GetLatestAvailableVersionForPins(availableVersions);
 
         REQUIRE(evaluator.IsUpdate(latestAvailable) == result.IsUpdateAvailable);
@@ -1150,13 +1157,13 @@ void RequireExpectedResultsWithPin(std::shared_ptr<ICompositePackage> package, c
     REQUIRE(availableVersionKeys.size() == expectedResult.AvailableVersions.size());
     for (size_t i = 0; i < availableVersionKeys.size(); ++i)
     {
-        auto evaluator = pinningData.CreatePinStateEvaluator(PinBehavior::ConsiderPins, GetInstalledVersion(package));
+        auto evaluator = pinningData.CreatePinStateEvaluator(PinBehavior::ConsiderPins, packageVersion);
 
-        auto packageVersion = availableVersions->GetVersion(expectedResult.AvailableVersions[i]);
-        REQUIRE(packageVersion);
+        auto availableVersion = availableVersions->GetVersion(expectedResult.AvailableVersions[i]);
+        REQUIRE(availableVersion);
         REQUIRE(availableVersionKeys[i].SourceId == expectedResult.AvailableVersions[i].SourceId);
         REQUIRE(availableVersionKeys[i].Version == expectedResult.AvailableVersions[i].Version);
-        REQUIRE(evaluator.EvaluatePinType(packageVersion) == expectedResult.AvailableVersions[i].PinnedState);
+        REQUIRE(evaluator.EvaluatePinType(availableVersion) == expectedResult.AvailableVersions[i].PinnedState);
     }
 }
 
@@ -1418,12 +1425,12 @@ TEST_CASE("CompositeSource_Pinning_MultipleInstalled", "[CompositeSource][PinFlo
         SearchResult result;
         if (isSearchById || SearchRequestIncludes(request.Inclusions, PackageMatchField::ProductCode, MatchType::Exact, productCode1))
         {
-            result.Matches.emplace_back(installedPackage1, Criteria());
+            result.Matches.emplace_back(installedPackage1, Criteria(request.Inclusions[0].Field));
         }
 
         if (isSearchById || SearchRequestIncludes(request.Inclusions, PackageMatchField::ProductCode, MatchType::Exact, productCode2))
         {
-            result.Matches.emplace_back(installedPackage2, Criteria());
+            result.Matches.emplace_back(installedPackage2, Criteria(request.Inclusions[0].Field));
         }
 
         return result;
@@ -1497,18 +1504,22 @@ TEST_CASE("CompositeSource_Pinning_MultipleInstalled", "[CompositeSource][PinFlo
     searchRequest.Inclusions.emplace_back(PackageMatchField::Id, MatchType::Exact, packageId);
     SearchResult result = setup.Composite.Search(searchRequest);
 
-    REQUIRE(result.Matches.size() == 2);
+    REQUIRE(result.Matches.size() == 1);
+    auto installedPackage = result.Matches[0].Package->GetInstalled();
+    REQUIRE(installedPackage);
+    auto installedVersions = installedPackage->GetVersionKeys();
+    REQUIRE(installedVersions.size() == 2);
 
     // Here we assume that the order we return the packages in the installed source
     // search is preserved. We'll need to change it if that stops being the case.
-    auto package1 = result.Matches[0].Package;
-    REQUIRE(package1);
+    auto packageVersion1 = installedPackage->GetVersion(installedVersions[1]);
+    REQUIRE(packageVersion1);
 
-    auto package2 = result.Matches[1].Package;
-    REQUIRE(package2);
+    auto packageVersion2 = installedPackage->GetVersion(installedVersions[0]);
+    REQUIRE(packageVersion2);
 
-    RequireExpectedResultsWithPin(package1, expectedResult1);
-    RequireExpectedResultsWithPin(package2, expectedResult2);
+    RequireExpectedResultsWithPin(result.Matches[0].Package, expectedResult1, packageVersion1);
+    RequireExpectedResultsWithPin(result.Matches[0].Package, expectedResult2, packageVersion2);
 }
 
 TEST_CASE("CompositeSource_CorrelateToInstalledContainsManifestData", "[CompositeSource]")
@@ -1597,8 +1608,6 @@ TEST_CASE("CompositeSource_Respects_FeatureFlag_ManifestMayContainAdditionalSyst
 
 TEST_CASE("CompositeSource_SxS_TwoVersions_NoAvailable", "[CompositeSource][SideBySide]")
 {
-    auto enableFeature = TestUserSettings::EnableExperimentalFeature(Settings::ExperimentalFeature::Feature::SideBySide);
-
     std::string productCode1 = "PC1";
     std::string productCode2 = "PC2";
 
@@ -1615,8 +1624,6 @@ TEST_CASE("CompositeSource_SxS_TwoVersions_NoAvailable", "[CompositeSource][Side
 
 TEST_CASE("CompositeSource_SxS_TwoVersions_DifferentAvailable", "[CompositeSource][SideBySide]")
 {
-    auto enableFeature = TestUserSettings::EnableExperimentalFeature(Settings::ExperimentalFeature::Feature::SideBySide);
-
     std::string productCode1 = "PC1";
     std::string productCode2 = "PC2";
 
@@ -1660,8 +1667,6 @@ TEST_CASE("CompositeSource_SxS_TwoVersions_DifferentAvailable", "[CompositeSourc
 
 TEST_CASE("CompositeSource_SxS_TwoVersions_SameAvailable", "[CompositeSource][SideBySide]")
 {
-    auto enableFeature = TestUserSettings::EnableExperimentalFeature(Settings::ExperimentalFeature::Feature::SideBySide);
-
     std::string version1 = "1.0";
     std::string version2 = "2.0";
     std::string productCode1 = "PC1";
@@ -1698,8 +1703,6 @@ TEST_CASE("CompositeSource_SxS_TwoVersions_SameAvailable", "[CompositeSource][Si
 
 TEST_CASE("CompositeSource_SxS_ThreeVersions_SameAvailable", "[CompositeSource][SideBySide]")
 {
-    auto enableFeature = TestUserSettings::EnableExperimentalFeature(Settings::ExperimentalFeature::Feature::SideBySide);
-
     std::string version1 = "1.0";
     std::string version2 = "2.0";
     std::string version3 = "3.0";
@@ -1740,8 +1743,6 @@ TEST_CASE("CompositeSource_SxS_ThreeVersions_SameAvailable", "[CompositeSource][
 
 TEST_CASE("CompositeSource_SxS_TwoVersions_SameAvailable_Tracking", "[CompositeSource][SideBySide]")
 {
-    auto enableFeature = TestUserSettings::EnableExperimentalFeature(Settings::ExperimentalFeature::Feature::SideBySide);
-
     std::string version1 = "1.0";
     std::string version2 = "2.0";
     std::string productCode1 = "PC1";
@@ -1780,8 +1781,6 @@ TEST_CASE("CompositeSource_SxS_TwoVersions_SameAvailable_Tracking", "[CompositeS
 
 TEST_CASE("CompositeSource_SxS_Available_TwoVersions_SameAvailable", "[CompositeSource][SideBySide]")
 {
-    auto enableFeature = TestUserSettings::EnableExperimentalFeature(Settings::ExperimentalFeature::Feature::SideBySide);
-
     std::string version1 = "1.0";
     std::string version2 = "2.0";
     std::string productCode1 = "PC1";

src/AppInstallerCLITests/TestCommon.cpp

@@ -264,9 +264,6 @@ namespace TestCommon
         // Due to the template usage, this needs to be updated for any features that want to use it.
         switch (feature)
         {
-        case Settings::ExperimentalFeature::Feature::SideBySide:
-            result->Set<Settings::Setting::EFSideBySide>(true);
-            break;
         case Settings::ExperimentalFeature::Feature::StoreDownload:
             result->Set<Settings::Setting::EFStoreDownload>(true);
             break;

src/AppInstallerCommonCore/ExperimentalFeature.cpp

@@ -44,8 +44,6 @@ namespace AppInstaller::Settings
                 return userSettings.Get<Setting::EFResume>();
             case ExperimentalFeature::Feature::Configuration03:
                 return userSettings.Get<Setting::EFConfiguration03>();
-            case ExperimentalFeature::Feature::SideBySide:
-                return userSettings.Get<Setting::EFSideBySide>();
             case ExperimentalFeature::Feature::ConfigureSelfElevation:
                 return userSettings.Get<Setting::EFConfigureSelfElevation>();
             case ExperimentalFeature::Feature::StoreDownload:
@@ -87,8 +85,6 @@ namespace AppInstaller::Settings
             return ExperimentalFeature{ "Resume", "resume", "https://aka.ms/winget-settings", Feature::Resume };
         case Feature::Configuration03:
             return ExperimentalFeature{ "Configuration Schema 0.3", "configuration03", "https://aka.ms/winget-settings", Feature::Configuration03 };
-        case Feature::SideBySide:
-            return ExperimentalFeature{ "Side-by-side improvements", "sideBySide", "https://aka.ms/winget-settings", Feature::SideBySide };
         case Feature::ConfigureSelfElevation:
             return ExperimentalFeature{ "Configure Self Elevation", "configureSelfElevate", "https://aka.ms/winget-settings", Feature::ConfigureSelfElevation };
         case Feature::StoreDownload:

src/AppInstallerCommonCore/Public/winget/ExperimentalFeature.h

@@ -25,11 +25,10 @@ namespace AppInstaller::Settings
             DirectMSI = 0x1,
             Resume = 0x2,
             Configuration03 = 0x4,
-            SideBySide = 0x8,
-            ConfigureSelfElevation = 0x10,
-            StoreDownload = 0x20,
-            ConfigureExport = 0x40,
-            IndexV2 = 0x80,
+            ConfigureSelfElevation = 0x8,
+            StoreDownload = 0x10,
+            ConfigureExport = 0x20,
+            IndexV2 = 0x40,
             Max, // This MUST always be after all experimental features
 
             // Features listed after Max will not be shown with the features command