Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implementation for Zip Install (Non-Portable) #2320

Merged
merged 8 commits into from
Jul 13, 2022
Merged

Implementation for Zip Install (Non-Portable) #2320

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9da58fa
fix spelling
Jul 8, 2022
bbba7c7
Initial commit for supporting zip install (non-portable) (#5)
ryfu-msft Jul 8, 2022
de2688c
remove itemgroup
Jul 8, 2022
b40196d
fix E2E tests
Jul 11, 2022
516b992
save work
Jul 12, 2022
399367b
address PR comments
Jul 12, 2022
f6e4f88
move using definitions
Jul 13, 2022
e260da9
address second round of comments
Jul 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/AppInstallerCLICore/ExecutionContext.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@ namespace AppInstaller::CLI::Execution
// TODO: Remove when the source interface is refactored.
TreatSourceFailuresAsWarning = 0x10,
ShowSearchResultsOnPartialFailure = 0x20,
InstallerExtractedFromArchive = 0x40,
};

DEFINE_ENUM_FLAG_OPERATORS(ContextFlag);
Expand Down
1 change: 1 addition & 0 deletions src/AppInstallerCLICore/Resources.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@ namespace AppInstaller::CLI::Resource
WINGET_DEFINE_RESOURCE_STRINGID(MultipleInstalledPackagesFound);
WINGET_DEFINE_RESOURCE_STRINGID(MultiplePackagesFound);
WINGET_DEFINE_RESOURCE_STRINGID(NameArgumentDescription);
WINGET_DEFINE_RESOURCE_STRINGID(NestedInstallerFilesNotSpecified);
WINGET_DEFINE_RESOURCE_STRINGID(NestedInstallerNotFound);
WINGET_DEFINE_RESOURCE_STRINGID(NoApplicableInstallers);
WINGET_DEFINE_RESOURCE_STRINGID(NoExperimentalFeaturesMessage);
Expand Down
19 changes: 14 additions & 5 deletions src/AppInstallerCLICore/Workflows/ArchiveFlow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,21 +6,22 @@

namespace AppInstaller::CLI::Workflow
{
void ExtractInstallerFromArchive(Execution::Context& context)
void ExtractFilesFromArchive(Execution::Context& context)
{
const auto& installerPath = context.Get<Execution::Data::InstallerPath>();
const auto& installerParentPath = installerPath.parent_path();

HRESULT hr = AppInstaller::Archive::ExtractArchive(installerPath, installerParentPath);
// TODO: For portables, extract portables to final install location and log to local database.
HRESULT hr = AppInstaller::Archive::TryExtractArchive(installerPath, installerParentPath);
AICLI_LOG(CLI, Info, << "Extracting archive to: " << installerParentPath);

if (SUCCEEDED(hr))
{
AICLI_LOG(CLI, Info, << "Successfully extracted archive to: " << installerParentPath );
context.SetFlags(Execution::ContextFlag::InstallerExtractedFromArchive);
AICLI_LOG(CLI, Info, << "Successfully extracted archive");
ryfu-msft marked this conversation as resolved.
Show resolved Hide resolved
}
else
{
AICLI_LOG(CLI, Info, << "Failed to extract archive to: " << installerParentPath);
AICLI_LOG(CLI, Info, << "Failed to extract archive with code " << hr);
context.Reporter.Error() << Resource::String::ExtractArchiveFailed << std::endl;
AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_EXTRACT_ARCHIVE_FAILED);
}
Expand All @@ -29,8 +30,16 @@ namespace AppInstaller::CLI::Workflow
void VerifyAndSetNestedInstaller(Execution::Context& context)
{
const auto& installer = context.Get<Execution::Data::Installer>().value();
if (installer.NestedInstallerFiles.empty())
{
AICLI_LOG(CLI, Error, << "No entries specified for NestedInstallerFiles");
context.Reporter.Error() << Resource::String::NestedInstallerFilesNotSpecified << std::endl;
AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_INVALID_MANIFEST);
}

const auto& installerPath = context.Get<Execution::Data::InstallerPath>();
const auto& installerParentPath = installerPath.parent_path();

const auto& relativeFilePath = ConvertToUTF16(installer.NestedInstallerFiles[0].RelativeFilePath);
ryfu-msft marked this conversation as resolved.
Show resolved Hide resolved

std::filesystem::path nestedInstallerPath = installerParentPath / relativeFilePath;
Expand Down
10 changes: 9 additions & 1 deletion src/AppInstallerCLICore/Workflows/ArchiveFlow.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,15 @@

namespace AppInstaller::CLI::Workflow
{
void ExtractInstallerFromArchive(Execution::Context& context);
// Extracts the files from an archive
// Required Args: None
// Inputs: InstallerPath
// Outputs: None
void ExtractFilesFromArchive(Execution::Context& context);

// Verifies that the NestedInstaller exists and sets the InstallerPath
// Required Args: None
// Inputs: Installer, InstallerPath
// Outputs: None
void VerifyAndSetNestedInstaller(Execution::Context& context);
}
35 changes: 16 additions & 19 deletions src/AppInstallerCLICore/Workflows/InstallFlow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,22 +318,10 @@ namespace AppInstaller::CLI::Workflow
}
}

void ExecuteInstaller(Execution::Context& context)
void ExecuteInstallerForType(Execution::Context& context, InstallerTypeEnum installerType)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created a WorkflowTask derivative for ExecuteInstallerForType

{
const auto& installer = context.Get<Execution::Data::Installer>().value();

bool isUpdate = WI_IsFlagSet(context.GetFlags(), Execution::ContextFlag::InstallerExecutionUseUpdate);

InstallerTypeEnum installerType;

if (WI_IsFlagSet(context.GetFlags(), Execution::ContextFlag::InstallerExtractedFromArchive))
{
installerType = installer.NestedInstallerType;
}
else
{
installerType = installer.InstallerType;
}
UpdateBehaviorEnum updateBehavior = context.Get<Execution::Data::Installer>().value().UpdateBehavior;

switch (installerType)
{
Expand All @@ -343,7 +331,7 @@ namespace AppInstaller::CLI::Workflow
case InstallerTypeEnum::Msi:
case InstallerTypeEnum::Nullsoft:
case InstallerTypeEnum::Wix:
if (isUpdate && installer.UpdateBehavior == UpdateBehaviorEnum::UninstallPrevious)
if (isUpdate && updateBehavior == UpdateBehaviorEnum::UninstallPrevious)
{
context <<
GetUninstallInfo <<
Expand All @@ -368,7 +356,7 @@ namespace AppInstaller::CLI::Workflow
(isUpdate ? MSStoreUpdate : MSStoreInstall);
break;
case InstallerTypeEnum::Portable:
if (isUpdate && installer.UpdateBehavior == UpdateBehaviorEnum::UninstallPrevious)
if (isUpdate && updateBehavior == UpdateBehaviorEnum::UninstallPrevious)
{
context <<
GetUninstallInfo <<
Expand All @@ -385,12 +373,21 @@ namespace AppInstaller::CLI::Workflow
}
}

void ExecuteInstaller(Execution::Context& context)
{
ExecuteInstallerForType(context, context.Get<Execution::Data::Installer>().value().InstallerType);
}

void ArchiveInstall(Execution::Context& context)
{
context <<
ExtractInstallerFromArchive <<
VerifyAndSetNestedInstaller <<
ExecuteInstaller;
ExtractFilesFromArchive <<
VerifyAndSetNestedInstaller;

if (!context.IsTerminated())
{
ExecuteInstallerForType(context, context.Get<Execution::Data::Installer>().value().NestedInstallerType);
}
}

void ShellExecuteInstall(Execution::Context& context)
Expand Down
4 changes: 4 additions & 0 deletions src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

<comment>{Locked="NestedInstallerFiles"}</comment>
</data>
</root>
2 changes: 1 addition & 1 deletion src/AppInstallerCLITests/Archive.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ TEST_CASE("Extract_ZipArchive", "[archive]")
const auto& testZipPath = testZip.GetPath();
const auto& tempDirectoryPath = tempDirectory.GetPath();

HRESULT hr = ExtractArchive(testZipPath, tempDirectoryPath);
HRESULT hr = TryExtractArchive(testZipPath, tempDirectoryPath);

REQUIRE(SUCCEEDED(hr));
REQUIRE(std::filesystem::exists(tempDirectoryPath / "test.txt"));
Expand Down
8 changes: 3 additions & 5 deletions src/AppInstallerCLITests/WorkFlow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -624,9 +624,8 @@ void OverrideForEnsureSupportForPortable(TestContext& context)

void OverrideForArchiveInstall(TestContext& context)
{
context.Override({ ExtractInstallerFromArchive, [](TestContext& context)
context.Override({ ExtractFilesFromArchive, [](TestContext&)
{
context.SetFlags(Execution::ContextFlag::InstallerExtractedFromArchive);
} });

context.Override({ VerifyAndSetNestedInstaller, [](TestContext&)
Expand All @@ -636,9 +635,8 @@ void OverrideForArchiveInstall(TestContext& context)

void OverrideForExtractInstallerFromArchive(TestContext& context)
{
context.Override({ ExtractInstallerFromArchive, [](TestContext& context)
context.Override({ ExtractFilesFromArchive, [](TestContext&)
{
context.SetFlags(Execution::ContextFlag::InstallerExtractedFromArchive);
} });
}

Expand Down Expand Up @@ -1045,7 +1043,7 @@ TEST_CASE("ExtractInstallerFromArchive_InvalidZip", "[InstallFlow][workflow]")
context.Add<Data::Installer>(manifest.Installers.at(0));
// Provide an invalid zip file which should be handled appropriately.
context.Add<Data::InstallerPath>(TestDataFile("AppInstallerTestExeInstaller.exe"));
context << ExtractInstallerFromArchive;
context << ExtractFilesFromArchive;
REQUIRE_TERMINATED_WITH(context, APPINSTALLER_CLI_ERROR_EXTRACT_ARCHIVE_FAILED);
REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::ExtractArchiveFailed).get()) != std::string::npos);
}
Expand Down
84 changes: 31 additions & 53 deletions src/AppInstallerCommonCore/Archive.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,65 +4,43 @@

namespace AppInstaller::Archive
{
HRESULT ExtractArchive(const std::filesystem::path& archivePath, const std::filesystem::path& destPath)
using unique_pidlist_absolute = wil::unique_any<PIDLIST_ABSOLUTE, decltype(&::CoTaskMemFree), ::CoTaskMemFree>;
using unique_lpitemidlist = wil::unique_any<LPITEMIDLIST, decltype(&::CoTaskMemFree), ::CoTaskMemFree>;

HRESULT TryExtractArchive(const std::filesystem::path& archivePath, const std::filesystem::path& destPath)
{
IFileOperation* pFileOperation;
HRESULT hr = CoCreateInstance(CLSID_FileOperation, NULL, CLSCTX_ALL, IID_PPV_ARGS(&pFileOperation));
wil::com_ptr<IFileOperation> pFileOperation;
RETURN_IF_FAILED(CoCreateInstance(CLSID_FileOperation, NULL, CLSCTX_ALL, IID_PPV_ARGS(&pFileOperation)));
RETURN_IF_FAILED(pFileOperation->SetOperationFlags(FOF_NO_UI));

wil::com_ptr<IShellItem> pShellItemTo;
RETURN_IF_FAILED(SHCreateItemFromParsingName(destPath.c_str(), NULL, IID_PPV_ARGS(&pShellItemTo)));

unique_pidlist_absolute pidlFull;
RETURN_IF_FAILED(SHParseDisplayName(archivePath.c_str(), NULL, &pidlFull, 0, NULL));

if (SUCCEEDED(hr))
wil::com_ptr<IShellFolder> pArchiveShellFolder;
RETURN_IF_FAILED(SHBindToObject(NULL, pidlFull.get(), NULL, IID_PPV_ARGS(&pArchiveShellFolder)));

wil::com_ptr<IEnumIDList> pEnumIdList;
RETURN_IF_FAILED(pArchiveShellFolder->EnumObjects(nullptr, SHCONTF_FOLDERS | SHCONTF_NONFOLDERS, &pEnumIdList));

unique_lpitemidlist pidlChild;
ULONG nFetched;
while (pEnumIdList->Next(1, wil::out_param_ptr<LPITEMIDLIST*>(pidlChild), &nFetched) == S_OK && nFetched == 1)
{
hr = pFileOperation->SetOperationFlags(FOF_NO_UI);
if (SUCCEEDED(hr))
wil::com_ptr<IShellItem> pShellItemFrom;
STRRET strFolderName;
WCHAR szFolderName[MAX_PATH];
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

if ((pArchiveShellFolder->GetDisplayNameOf(pidlChild.get(), SHGDN_INFOLDER, &strFolderName) == S_OK) &&
(StrRetToBuf(&strFolderName, pidlChild.get(), szFolderName, MAX_PATH) == S_OK))
{
IShellItem* pShellItemTo;
hr = SHCreateItemFromParsingName(destPath.c_str(), NULL, IID_PPV_ARGS(&pShellItemTo));
if (SUCCEEDED(hr))
{
PIDLIST_ABSOLUTE pidlParent;
IShellFolder* pArchiveShellFolder;
IEnumIDList* pEnumIdList;
hr = SHParseDisplayName(archivePath.c_str(), NULL, &pidlParent, 0, NULL);
if (SUCCEEDED(hr))
{
hr = SHBindToObject(NULL, pidlParent, NULL, IID_PPV_ARGS(&pArchiveShellFolder));
if (SUCCEEDED(hr))
{
hr = pArchiveShellFolder->EnumObjects(nullptr, SHCONTF_FOLDERS | SHCONTF_NONFOLDERS, &pEnumIdList);
if (SUCCEEDED(hr))
{
LPITEMIDLIST pidlChild;
ULONG nFetched;
while (hr == S_OK && pEnumIdList->Next(1, &pidlChild, &nFetched) == S_OK && nFetched == 1)
{
IShellItem* pShellItemFrom;
STRRET strFolderName;
WCHAR szFolderName[MAX_PATH];
if ((pArchiveShellFolder->GetDisplayNameOf(pidlChild, SHGDN_INFOLDER, &strFolderName) == S_OK) &&
(StrRetToBuf(&strFolderName, pidlChild, szFolderName, MAX_PATH) == S_OK))
{
hr = SHCreateItemWithParent(pidlParent, pArchiveShellFolder, pidlChild, IID_PPV_ARGS(&pShellItemFrom));
if (SUCCEEDED(hr))
{
hr = pFileOperation->CopyItem(pShellItemFrom, pShellItemTo, NULL, NULL);
}
pShellItemFrom->Release();
}
ILFree(pidlChild);
}

hr = pFileOperation->PerformOperations();
pEnumIdList->Release();
}
pArchiveShellFolder->Release();
}
ILFree(pidlParent);
}
pShellItemTo->Release();
}
RETURN_IF_FAILED(SHCreateItemWithParent(pidlFull.get(), pArchiveShellFolder.get(), pidlChild.get(), IID_PPV_ARGS(&pShellItemFrom)));
RETURN_IF_FAILED(pFileOperation->CopyItem(pShellItemFrom.get(), pShellItemTo.get(), NULL, NULL));
}
pFileOperation->Release();
}

return hr;
RETURN_IF_FAILED(pFileOperation->PerformOperations());
return S_OK;
}
}
2 changes: 1 addition & 1 deletion src/AppInstallerCommonCore/Public/winget/Archive.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,5 @@

namespace AppInstaller::Archive
{
HRESULT ExtractArchive(const std::filesystem::path& archivePath, const std::filesystem::path& destPath);
HRESULT TryExtractArchive(const std::filesystem::path& archivePath, const std::filesystem::path& destPath);
}