search

microsoft / winget-create

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

Improve ZIP handling in interactive update #398

Closed mdanish-kh closed 1 year ago

mdanish-kh commented 1 year ago

This change makes it so that the zip archive is extracted in interactive update as well and the path to the nested installer is used in other functions. Made the logic of extracting and determining path of the nested installer into a function as it's common between autonomous and interactive updates.

The logic is based off of the fact that if InstallerType is ZIP, there will always be atleast one NestedInstallerFile node in the manifest that contains RelativeFilePath key and the first matching RelativeFilePath from an older manifest is used as the path to the nested installer. Multiple RelativeFilePath nodes under NestedInstallerFiles are only possible if the NestedInstallerType is portable. In this PR, we do not check the InstallerType of all RelativeFilePath nodes for NestedInstallerType: portable. The assumption is that it's very rare that if an older version contained all portable RelativeFilePath objects, then one of them has changed in newer version to be an another InstallerType (e.g. MSI, nullsoft etc). In case of portables as well, only the first matching RelativeFilePath object is passed to determine the InstallerType to keep the logic simple.

Shifting of values from root to installer is needed in cases where the NestedInstallerFiles object is common between all installer nodes and is present in the root of the manifest instead for better readability. One of the many examples of such manifests is junnegun.fzf. Since the interactive update only works by reading the Installers node under the installer manifest, we need a way to read the NestedInstallerFiles in the root of the manifest. I did it by shifting all related values to installer level at the start of the interactive update. Ofcourse this comes at a cost of slightly more verbose manifest, but I think it's okay for now considering the benefit of a successful update. We can make another function ShiftInstallerValuesToRoot to take out common fields to the root level but I think that's beyond the scope of this PR as it's applicable to new, autonomous update commands as well and being tracked in issue https://github.com/microsoft/winget-create/issues/185

Microsoft Reviewers: codeflow:open?pullrequest=https://github.com/microsoft/winget-create/pull/398&drop=dogfoodAlpha
ryfu-msft commented 1 year ago

/azp run

azure-pipelines[bot] commented 1 year ago
Azure Pipelines successfully started running 1 pipeline(s).
ryfu-msft commented 1 year ago

Apologies for the delay, since the interactive update needs to be manually tested, I needed to checkout your branch and try it manually in order to review this. Shifting the root properties to the installer node is a great addition and it will help improve the interactive flow.

Once thing I wanted to call out was that I found a bug when trying to update junnegun.fzf interactively. All of the architectures get updated to 'x64' thus making the updated manifest invalid. I created an issue here: https://github.com/microsoft/winget-create/issues/405 but I went ahead and approved this PR since that is not related to the feature you just added. Thanks!