search

microsoft / winget-cli

WinGet is the Windows Package Manager. This project includes a CLI (Command Line Interface), PowerShell modules, and a COM (Component Object Model) API (Application Programming Interface).
https://learn.microsoft.com/windows/package-manager/
MIT License
23.11k stars 1.44k forks source link

AppsAndFeaturesEntry not preventing upgrade #2366

Open Trenly opened 2 years ago

Trenly commented 2 years ago

Brief description of your issue

cc @denelon

The package Microsoft.NuGet was added as an example of a portable application. The AppsAndFeaturesEntries for the manifest are set such that the version shown on NuGet's website, 6.2.1, is the version written to the ARP table, and the full version, 6.2.1.2, is set to the Package Version. Because the version in AppsAndFeaturesEntries is an exact match for the ARP Table, and 6.2.1.2 is the latest version present, the application should not be detected as needing an upgrade. However, winget believes an upgrade is needed.

Steps to reproduce

Run winget install nuget -s winget and then winget upgrade

Expected behavior

Package which has a DisplayVersion matching the AppsAndFeaturesEntry of the latest version should not be upgraded

Actual behavior

NuGet is being detected for an upgrade image

WinGet-2022-07-23-17-13-23.454.log

Environment

Tested with winget v1.3.1872 and v1.4.2011-preview
denelon commented 2 years ago

https://github.com/microsoft/winget-pkgs/pull/67410/files changed version number temporarily to resolve issue.

JohnMcPMS commented 2 years ago

Since this is a portable and we fully control the version, why do we want to show 6.2.1 anyway?

It is never going to work to have either the PackageVersion or the DisplayVersion not change between versions of the package. Obviously the PackageVersion has to change to indicate a difference, but if the DisplayVersion is always listed as 6.2.1, then we would never know what is installed between 6.2.1.1, 6.2.1.2, 6.2.1.3, etc. The mapping feature was implemented to enable taking some range of DisplayVersions to another range of (really a single) PackageVersion. It will be an error during validation if the ranges of DisplayVersions overlap.

In the case of portables, we could use the fact that we own the installer stack to just know the answer, but that is orthogonal to the DisplayVersion feature. And it was my goal to enable disassociating the installed portable from the winget package, such that a full data reset or remove/re-add of the winget package would not stop us from continuing to act on the portable package.

Trenly commented 2 years ago

Since this is a portable and we fully control the version, why do we want to show 6.2.1 anyway?

I don’t disagree, but more on that later

It is never going to work to have either the PackageVersion or the DisplayVersion not change between versions of the package. Obviously the PackageVersion has to change to indicate a difference, but if the DisplayVersion is always listed as 6.2.1, then we would never know what is installed between 6.2.1.1, 6.2.1.2, 6.2.1.3, etc.

Correct; I’m still in 100% agreeance

The mapping feature was implemented to enable taking some range of DisplayVersions to another range of (really a single) PackageVersion. It will be an error during validation if the ranges of DisplayVersions overlap.

This is precisely the behavior I’m expecting. The issue here is that the DisplayVersion isn’t correlating it to the PackageVersion. If it were correlated properly, the package shouldn’t be showing for upgrade

In the case of portables, we could use the fact that we own the installer stack to just know the answer, but that is orthogonal to the DisplayVersion feature. And it was my goal to enable disassociating the installed portable from the winget package, such that a full data reset or remove/re-add of the winget package would not stop us from continuing to act on the portable package.

I think the key here is to not think of it as an issue applying solely to portable packages. Based on my understanding, the same behavior would occur even if it was an MSI or Inno or Nullsoft installer. If the PackageVersion is greater than the display version, even assuming uniqueness of display versions, the package would still be listed for upgrade

yao-msft commented 2 years ago

If the PackageVersion is greater than the display version, even assuming uniqueness of display versions, the package would still be listed for upgrade

I think with PackageVersion mapping feature, this should not be the case. The installed version will be mapped to the PackageVersion. Regarding portable, it's because we exclude portable from version mapping here.

Trenly commented 2 years ago

If the PackageVersion is greater than the display version, even assuming uniqueness of display versions, the package would still be listed for upgrade

I think with PackageVersion mapping feature, this should not be the case. The installed version will be mapped to the PackageVersion. Regarding portable, it's because we exclude portable from version mapping here.

Ahhhh, I see. Why are portable apps excluded from version mapping? There are many times where the VersionInfo of the executable isn’t the same as the marketed version. Certainly we do have contol over how version information is entered for portable packages, but for completeness I don’t understand why mapping wouldn’t be in place

yao-msft commented 2 years ago

Ahhhh, I see. Why are portable apps excluded from version mapping?

I think it's because winget is acting as the installer for the portables, winget controls what registry values to write. Thus we don't need another layer of version mapping to get back the PackageVersion we already know(i.e. we should just write the PackageVersion in the registry). Now I'm wondering we should revisit the logic here where I think we should just use the manifest values(including version, name and publisher). In my thought, the AppsAndFeatures entries are just additional info given to winget about what an installer writes in registry so winget can do proper mapping, but these entries are not for instructing winget what values to write in registry(in portable case).

Trenly commented 2 years ago

I think that overlooks the case where the Marketing Version and Package Version are different, even in the case of Portable packages.

NuGet is a great example - if you run nuget in the command line, it will spit out the full version - 6.2.1.2. However, on the website it is “marketed” as 6.2.1

denelon commented 2 years ago

I'm inclined to agree with @Trenly on this one @yao-msft. I think we're going to see this as a recurring theme. I am concerned, however, that we don't have a good way for multiple "longer" versions to be supported with a single "marketing" version of a package. If we run into that case, we may need to revisit our thinking.

JohnMcPMS commented 2 years ago

I am concerned, however, that we don't have a good way for multiple "longer" versions to be supported with a single "marketing" version of a package. If we run into that case, we may need to revisit our thinking.

The feature is designed to map a DisplayVersion into a PackageVersion and then forever leave DisplayVersion values behind. PackageVersion then becomes the only value. It is possibly misguided, but it was not intended to add a separate version to compare between.

While nuget isn't a good case, lets roll with it as theoretical example. Imagine I have 6.2.1 displayed with 6.2.1.2 installed. An update is available for 6.2.1.3, and is listed as such (or does it say an update to 6.2.1 is available?). I upgrade, but I still see that 6.2.1 is installed? And it is winget doing that specifically, the DisplayVersion changed in the background, but we remapped it.

You are going to have a very hard time selling me on the desire to have different sets of bits presenting as the same version. I think that will lead to more confusion than MarketingVersion + .1, MarketingVersion + .2, etc. Note that PackageVersion is MarketingVersion and "the version that winget displays".

denelon commented 2 years ago

All the cases I've seen change both the #.#.z and the #.#.#.aa and later positions for releases. If we don't run into this case I would be pleasantly surprised.

denelon commented 2 years ago

Since we can't reasonably require all software to adhere to semantic versioning, it's a hard problem to solve.

Trenly commented 2 years ago

All the cases I've seen change both the #.#.z and the #.#.#.aa and later positions for releases. If we don't run into this case I would be pleasantly surprised.

I think we already see this case, or rather, the opposite if it. Take a look at Aiursoft.Kahla. The DisplayVersion is always 4.5.0, even though the actual version is longer.

I wouldn’t be shocked if we don’t already have a case where the Marketing version remains the same but the DisplayVersion changed.

denelon commented 2 years ago

We don't have a good way to reason about this particular scenario given the following edge case:

It would be breaking if multiple versions of an installer get added to a manifests "AppsAndFeatures" entry, or it changes over time for a single "marketing" (packageVersion). We need to think a bit more deeply.

I'd love to have that single example of a package with "marketing version" 1.2.3 and "AppsAndFeatures" "displayVersion" could have more than one entry like 1.2.3.0 and 1.2.3.1.

It would certainly look strange for a user to see

Contoso.AwesomeApp version 1.2.3 has an upgrade available of version 1.2.3

Trenly commented 2 years ago

It would certainly look strange for a user to see

Contoso.AwesomeApp version 1.2.3 has an upgrade available of version 1.2.3

If the marketing version stays the same, an indicator could be displayed. I think 1.2.3 has an upgrade available of version 1.2.3* would make sense to most users that it is the “same” version, but is a different installer