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.

Sign up for GitHub

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

Jump to bottom

Allow upgrades in packages that register a different installer type #1796

Merged
merged 3 commits into from
Jan 20, 2022
Merged

Allow upgrades in packages that register a different installer type #1796

merged 3 commits into from
Jan 20, 2022

Conversation

florelis
Copy link
Member

@florelis florelis commented Dec 14, 2021 •
edited by ghost
Loading

  • Modified the applicability check in InstalledTypeComparator to also consider the installer types listed under AppsAndFeaturesEntries
  • Extended string returned from ExplainInapplicable to also mention the installed type and other types accepted by the manifest.

Closes #1242. Validated that upgrading PowerToys (mentioned in that issue) worked correctly using a local manifest edited to include the AppsAndFeaturesEntries and installed type msi.

Microsoft Reviewers: Open in CodeFlow

@florelis florelis requested a review from a team as a code owner December 14, 2021 01:42
@ghost ghost added Area-Manifest This may require a change to the manifest Issue-Feature This is a feature request for the Windows Package Manager client. labels Dec 14, 2021
@jedieaston
Copy link
Contributor

Another test case for this is iTunes, it packages a msi inside a exe.

If you add this ARP entry:

  AppsAndFeaturesEntries:
    - DisplayName: iTunes
      Publisher: Apple Inc.
      DisplayVersion: 12.12.1.1
      InstallerType: msi

to the manifest, it works great!

PS C:\Users\easton\Downloads> winget upgrade -m .\12.12.1.1\
No applicable installer found; see logs for more details.
PS C:\Users\easton\Downloads> wingetdev upgrade -m .\12.12.1.1\
Found iTunes [Apple.iTunes] Version 12.12.1.1
This application is licensed to you by its owner.
Microsoft is not responsible for, nor does it grant any licenses to, third-party packages.
Successfully verified installer hash
Starting package install...
Successfully installed
PS C:\Users\easton\Downloads>

@denelon
Copy link
Contributor

denelon commented Dec 14, 2021

This addresses at least one version of Adobe Acrobat, and PowerToys (@crutkas).

florelis
florelis commented Dec 14, 2021
View reviewed changes
src/AppInstallerCLITests/WorkFlow.cpp
@@ -210,6 +210,21 @@ namespace
PackageMatchFilter(PackageMatchField::Id, MatchType::Exact, "AppInstallerCliTest.TestExeInstaller")));
}

if (input == "TestExeInstallerWithDifferentInstalledType")
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not a fan of how long this function is getting. Many tests seem to use only a single case of all this and most are used in a single test, so we probably could add a constructor that took the manifests and always returned that. It would also put the search results closer to where it's used. Putting this here to see what others think of doing that..

Copy link
Member

Choose a reason for hiding this comment

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

Agree, making it easy to create specific new cases rather than extending this would be good.

@denelon denelon mentioned this pull request Dec 15, 2021
Closed
@florelis florelis marked this pull request as draft January 5, 2022 19:05
@Trenly Trenly mentioned this pull request Jan 7, 2022
Closed
JohnMcPMS
JohnMcPMS approved these changes Jan 19, 2022
View reviewed changes
src/AppInstallerCLITests/WorkFlow.cpp
@@ -210,6 +210,21 @@ namespace
PackageMatchFilter(PackageMatchField::Id, MatchType::Exact, "AppInstallerCliTest.TestExeInstaller")));
}

if (input == "TestExeInstallerWithDifferentInstalledType")
Copy link
Member

Choose a reason for hiding this comment

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

Agree, making it easy to create specific new cases rather than extending this would be good.

@florelis florelis marked this pull request as ready for review January 20, 2022 02:32
@florelis florelis merged commit 9dcbf99 into microsoft:master Jan 20, 2022
@jedieaston jedieaston mentioned this pull request Jan 20, 2022
Closed
@OfficialEsco OfficialEsco mentioned this pull request Feb 27, 2022
Closed
2 tasks
@florelis florelis deleted the complexInstallers branch May 28, 2022 00:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JohnMcPMS JohnMcPMS JohnMcPMS approved these changes

Assignees
No one assigned
Labels
Area-Manifest This may require a change to the manifest Issue-Feature This is a feature request for the Windows Package Manager client.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Complex installers
4 participants
@florelis @jedieaston @denelon @JohnMcPMS