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 [WingetVersion] Badge #10245

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

anatawa12
Copy link
Contributor

@anatawa12 anatawa12 commented Jun 11, 2024

Fixes #5548

Writing test in progress (might be tomorrow in JST) I wrote tests

This implementation fetches version information from https://github.com/microsoft/winget-pkgs/ with graphql api.
However, actual vrc-get command line interface retrieves package information from sqlite database file in https://cdn.winget.microsoft.com/cache/source.msix.

I don't know if this difference is acceptable for shelds.

Copy link
Contributor

github-actions bot commented Jun 11, 2024

Messages
📖 ✨ Thanks for your contribution to Shields, @anatawa12!

Generated by 🚫 dangerJS against 439c5ef

@anatawa12 anatawa12 marked this pull request as ready for review June 12, 2024 03:43
Copy link
Member

@chris48s chris48s left a comment

Choose a reason for hiding this comment

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

thanks for having a look at this one

services/winget/winget-version.service.js Outdated Show resolved Hide resolved
const entries = json.data.repository.object.entries
const directories = entries.filter(file => file.type === 'tree')
const versions = directories.map(file => file.name)
const version = latest(versions)
Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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

Copy link
Contributor Author

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

Copy link
Member

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 got Version: 213.6461.785 but the badge says MPS-241.18034.990
For winget show niconico.nair.live I got Version: latest but the badge says v1.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?

Copy link
Contributor Author

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)

Copy link
Member

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).

Copy link
Member

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.

services/winget/winget-version.service.js Show resolved Hide resolved
@chris48s chris48s added the service-badge Accepted and actionable changes, features, and bugs label Jun 14, 2024
Copy link
Contributor

github-actions bot commented Jul 9, 2024

🚀 Updated review app: https://pr-10245-badges-shields.fly.dev

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge Accepted and actionable changes, features, and bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New service: winget
3 participants