Skip to content

fix extractTar on Windows #264

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

Merged
ericsciple merged 11 commits into from
Dec 19, 2019
Merged

fix extractTar on Windows #264

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
8424027
fix extractTar on Windows
ericsciple Dec 17, 2019
6c95eda
.
ericsciple Dec 17, 2019
e372509
.
ericsciple Dec 17, 2019
0071524
.
ericsciple Dec 17, 2019
443db38
.
ericsciple Dec 17, 2019
4b8b9fd
.
ericsciple Dec 17, 2019
46de6c6
Revert "."
ericsciple Dec 17, 2019
9eee88d
.
ericsciple Dec 17, 2019
6af6cc0
Revert "Revert ".""
ericsciple Dec 17, 2019
67b788f
Update packages/tool-cache/src/tool-cache.ts
ericsciple Dec 18, 2019
45be6ad
.
ericsciple Dec 19, 2019
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
Binary file modified packages/tool-cache/__tests__/data/test.tar.gz
View file
Open in desktop
Binary file not shown.
144 changes: 68 additions & 76 deletions packages/tool-cache/__tests__/tool-cache.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,96 +268,88 @@ describe('@actions/tool-cache', function() {
await io.rmRF(tempDir)
Copy link
Copy Markdown
Contributor Author

@ericsciple ericsciple Dec 17, 2019

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
Copy Markdown
Contributor Author

@ericsciple ericsciple Dec 17, 2019

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

}
})
} else {
it('extract .tar.gz', async () => {
const tempDir = path.join(tempPath, 'test-install-tar.gz')
}

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

@ericsciple ericsciple Dec 17, 2019

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

const tempDir = path.join(tempPath, 'test-install-tar.gz')

// copy the .tar.gz file to the test dir
const _tgzFile: string = path.join(tempDir, 'test.tar.gz')
await io.cp(path.join(__dirname, 'data', 'test.tar.gz'), _tgzFile)
await io.mkdirP(tempDir)

// extract/cache
const extPath: string = await tc.extractTar(_tgzFile)
await tc.cacheDir(extPath, 'my-tgz-contents', '1.1.0')
const toolPath: string = tc.find('my-tgz-contents', '1.1.0')
// copy the .tar.gz file to the test dir
const _tgzFile: string = path.join(tempDir, 'test.tar.gz')
await io.cp(path.join(__dirname, 'data', 'test.tar.gz'), _tgzFile)

expect(fs.existsSync(toolPath)).toBeTruthy()
expect(fs.existsSync(`${toolPath}.complete`)).toBeTruthy()
expect(fs.existsSync(path.join(toolPath, 'file.txt'))).toBeTruthy()
expect(
fs.existsSync(path.join(toolPath, 'file-with-ç-character.txt'))
).toBeTruthy()
expect(
fs.existsSync(path.join(toolPath, 'folder', 'nested-file.txt'))
).toBeTruthy()
expect(
fs.readFileSync(
path.join(toolPath, 'folder', 'nested-file.txt'),
'utf8'
)
).toBe('folder/nested-file.txt contents')
})
// extract/cache
const extPath: string = await tc.extractTar(_tgzFile)
await tc.cacheDir(extPath, 'my-tgz-contents', '1.1.0')
const toolPath: string = tc.find('my-tgz-contents', '1.1.0')

it('extract .tar.gz to a directory that does not exist', async () => {
const tempDir = path.join(tempPath, 'test-install-tar.gz')
const destDir = path.join(tempDir, 'not-exist')
expect(fs.existsSync(toolPath)).toBeTruthy()
expect(fs.existsSync(`${toolPath}.complete`)).toBeTruthy()
expect(fs.existsSync(path.join(toolPath, 'file.txt'))).toBeTruthy()
expect(
fs.existsSync(path.join(toolPath, 'file-with-ç-character.txt'))
).toBeTruthy()
expect(
fs.existsSync(path.join(toolPath, 'folder', 'nested-file.txt'))
).toBeTruthy()
expect(
fs.readFileSync(path.join(toolPath, 'folder', 'nested-file.txt'), 'utf8')
).toBe('folder/nested-file.txt contents')
})

await io.mkdirP(tempDir)
it('extract .tar.gz to a directory that does not exist', async () => {
const tempDir = path.join(tempPath, 'test-install-tar.gz')
const destDir = path.join(tempDir, 'not-exist')

// copy the .tar.gz file to the test dir
const _tgzFile: string = path.join(tempDir, 'test.tar.gz')
await io.cp(path.join(__dirname, 'data', 'test.tar.gz'), _tgzFile)
await io.mkdirP(tempDir)

// extract/cache
const extPath: string = await tc.extractTar(_tgzFile, destDir)
await tc.cacheDir(extPath, 'my-tgz-contents', '1.1.0')
const toolPath: string = tc.find('my-tgz-contents', '1.1.0')
// copy the .tar.gz file to the test dir
const _tgzFile: string = path.join(tempDir, 'test.tar.gz')
await io.cp(path.join(__dirname, 'data', 'test.tar.gz'), _tgzFile)

expect(extPath).toContain('not-exist')
expect(fs.existsSync(toolPath)).toBeTruthy()
expect(fs.existsSync(`${toolPath}.complete`)).toBeTruthy()
expect(fs.existsSync(path.join(toolPath, 'file.txt'))).toBeTruthy()
expect(
fs.existsSync(path.join(toolPath, 'file-with-ç-character.txt'))
).toBeTruthy()
expect(
fs.existsSync(path.join(toolPath, 'folder', 'nested-file.txt'))
).toBeTruthy()
expect(
fs.readFileSync(
path.join(toolPath, 'folder', 'nested-file.txt'),
'utf8'
)
).toBe('folder/nested-file.txt contents')
})
// extract/cache
const extPath: string = await tc.extractTar(_tgzFile, destDir)
await tc.cacheDir(extPath, 'my-tgz-contents', '1.1.0')
const toolPath: string = tc.find('my-tgz-contents', '1.1.0')

it('extract .tar.xz', async () => {
const tempDir = path.join(tempPath, 'test-install-tar.xz')
expect(extPath).toContain('not-exist')
expect(fs.existsSync(toolPath)).toBeTruthy()
expect(fs.existsSync(`${toolPath}.complete`)).toBeTruthy()
expect(fs.existsSync(path.join(toolPath, 'file.txt'))).toBeTruthy()
expect(
fs.existsSync(path.join(toolPath, 'file-with-ç-character.txt'))
).toBeTruthy()
expect(
fs.existsSync(path.join(toolPath, 'folder', 'nested-file.txt'))
).toBeTruthy()
expect(
fs.readFileSync(path.join(toolPath, 'folder', 'nested-file.txt'), 'utf8')
).toBe('folder/nested-file.txt contents')
})

await io.mkdirP(tempDir)
it('extract .tar.xz', async () => {
const tempDir = path.join(tempPath, 'test-install-tar.xz')

// copy the .tar.gz file to the test dir
const _txzFile: string = path.join(tempDir, 'test.tar.xz')
await io.cp(path.join(__dirname, 'data', 'test.tar.xz'), _txzFile)
await io.mkdirP(tempDir)

// extract/cache
const extPath: string = await tc.extractTar(_txzFile, undefined, 'x')
await tc.cacheDir(extPath, 'my-txz-contents', '1.1.0')
const toolPath: string = tc.find('my-txz-contents', '1.1.0')
// copy the .tar.gz file to the test dir
const _txzFile: string = path.join(tempDir, 'test.tar.xz')
await io.cp(path.join(__dirname, 'data', 'test.tar.xz'), _txzFile)

expect(fs.existsSync(toolPath)).toBeTruthy()
expect(fs.existsSync(`${toolPath}.complete`)).toBeTruthy()
expect(fs.existsSync(path.join(toolPath, 'bar.txt'))).toBeTruthy()
expect(
fs.existsSync(path.join(toolPath, 'foo', 'hello.txt'))
).toBeTruthy()
expect(
fs.readFileSync(path.join(toolPath, 'foo', 'hello.txt'), 'utf8')
).toBe('foo/hello: world')
})
}
// extract/cache
const extPath: string = await tc.extractTar(_txzFile, undefined, 'x')
await tc.cacheDir(extPath, 'my-txz-contents', '1.1.0')
const toolPath: string = tc.find('my-txz-contents', '1.1.0')

expect(fs.existsSync(toolPath)).toBeTruthy()
expect(fs.existsSync(`${toolPath}.complete`)).toBeTruthy()
expect(fs.existsSync(path.join(toolPath, 'bar.txt'))).toBeTruthy()
expect(fs.existsSync(path.join(toolPath, 'foo', 'hello.txt'))).toBeTruthy()
expect(
fs.readFileSync(path.join(toolPath, 'foo', 'hello.txt'), 'utf8')
).toBe('foo/hello: world')
})

it('installs a zip and finds it', async () => {
const tempDir = path.join(__dirname, 'test-install-zip')
Expand Down
36 changes: 34 additions & 2 deletions packages/tool-cache/src/tool-cache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,9 +202,41 @@ export async function extractTar(
throw new Error("parameter 'file' is required")
}

// Create dest
dest = await _createExtractFolder(dest)
const tarPath: string = await io.which('tar', true)
await exec(`"${tarPath}"`, [flags, '-C', dest, '-f', file])

// Determine whether GNU tar
let versionOutput = ''
await exec('tar --version', [], {
ignoreReturnCode: true,
listeners: {
stdout: (data: Buffer) => (versionOutput += data.toString()),
stderr: (data: Buffer) => (versionOutput += data.toString())
}
})
const isGnuTar = versionOutput.toUpperCase().includes('GNU TAR')
Copy link
Copy Markdown
Collaborator

@thboop thboop Dec 18, 2019

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
Copy Markdown
Contributor Author

@ericsciple ericsciple Dec 18, 2019

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


// Initialize args
const args = [flags]

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

@joshmgross joshmgross Dec 18, 2019

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
Copy Markdown
Contributor Author

@ericsciple ericsciple Dec 18, 2019

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
Copy Markdown
Contributor Author

@ericsciple ericsciple Dec 18, 2019

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


// Technically only the dest needs to have `/` but for aesthetic consistency
// convert slashes in the file arg too.
fileArg = file.replace(/\\/g, '/')
}

if (isGnuTar) {
// Suppress warnings when using GNU tar to extract archives created by BSD tar
args.push('--warning=no-unknown-keyword')
}

args.push('-C', destArg, '-f', fileArg)
await exec(`tar`, args)

return dest
}
Expand Down