Implementation for Zip Install (Non-Portable) #2320

ryfu-msft · 2022-07-08T20:34:48Z

Related to:
#140

Changes:

Adds the initial implementation for supporting installing from a zip file that contains a nested installer
Does not add support for nested Portable packages which will be addressed in a separate PR due to complexity

Tests:

Added E2E tests
Added Unit tests for verifying zip workflow and archive extraction behavior

Microsoft Reviewers: Open in CodeFlow

src/AppInstallerCLICore/Workflows/ArchiveFlow.cpp

JohnMcPMS · 2022-07-11T22:44:13Z

src/AppInstallerCLICore/ExecutionContext.h

@@ -64,6 +64,7 @@ namespace AppInstaller::CLI::Execution
        // TODO: Remove when the source interface is refactored.
        TreatSourceFailuresAsWarning = 0x10,
        ShowSearchResultsOnPartialFailure = 0x20,
+        InstallerExtractedFromArchive = 0x40,


There are a few options here:

This

Move installer type to be a context data item

Leverage IsArchiveType

Option 3 seems to be the least amount of adding additional context data and the easiest. Is there any case in which we wouldn't use the nested type for an archive?

Removed InstallerExtractedFromArchive context flag.

Added ExecuteInstallerForInstallerType which selects the appropriate install flow based on the installerType to handle whether to use InstallerType or NestedInstallerType

src/AppInstallerCLICore/Workflows/ArchiveFlow.cpp

src/AppInstallerCLICore/Workflows/ArchiveFlow.h

JohnMcPMS · 2022-07-11T23:05:00Z

src/AppInstallerCLICore/Workflows/InstallFlow.cpp

+        context <<
+            ExtractInstallerFromArchive <<
+            VerifyAndSetNestedInstaller <<
+            ExecuteInstaller;


This flow will be less efficient for portables; we should just extract directly to the final location. Not sure if that works great with the database tracking files though. We might need to be less efficient just so any files we could lose track of are in %TEMP% and will be more readily cleaned up.

I am thinking that portables will require more tasks than the one I have currently specified.
Here is what I am proposing when we start the implementation for portable(s) in an archive.

ExtractInstallerFromArchive (now called ExtractFilesFromArchive) will have a separate flow for portables that will copy the extracted files to the final portable install location

TryExtractArchive will have an additional parameter to indicate whether we will append to the database file in the final install location to keep track of the files that are extracted and copied.

The install location will need to have the directory created prior to extraction, so this will likely need to be its own step that creates the InstallDirectory and the local db file if it does not exist already. So something like SetupPortableInstallDirectory that we call prior to all of these steps if the NestedInstallerType is Portable

Once we enter the portable install flow, we can then use IsArchiveType to check whether we should install multiple portables for a given archive package.

JohnMcPMS · 2022-07-11T23:09:41Z

src/AppInstallerCLICore/Workflows/InstallFlow.cpp

+        context <<
+            ExtractInstallerFromArchive <<
+            VerifyAndSetNestedInstaller <<
+            ExecuteInstaller;


My gut is to create ExecuteInstallerForType(InstallerType) and then have ExecuteInstaller call it with installer.InstallerType and this call it with installer.NestedInstallerType rather than the extra state management.

Added ExecuteInstallerForType(Context, InstallerType) to handle the install flow selection logic based on installerType

src/AppInstallerCommonCore/Archive.cpp

JohnMcPMS · 2022-07-13T16:26:06Z

src/AppInstallerCLICore/Workflows/InstallFlow.cpp

@@ -318,22 +318,10 @@ namespace AppInstaller::CLI::Workflow
        }
    }

-    void ExecuteInstaller(Execution::Context& context)
+    void ExecuteInstallerForType(Execution::Context& context, InstallerTypeEnum installerType)


The intention was to create a WorkflowTask derivative. Example:

winget-cli/src/AppInstallerCLICore/Workflows/InstallFlow.h

Line 42 in 18ee699

struct ShowPackageAgreements : public WorkflowTask

That allows the continued use of the stream operators rather than checking IsTerminated.

Created a WorkflowTask derivative for ExecuteInstallerForType

JohnMcPMS · 2022-07-13T16:27:38Z

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

@@ -1370,4 +1370,8 @@ Please specify one of them using the `--source` option to proceed.</value>
  <data name="NestedInstallerNotFound" xml:space="preserve">
    <value>Nested installer file does not exist. Ensure the specified relative path of the nested installer matches: </value>
  </data>
+  <data name="NestedInstallerFilesNotSpecified" xml:space="preserve">
+    <value>No nested installer files specified; ensure that your manifest includes an entry for NestedInstallerFiles</value>


Suggested change

<value>No nested installer files specified; ensure that your manifest includes an entry for NestedInstallerFiles</value>

<value>No nested installer files specified; ensure that the manifest includes an entry for NestedInstallerFiles</value>

Is this actually possible per the basic manifest validation? I wanted the check for empty to ensure that we didn't crash, but if we don't think it will actually happen due to previous checks then the localized string doesn't seem necessary.

Added a comment clarifying that this check should be prevented by manifest validation and removed localized string.

JohnMcPMS · 2022-07-13T16:34:01Z

src/AppInstallerCommonCore/Archive.cpp

-            if (SUCCEEDED(hr))
+            wil::com_ptr<IShellItem> pShellItemFrom;
+            STRRET strFolderName;
+            WCHAR szFolderName[MAX_PATH];


Are the paths actually limited to MAX_PATH? If not, I would prefer if we could do a variable length path so that we don't hit a length issue here.

STRRET is has a max buffer length of MAX_PATH but this should be okay as it is only the name of the file

Ryan Fu and others added 3 commits July 8, 2022 13:25

fix spelling

9da58fa

Initial commit for supporting zip install (non-portable) (#5)

bbba7c7

remove itemgroup

de2688c

ryfu-msft requested a review from a team as a code owner July 8, 2022 20:34

Trenly reviewed Jul 9, 2022

View reviewed changes

src/AppInstallerCLICore/Workflows/ArchiveFlow.cpp Show resolved Hide resolved

src/AppInstallerCLICore/Workflows/ArchiveFlow.cpp Outdated Show resolved Hide resolved

fix E2E tests

b40196d

JohnMcPMS requested changes Jul 11, 2022

View reviewed changes

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

save work

516b992

vedantmgoyal9 mentioned this pull request Jul 12, 2022

[New Package]: MobaXterm.Mobatek vedantmgoyal9/vedantmgoyal9#1196

Closed

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

address PR comments

399367b

ryfu-msft requested a review from JohnMcPMS July 12, 2022 23:38

move using definitions

f6e4f88

JohnMcPMS requested changes Jul 13, 2022

View reviewed changes

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

address second round of comments

e260da9

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

JohnMcPMS approved these changes Jul 13, 2022

View reviewed changes

ryfu-msft merged commit 662842c into microsoft:master Jul 13, 2022

ryfu-msft deleted the zipInstall branch July 13, 2022 20:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation for Zip Install (Non-Portable) #2320

Implementation for Zip Install (Non-Portable) #2320

ryfu-msft commented Jul 8, 2022 •

edited by ghost

Loading

JohnMcPMS Jul 11, 2022

ryfu-msft Jul 12, 2022

JohnMcPMS Jul 11, 2022

ryfu-msft Jul 12, 2022 •

edited

Loading

JohnMcPMS Jul 11, 2022

ryfu-msft Jul 12, 2022

JohnMcPMS Jul 13, 2022

ryfu-msft Jul 13, 2022

JohnMcPMS Jul 13, 2022

ryfu-msft Jul 13, 2022

JohnMcPMS Jul 13, 2022

ryfu-msft Jul 13, 2022

	<value>No nested installer files specified; ensure that your manifest includes an entry for NestedInstallerFiles</value>
	<value>No nested installer files specified; ensure that the manifest includes an entry for NestedInstallerFiles</value>

Implementation for Zip Install (Non-Portable) #2320

Implementation for Zip Install (Non-Portable) #2320

Conversation

ryfu-msft commented Jul 8, 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

ryfu-msft Jul 12, 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

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

ryfu-msft commented Jul 8, 2022 •

edited by ghost

Loading

ryfu-msft Jul 12, 2022 •

edited

Loading