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

added DominikReichl.KeePass 2.56.0 #137901

Closed
wants to merge 1 commit into from

Conversation

mluckam
Copy link
Contributor

@mluckam mluckam commented Feb 5, 2024

  • Have you signed the Contributor License Agreement?
  • Have you checked that there aren't other open pull requests for the same manifest update/change?
  • This PR only modifies one (1) manifest
  • Have you validated your manifest locally with winget validate --manifest <path>?
  • Have you tested your manifest locally with winget install --manifest <path>?
  • Does your manifest conform to the 1.5 schema?

Note: <path> is the name of the directory containing the manifest you're submitting.


Microsoft Reviewers: Open in CodeFlow

@wingetbot
Copy link
Collaborator

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@wingetbot wingetbot added Azure-Pipeline-Passed Validation pipeline passed. There may still be manual validation requirements. Validation-Completed Validation passed labels Feb 5, 2024
@stephengillie
Copy link
Collaborator

Hi @mluckam,

This manifest removes Apps and Features entries that are present in previous PR versions. Should these entries also be added to this version?

(Automated response - build 701.)

@stephengillie stephengillie added the Needs-Author-Feedback This needs a response from the author. label Feb 5, 2024
@mluckam
Copy link
Contributor Author

mluckam commented Feb 5, 2024

@stephengillie I intentionally removed the AppsAndFeaturesEntries. Notice that in 2.55 DisplayName is 2.54 and the UpgradeCode was not updated, see code. When updating using wingetcreate, the entries DisplayName and UpgradeCode were not updated.
wingetcreate.exe update DominikReichl.KeePass --version 2.56.0 --interactive

My thought is that DisplayName and UpgradeCode are additional entries that were providing incorrect information and not being updated by the wingetcreate requiring manual intervention to correct. After removal, the 3 installers were able to install and upgrade properly.

Any thoughts as to these changes having consequences?

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Attention This work item needs to be reviewed by a member of the core team. and removed Needs-Author-Feedback This needs a response from the author. labels Feb 5, 2024
Comment on lines +35 to +42
- Architecture: x86
InstallerType: msi
InstallerUrl: https://sourceforge.net/projects/keepass/files/KeePass%202.x/2.56/KeePass-2.56.msi/download
InstallerSha256: AF7C504B2B16F6F664B5154921B7F8AA3F44FA593D90F96F858A66F6A4C3F4BC
InstallerSwitches:
Custom: MSIINSTALLPERUSER=1
ProductCode: '{03F9E7B5-495F-4605-BF59-358B2CA33280}'
Scope: user
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Architecture: x86
InstallerType: msi
InstallerUrl: https://sourceforge.net/projects/keepass/files/KeePass%202.x/2.56/KeePass-2.56.msi/download
InstallerSha256: AF7C504B2B16F6F664B5154921B7F8AA3F44FA593D90F96F858A66F6A4C3F4BC
InstallerSwitches:
Custom: MSIINSTALLPERUSER=1
ProductCode: '{03F9E7B5-495F-4605-BF59-358B2CA33280}'
Scope: user

The user scope does not work as expected here. There's an issue with many MSIs that install to %LOCALAPPDATA% for user scope, but still put their registry entries to HKEY_LOCAL_MACHINE instead of HKEY_CURRENT_USER, making WinGet think that it's a machine scope install.

A user may get the "user" scope installation for the first time when installing with WinGet, but WinGet will mark the current installation as "machine" scope. This means, for subsequent upgrades through winget upgrade, the user will end up getting the "machine" scope installation resulting in a dual installation of KeePass (both user and machine scope).

Related to

I would suggest removing this installer node entirely to prevent such cases of dual installations in the future

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 looked at the above issue, and the resolution appears to be removing user scope installs. User scope installs provide a valuable feature to install updates without requiring permission elevation. Is there a suggestion on how to correct the manifest or msi to address this issue without removing the user scope install?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that it's a useful feature, but unfortunately the only users actually getting the user scope install would be those that don't have an existing installation of KeePass and are using WinGet to install this package. Even then, the users planning to upgrade this package through winget would get the "machine" scope install when they try to upgrade, leaving side-by-side installations on their machine.

See -

video.mp4
  1. winget detects the current 'user' scope installation as 'machine' scope
  2. User upgrading get the machine scope install that requires elevation
  3. If the user does proceed with the install, user is left with a side-by-side installation, becoming another source of confusion

image

Is there a suggestion on how to correct the manifest or msi to address this issue without removing the user scope install?

Unless there is a resolution from WinGet's side (tracked in linked issue above) to handle these situations better, or MSIs start putting the registry entries in the correct place, I don't know of a better solution than just removing the user scope from the manifest.

Copy link
Contributor

Choose a reason for hiding this comment

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

Technical users can still retain the user scope install if they want by passing the args to the installer on update:

winget upgrade DominikReichl.KeePass --custom "MSIINSTALLPERUSER=1"

But the manifest should exhibit a behavior that causes the least issues for majority users.

Copy link
Contributor Author

@mluckam mluckam Feb 6, 2024

Choose a reason for hiding this comment

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

MSIs start putting the registry entries in the correct place

Is this problem MSIs as a whole or misuse per MSI. If this issue could be corrected in the KeePass MSI, I can reach out to the maintainer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Reading the linked issue, and reviewing many MSIs PRs, I'm under the impression this is a problem with MSIs (or WiX - a popular toolset to create MSIs?) as a whole. I've yet to encounter an MSI package that does this correctly.

@mdanish-kh
Copy link
Contributor

When updating using wingetcreate, the entries DisplayName and UpgradeCode were not updated.

Unless it's an MSI / MSIX, these registry values cannot be fetched directly from the installer file. One needs to install the package first, and wingetcreate doesn't have the ability to do that

Any thoughts as to these changes having consequences?

These are additional values that help in package correlation for WinGet. Having incorrect values may have adverse effects. The existing metadata already looks enough for correlation since we have an exact match on ProductCode and Publisher values. Sure, the inno installer the MSI do have a different DisplayName (inno -> KeePass Password Safe <version> and msi -> KeePass <version>), but I believe the fuzzy matching for PackageName, and having an exact match on ProductCode and Publisher will still provide good correlation.

The manifest creators don't have the ability yet to update these AppsAndFeaturesEntries automatically, so for simplifying package updates / contributions to the package, we have more value in removing these entries entirely (unless someone reports an issue where the package isn't getting matched correctly, then we'll add these back in)

@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs-Author-Feedback This needs a response from the author. label Feb 6, 2024
@stephengillie
Copy link
Collaborator

Duplicate of #138277

@stephengillie stephengillie marked this as a duplicate of #138277 Feb 7, 2024
auto-merge was automatically disabled February 7, 2024 20:11

Pull request was closed

@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs-Attention This work item needs to be reviewed by a member of the core team. label Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure-Pipeline-Passed Validation pipeline passed. There may still be manual validation requirements. Changes-Requested Changes Requested Validation-Completed Validation passed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants