-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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.
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
Add support for extracting archives using tar.exe #4541
Conversation
auto tarExecPath = AppInstaller::Filesystem::GetExpandedPath("%windir%\\system32\\tar.exe"); | ||
|
||
std::string args = "-xf " + m_archivePath.u8string() + " -C " + m_destPath.u8string(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you try paths with spaces (i.e. do we need to quote the path)?
tar.exe -xf c:\t e s t.zip -C C:\d e s t
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added single quotes to avoid issues with space in path
doc/Settings.md
Outdated
|
||
```json | ||
"installBehavior": { | ||
"extractArchiveWithTar": true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was expecting a string setting like below for extensibility
"archiveExtraction": "tar" allowed values are (tar, shellApi)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed to archiveExtractionMethod that takes in string values.
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@@ -514,6 +514,24 @@ TEST_CASE("ExtractInstallerFromArchive_InvalidZip", "[InstallFlow][workflow]") | |||
REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::ExtractArchiveFailed).get()) != std::string::npos); | |||
} | |||
|
|||
TEST_CASE("ExtractInstallerFromArchiveWithTar_InvalidZip", "[InstallFlow][workflow]") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please have a success case in the unit tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a success case unit test that uses a test hook to override the result of invoking tar.exe so that it returns success and verifies that the install flow can run successfully with the correct string output.
return Archive::ExtractionMethod::ShellApi; | ||
} | ||
|
||
return {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is "return {}" for enum return values? I guess we could add Archive::ExtractionMethod::Unknown and return the unknown enum
We have received feedback noting that the shell api fails to extract archives in certain scenarios. This adds an alternative way of extracting archives by invoking the tar.exe executable which is included in version later than 17063.
extractArchiveWithTar
that takes in a boolean value. If no value is specified or value is not valid, the default extract behavior will utilize the Windows Shell API.Microsoft Reviewers: Open in CodeFlow