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.

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

fix extractTar on Windows #264

Merged
merged 11 commits into from
Dec 19, 2019
Merged

fix extractTar on Windows #264

merged 11 commits into from
Dec 19, 2019

Conversation

ericsciple
Copy link
Contributor

@ericsciple ericsciple commented Dec 17, 2019

Fixes extractTar on Windows by formatting the arguments different depending on whether BSD tar or GNU tar is being called.

@@ -268,96 +268,88 @@ describe('@actions/tool-cache', function() {
await io.rmRF(tempDir)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i had to recreate test.tar.gz with the option --format pax otherwise bsdtar doesnt preserve UTF file names correctly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i added a README.txt to the archive for future reference


await io.mkdirP(tempDir)
it('extract .tar.gz', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

no changes here, just eliminated the else condition and de-indented

let destArg = dest
if (IS_WINDOWS && isGnuTar) {
args.push('--force-local')
destArg = dest.replace(/\\/g, '/')
Copy link
Member

Choose a reason for hiding this comment

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

Both destArg and file need the \ replacement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can add for file too, but when i tested file doesnt appear to be required

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'll push this change, aesthetically it will look good for consistency

packages/tool-cache/src/tool-cache.ts Outdated Show resolved Hide resolved
Co-Authored-By: Josh Gross <joshmgross@github.com>
@bryanmacfarlane
Copy link
Member

Add a description for the PR ☝️

@bryanmacfarlane
Copy link
Member

Other than description, LGTM

stderr: (data: Buffer) => (versionOutput += data.toString())
}
})
const isGnuTar = versionOutput.toUpperCase().includes('GNU TAR')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider debugging this so we can troubleshoot easier if for some reason this breaks. (Analyzing text output of a cli can be a little fragile)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the output flows to the console too, so we're good wrt that aspect

Copy link
Collaborator

@thboop thboop left a comment

Choose a reason for hiding this comment

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

LGTM minor thoughts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants