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

Validate against NestedInstaller / Base Installer is not Archive #5236

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Trenly
Copy link
Contributor

@Trenly Trenly commented Feb 21, 2025

Description:

This PR adds validation to ensure that NestedInstallerType and NestedInstallerFiles are not present for the installer if the base installer type is not an archive type. This will avoid confusion as the fields have no effect if the base installer type is not an archive.

{1108C44F-4E40-4275-8522-359E5EB58893}


Microsoft Reviewers: Open in CodeFlow

@Trenly Trenly requested a review from a team as a code owner February 21, 2025 04:43
@microsoft-github-policy-service microsoft-github-policy-service bot added the Issue-Bug It either shouldn't be doing this or needs an investigation. label Feb 21, 2025
@yao-msft
Copy link
Contributor

This change does not fix the original issue's related pr microsoft/winget-pkgs@5694bd8 (#162833). In that pr, the NestedInstaller* are defined at root. During default installer info population, the related entries are not copied to individual installers. So this change does not catch that pr's case.

When I was first implementing the ManifestValidation.cpp, I was thinking of only doing validations that are critical in the winget client's operations so it's a cheap operation. The nested installer files are mostly harmless in this case. I can see that we are starting to add non-critical validations like "schema header validation" based on community feedback, but they are behind fullValidation and as warning only. (Making it warning also prevents failures in wingetsvc index rebuild if some existing manifests already have inaccurate but harmless entries). So if we want to enforce this change, it would better be behind fullValidation and warning only too.

@Trenly
Copy link
Contributor Author

Trenly commented Feb 21, 2025

Ah, thanks for catching the root case; Although, I am confused then - If the field's aren't parsed into each installer in parsing the Yaml into the Manifest object, then how does the validation to ensure that each installer has NestedInstallerType if that is at the root level?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Bug It either shouldn't be doing this or needs an investigation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Manifest validation succeeds when NestedInstaller(Type | Files) is used for non-zip packages
2 participants