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

NVAccess.NVDA 2020.4 classifies as update for newer version of package #1209

Closed
LeonarddeR opened this issue Jun 23, 2021 · 16 comments
Closed
Labels
Issue-Bug It either shouldn't be doing this or needs an investigation.
Milestone

Comments

@LeonarddeR
Copy link

Originally posted in #976 (comment)

I have the following issue with the NVAccess.NVDA package. I have version 'alpha-22934,e05715d3' installed whereas winget tries to upgrade (or rather downgrade) to version 2020.4, which is the most recent stable version available in the winget repostiroy.

When I looked at how winget determines the installed version, I noticed that DisplayVersion in Add/Remove programs is used. It seems that the Version, VersionMajor and VersionMinor fields in the uninstall registry key are ignored. Also what might make it more difficult for winget is that NVDA has a 4 part version scheme for its full version, e.g. 2021.2.0.22934 for the version with display version alpha-22934,e05715d3.

Interesting enough though, A version string that starts with the word 'Alpha' which is non-numeric, shouldn't that be treated as newer than a version string that starts with a number (e.g. 2020)?

See also nvaccess/nvda#12469

@ghost ghost added the Needs-Triage Issue need to be triaged label Jun 23, 2021
@denelon
Copy link
Contributor

denelon commented Jun 23, 2021

@LeonarddeR can you share output from winget --info and winget list nvda? I'd like to make sure which version of the Windows Package Manager is installed, and how the "NVAccess.NVDA" package is displayed.

@denelon denelon added Needs-Author-Feedback Issue needs attention from issue or PR author Issue-Bug It either shouldn't be doing this or needs an investigation. and removed Needs-Triage Issue need to be triaged labels Jun 23, 2021
@LeonarddeR
Copy link
Author

Thanks for your quick reply.

The relevant stuff from winget --info:

Windows Package Manager v1.0.11451
Windows: Windows.Desktop v10.0.19043.1081
Package: Microsoft.DesktopAppInstaller v1.11.11451.0

Logs: %LOCALAPPDATA%\Packages\Microsoft.DesktopAppInstaller_8wekyb3d8bbwe\LocalState\DiagOutputDir

Output from winget list nvda:

Name Id            Version              Available Source
--------------------------------------------------------
NVDA NVAccess.NVDA alpha-23168,b1a52939 2020.4    winget

@denelon
Copy link
Contributor

denelon commented Jun 23, 2021

That is an interesting version number "alpha-23168,bla52939" 😕

Do you know if the "alpha" is a different release channel?
If it is, do you know if it can be installed side by side with the stable version?

We don't support #147 release channels yet.

@JohnMcPMS
Copy link
Member

Versions are assumed to be number [extra string part] (dot number [extra string part])* as this is the most popular format. What this means for the way that we parse is that “alpha-23168,b1a52939” is all version 0 with [extra string part] being the whole thing. So the official version, which is 2020.4 will have its first version number be 2020 which is > 0.

The reason that we use DisplayVersion if it is available is due to multiple factors, but one is simply that this is the string that people will see and thus the one they are likely to put into the manifest. It also means that the Add/Remove Programs UI and our display number match.

There has been a desire to add "hidden" data to manifests for correlative purposes, and I think if we were to do that for Name and Publisher, Version might be a good idea too. Along with that I think that we could probably make the numeric versions the ones used for comparison.

In addition, channel tracking would probably aid you here, but without the version work being done as well it would be hard to actually compare "alpha-23168,b1a52939" to some theoretical future "alpha-14821,934745fa".

@LeonarddeR
Copy link
Author

LeonarddeR commented Jun 23, 2021

That is an interesting version number "alpha-23168,bla52939" 😕

Agreed. It is build from <channel>-<buildNumber>,<GitCommit>. It is, however, the user visible version.
As of writing, the current version is alpha-23168,b1a52939. The year.major.minor.build version string of this version is 2021.2.0.23168. In theory, it could be sufficient to write the 2021.2.0.23168 string to the DisplayVersion field in the registry, but I'd rather have Winget use something else that's less presentational.

Do you know if the "alpha" is a different release channel?

Yes. We have alpha, beta and stable channels.

If it is, do you know if it can be installed side by side with the stable version?

Nope, it can't.

@ghost ghost added Needs-Attention Issue needs attention from Microsoft and removed Needs-Author-Feedback Issue needs attention from issue or PR author labels Jun 23, 2021
@denelon
Copy link
Contributor

denelon commented Jun 24, 2021

We will be separating the "actual" version as reported in Add / Remove Programs #1073 into a different area in the v1.1 manifest schema so the "PackageVersion" field could be something different in the future. We've seen at least one other package with "Marketing" versions" different from "Add / Remove Programs" versions.

Our current best case scenario would be semantic versioning as it's deterministic for identifying the order of releases for a given package.

I think your suggestion would be the next best option.

For the release channels what would happen if you tried to install a stable on an alpha or vice versa?

Until we have support for channels in a single "package", maybe there should be a separate alpha package for NVDA.

@LeonarddeR
Copy link
Author

For the release channels what would happen if you tried to install a stable on an alpha or vice versa?

Alpha replaces stable and stable replaces alpha. NVDA compares the installing copy against the installed copy based on the file version of the main executable. If the version to be installed is older, a downgrade warning is displayed which you can dismiss.

Honestly, I'm not intending to add/maintain manifests for Alpha versions, as they sometimes appear daily and their builds are dismissed by Appveyor in a month or six.

@LeonarddeR
Copy link
Author

As it stands now, the authors from NVDA aren't willing to change their display version. It is possible to expand on other registry fields though, so as long as we could somehow tell winget to use the product version instead of the display version, that could improve things.

@denelon
Copy link
Contributor

denelon commented Mar 17, 2022

@LeonarddeR that work is in progress on our side. We have some work to do in the client and the cache to explicitly show one value and use the other, but the path forward is clear.

@denelon denelon modified the milestones: v1.3-Client, v1.4-Client May 31, 2022
@LeonarddeR
Copy link
Author

@denelon Given recent updates in winget, would it help to do the following?

  • Add the ProductVersion to the ARP entry in the registry
  • To the NVDA manifest, add something like this:
    AppsAndFeaturesEntries: 
    - DisplayName: NVDA
    ProductVersion: 2022.3.0.25844
    

note that DisplayVersion and the PackageVersion in the manifest will still have the form of `alpha-22934,e05715d3`. Will Winget use the ProductVersion or the DisplayVersion for comparison in this case? In other words, will a current entry in ARP with a higher ProductVersion be ignored for updates?

@denelon
Copy link
Contributor

denelon commented Jul 14, 2022

@LeonarddeR we're still working on the service side components, but they will enable matching for upgrade on the "AppsAndFeatures" "displayValue" and rendering the "packageVersion" to the user in command line output.

@LeonarddeR
Copy link
Author

@denelon, thanks for your quick reply. Turns out I was wrong in my assumption that add/remove programs contains something like a ProductVersion string though.

Let me reformulate my question properly this time. Assuming the people at NVDA insist on keeping DisplayVersion set to the format outlined above (e.g. alpha-25875,d6a193ae, which is considered a more readable form than its file version 2022.3.0.25875. This means that winget considers version 2022.1 newer than this particular Alpha version even though it's older. Could we avoid this by defining AppsAndFeaturesEntries properly in the manifest for every release version if we add a Version dword to the registry? Will that Version dword in the manifest be compared against the current Version field in the registry? Or will changing the DisplayVersion format be our only option?

@JohnMcPMS
Copy link
Member

This is complicated to discuss, so some ground rules:

  1. DisplayVersion will be the value that winget reads from the registry (which may come from DisplayVersion or the other locations that we read from).
  2. PackageVersion will be the value that is defined in the manifest

The new feature attempts to map the detected DisplayVersion to a known PackageVersion, and then use the for all future considerations (output and/or comparison for upgrade, etc.). If the mapping is strongly ordered, which I am using here to mean that the ordering of PackageVersion as winget defines it matches with the ordering of the mapped DisplayVersion, then winget will also map unknown DisplayVersion values into ranges between the known PackageVersion values.

Given that hashes are being used, I doubt you will ever get a strong ordering. So the best you can do is have every released version defined so that there is no ambiguous mapping. Even if you don't have every version, any one that we know of will start using the PackageVersion and be able to be compared against the incoming available versions. If you only installed from winget, you should be on the golden path.

Alternately, I think that there are code changes that we could make, although I don't love any of the options that I have thought of. Channels could be helpful too, but they don't solve the underlying problem.

First option would be to add custom version parsing and comparison rules. The easiest way would be to add incremental quirking rules, like "Use the NVDA version format" or something. Easiest in the short term, but opens the door to everyone wanting their version format quirked. We also do not have a good story around package level data...

Second option would be to change the code to look at the various ways in which we attempt to form a comparable version string, and choose "the best" one. Currently we take the first one that we find, as can be seen in the code. We could look at all of them and decide which has the most number of version parts, using it instead. I think that has a potential for strange behavior though, and would prefer not to use it (it also potentially impacts other packages that are working fine now).

std::string ARPHelper::DetermineVersion(const Registry::Key& arpKey) const

@LeonarddeR
Copy link
Author

I guess the short story is that NVDA will need to change anyway. 😉 Thanks for elaborating!

seanbudd pushed a commit to nvaccess/nvda that referenced this issue Aug 1, 2022
…upport (#13911)

Fixes #12469

Summary of the issue:
Winget is the new package manager in the most recent versions of Windows 10 and 11. It allows automatic updating via the command line. However, Winget threats Alpha versions of NVDA older than any released version. This means that, when an Alpha version for NVDA 2022.3 is installed, Winget will yet threat NVDA 2022.1 as an upgrade. See microsoft/winget-cli#1209 (comment) for more information about version parsing in Winget. In short, our versioning scheme for alpha versions (e.g. alpha-25875,d6a193ae) is considered pretty exotic.

I proposed three possible solutions in #12469 (comment). Option 3 is not possible with winget, see microsoft/winget-cli#1209 (comment). Therefore, this pr chooses option 1, but it extends on that (see below)

Description of user facing changes

In add/remove programs:

Old situation:
DisplayName: NVDA
DisplayVersion: alpha-25875,d6a193ae

New situation:
DisplayName: NVDA alpha-25875,d6a193ae
DisplayVersion: '2022.3.0.25875'

In about window:

Old situation: Version: alpha-25875,d6a193ae

New situation: Version: alpha-25875,d6a193ae (2022.3.0.25875)
Apart from fixing Winget this way, it will be much easier for end users to see on what year.major.minor branch of NVDA they are with their alpha version.
note that versions in other places are really not affected. Therefore this change is barely visible. Also note that having a version in the Displayname and the product version in the DisplayVersion for ARP is pretty common. This is used by Visual Studio for example.
@denelon
Copy link
Contributor

denelon commented Aug 8, 2022

Note: We've released Windows Package Manager 1.3, and support for version mapping against the "displayVersion" in the registry can happen while still allowing the "packageVersion" to be more of a "marketing" type version.

@LeonarddeR
Copy link
Author

Thanks for pointing out @denelon. We eventually settled on saving the display version as year.major.minor.build in the DisplayVersion string in the registry, so that fixes the version comparison for us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Bug It either shouldn't be doing this or needs an investigation.
Projects
Development

No branches or pull requests

3 participants