Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 [WingetVersion] Badge #10245
base: master
Are you sure you want to change the base?
add [WingetVersion] Badge #10245
Changes from 3 commits
7a22dbc
f71b6b8
d387631
d9963b5
6b70732
439c5ef
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way we could find out what the registry considers the "latest" version of a package? Do we have any info on how WinGet sorts versions?
I'm a bit worried that there doesn't really seem to be any constraint on what a version "number" can be. Here's some examples that have one or more odd ones:
Given it seems a version number can be basically any string, it seems like this we are going to get this wrong in some cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for late response. I will investigate the update process in winget database
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could noy find meaningful source code of winget-pkgs pipeline but I found version comparesion in winget-cli, that are used when one package is provided by multiple multiple sources.
I don't know if this compression is used by winget-pkgs pipeline and may not work well with some if your example packages but I think it's reasonable to use this comparesion.
I will implement this compression in services/winget folder with unit tests.
Version Implementation: https://github.com/microsoft/winget-cli/blob/ae566c7bf21cfcc75be7ec30e4036a30eede8396/src/AppInstallerSharedLib/Versions.cpp
Use case in winget-cli: https://github.com/microsoft/winget-cli/blob/ae566c7bf21cfcc75be7ec30e4036a30eede8396/src/AppInstallerRepositoryCore/PackageVersionSelection.cpp#L82-L100
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have implemented in 6b70732
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Thanks for following up. I set up a windows VM and installed winget on it to try and do a bit of testing.
My list above is not an exhaustive list - it is just a handful of examples I found by searching on github for examples I thought might be weird. As a first pass of review, I went though that list of 5 packages and compared the version reported by
winget show
to the version on the badge in this PR.For
winget show JetBrains.MPS.EAP
I gotVersion: 213.6461.785
but the badge saysMPS-241.18034.990
For
winget show niconico.nair.live
I gotVersion: latest
but the badge saysv1.1.20240527-1
Obviously we can go back and review those edge cases, but my overall feeling on this one is that given we don't seem to be able to explicitly ask the registry "what is the latest version of this package?", it is going to be very hard to merge this with a high level of confidence we are doing the right thing or know how to respond/fix in future if a user raises a support request saying "winget badge reports wrong latest version for package X". I'm not even really sure if the discrepancies we're seeing there are because there's a bug in the ported code or because we're trying to port the wrong thing. I think the most important thing is the badge needs to agree with the tool/registry and there's not much transparency on how to achieve this.
At this stage, I think I'm inclined to take a step back from this one. I don't really want to encourage you to write more code on this unless there's a high chance we're heading towards merging this.
@PyvesB do you have any thoughts on this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry forget to impement latest case so nair case is my fault.
however, I feel that JetBrains.MPS.EAP case is bug in winget registorysince actual latest version is 241.18034.990 and I think return it as latest with wingwt-cli version comparator.(I'll test later)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Glancing at microsoft/winget-pkgs/, it looks like the number of package is in the order of magnitude of a few hundreds, or at most a few thousands. I'm wondering how hard it would be to write a small script that checks out the repository, runs our bit of JS and the CLI on all packages (possibly batching in parallel if the CLI is slow), and prints out all diffs it finds. This would give us much higher confidence that we're aligned with the CLI (or that the diffs are intentional, e.g. the JetBrains.MPS.EAP bug that was previously mentioned).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be down with that as a one-off check to verify this.
I'm not sure I have the appetite to write it.