Create WinGetUtil functionality for running installed package correlation #2221

JohnMcPMS · 2022-06-06T18:49:52Z

Change

The goal of this change is to expose functionality to run the installed package correlation and gather the results. This will be used to validate and enhance manifests and allows us to use the same correlation code throughout the system.

The biggest changes to existing code are:

Moving the JSON utility methods out of Repository and into Common
Moving the correlation code to be object oriented, enabling easier reuse (and allowing for more control in the future)

Beyond those changes, the majority of the work is in creating the JSON (de)serialization and handling updating the metadata that has been collected.

More work is still needed to:

Collect more information for diagnostic purposes, like the top results for diagnostics when there is no confident match.
Leveraging the previous metadata results to improve correlation.
Enable more control over weighting the correlation heuristics, enabling a difference between running on an end user system and in a "clean room" of validation.

Validation

Unit tests are added covering scenarios around updating the metadata with results.

Microsoft Reviewers: Open in CodeFlow

…ext to repository

…rsing; needs AppsAndFeatures exposed

…ntext

florelis

Still reading the InstallerMetadataCollectionContext...

florelis · 2022-06-07T16:58:55Z

.github/actions/spelling/allow.txt

@@ -109,6 +109,7 @@ denelon
 depersist
 deque
 deserialize
+deserializes
 Deserialize


We need both deserialize and Deserialize? Isn't it case insensitive?

florelis · 2022-06-07T18:22:28Z

src/AppInstallerCommonCore/AppInstallerCommonCore.vcxproj

-      <AdditionalIncludeDirectories Condition="'$(Configuration)|$(Platform)'=='Debug|ARM'">$(ProjectDir);$(ProjectDir)Public;$(ProjectDir)Telemetry;$(ProjectDir)..\binver;$(ProjectDir)..\YamlCppLib\libyaml\include;$(ProjectDir)..\JsonCppLib\json;$(ProjectDir)..\JsonCppLib;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
-      <AdditionalIncludeDirectories Condition="'$(Configuration)|$(Platform)'=='Debug|ARM64'">$(ProjectDir);$(ProjectDir)Public;$(ProjectDir)Telemetry;$(ProjectDir)..\binver;$(ProjectDir)..\YamlCppLib\libyaml\include;$(ProjectDir)..\JsonCppLib\json;$(ProjectDir)..\JsonCppLib;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
-      <AdditionalIncludeDirectories Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">$(ProjectDir);$(ProjectDir)Public;$(ProjectDir)Telemetry;$(ProjectDir)..\binver;$(ProjectDir)..\YamlCppLib\libyaml\include;$(ProjectDir)..\JsonCppLib\json;$(ProjectDir)..\JsonCppLib;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
+      <PreprocessorDefinitions>_NO_ASYNCRTIMP;_DEBUG;%(PreprocessorDefinitions);CLICOREDLLBUILD</PreprocessorDefinitions>


What does _NO_ASYNCRTIMP do?

It does this in cpprest_compat.h:

#ifdef _NO_ASYNCRTIMP #define _ASYNCRTIMP #else // ^^^ _NO_ASYNCRTIMP ^^^ // vvv !_NO_ASYNCRTIMP vvv #ifdef _ASYNCRT_EXPORT #define _ASYNCRTIMP __declspec(dllexport) #else // ^^^ _ASYNCRT_EXPORT ^^^ // vvv !_ASYNCRT_EXPORT vvv #define _ASYNCRTIMP __declspec(dllimport) #endif // _ASYNCRT_EXPORT #endif // _NO_ASYNCRTIMP

Given that it means we don't import or export the functions tagged with this, we must be compiling them in.

florelis · 2022-06-07T23:50:56Z

src/AppInstallerRepositoryCore/Public/winget/InstallerMetadataCollectionContext.h

+        void FromJson(const web::json::value& json);
+
+        // Create a JSON value for the metadata using the given schema version.
+        web::json::value ToJson(const Utility::Version& version, size_t maximumSizeInBytes);


Nit: Maybe rename version -> schemaVersion? On a first reading I understood it as converting the version to JSON

florelis · 2022-06-07T23:59:50Z

src/AppInstallerRepositoryCore/InstallerMetadataCollectionContext.cpp

+            return AddIfNotPresentAndNotEmpty(strings, strings, string);
+        }
+
+        void AddIfNotPresent(std::vector<std::string>& strings, std::vector<std::string>& filter, const std::vector<std::string>& inputs)


If this is a common operation, why not use a set instead of a vector?

florelis · 2022-06-09T21:45:12Z

src/AppInstallerCommonCore/Public/AppInstallerDownloader.h

@@ -22,6 +22,7 @@ namespace AppInstaller::Utility
        Manifest,
        WinGetUtil,
        Installer,
+        InstallerMetadataCollectionInput,


Seems like the only place where this is used is in the DO download to decide whether to use DO? I saw a comment there explaining why the other types don't use DO. Can you add this one?

florelis · 2022-06-09T21:46:53Z

src/AppInstallerCommonCore/Public/winget/JsonUtil.h

+
+namespace AppInstaller::JSON
+{
+    // For JSON CPP LIb


florelis · 2022-06-09T21:48:58Z

src/AppInstallerCommonCore/Public/winget/JsonUtil.h

+
+    web::json::value GetStringValue(std::string_view value);
+
+    std::vector<std::string> GetRawStringArrayFromJsonNode(const web::json::value& node, const utility::string_t& keyName);


Nit: Could this be higher closer to the other GetRaw... functions?

florelis · 2022-06-09T21:49:29Z

src/AppInstallerCommonCore/JsonUtil.cpp


+    utility::string_t GetUtilityString(std::string_view nodeName)


Nit: Why Get instead of To?

I think it's moved from rest helpers and there're a lot of places calling

Indeed, I only moved these and did not attempt to rename them.

florelis · 2022-06-09T22:27:45Z

src/AppInstallerRepositoryCore/InstallerMetadataCollectionContext.cpp

+            if (success)
+            {
+                break;
+            }


Does breaking instead of setting success = true not work?

Changed it in the code I copied from as well.

florelis · 2022-06-09T22:32:45Z

src/AppInstallerRepositoryCore/InstallerMetadataCollectionContext.cpp

+        }
+        catch (...)
+        {
+            CollectErrorDataFromException(std::current_exception());


This is cool. I didn't know std::current_exception() existed.

It's great for making standardized exception handlers.

yao-msft · 2022-06-13T22:33:38Z

src/WinGetUtil/WinGetUtil.h

@@ -129,4 +129,46 @@ extern "C"
        WINGET_STRING versionA,
        WINGET_STRING versionB,
        INT* comparisonResult);
+
+    // A handle to the metadata collection object.
+    typedef void* WINGET_INSTALLER_METADATA_COLLECTION_HANDLE;


typedef void* WINGET_INSTALLER_METADATA_COLLECTION_HANDLE;

nit: should we put all typedefs and enums in the top?

Should we? I do like things being defined close to where they are used. If the type is being used in a broad fashion, the top makes sense to me. But if it is used narrowly, then immediately above the use feels better.

yao-msft · 2022-06-14T01:13:14Z

src/AppInstallerCommonCore/Public/winget/JsonUtil.h

@@ -0,0 +1,59 @@
+#pragma once


nit: remove

yao-msft · 2022-06-14T01:37:47Z

src/AppInstallerRepositoryCore/InstallerMetadataCollectionContext.cpp

+            utility::string_t InstallerHash = L"installerHash";
+            utility::string_t SubmissionIdentifier = L"submissionIdentifier";
+            utility::string_t Version = L"version";
+            utility::string_t AppsAndFeaturesEntries = L"AppsAndFeaturesEntries";


A

Is it intentional to have some values start with lower case and some values start with upper case?

It is in that those that start with upper case are the same names as from the manifest JSON schema.

yao-msft

yao-msft · 2022-06-14T02:14:54Z

src/AppInstallerRepositoryCore/InstallerMetadataCollectionContext.cpp

+        OutputStatus statusToUse = m_outputStatus;
+        if (statusToUse != OutputStatus::Success && statusToUse != OutputStatus::Error && statusToUse != OutputStatus::LowConfidence)
+        {
+            statusToUse = OutputStatus::Unknown;


statusToUse = OutputStatus::Unknown;

nit: should this be initialized to unknown as default and override if known

yao-msft · 2022-06-14T02:26:21Z

src/AppInstallerRepositoryCore/InstallerMetadataCollectionContext.cpp

+            }
+            newEntry.Publisher = package->GetProperty(PackageVersionProperty::Publisher).get();
+            // TODO: Support upgrade code throughout the code base...
+            //newEntry.UpgradeCode;


//newEntry.UpgradeCode;

nit: remove?

yao-msft · 2022-06-14T02:28:40Z

src/AppInstallerCLITests/InstallerMetadataCollectionContext.cpp

+    {
+        TestInput() = default;
+
+        TestInput(MinimalDefaults_t)


TestInput

Should we have an input and output test to showcase what the input json and output json looks like?

Do you mean a test case with a literal string input and verification as a string output looking for literal values?

github-actions · 2022-06-17T05:14:19Z

@check-spelling-bot Report

Unrecognized words, please review:

metadatas
verision

Previously acknowledged words that are now absent

activatable amd Archs dsc enr FWW Globals hackathon lww mytool OSVERSION Packagedx parametermap symlink Uninitialize WDAG whatif wsb

To accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands

... in a clone of the git@github.com:JohnMcPMS/winget-cli.git repository
on the metacollect branch:

update_files() {
perl -e '
my @expect_files=qw('".github/actions/spelling/expect.txt"');
@ARGV=@expect_files;
my @stale=qw('"$patch_remove"');
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);
use File::Basename qw(dirname);
make_path (dirname($new_expect_file));
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"$patch_add"');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a)."-".$a cmp lc($b)."-".$b} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;
system("git", "add", $new_expect_file);
'
}

comment_json=$(mktemp)
curl -L -s -S \
  --header "Content-Type: application/json" \
  "https://api.github.com/repos/microsoft/winget-cli/issues/comments/1158498292" > "$comment_json"
comment_body=$(mktemp)
jq -r .body < "$comment_json" > $comment_body
rm $comment_json

patch_remove=$(perl -ne 'next unless s{^</summary>(.*)</details>$}{$1}; print' < "$comment_body")
  

patch_add=$(perl -e '$/=undef;
$_=<>;
s{<details>.*}{}s;
s{^#.*}{};
s{\n##.*}{};
s{(?:^|\n)\s*\*}{}g;
s{\s+}{ }g;
print' < "$comment_body")
  
update_files
rm $comment_body
git add -u

github-actions · 2022-06-22T03:05:14Z

@check-spelling-bot Report

Unrecognized words, please review:

metadatas
verision

Previously acknowledged words that are now absent

activatable amd Archs dsc enr FWW Globals hackathon lww mytool OSVERSION Packagedx parametermap symlink Uninitialize WDAG whatif wsb

To accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands

... in a clone of the git@github.com:JohnMcPMS/winget-cli.git repository
on the metacollect branch:

update_files() {
perl -e '
my @expect_files=qw('".github/actions/spelling/expect.txt"');
@ARGV=@expect_files;
my @stale=qw('"$patch_remove"');
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);
use File::Basename qw(dirname);
make_path (dirname($new_expect_file));
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"$patch_add"');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a)."-".$a cmp lc($b)."-".$b} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;
system("git", "add", $new_expect_file);
'
}

comment_json=$(mktemp)
curl -L -s -S \
  --header "Content-Type: application/json" \
  "https://api.github.com/repos/microsoft/winget-cli/issues/comments/1162583042" > "$comment_json"
comment_body=$(mktemp)
jq -r .body < "$comment_json" > $comment_body
rm $comment_json

patch_remove=$(perl -ne 'next unless s{^</summary>(.*)</details>$}{$1}; print' < "$comment_body")
  

patch_add=$(perl -e '$/=undef;
$_=<>;
s{<details>.*}{}s;
s{^#.*}{};
s{\n##.*}{};
s{(?:^|\n)\s*\*}{}g;
s{\s+}{ }g;
print' < "$comment_body")
  
update_files
rm $comment_body
git add -u

JohnMcPMS added 17 commits May 12, 2022 17:27

Initial interface and runtime setup for API

24bbb4e

Most of the work of moving the cpprestsdk JSON helpers into common

409e0c0

Moving of JSON helpers compiles

9141bcc

Needs refactoring to expose JSON manifest parsing

ec7cb5e

Created public exposure of json parsing; need to move collection cont…

8baa220

…ext to repository

Move metadata collector to repository; rename blob; implement more pa…

5b0dcf8

…rsing; needs AppsAndFeatures exposed

Make AppsAndFeaturesEntries parsable from outside

5d09217

Part way through refactoring the correlation data to be more reusable

250442a

Basic refactor to make the ARP correlation path directly reusable

dbf58e2

Merge from master

cd7d437

Output code complete, capstone is to update with the new metadata

4345add

Code complete of initial version

6cf5105

A few minor tests and the method to enable detailed testing of the co…

851770c

…ntext

Beginnings of full tests

397368b

Test progress

113d32e

Initial test debugging and more implementation

0fe79e0

Finish unit tests

4f8de0b

JohnMcPMS requested a review from a team as a code owner June 6, 2022 18:49

florelis reviewed Jun 9, 2022

View reviewed changes

Add new exports to def

7362deb

yao-msft reviewed Jun 13, 2022

View reviewed changes

yao-msft reviewed Jun 14, 2022

View reviewed changes

src/AppInstallerCommonCore/Public/winget/JsonUtil.h Outdated

@@ -0,0 +1,59 @@

#pragma once

Copy link

Contributor

yao-msft Jun 14, 2022

Choose a reason for hiding this comment

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

nit: remove

yao-msft reviewed Jun 14, 2022

View reviewed changes

yao-msft approved these changes Jun 14, 2022

View reviewed changes

yao-msft reviewed Jun 14, 2022

View reviewed changes

JohnMcPMS added 3 commits June 16, 2022 19:38

Add merge function prototype

2ccf806

Merge from master

06992e8

PR feedback

b95fdfa

Begin implementation of the merge function

1a07763

Update to new design, adding Merge function

3411c1c

JohnMcPMS added 2 commits June 22, 2022 13:13

Spellin

4335883

Merge from master

11cfe73

JohnMcPMS merged commit 9f8ceea into microsoft:master Jun 24, 2022

JohnMcPMS deleted the metacollect branch June 24, 2022 17:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create WinGetUtil functionality for running installed package correlation #2221

Create WinGetUtil functionality for running installed package correlation #2221

JohnMcPMS commented Jun 6, 2022 •

edited by ghost

Loading

florelis left a comment

florelis Jun 7, 2022

florelis Jun 7, 2022

JohnMcPMS Jun 17, 2022

florelis Jun 7, 2022

florelis Jun 7, 2022

florelis Jun 9, 2022

florelis Jun 9, 2022

florelis Jun 9, 2022

florelis Jun 9, 2022

yao-msft Jun 14, 2022

JohnMcPMS Jun 17, 2022

florelis Jun 9, 2022

JohnMcPMS Jun 17, 2022

florelis Jun 9, 2022

JohnMcPMS Jun 17, 2022

yao-msft Jun 13, 2022 •

edited

Loading

JohnMcPMS Jun 17, 2022

yao-msft Jun 14, 2022

yao-msft Jun 14, 2022

JohnMcPMS Jun 17, 2022

yao-msft left a comment

yao-msft Jun 14, 2022

yao-msft Jun 14, 2022

yao-msft Jun 14, 2022

JohnMcPMS Jun 17, 2022

github-actions bot commented Jun 17, 2022 •

edited

Loading

github-actions bot commented Jun 22, 2022 •

edited

Loading


		web::json::value GetStringValue(std::string_view value);

		std::vector<std::string> GetRawStringArrayFromJsonNode(const web::json::value& node, const utility::string_t& keyName);


		utility::string_t GetUtilityString(std::string_view nodeName)

Create WinGetUtil functionality for running installed package correlation #2221

Create WinGetUtil functionality for running installed package correlation #2221

Conversation

JohnMcPMS commented Jun 6, 2022 • edited by ghost Loading

Change

Validation

Microsoft Reviewers: Open in CodeFlow

florelis left a 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

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

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

yao-msft Jun 13, 2022 • edited Loading

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

yao-msft left a 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

github-actions bot commented Jun 17, 2022 • edited Loading

@check-spelling-bot Report

Unrecognized words, please review:

github-actions bot commented Jun 22, 2022 • edited Loading

@check-spelling-bot Report

Unrecognized words, please review:

JohnMcPMS commented Jun 6, 2022 •

edited by ghost

Loading

yao-msft Jun 13, 2022 •

edited

Loading

github-actions bot commented Jun 17, 2022 •

edited

Loading

github-actions bot commented Jun 22, 2022 •

edited

Loading