Return error codes for dependencies failure #2410

hackean-msft · 2022-08-03T16:26:55Z

The goal of these change is to allow the WinGetValidateManifestV3 to return dependencies failure as error code for programmatic usage. This prevents the need to parse strings to decode the different failure types associated with dependences.

Changes:

Use localized messages for validation errors.
Parse exceptions to decode failure types and surface that to the caller.

Microsoft Reviewers: Open in CodeFlow

src/AppInstallerCommonCore/Public/winget/ManifestValidation.h

src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp

src/AppInstallerCommonCore/Manifest/YamlParser.cpp

src/AppInstallerRepositoryCore/ArpVersionValidation.cpp

src/AppInstallerRepositoryCore/PackageDependenciesValidationUtil.h

src/AppInstallerRepositoryCore/PackageDependenciesValidationUtil.cpp

Trenly · 2022-08-03T18:45:30Z

src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp

+            { AppInstaller::Manifest::ManifestError::DuplicateReturnCodeEntry, "Duplicate installer return code found." },
+            { AppInstaller::Manifest::ManifestError::FieldRequireVerifiedPublisher, "Field usage requires verified publishers." },
+            { AppInstaller::Manifest::ManifestError::SingleManifestPackageHasDependencies, "Package has a single manifest and is a dependency of other manifests." },
+            { AppInstaller::Manifest::ManifestError::MultiManifestPackageHasDependencies, "Deleting the manifest will be break the following dependencies." },


Suggested change

{ AppInstaller::Manifest::ManifestError::MultiManifestPackageHasDependencies, "Deleting the manifest will be break the following dependencies." },

{ AppInstaller::Manifest::ManifestError::MultiManifestPackageHasDependencies, "Deleting the manifest will break the following dependencies." },

Trenly · 2022-08-03T18:46:25Z

src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp

+            { AppInstaller::Manifest::ManifestError::RequiredFieldEmpty, "Required field with empty value." },
+            { AppInstaller::Manifest::ManifestError::RequiredFieldMissing,  "Required field missing." },
+            { AppInstaller::Manifest::ManifestError::InvalidFieldValue, "Invalid field value." },
+            { AppInstaller::Manifest::ManifestError::ExeInstallerMissingSilentSwitches, "Silent and SilentWithProgress switches are not specified for InstallerType exe.Please make sure the installer can run unattended." },


Suggested change

{ AppInstaller::Manifest::ManifestError::ExeInstallerMissingSilentSwitches, "Silent and SilentWithProgress switches are not specified for InstallerType exe.Please make sure the installer can run unattended." },

{ AppInstaller::Manifest::ManifestError::ExeInstallerMissingSilentSwitches, "Silent and SilentWithProgress switches are not specified for InstallerType 'exe'. Please make sure the installer can run unattended." },

Trenly · 2022-08-03T18:47:18Z

src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp

+            { AppInstaller::Manifest::ManifestError::DuplicateInstallerEntry, "Duplicate installer entry found." },
+            { AppInstaller::Manifest::ManifestError::InstallerTypeDoesNotSupportPackageFamilyName, "The specified installer type does not support PackageFamilyName." },
+            { AppInstaller::Manifest::ManifestError::InstallerTypeDoesNotSupportProductCode, "The specified installer type does not support ProductCode." },
+            { AppInstaller::Manifest::ManifestError::InstallerTypeDoesNotWriteAppsAndFeaturesEntry, "The specified installer type does not write to Apps and Features entry." },


Suggested change

{ AppInstaller::Manifest::ManifestError::InstallerTypeDoesNotWriteAppsAndFeaturesEntry, "The specified installer type does not write to Apps and Features entry." },

{ AppInstaller::Manifest::ManifestError::InstallerTypeDoesNotWriteAppsAndFeaturesEntry, "The specified installer type does not write an entry to Apps and Features." },

Trenly · 2022-08-03T18:48:00Z

src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp

+            { AppInstaller::Manifest::ManifestError::InstallerTypeDoesNotSupportPackageFamilyName, "The specified installer type does not support PackageFamilyName." },
+            { AppInstaller::Manifest::ManifestError::InstallerTypeDoesNotSupportProductCode, "The specified installer type does not support ProductCode." },
+            { AppInstaller::Manifest::ManifestError::InstallerTypeDoesNotWriteAppsAndFeaturesEntry, "The specified installer type does not write to Apps and Features entry." },
+            { AppInstaller::Manifest::ManifestError::IncompleteMultiFileManifest, "The multi file manifest is incomplete.A multi file manifest must contain at least version, installer and defaultLocale manifest."},


Suggested change

{ AppInstaller::Manifest::ManifestError::IncompleteMultiFileManifest, "The multi file manifest is incomplete.A multi file manifest must contain at least version, installer and defaultLocale manifest."},

{ AppInstaller::Manifest::ManifestError::IncompleteMultiFileManifest, "The multi file manifest is incomplete. A multi file manifest must contain at least a version, installer, and defaultLocale manifest file."},

Trenly · 2022-08-03T18:48:35Z

src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp

+            { AppInstaller::Manifest::ManifestError::InconsistentMultiFileManifestFieldValue, "The multi file manifest has inconsistent field values." },
+            { AppInstaller::Manifest::ManifestError::DuplicateMultiFileManifestType, "The multi file manifest should contain only one file with the particular ManifestType." },
+            { AppInstaller::Manifest::ManifestError::DuplicateMultiFileManifestLocale, "The multi file manifest contains duplicate PackageLocale." },
+            { AppInstaller::Manifest::ManifestError::UnsupportedMultiFileManifestType, "The multi file manifest should not contain file with the particular ManifestType." },


Suggested change

{ AppInstaller::Manifest::ManifestError::UnsupportedMultiFileManifestType, "The multi file manifest should not contain file with the particular ManifestType." },

{ AppInstaller::Manifest::ManifestError::UnsupportedMultiFileManifestType, "The multi file manifest should not contain a file with the particular ManifestType." },

Trenly · 2022-08-03T18:50:03Z

src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp

+            { AppInstaller::Manifest::ManifestError::InstallerFailedToProcess, "Failed to process installer." },
+            { AppInstaller::Manifest::ManifestError::NoSupportedPlatforms, "No supported platforms." },
+            { AppInstaller::Manifest::ManifestError::ApproximateVersionNotAllowed, "Approximate version not allowed." },
+            { AppInstaller::Manifest::ManifestError::ArpVersionOverlapWithIndex, "DisplayVersion declared in the manifest has overlap with existing DisplayVersion range in the index.Existing DisplayVersion range in index : " },


Suggested change

{ AppInstaller::Manifest::ManifestError::ArpVersionOverlapWithIndex, "DisplayVersion declared in the manifest has overlap with existing DisplayVersion range in the index.Existing DisplayVersion range in index : " },

{ AppInstaller::Manifest::ManifestError::ArpVersionOverlapWithIndex, "DisplayVersion declared in the manifest has overlap with existing DisplayVersion range in the index. Existing DisplayVersion range in index : " },

src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp

… user/akalagbe/surface-dep-errors

JohnMcPMS · 2022-08-03T22:02:16Z

Note to other reviewers: The main goal of the change was to get the specifics of the errors out of the function, but I suggested that it be done this way in order to move 1 step closer to actually localizing these messages (and any/all others in common).

… user/akalagbe/surface-dep-errors

src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp

src/AppInstallerCommonCore/Public/winget/ManifestValidation.h

JohnMcPMS · 2022-08-10T20:19:45Z

src/AppInstallerRepositoryCore/PackageDependenciesValidationUtil.cpp

+
+            if (itr != dependenciesErrorMessageMap.end())
+            {
+                return itr->second;


Shouldn't we || together all of the errors that we find?

Dependencies error are mutually exclusive, Renamed the method name to properly convey its functionality.

I think || is more future proof

ghost · 2022-08-18T00:06:05Z

@hackean-msft this pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

JohnMcPMS

Removing my blocking status for now.

… user/akalagbe/surface-dep-errors

src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp

src/AppInstallerCommonCore/Public/winget/ManifestValidation.h

src/AppInstallerRepositoryCore/AppInstallerRepositoryCore.vcxproj.filters

src/AppInstallerRepositoryCore/PackageDependenciesValidation.cpp

src/AppInstallerRepositoryCore/PackageDependenciesValidationUtil.cpp

src/WinGetUtil/Exports.cpp

src/WinGetUtil/WinGetUtil.h

src/WinGetUtil/Exports.cpp

src/AppInstallerRepositoryCore/Rest/Schema/1_0/RestInterface_1_0.cpp

src/AppInstallerCommonCore/Public/winget/ManifestValidation.h

src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp

src/AppInstallerCLITests/PackageDependenciesValidationUtil.cpp

src/AppInstallerRepositoryCore/PackageDependenciesValidationUtil.h

src/AppInstallerRepositoryCore/PackageDependenciesValidation.cpp

src/AppInstallerRepositoryCore/ArpVersionValidation.cpp

yao-msft

🕐

… user/akalagbe/surface-dep-errors

src/AppInstallerRepositoryCore/PackageDependenciesValidationUtil.h

src/AppInstallerCommonCore/Public/winget/ManifestValidation.h

src/AppInstallerRepositoryCore/AppInstallerRepositoryCore.vcxproj.filters

src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp

Not blocking

Original issues resolved, others have reviewed.

Akinwale Alagbe added 2 commits August 2, 2022 20:00

Surface dependencies specific errors

9af9394

Updated flags result

51cd847

hackean-msft requested a review from a team as a code owner August 3, 2022 16:26

florelis reviewed Aug 3, 2022

View reviewed changes

Trenly reviewed Aug 3, 2022

View reviewed changes

Merge branch 'master' of https://github.com/microsoft/winget-cli into…

20fb279

… user/akalagbe/surface-dep-errors

Akinwale Alagbe added 7 commits August 5, 2022 15:28

Merge branch 'master' of https://github.com/microsoft/winget-cli into…

a74aaeb

… user/akalagbe/surface-dep-errors

Sort alphabetically

6f667f9

Return message ID when string mapping not found

3432d72

Addressed PR comments

9d48bb3

Renamed field to context

1ee72da

Proper name change

d87f729

Addressed PR comments

e7c3978

JohnMcPMS previously requested changes Aug 10, 2022

View reviewed changes

ghost added the Needs-Author-Feedback Issue needs attention from issue or PR author label Aug 10, 2022

ghost added the No-Recent-Activity Issue has no recent activity label Aug 18, 2022

JohnMcPMS reviewed Aug 18, 2022

View reviewed changes

ghost removed the No-Recent-Activity Issue has no recent activity label Aug 18, 2022

JohnMcPMS self-requested a review August 18, 2022 21:28

Akinwale Alagbe added 2 commits August 20, 2022 15:30

Addressed PR comments

7b46663

Merge branch 'master' of https://github.com/microsoft/winget-cli into…

ba86644

… user/akalagbe/surface-dep-errors

ghost removed the Needs-Author-Feedback Issue needs attention from issue or PR author label Aug 22, 2022

Change method name to reflect functionality.

d97ea44

florelis approved these changes Aug 23, 2022

View reviewed changes

yao-msft reviewed Aug 23, 2022

View reviewed changes

src/WinGetUtil/WinGetUtil.h Outdated Show resolved Hide resolved

yao-msft reviewed Aug 24, 2022

View reviewed changes

src/WinGetUtil/Exports.cpp Outdated Show resolved Hide resolved

yao-msft reviewed Aug 24, 2022

View reviewed changes

src/AppInstallerRepositoryCore/Rest/Schema/1_0/RestInterface_1_0.cpp Outdated Show resolved Hide resolved

yao-msft reviewed Aug 24, 2022

View reviewed changes

src/AppInstallerCommonCore/Public/winget/ManifestValidation.h Outdated Show resolved Hide resolved

yao-msft reviewed Aug 24, 2022

View reviewed changes

src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp Outdated Show resolved Hide resolved

yao-msft reviewed Aug 24, 2022

View reviewed changes

src/AppInstallerCLITests/PackageDependenciesValidationUtil.cpp Show resolved Hide resolved

yao-msft reviewed Aug 24, 2022

View reviewed changes

src/AppInstallerCLITests/PackageDependenciesValidationUtil.cpp Outdated Show resolved Hide resolved

yao-msft reviewed Aug 24, 2022

View reviewed changes

src/AppInstallerRepositoryCore/PackageDependenciesValidationUtil.h Outdated Show resolved Hide resolved

yao-msft reviewed Aug 24, 2022

View reviewed changes

src/AppInstallerRepositoryCore/PackageDependenciesValidation.cpp Outdated Show resolved Hide resolved

yao-msft reviewed Aug 24, 2022

View reviewed changes

src/AppInstallerRepositoryCore/ArpVersionValidation.cpp Outdated Show resolved Hide resolved

yao-msft previously requested changes Aug 24, 2022

View reviewed changes

ghost added the Needs-Author-Feedback Issue needs attention from issue or PR author label Aug 24, 2022

Akinwale Alagbe added 2 commits August 29, 2022 00:23

Merge branch 'master' of https://github.com/microsoft/winget-cli into…

0fc5742

… user/akalagbe/surface-dep-errors

Included excluded files.

fb8354f

ghost removed the Needs-Author-Feedback Issue needs attention from issue or PR author label Aug 29, 2022

Akinwale Alagbe added 3 commits August 29, 2022 10:45

Addressed PR comments

aa2c6d0

Spaces and indentation fix.

22fa639

Add capability to return multiple error result.

bcc4cd3

yao-msft reviewed Aug 29, 2022

View reviewed changes

src/AppInstallerRepositoryCore/PackageDependenciesValidationUtil.h Outdated Show resolved Hide resolved

Akinwale Alagbe added 2 commits August 29, 2022 16:42

code refactor

f069c23

Addressed pr comment

56717f6

yao-msft reviewed Sep 1, 2022

View reviewed changes

src/AppInstallerCommonCore/Public/winget/ManifestValidation.h Outdated Show resolved Hide resolved

yao-msft reviewed Sep 1, 2022

View reviewed changes

src/AppInstallerRepositoryCore/AppInstallerRepositoryCore.vcxproj.filters Outdated Show resolved Hide resolved

yao-msft reviewed Sep 2, 2022

View reviewed changes

src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp Outdated Show resolved Hide resolved

Akinwale Alagbe added 2 commits September 7, 2022 17:27

Addressed PR comments

96e9433

Addressed pr comment

70b01cc

yao-msft approved these changes Sep 8, 2022

View reviewed changes

JohnMcPMS removed their request for review September 13, 2022 18:28

hackean-msft merged commit 993de6b into microsoft:master Sep 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return error codes for dependencies failure #2410

Return error codes for dependencies failure #2410

hackean-msft commented Aug 3, 2022 •

edited by ghost

Loading

Trenly Aug 3, 2022

Trenly Aug 3, 2022

Trenly Aug 3, 2022

Trenly Aug 3, 2022

Trenly Aug 3, 2022

Trenly Aug 3, 2022

JohnMcPMS commented Aug 3, 2022

JohnMcPMS Aug 10, 2022

hackean-msft Aug 22, 2022

yao-msft Aug 24, 2022

ghost commented Aug 18, 2022

JohnMcPMS left a comment

yao-msft left a comment

	{ AppInstaller::Manifest::ManifestError::MultiManifestPackageHasDependencies, "Deleting the manifest will be break the following dependencies." },
	{ AppInstaller::Manifest::ManifestError::MultiManifestPackageHasDependencies, "Deleting the manifest will break the following dependencies." },

Return error codes for dependencies failure #2410

Return error codes for dependencies failure #2410

Conversation

hackean-msft commented Aug 3, 2022 • edited by ghost Loading

Microsoft Reviewers: Open in CodeFlow

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnMcPMS commented Aug 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Aug 18, 2022

JohnMcPMS left a comment

Choose a reason for hiding this comment

yao-msft left a comment

Choose a reason for hiding this comment

hackean-msft commented Aug 3, 2022 •

edited by ghost

Loading