search

microsoft / winget-create

The Windows Package Manager Manifest Creator command-line tool (aka wingetcreate)
MIT License
472 stars 83 forks source link

Support updating DisplayVersion in autonomous update #520

Open mdanish-kh opened 3 months ago

mdanish-kh commented 3 months ago

Changes

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
vedantmgoyal9 commented 3 months ago

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:

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 commented 3 months ago

I think your concern is valid. I like the idea of an input object (https://github.com/microsoft/winget-create/issues/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 commented 3 months ago

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.

https://github.com/microsoft/winget-create/issues/521#issuecomment-1989044471 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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

@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!

ryfu-msft commented 4 days ago

/azp run

azure-pipelines[bot] commented 4 days ago
Azure Pipelines successfully started running 1 pipeline(s).