Implement store app installation from manifest #493

yao-msft · 2020-07-13T18:37:24Z

This is the first part of #117

Change

Refactor manifest code to separate parsing logic from object model
Added ProductId and MSStore InstallerType and related validation
Added Store app installation code from a manifest

Validation

Existing tests passed
Added new tests to Manifest and Workflow
Manually tested on a VM and make sure the UI behaves as expected

Microsoft Reviewers: Open in CodeFlow

JohnMcPMS · 2020-07-13T22:09:56Z

src/AppInstallerCLICore/ExecutionProgress.cpp

@@ -192,7 +192,6 @@ namespace AppInstaller::CLI::Execution
        for (size_t i = 0; !m_canceled; ++i)
        {
            constexpr size_t repetitionCount = 20;
-            ApplyStyle(i % repetitionCount, repetitionCount, true);


ApplyStyle(i % repetitionCount, repetitionCount, true); [](start = 12, length = 55)

Why has this been removed? Seems unrelated. #Closed

When I run locally to verify the UI behavior, the spinner applies the theme color, but on exit, it's not reverting to default. And this method is called NoVT, so I thought we left it here by accident?

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

No style is applied in the NoVT case. If you are seeing that happening, it is due to a different bug.

In reply to: 453998322 [](ancestors = 453998322,453968675)

JohnMcPMS · 2020-07-13T22:55:06Z

src/AppInstallerCLICore/Workflows/InstallFlow.cpp

+        if (installer.InstallerType == ManifestInstaller::InstallerTypeEnum::MSStore)
+        {
+            return;
+        }


I would rather change how this is decided to be run than to start building early outs into every function. Probably better to move calling it out of the base flow and into DownloadInstaller where it is needed. #Closed

JohnMcPMS · 2020-07-13T22:55:58Z

void ShowInstallationDisclaimer(Execution::Context& context)

This might be different for Store. Should investigate more. #Closed

Refers to: src/AppInstallerCLICore/Workflows/InstallFlow.cpp:41 in ae95b7f. [](commit_id = ae95b7f, deletion_comment = False)

JohnMcPMS · 2020-07-13T23:12:39Z

src/AppInstallerCLICore/Workflows/InstallFlow.cpp

+        // Feature toggle check
+        if (!Settings::ExperimentalFeature::IsEnabled(Settings::ExperimentalFeature::Feature::ExperimentalMSStore))
+        {
+            context.Reporter.Error() << Resource::String::FeatureDisabledMessage << std::endl;


context.Reporter.Error() << Resource::String::FeatureDisabledMessage << std::endl; [](start = 12, length = 82)

This is typically coupled with the string that indicates which feature needs to be enabled. I would suggest creating a Workflow object that can handle this generically. So when invoking this from ExecuteInstaller:

context <<
EnsureFeatureEnabled(Settings::ExperimentalFeature::Feature::ExperimentalMSStore) <<
MSStoreInstall;

That way it can be reused easily. #Closed

JohnMcPMS · 2020-07-13T23:16:40Z

src/AppInstallerCLICore/Workflows/InstallFlow.cpp

+
+        // Policy check
+        AppInstallManager installManager;
+        if (installManager.IsStoreBlockedByPolicyAsync(s_StoreClientName, s_StoreClientPublisher).get())


IsStoreBlockedByPolicyAsync [](start = 27, length = 27)

Given the name of this function, I would expect that it already knows the Store's identity and that I wouldn't need to provide it. #Closed

Yeah, unfortunately this is how the api is supposed to be called. Maybe they are thinking about multiple stores when they design this API

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

JohnMcPMS · 2020-07-13T23:30:20Z

src/AppInstallerCLICore/Workflows/InstallFlow.cpp

+            winrt::hstring(),
+            nullptr).get();
+
+        auto installItem = installItems.GetAt(0);


auto installItem = installItems.GetAt(0); [](start = 8, length = 41)

Do we not need to handle all of the items? A comment is probably required here. #Closed

From my reading of the docs, if the target is a bundle this will have every package. I think we do need to watch all of them.

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

JohnMcPMS · 2020-07-14T00:05:15Z

src/AppInstallerCLICore/Workflows/InstallFlow.cpp

+
+        context.Reporter.ShowIndefiniteProgress(false);
+
+        context.Reporter.ExecuteWithProgress(std::bind(


ExecuteWithProgress [](start = 25, length = 19)

ExecuteWithProgress handles the "show indefinite until progress" if you don't send any progress. I think you can roll the above while into the lower loop to achieve the same (don't call OnProgress until PercentComplete is > 0. #Closed

That's convenient. I'll clean it up

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

JohnMcPMS · 2020-07-14T00:09:36Z

src/AppInstallerCLICore/Workflows/InstallFlow.cpp

+
+        context.Reporter.ShowIndefiniteProgress(false);
+
+        context.Reporter.ExecuteWithProgress(std::bind(


std::bind [](start = 45, length = 9)

You don't need to bind; the whole point of that is to create a callable with IProgressCallback& progress as its only parameter. #Closed

JohnMcPMS · 2020-07-14T00:10:57Z

src/AppInstallerCLICore/Workflows/InstallFlow.cpp

+                    {
+                        installItem.Cancel();
+                    }
+                    Sleep(500);


Sleep(500); [](start = 20, length = 11)

I would suggest sleeping more like 100ms, otherwise cancellation appears to be sluggish. #Closed

JohnMcPMS · 2020-07-14T00:26:03Z

src/AppInstallerRepositoryCore/Manifest/YamlManifestParser.h

+    static const ManifestVer PreviewManifestVersion = ManifestVer("0.1.0", false);
+    static const ManifestVer PreviewManifestVersionV2 = ManifestVer("0.2.0", false);
+
+    struct YamlManifestParser


YamlManifestParser [](start = 11, length = 18)

Can we drop Manifest from so many names since we are in the Manifest namespace?

Manifest::YamlParser::Create
Manifest::YamlParser::CreateFromPath

You don't need to have names repeated if you don't using namespace the previous names out. #Closed

Sure

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

JohnMcPMS · 2020-07-14T00:27:24Z

src/AppInstallerRepositoryCore/Manifest/YamlManifestParser.h

+        AppInstaller::Manifest::Manifest* m_p_manifest;
+        AppInstaller::Manifest::ManifestInstaller* m_p_installer;
+        std::map<ManifestInstaller::InstallerSwitchType, Utility::NormalizedString>* m_p_switches;
+        AppInstaller::Manifest::ManifestLocalization* m_p_localization;


Initialize all with nullptr. #Closed

JohnMcPMS · 2020-07-14T00:47:39Z

src/AppInstallerRepositoryCore/Manifest/YamlManifestParser.cpp

+        RootFieldInfos =
+        {
+            { "ManifestVersion", PreviewManifestVersion, [this](const YAML::Node&) { /* ManifestVersion already processed */ }, false,
+            // Regex here is to prevent leading 0s in the version, this also keeps consistent with other versions in the manifest


Regex [](start = 15, length = 5)

Moving the regex seems to be too much. If we are separating the parser, it should be to have it parse the syntax of the format, and then we should add these semantics onto that.

Parser > Raw Manifest > Validation

Either that or the parser gets an interface to expose selection of individual values out via some syntax we define, and the top level parser gets passed a syntactic parser to provide that.

Basically, if we are separating the code out, it should be written to enable any syntax. #Closed

Chatted offline and leave it for now.

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

JohnMcPMS · 2020-07-14T00:59:11Z

src/AppInstallerRepositoryCore/Manifest/ManifestValidation.cpp

+            {
+                // MSStore type is not supported in community repo
+                resultErrors.emplace_back(
+                    ManifestError::FieldNotSupported, "InstallerType",


ManifestError::FieldNotSupported, "InstallerType", [](start = 20, length = 50)

Probably need ValueNotSupported so that it can be more clear that it isn't InstallerType, but it's value that is bad. #Closed

yao-msft · 2020-07-14T00:59:36Z

src/AppInstallerRepositoryCore/Manifest/ManifestValidation.cpp

-                    errors.emplace_back(ManifestError::FieldUnknown, key, "", keyValuePair.first.Mark().line, keyValuePair.first.Mark().column, ValidationError::Level::Warning);
+                    resultErrors.emplace_back(ManifestError::RequiredFieldMissing, "Sha256");
+                }
+                // ProductId should be used


should be [](start = 29, length = 9)

should not be used #Closed

yao-msft · 2020-07-14T20:10:48Z

src/AppInstallerRepositoryCore/Manifest/YamlManifestParser.h

+{
+    static const uint64_t MaxSupportedMajorVersion = 1;
+    static const ManifestVer PreviewManifestVersion = ManifestVer("0.1.0", false);
+    static const ManifestVer PreviewManifestVersionV2 = ManifestVer("0.2.0", false);


"0.2.0" [](start = 68, length = 7)

Per discussion, this should be 0.2.0-msstore #Closed

yao-msft · 2020-07-14T23:08:01Z

void ShowInstallationDisclaimer(Execution::Context& context)

Added placeholder message

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

Refers to: src/AppInstallerCLICore/Workflows/InstallFlow.cpp:41 in ae95b7f. [](commit_id = ae95b7f, deletion_comment = False)

JohnMcPMS · 2020-07-15T17:42:45Z

src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw

@@ -236,6 +240,10 @@ They can be configured through the settings file 'winget settings'.</value>
  <data name="InstallationDisclaimer2" xml:space="preserve">
    <value>Microsoft is not responsible for, nor does it grant any licenses to, third-party packages.</value>
  </data>
+  <data name="InstallationDisclaimerMSStore" xml:space="preserve">
+    <value>This package is provided through Microsoft Store. Winget may need to acquire the package from Microsoft Store on behalf of the current user.</value>


Winget [](start = 61, length = 6)

Should probably not be capitalized, as the proper noun is not. #Closed

JohnMcPMS · 2020-07-15T17:45:35Z

src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw

+    <value>This feature is a work in progress, and may be changed dramatically or removed altogether in the future. To enable it, edit your settings ('winget settings') to include the experimental feature</value>
+    <comment>{Locked="winget settings"}</comment>
+  </data>
+  <data name="FeatureDisabledMessageWithFeatureName" xml:space="preserve">


FeatureDisabledMessageWithFeatureName [](start = 14, length = 37)

The other value is already used this way, just as the exception message that gets output as:

:

Don't make a second one. #Closed

JohnMcPMS · 2020-07-15T17:46:36Z

src/AppInstallerCLICore/Workflows/WorkflowBase.h

+    // Required Args: None
+    // Inputs: Feature
+    // Outputs: None
+    void EnsureFeatureEnabled(Execution::Context& context);


EnsureFeatureEnabled [](start = 9, length = 20)

Don't read the feature out of a context value, construct an object (see VerifyFile above). #Closed

JohnMcPMS · 2020-07-15T17:48:11Z

src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw

@@ -236,6 +240,10 @@ They can be configured through the settings file 'winget settings'.</value>
  <data name="InstallationDisclaimer2" xml:space="preserve">
    <value>Microsoft is not responsible for, nor does it grant any licenses to, third-party packages.</value>
  </data>
+  <data name="InstallationDisclaimerMSStore" xml:space="preserve">


InstallationDisclaimerMSStore [](start = 14, length = 29)

If this is a placeholder, did you send mail to ask for an official message? #Pending

I asked Kevin to give me one. We'll update later

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

JohnMcPMS · 2020-07-15T17:49:53Z

src/AppInstallerCLICore/Workflows/InstallFlow.cpp

@@ -192,6 +205,10 @@ namespace AppInstaller::CLI::Workflow
        case ManifestInstaller::InstallerTypeEnum::Msix:
            context << MsixInstall;
            break;
+        case ManifestInstaller::InstallerTypeEnum::MSStore:
+            context.Add<Execution::Data::Feature>(Settings::ExperimentalFeature::Feature::ExperimentalMSStore);
+            context << EnsureFeatureEnabled << MSStoreInstall;


EnsureFeatureEnabled [](start = 23, length = 20)

So this becomes:

context << EnsureFeatureEnabled(Settings::ExperimentalFeature::Feature::ExperimentalMSStore) << MSStoreInstall; ``` #Closed

JohnMcPMS · 2020-07-15T17:57:18Z

src/AppInstallerCLICore/Workflows/InstallFlow.cpp

+                // request in Store client installation queue.
+                installItem.Cancel();
+
+                context.Reporter.Info() << Resource::String::MSStoreInstallFailed << ' ' << errorCode << std::endl;


errorCode [](start = 92, length = 9)

I don't think HRESULT automatically outputs nicely. I would suggest adding an operator<<(HRESULT) to do so.

out << "0x" << std::hex << std::fill('0') << std::width(8) << hr; #Closed

I created a #define so that not every long will be printed as HResult

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

JohnMcPMS · 2020-07-15T17:59:47Z

src/AppInstallerCLICore/Workflows/InstallFlow.cpp

+        context.Reporter.ExecuteWithProgress(
+            [&](IProgressCallback& progress)
+            {
+                while (installItem.GetCurrentStatus().PercentComplete() < 100)


installItem.GetCurrentStatus().PercentComplete() [](start = 23, length = 48)

Since you need this 3 times each iteration and it might require a cross process COM call to get, maybe just save it in a local each time through the loop? #Closed

JohnMcPMS · 2020-07-15T18:00:50Z

src/AppInstallerCLICore/Workflows/WorkflowBase.cpp

+        {
+            context.Reporter.Error() << Resource::String::FeatureDisabledMessageWithFeatureName << ' ' <<
+                Settings::ExperimentalFeature::GetFeature(feature).Name() << std::endl;
+            AICLI_LOG(CLI, Error, << Settings::ExperimentalFeature::GetFeature(feature).Name() << "feature is disabled. Execution cancelled.");


feature [](start = 99, length = 7)

space in front of feature #Closed

JohnMcPMS · 2020-07-15T18:06:30Z

src/AppInstallerCLICore/Workflows/InstallFlow.cpp

+            nullptr).get();
+
+        // We install one package at a time so the AppInstallItem list returned should contain only one entry.
+        auto installItem = installItems.GetAt(0);


auto installItem = installItems.GetAt(0); [](start = 8, length = 41)

We should probably log a bunch of info about these requests so that we can easily associate them with store/deployment logs. Store request ids, PFNs, anything else that will make it easier to link the two. #Closed

Looking at the properties of AppInstallItem, only ProductId and PFN are related to our diagnosis. Logged these 2.

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

JohnMcPMS · 2020-07-16T00:17:35Z

src/AppInstallerCLICore/Workflows/WorkflowBase.cpp

+        if (!Settings::ExperimentalFeature::IsEnabled(m_feature))
+        {
+            context.Reporter.Error() << Resource::String::FeatureDisabledMessage << ' ' <<
+                Resource::String::FeatureDisabledFeatureName << ' ' <<


Why not just: ": ". That is what is happening in the case that the whole command or an argument is not available. #Closed

JohnMcPMS · 2020-07-16T00:24:41Z

src/AppInstallerCLICore/Workflows/InstallFlow.cpp

+                    for (auto const& installItem : installItems)
+                    {
+                        const auto& status = installItem.GetCurrentStatus();
+                        currentProgress += (uint64_t)(status.PercentComplete());


(uint64_t) [](start = 43, length = 10)

static_cast<uint64_t> #Closed

JohnMcPMS · 2020-07-16T00:27:40Z

src/AppInstallerCLICore/Workflows/InstallFlow.cpp

+                        if (!SUCCEEDED(errorCode))
+                        {
+                            context.Reporter.Info() << Resource::String::MSStoreInstallFailed << ' ' << WINGET_OSTREAM_FORMAT_HRESULT(errorCode) << std::endl;
+                            AICLI_LOG(CLI, Error, << "MSStore install failed. ProductId: " << Utility::ConvertToUTF8(productId) << " HResult: " << errorCode);


errorCode [](start = 147, length = 9)

WINGET_OSTREAM_FORMAT_HRESULT #Closed

JohnMcPMS

Whoerr

exit

Whoerr · 2020-10-30T00:46:52Z

closed

yao-msft added 4 commits July 10, 2020 11:52

Refactor manifest parsing code

89a6f5f

Added Store install type to manifest and Install flow

4888672

Test and fixes

8c9869b

Localization and logging

ae95b7f

yao-msft requested a review from a team as a code owner July 13, 2020 18:37

JohnMcPMS reviewed Jul 13, 2020

View reviewed changes

JohnMcPMS reviewed Jul 14, 2020

View reviewed changes

yao-msft commented Jul 14, 2020

View reviewed changes

PR feedback

3eeed93

JohnMcPMS reviewed Jul 15, 2020

View reviewed changes

PR feedback

bceb21f

JohnMcPMS reviewed Jul 16, 2020

View reviewed changes

PR feedback

0f545e7

JohnMcPMS approved these changes Jul 16, 2020

View reviewed changes

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

f54e2bc

yao-msft merged commit b79bf10 into microsoft:master Jul 16, 2020

yao-msft deleted the storeappinstall branch July 16, 2020 19:20

Whoerr reviewed Oct 30, 2020

View reviewed changes


		context.Reporter.ShowIndefiniteProgress(false);

		context.Reporter.ExecuteWithProgress(std::bind(

Implement store app installation from manifest #493

Implement store app installation from manifest #493

Conversation

yao-msft commented Jul 13, 2020 • edited by ghost Loading

Change

Validation

Microsoft Reviewers: Open in CodeFlow

JohnMcPMS Jul 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnMcPMS Jul 13, 2020 • edited Loading

Choose a reason for hiding this comment

JohnMcPMS commented Jul 13, 2020 • edited Loading

JohnMcPMS Jul 13, 2020 • edited Loading

Choose a reason for hiding this comment

JohnMcPMS Jul 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnMcPMS Jul 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnMcPMS Jul 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnMcPMS Jul 14, 2020 • edited Loading

Choose a reason for hiding this comment

JohnMcPMS Jul 14, 2020 • edited Loading

Choose a reason for hiding this comment

JohnMcPMS Jul 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnMcPMS Jul 14, 2020 • edited Loading

Choose a reason for hiding this comment

JohnMcPMS Jul 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnMcPMS Jul 14, 2020 • edited Loading

Choose a reason for hiding this comment

yao-msft Jul 14, 2020 • edited Loading

Choose a reason for hiding this comment

yao-msft Jul 14, 2020 • edited Loading

Choose a reason for hiding this comment

yao-msft commented Jul 14, 2020

JohnMcPMS Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

JohnMcPMS Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

JohnMcPMS Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

JohnMcPMS Jul 15, 2020 • edited by yao-msft Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnMcPMS Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

JohnMcPMS Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnMcPMS Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

JohnMcPMS Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

JohnMcPMS Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnMcPMS Jul 16, 2020 • edited Loading

Choose a reason for hiding this comment

JohnMcPMS Jul 16, 2020 • edited Loading

Choose a reason for hiding this comment

JohnMcPMS Jul 16, 2020 • edited Loading

Choose a reason for hiding this comment

JohnMcPMS left a comment

Choose a reason for hiding this comment

Whoerr left a comment

Choose a reason for hiding this comment

Whoerr commented Oct 30, 2020

yao-msft commented Jul 13, 2020 •

edited by ghost

Loading

JohnMcPMS Jul 13, 2020 •

edited

Loading

JohnMcPMS Jul 13, 2020 •

edited

Loading

JohnMcPMS commented Jul 13, 2020 •

edited

Loading

JohnMcPMS Jul 13, 2020 •

edited

Loading

JohnMcPMS Jul 13, 2020 •

edited

Loading

JohnMcPMS Jul 13, 2020 •

edited

Loading

JohnMcPMS Jul 14, 2020 •

edited

Loading

JohnMcPMS Jul 14, 2020 •

edited

Loading

JohnMcPMS Jul 14, 2020 •

edited

Loading

JohnMcPMS Jul 14, 2020 •

edited

Loading

JohnMcPMS Jul 14, 2020 •

edited

Loading

JohnMcPMS Jul 14, 2020 •

edited

Loading

JohnMcPMS Jul 14, 2020 •

edited

Loading

yao-msft Jul 14, 2020 •

edited

Loading

yao-msft Jul 14, 2020 •

edited

Loading

JohnMcPMS Jul 15, 2020 •

edited

Loading

JohnMcPMS Jul 15, 2020 •

edited

Loading

JohnMcPMS Jul 15, 2020 •

edited

Loading

JohnMcPMS Jul 15, 2020 •

edited by yao-msft

Loading

JohnMcPMS Jul 15, 2020 •

edited

Loading

JohnMcPMS Jul 15, 2020 •

edited

Loading

JohnMcPMS Jul 15, 2020 •

edited

Loading

JohnMcPMS Jul 15, 2020 •

edited

Loading

JohnMcPMS Jul 15, 2020 •

edited

Loading

JohnMcPMS Jul 16, 2020 •

edited

Loading

JohnMcPMS Jul 16, 2020 •

edited

Loading

JohnMcPMS Jul 16, 2020 •

edited

Loading