Implement V1 manifest and schema validation by yao-msft · Pull Request #740 · microsoft/winget-cli

yao-msft · 2021-02-02T20:43:24Z

Change

Implemented schema validation using Valijson
Added manifest schemas for preview and v1 manifests
Changed Manifest object model to better reflect v1 manifests
Changed ManifestLocalization to be std::map based for easier future locale processing
Implemented merge logic for multiple file manifests

Validation

Existing tests and added new tests
Validated all 3000+ manifests currently in winget-pkgs passed manifest validation

Note: After this change, we might need to revisit and relax restrictions in some of v1 manifest schemas as I found around 300+ manifests in current winget-pkgs will fail in new restrictions imposed by V1 manifest schemas

Microsoft Reviewers: Open in CodeFlow

JohnMcPMS · 2021-02-05T23:43:25Z

-            !Runtime::IsCurrentOSVersionGreaterThanOrEqual(Version(manifest.MinOSVersion)))
-        {
-            context.Reporter.Error() << Resource::String::InstallationRequiresHigherWindows << ' ' << manifest.MinOSVersion << std::endl;
-            AICLI_TERMINATE_CONTEXT(HRESULT_FROM_WIN32(ERROR_OLD_WIN_VERSION));


I understand the motivation to remove this, but it does mean that we will not message the reason for failure to the user (or ourselves for the failure code). Maybe it is upcoming in my reading of the PR, but we need a way to message the failure state of no applicable installers. #Resolved

Yes, the MinOSVersion check is moved to installer selection logic since in the new world each installer has its own MinOSVersion. It will be part of the No_Applicable_Installer error
#Resolved

But I think we need to do a better job of messaging the reason that there are not applicable installers. Some of that burden could be placed on search to filter them out before we even attempt to install, but I prefer to show everything and have the user be informed that we don't have their flavor of installer rather than we don't even know what they are talking about.

It might be more complex, and thus not piled on to this PR, but I think we should attempt to give some reason as to why there are no applicable installers. Imagine:

for each installer reason[i] = GetAllReasonsWhyNotApplicable(installer) if (AndAllReasons() != 0) ReportReasons(AndAllReasons())

So if all installers require a newer OS, we say that. If they are all ARM, we can say that they are for a different processor architecture. If one required a newer OS and one is ARM, we say both somehow.

In reply to: 571311595 [](ancestors = 571311595)

I'll add a todo for better message regarding installer selection

In reply to: 571318031 [](ancestors = 571318031,571311595)

JohnMcPMS · 2021-02-05T23:49:11Z

+            if (isOSVersionSatisfied1 && !isOSVersionSatisfied2)
+            {
+                return true;
+            }


Let's re-write the loop to check IsInstallerApplicable first before ever calling IsInstallerBetterMatch. That way we don't need to duplicate these kinds of boolean checks. It ends up making this function just the arch > arch comparison. Not sure why I didn't think of it when I rewrote this a while back. #Resolved

JohnMcPMS · 2021-02-05T23:54:02Z

    {
        const auto& manifest = context.Get<Execution::Data::Manifest>();

+        // Channel is moved to installer level, get all channels from all installers


// Channel is moved to installer level, get all channels from all installers [](start = 8, length = 76)

Channel is not an installer level item. It is a part of the version. If the schema has this change, I obviously missed it in review. #Resolved

JohnMcPMS · 2021-02-06T00:02:51Z


        context <<
-            VerifyFile(Execution::Args::Type::Manifest) <<
+            VerifyPath(Execution::Args::Type::Manifest) <<


VerifyPath [](start = 12, length = 10)

This one does not seem like it should have changed, winget install -m a_directory will now work? #Resolved

It will work if the directory contains only one manifest(either split manifest files or a singleton inside). The input will be checked in ValidateInput method in manifest parsing

In reply to: 571315313 [](ancestors = 571315313)

JohnMcPMS · 2021-02-06T00:04:30Z

            var result = TestCommon.RunAICLICommand("hash", TestCommon.GetTestDataFile("DoesNot.Exist"));
            Assert.AreEqual(Constants.ErrorCode.ERROR_FILE_NOT_FOUND, result.ExitCode);
-            Assert.True(result.StdOut.Contains("File does not exist"));
+            Assert.True(result.StdOut.Contains("Path does not exist"));


Path [](start = 48, length = 4)

This one should not have changed. #Resolved

JohnMcPMS · 2021-02-06T00:17:19Z

@@ -0,0 +1,23 @@
+PackageIdentifier: microsoft.msixsdk


These split manifests should be in a directory for organization and testing. Was it complicating the copy on build? #Resolved

JohnMcPMS · 2021-02-06T00:29:21Z

+        }
+    }
+
+    std::string LoadResourceAsString(PCWSTR resourceModuleName, PCWSTR resourceName, PCWSTR resourceType)


PCWSTR [](start = 37, length = 6)

std::wstring_view #Resolved

Although I suppose that doesn't require null termination, so it is suspect. We should probably add a utility type that can handle this for us.

In reply to: 571321711 [](ancestors = 571321711)

per offline chat, we decided to remove the resourceModuleName parameter, leave the other 2 PCWSTR as they are because that's what MAKEINTRESOURCE produces and later consumed by resource related apis

In reply to: 571322386 [](ancestors = 571322386,571321711)

JohnMcPMS · 2021-02-06T00:33:49Z

+
+    std::string LoadResourceAsString(PCWSTR resourceModuleName, PCWSTR resourceName, PCWSTR resourceType)
+    {
+        HMODULE resourceModule = GetModuleHandle(resourceModuleName);


GetModuleHandle [](start = 33, length = 15)

If you are going to allow arbitrary modules here, you should use the proper, thread safe GetModuleHandleEx and a wil::unique_hmodule to hold it. #Resolved

yao-msft · 2021-02-06T00:43:00Z

+        //   - DefaultLocale matches in version manifest and defaultLocale manifest
+        // - Validate manifest type correctness
+        //   - Allowed file type in multi file manifest: version, installer, defaultLocale, locale
+        //   - Allowed file type in multi file manifest: preview manifest, merged and singleton


multi [](start = 36, length = 5)

single #Resolved

JohnMcPMS · 2021-02-06T00:54:08Z

+        template <Localization L>
+        struct LocalizationMapping
+        {
+            // value_t type specifies the type of this data


// value_t type specifies the type of this data [](start = 12, length = 47)

Since almost everything is going to be string_t, you could define it here in the unspecialized definition. Then one only needs to specify the very few exceptions to the rule (1 right now). #Resolved

JohnMcPMS · 2021-02-06T01:00:54Z

+    // fullValidation: Bool to set if manifest creation should perform extra validation that client does not need.
+    //                 e.g. Channel should be null. Client code does not need this check to work properly.
+    // throwOnWarning: Bool to indicate if an exception should be thrown with only warnings detected in the manifest.
+    // resourceDll:    Binary where schemas are embedded, or nullptr if they are embedded in the binary that created the process


// resourceDll: Binary where schemas are embedded, or nullptr if they are embedded in the binary that created the process [](start = 4, length = 124)

This seems like it is only marginally useful for some negative test case. Isn't it possible to determine the module that the current code is executing in? #Resolved

It's mainly for WinGetUtil.dll, I cannot find a simple way to detect the code is being called by wingetutil(or you prefer a bool like isFromWinGetUtil)?

In reply to: 571327922 [](ancestors = 571327922)

Either use GetModuleHandleEx or https://devblogs.microsoft.com/oldnewthing/20041025-00

In reply to: 571330356 [](ancestors = 571330356,571327922)

JohnMcPMS · 2021-02-06T01:08:13Z

@@ -76,9 +76,11 @@ extern "C"
    // Validates a given manifest. Returns a bool for validation result and
    // a string representing validation errors if validation failed.
    WINGET_UTIL_API WinGetValidateManifest(


WinGetValidateManifest [](start = 20, length = 22)

This will need to be a new function and the old one will need to continue to work with it's existing signature. #Resolved

JohnMcPMS · 2021-02-06T01:12:46Z

-        WINGET_STRING_OUT* message) try
+        WINGET_STRING_OUT* message,
+        WINGET_STRING mergedManifestPath,
+        BOOL isPartialManifest) try


isPartialManifest [](start = 13, length = 17)

We need to know the expected type, or return it, or the service code will need to parse the manifest enough to answer the question of "Does this file name pattern match the type in the manifest?".

Rather than a BOOL here, we should take in an enum with values for single file, split, sub-component of a split. It could even be an in-out where we also let them give us "don't know" and we give them back the answer. #Resolved

JohnMcPMS

🕐

github-actions · 2021-02-17T09:15:12Z

Misspellings found, please review:

Defaut
validators

To accept these changes, run the following commands from this repository on this branch

pushd $(git rev-parse --show-toplevel)
perl -e '
my @expect_files=qw('".github/actions/spelling/expect.txt"');
@ARGV=@expect_files;
my @stale=qw('"validator valijson valueiterator "');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
  if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
  next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect.txt";
use File::Path qw(make_path);
make_path ".github/actions/spelling";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"Defaut validators "');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a) cmp lc($b)} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;'
popd

JohnMcPMS

yao-msft added 10 commits January 23, 2021 18:46

vcxitems for valijson

6ea8695

vcxitems for schemas

6a598a9

initial check

b2e14f7

second check

1d73ba0

Working snapshot

571de43

fix

9bcd117

Preview compatibility fix

7550362

V2 compatibility fix

28e21bf

Working snapshot 2

20a72b5

new tests

e4595e0

yao-msft requested a review from a team as a code owner February 2, 2021 20:43

yao-msft added 5 commits February 2, 2021 13:56

Release build and spell checking fix

77b40df

More spell check fix

42c6b10

Fix test failure

16c1cfd

Add more tests and e2e test fix and add some comments

c9b16aa

Update e2e test expected message

cd13a06

JohnMcPMS reviewed Feb 5, 2021

View reviewed changes

JohnMcPMS reviewed Feb 6, 2021

View reviewed changes

yao-msft commented Feb 6, 2021

View reviewed changes

JohnMcPMS reviewed Feb 6, 2021

View reviewed changes

JohnMcPMS requested changes Feb 6, 2021

View reviewed changes

ghost added Needs-Author-Feedback Issue needs attention from issue or PR author and removed Needs-Author-Feedback Issue needs attention from issue or PR author labels Feb 6, 2021

yao-msft added 3 commits February 17, 2021 00:17

PR feedback

2f805d9

Merge remote-tracking branch 'upstream/master' into schemavalidation

29e1adb

Move manifest schemas under <root>/schemas/JSON folder

a237be1

spelling

b5b8d74

JohnMcPMS approved these changes Feb 17, 2021

View reviewed changes

yao-msft merged commit cca8545 into microsoft:master Feb 17, 2021

yao-msft deleted the schemavalidation branch February 17, 2021 21:33

Uh oh!

Conversation

yao-msft commented Feb 2, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change

Validation

Microsoft Reviewers: Open in CodeFlow

Uh oh!

JohnMcPMS Feb 5, 2021 • edited by yao-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yao-msft Feb 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JohnMcPMS Feb 6, 2021

Choose a reason for hiding this comment

Uh oh!

yao-msft Feb 6, 2021

Choose a reason for hiding this comment

Uh oh!

JohnMcPMS Feb 5, 2021 • edited by yao-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JohnMcPMS Feb 5, 2021 • edited by yao-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JohnMcPMS Feb 6, 2021 • edited by yao-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yao-msft Feb 6, 2021

Choose a reason for hiding this comment

Uh oh!

JohnMcPMS Feb 6, 2021 • edited by yao-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JohnMcPMS Feb 6, 2021 • edited by yao-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JohnMcPMS Feb 6, 2021 • edited by yao-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JohnMcPMS Feb 6, 2021

Choose a reason for hiding this comment

Uh oh!

yao-msft Feb 17, 2021

Choose a reason for hiding this comment

Uh oh!

JohnMcPMS Feb 6, 2021 • edited by yao-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yao-msft Feb 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JohnMcPMS Feb 6, 2021 • edited by yao-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JohnMcPMS Feb 6, 2021 • edited by yao-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yao-msft Feb 6, 2021

Choose a reason for hiding this comment

Uh oh!

JohnMcPMS Feb 6, 2021

Choose a reason for hiding this comment

Uh oh!

JohnMcPMS Feb 6, 2021 • edited by yao-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

yao-msft commented Feb 2, 2021 •

edited by ghost

Loading

JohnMcPMS Feb 5, 2021 •

edited by yao-msft

Loading

yao-msft Feb 5, 2021 •

edited

Loading

JohnMcPMS Feb 5, 2021 •

edited by yao-msft

Loading

JohnMcPMS Feb 5, 2021 •

edited by yao-msft

Loading

JohnMcPMS Feb 6, 2021 •

edited by yao-msft

Loading

JohnMcPMS Feb 6, 2021 •

edited by yao-msft

Loading

JohnMcPMS Feb 6, 2021 •

edited by yao-msft

Loading

JohnMcPMS Feb 6, 2021 •

edited by yao-msft

Loading

JohnMcPMS Feb 6, 2021 •

edited by yao-msft

Loading

yao-msft Feb 6, 2021 •

edited

Loading

JohnMcPMS Feb 6, 2021 •

edited by yao-msft

Loading

JohnMcPMS Feb 6, 2021 •

edited by yao-msft

Loading

JohnMcPMS Feb 6, 2021 •

edited by yao-msft

Loading

JohnMcPMS Feb 6, 2021 •

edited by yao-msft

Loading