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

Support updating DisplayVersion in autonomous update #520

Merged
merged 3 commits into from
Jul 8, 2024

Conversation

mdanish-kh
Copy link
Contributor

@mdanish-kh mdanish-kh commented Mar 10, 2024

Changes

  • Adds --display-version CLI arg and support for providing display version with the installer URL
  • The value in the installer URL takes precedence over value provided with --display-version.
  • If a single installer has multiple DisplayVersion entries, we aren't sure which one user is targeting to update. In this case, we only update the first DisplayVersion found.

The thing I struggled the most while making this PR is the correct naming convention to be used. I stayed away from calling the value provided in the installer URL as an "override". Winget-Create does not detect the display-version, so calling it an override didn't make much sense to me. It is extra information user is providing in the URL to update. I went with "Installer URL arguments" as the general term for metadata provided with the installer URL. As such, I had to refactor a bunch of existing stuff. I'm still not sure if that's the correct naming, but I'll leave it up for debate here.


Microsoft Reviewers: Open in CodeFlow

@mdanish-kh mdanish-kh requested a review from a team as a code owner March 10, 2024 20:13
@mdanish-kh mdanish-kh requested review from yao-msft and ryfu-msft and removed request for a team March 10, 2024 20:13
@microsoft-github-policy-service microsoft-github-policy-service bot added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Mar 10, 2024
@vedantmgoyal9
Copy link
Contributor

vedantmgoyal9 commented Mar 10, 2024

Is there any reason we're going with approach for now? It would create more work when support for input object for overrides is added. This PR adds two ways to specify display version:

  • as an installer url override
  • as a parameter

When we add support for input objects, it would be a third way to pass display version to winget-create. This may give a hard time to new users, as well as publishers who are unfamiliar with winget's manifest structure but want to publish their software to winget in one go. I think we should keep the interface of winget-create as much as possible. Too may parameters make it a bit complex, and we have faced this issue in YamlCreate previously, which is why we came up with the idea of an "input object"

@mdanish-kh
Copy link
Contributor Author

I think your concern is valid. I like the idea of an input object (#521), but I don't think we can get rid of URL overrides completely. That would be a breaking change for many of current CI/CD's that use this tool. What we may want to do is allow scope & arch as they are, but any additional metadata may need to be specified in an object. I see problems with extending overrides as they are, if I want to add support additional string inputs (not convertible to a known enum value like arch, scope, installerType), then winget-create will have a hard time parsing it with the current implementation. I would like to hear @ryfu-msft's thoughts on this one. I'm not that concerned tho about multiple ways for specifying display version as part of this PR. The both ways function differently, and are not a replica of each other. I believe they offer user flexibility in the sense that if user knows that display version is the same for all installers, then specifying --display-version may suffice instead of appending the same version with each URL.

If we don't want to allow this as an override by favoring input object method, maybe we can only take the --display-version implementation in this PR

@vedantmgoyal9
Copy link
Contributor

I believe that leaving installer URL overrides as-is (with only arch and scope option) is the best choice. I'm not sure about --display-version argument, I'll leave it to @ryfu-msft and @yao-msft.

#521 (comment) can help while planning this further, as I've considered input object not only means to pass overrides that we currently do using installer URL arguments, but also as a way to pass additional metadata to winget-create, so we don't have to edit manifest by hand after generation.

@yao-msft
Copy link

The DisplayVersion field in winget package's manifest is used for installed package version mapping. We advise not to use this field unless there's a strong need to. Basically, when winget sees one DisplayVersion defined in the manifest, it tries to download all manifest versions of the package and read all DisplayVersion values, then winget will create ranges of DisplayVersion to map the Installed Package's ARP DisplayVersion, then the installed package version will be mapped to a PackageVersion of the winget manifest for upgradability check.

This operation is a bit expensive on the client.

Also adding DisplayVersion to manifest has possibility to break the winget-pkg publish pipeline if manifest authors do not know the relation between ARP DisplayVersion and winget manifest PackageVersion because winget-pkgs pipeline has integrity check on the DisplayVersion ranges.

Details can be found here

@vedantmgoyal9
Copy link
Contributor

I realized users can easily confuse between --version and --display-version flags. I think we should switch to input object since we can override much more fields with it.

@mdanish-kh
Copy link
Contributor Author

Also adding DisplayVersion to manifest has possibility to break the winget-pkg publish pipeline if manifest authors do not know the relation between ARP DisplayVersion and winget manifest PackageVersion because winget-pkgs pipeline has integrity check on the DisplayVersion ranges.

@yao-msft This PR doesn't add support for adding DisplayVersion, it adds support for updating DisplayVersion if the existing manifest already has a DisplayVersion value. In fact, if previous manifest does not have a DisplayVersion value, then the new args will do nothing. This is done to improve the current flow where if an existing manifest contains DisplayVersion field, then winget-create does nothing and leaves the field unchanged in the update flow. This causes the PR to get flagged with error labels over at winget-pkgs since a duplicate DisplayVersion is not allowed within the same PackageIdentifier. User has to manually update this field after creating a PR. This PR improves the UX and allows the user to specify the value while updating the package manifest.

@yao-msft
Copy link

@yao-msft This PR doesn't add support for adding DisplayVersion, it adds support for updating DisplayVersion if the existing manifest already has a DisplayVersion value. In fact, if previous manifest does not have a DisplayVersion value, then the new args will do nothing. This is done to improve the current flow where if an existing manifest contains DisplayVersion field, then winget-create does nothing and leaves the field unchanged in the update flow. This causes the PR to get flagged with error labels over at winget-pkgs since a duplicate DisplayVersion is not allowed within the same PackageIdentifier. User has to manually update this field after creating a PR. This PR improves the UX and allows the user to specify the value while updating the package manifest.

Thanks for the clarification. That's really helpful for reducing errors in winget-pkgs!

Copy link
Contributor

@ryfu-msft ryfu-msft left a comment

Choose a reason for hiding this comment

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

Sorry this review is so late, just a couple minor comments but overall great new feature 😄

doc/update.md Outdated Show resolved Hide resolved
src/WingetCreateCLI/Commands/UpdateCommand.cs Outdated Show resolved Hide resolved
src/WingetCreateCore/Common/PackageParser.cs Outdated Show resolved Hide resolved
src/WingetCreateCore/Models/InstallerMetadata.cs Outdated Show resolved Hide resolved
@ryfu-msft
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ryfu-msft
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ryfu-msft
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ryfu-msft ryfu-msft merged commit 7826f51 into microsoft:main Jul 8, 2024
5 checks passed
@mdanish-kh mdanish-kh deleted the displayVersion branch July 8, 2024 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to update DisplayVersion in autonomous update
4 participants