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

Add experimental support for multiple installed versions #4282

Merged
merged 29 commits into from
Mar 22, 2024

Conversation

JohnMcPMS
Copy link
Member

@JohnMcPMS JohnMcPMS commented Mar 18, 2024

Movement toward north star #2129

Change

This change introduces a new experimental feature flag called sideBySide that enables multiple instances of an installed package to be found by search results. The CompositeSource will fold these all together into a single package result through their common association with available packages.

Warning

This experimental feature affects the search results that the rest of the Windows Package Manager code relies on. Enabling it may change the behavior of downstream components.

Important

Only the commands in winget.exe have been updated to account for this change. Other interfaces will get reasonable defaults, but may not be able to interact properly in all cases.

Installed identifier change

The most visible effect of this feature being enabled is that the identifier generated for installed items is no longer simply the registry key name or package family name but is instead constructed to make it fully unique. This is mostly relevant to packages without an available counterpart. For packages that are registered with the system through the registry, the identifier is now of the form ARP\User\x64\ProductCode. For MSIX, the form is MSIX\Package_Full_Name. The previous identifiers are still searchable data though, so it is hopefully not more difficult to list installed items.

Installed item correlations

The implementation in CompositeSource tries to separate the steps into:

  1. enumerating the basic results with each having at most one installed item and its correlated available packages.
  2. folding the results together to create the final return value.

In an effort to prevent overly broad correlations from available to installed items, there is an additional attempt to ensure that there are no stronger correlations from installed to available. As an example, if a package is split by version while still sharing a name that is reasonably close, searching for one of the version subsets should not attempt to associate all of the installed items so long as the other versions have stronger correlations.

  1. Package A1 has versions 1.0 - 1.5 with names like A Package (1.0)
  2. Package A2 has versions 2.0 - 2.2 with names like A Package (2.0)
  3. Searching for A2 would correlate with version 1.0 because the normalized names are the same
  4. With the above check, it should not return installed version 1.0 as long as the product code is associated with A1

Once the basic results are discovered, the results are folded together. A comment on that function indicates the methodology and possible future improvements:

// Group results in an attempt to have a single result that covers all installed versions.
// This is expected to be called immediately after the installed search portion,
// when each result will contain a single installed version and some number of available packages.
// 
// The folds that happen are:
//  1. When results have the same primary available package (the primary available package is set due to tracking data)
//  2. When a result has no primary available package, but another result does have a primary that matches one of the available
//      a. Choose the latest primary if there are multiple
//  3. When multiple results have no primary available package and share the same available package set
//      a. There are many potential additional rules that could be made here, but we will start with the simplest version.
//
// Potential improvements:
//  1. Attempting correlation of non-primary available packages to allow folding in more complex cases
//      a. For example, if installed A has {source1:package1, source2:package2} and installed B has {source1:package1}, can we
//          make sure that source1:package1 and source2:package2 are in fact "the same" to confidently say that installed A and B
//          are side by side versions.
//  2. Attempt correlation by installed data only
//      a. We can potentially detect multiple instances of the same installed item with the same correlation logic turned back on
//          the installed source.  This would allow for folding even when the package is not in any available source.

Command changes

list

list has been updated to simply output all versions contained within a single search result as independent lines. This keeps the resulting output basically the same, but that has the downside of not explicitly indicating the correlation between the items.

repair

repair was updated internally so that passing in a --version would cause that specific version to be targeted for repair.

uninstall

uninstall should behave as before if only a single installed version is present. When multiple installed versions exist, an error is produced unless one of two new arguments is supplied:

  1. --all-versions / --all will uninstall all installed versions
  2. --version / -v will uninstall the targeted version only

Discussion

I have created a discussion for this experimental feature: #4281

The goal is to allow feedback to be better collected and threaded, although I expect that there will be issues created independently as well.

Additionally...

I added a script in the tools that launches the sandbox with the dev package registered: Launch-DevPackageInSandbox.ps1

Since it is dev registered in the sandbox, all you need to do to update it is build and deploy from Visual Studio and the sandbox version will just use those latest files.

Validation

Manual validation of the target scenario (multiple installed versions).
Unit tests added for core folding behavior.
More tests should be added before / as part of moving out of experimental.

Microsoft Reviewers: Open in CodeFlow

@JohnMcPMS JohnMcPMS requested a review from a team as a code owner March 18, 2024 18:03
@Trenly
Copy link
Contributor

Trenly commented Mar 18, 2024

uninstall

uninstall should behave as before if only a single installed version is present. When multiple installed versions exist, an error is produced unless one of two new arguments is supplied:

  1. --all-versions / --all will uninstall all installed versions
  2. --version / -v will uninstall the targeted version only

Will this also resolve this issue once out of experimental? I only ask because I don't see anywhere in the uninstall flow where --version is being checked, unless I'm missing it in one of the other function calls

Comment on lines +1379 to +1392
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);

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));
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Trenly , selection of target version.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I must have missed that

@JohnMcPMS
Copy link
Member Author

uninstall

uninstall should behave as before if only a single installed version is present. When multiple installed versions exist, an error is produced unless one of two new arguments is supplied:

  1. --all-versions / --all will uninstall all installed versions
  2. --version / -v will uninstall the targeted version only

Will this also resolve this issue once out of experimental? I only ask because I don't see anywhere in the uninstall flow where --version is being checked, unless I'm missing it in one of the other function calls

Yes, I believe it would resolve that issue.

yao-msft
yao-msft previously approved these changes Mar 22, 2024
@@ -180,6 +180,11 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "WinGetSourceCreator", "WinG
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.WinGet.SharedLib", "PowerShell\Microsoft.WinGet.SharedLib\Microsoft.WinGet.SharedLib.csproj", "{272B2B0E-40D4-4F0F-B187-519A6EF89B10}"
EndProject
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "schema", "schema", "{46E419FD-C77F-4EFC-A0FC-2BAD93E60D12}"
ProjectSection(SolutionItems) = preProject
..\schemas\JSON\settings\settings.schema.0.2.json = ..\schemas\JSON\settings\settings.schema.0.2.json
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: settings.schema.0.2.json is already listed under WingetSchemas in the solution unless we wanted to explicitly listed here

@@ -346,45 +343,74 @@ namespace AppInstaller::Repository
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the comment "Supports only a single version of a single package at this time." could be removed

continue;
}

// We use the `SourceId` field to store the installed package identifier so that we can disambiguate keys is they have the same version.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// We use the `SourceId` field to store the installed package identifier so that we can disambiguate keys is they have the same version.
// We use the `SourceId` field to store the installed package identifier so that we can disambiguate keys if they have the same version.

m_versionKeyData.emplace_back(std::move(keyData));
}

m_packages.emplace_back(std::move(package));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In rare cases that all the versions of the package get dropped, do we want to drop the package as well?


for (const auto& availablePackage : currentMatch.Package->GetAvailablePackages())
{
auto& packageFoldData = foldData.at(availablePackage);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the availablePackage converted to InstalledResultFoldKey automatically here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, there is a non-explicit constructor from a std::shared_ptr<IPackage>.

@JohnMcPMS JohnMcPMS merged commit 8e82bfe into microsoft:master Mar 22, 2024
8 checks passed
@JohnMcPMS JohnMcPMS deleted the sxs-test branch March 22, 2024 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants