search

microsoft / winget-create

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

Root Level overwrites Installer Level Incorrectly #490

Closed Trenly closed 9 months ago

Trenly commented 10 months ago

I found the flow confusing in case of Root level and Installer level ARP defined side-by-side. I was testing with a manifest where I had 3 installers and 2 had Installer level ARP and then I defined a root level ARP for testing.

The Root level ARP was considered for the installer with no ARP entry, but rest of the installers had old variables set to their own respective installer level ARP entries. After the update, the root level ARP was shifted to the installer that originally had no ARP entry.

I believe (and this is how wingetcreate does it as well) that Installer level fields (not just limited to ARP entries) should be overwritten by Root level fields at the beginning of the update. This means if Root and installer level ARP are defined side-by-side in the manifest, then this should be considered an inconsistency, and the root level ARP should take precedence and overwrite whatever is at the Installer level.

I would say we can leave this in this PR since the implementation isn't strictly related to ARP, but is applicable to all installer fields. Just wanted to call this out here though.

For reference, I was testing this with Coder.Coder version 2.4.0 with the following installer manifest:

Installer manifest ```yaml ```yaml # Created using wingetcreate 1.5.7.0 # yaml-language-server: $schema=https://aka.ms/winget-manifest.installer.1.5.0.schema.json PackageIdentifier: Coder.Coder PackageVersion: 2.4.0 AppsAndFeaturesEntries: - DisplayName: Test Installers: - Architecture: x64 InstallerType: nullsoft Scope: machine ElevationRequirement: elevatesSelf InstallerUrl: https://github.com/coder/coder/releases/download/v2.4.0/coder_2.4.0_windows_amd64_installer.exe InstallerSha256: 5C26EC6F89BBE75E89BC4DB9E9DA3681F20BCEEC9DD2DB21176B5559687CCA76 ProductCode: Coder - Architecture: x64 InstallerType: zip NestedInstallerType: portable NestedInstallerFiles: - RelativeFilePath: coder.exe PortableCommandAlias: coder AppsAndFeaturesEntries: - DisplayName: Coder (portable) InstallerUrl: https://github.com/coder/coder/releases/download/v2.4.0/coder_2.4.0_windows_amd64.zip InstallerSha256: 4E53AEDB33A5B8EEC78A203E5C2B41269BC44E61502F576DDAD7DB94B6076093 - Architecture: arm64 InstallerType: zip NestedInstallerType: portable NestedInstallerFiles: - RelativeFilePath: coder.exe PortableCommandAlias: coder AppsAndFeaturesEntries: - DisplayName: Coder (portable) InstallerUrl: https://github.com/coder/coder/releases/download/v2.4.0/coder_2.4.0_windows_arm64.zip InstallerSha256: 8D711A10EF3266317B63A05A47C30619390D04C47765119E3A24A0976B7107F5 ManifestType: installer ManifestVersion: 1.5.0 ```

_Originally posted by @mdanish-kh in https://github.com/microsoft/winget-pkgs/pull/129471#discussion_r1417752698_

If the behavior of wingetcreate is truly as @mdanish-kh described, then it is likely an error. Items at the installer level take precedence over items at the root level in winget-cli.

The flow would ideally be (and how YamlCreate does it) : Try to move root level items to installer level. If installer level does not exist, root can be applied successfully. If installer level exists, it overrides root level and root is not applied.