Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
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
20 changes: 10 additions & 10 deletions packages/cache/__tests__/tar.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ test('gzip extract GNU tar on windows with GNUtar in path', async () => {

await tar.extractTar(archivePath, CompressionMethod.Gzip)

expect(isGnuMock).toHaveBeenCalledTimes(1)
expect(isGnuMock).toHaveBeenCalledTimes(2)
expect(execMock).toHaveBeenCalledTimes(1)
expect(execMock).toHaveBeenCalledWith(
`"tar"`,
Expand Down Expand Up @@ -149,17 +149,17 @@ test('zstd create tar', async () => {
`"${tarPath}"`,
[
'--posix',
'--use-compress-program',
IS_WINDOWS ? 'zstd -T0 --long=30' : 'zstdmt --long=30',
'-cf',
IS_WINDOWS ? CacheFilename.Zstd.replace(/\\/g, '/') : CacheFilename.Zstd,
'--exclude',
IS_WINDOWS ? CacheFilename.Zstd.replace(/\\/g, '/') : CacheFilename.Zstd,
'-P',
'-C',
IS_WINDOWS ? workspace?.replace(/\\/g, '/') : workspace,
'--files-from',
'manifest.txt'
'manifest.txt',
Comment thread
Phantsure marked this conversation as resolved.
Outdated
'--use-compress-program',
IS_WINDOWS ? 'zstd -T0 --long=30' : 'zstdmt --long=30',
'-cf',
IS_WINDOWS ? CacheFilename.Zstd.replace(/\\/g, '/') : CacheFilename.Zstd
]
.concat(IS_WINDOWS ? ['--force-local'] : [])
.concat(IS_MAC ? ['--delay-directory-restore'] : []),
Expand Down Expand Up @@ -187,16 +187,16 @@ test('gzip create tar', async () => {
`"${tarPath}"`,
[
'--posix',
'-z',
'-cf',
IS_WINDOWS ? CacheFilename.Gzip.replace(/\\/g, '/') : CacheFilename.Gzip,
'--exclude',
IS_WINDOWS ? CacheFilename.Gzip.replace(/\\/g, '/') : CacheFilename.Gzip,
'-P',
'-C',
IS_WINDOWS ? workspace?.replace(/\\/g, '/') : workspace,
'--files-from',
'manifest.txt'
'manifest.txt',
'-z',
'-cf',
IS_WINDOWS ? CacheFilename.Gzip.replace(/\\/g, '/') : CacheFilename.Gzip
]
.concat(IS_WINDOWS ? ['--force-local'] : [])
.concat(IS_MAC ? ['--delay-directory-restore'] : []),
Expand Down
8 changes: 4 additions & 4 deletions packages/cache/src/internal/cacheUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -94,10 +94,10 @@ async function getVersion(app: string): Promise<string> {

// Use zstandard if possible to maximize cache performance
export async function getCompressionMethod(): Promise<CompressionMethod> {
if (process.platform === 'win32' && !(await getGnuTarPathOnWindows())) {
// Disable zstd due to bug https://github.com/actions/cache/issues/301
return CompressionMethod.Gzip
}
// if (process.platform === 'win32' && !(await getGnuTarPathOnWindows())) {
// // Disable zstd due to bug https://github.com/actions/cache/issues/301
// return CompressionMethod.Gzip
// }

const versionOutput = await getVersion('zstd')
const version = semver.clean(versionOutput)
Expand Down
2 changes: 2 additions & 0 deletions packages/cache/src/internal/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,5 @@ export const SocketTimeout = 5000

// The default path of GNUtar on hosted Windows runners
export const GnuTarPathOnWindows = `${process.env['PROGRAMFILES']}\\Git\\usr\\bin\\tar.exe`

export const SystemTarPathOnWindows = `${process.env['SYSTEMDRIVE']}\\Windows\\System32\\tar.exe`
55 changes: 41 additions & 14 deletions packages/cache/src/internal/tar.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,21 @@ import * as io from '@actions/io'
import {existsSync, writeFileSync} from 'fs'
import * as path from 'path'
import * as utils from './cacheUtils'
import {CompressionMethod} from './constants'
import {CompressionMethod, SystemTarPathOnWindows} from './constants'

const IS_WINDOWS = process.platform === 'win32'

// Function also mutates the args array. For non-mutation call with passing an empty array.
async function getTarPath(args: string[]): Promise<string> {
switch (process.platform) {
case 'win32': {
const gnuTar = await utils.getGnuTarPathOnWindows()
const systemTar = `${process.env['windir']}\\System32\\tar.exe`
if (gnuTar) {
// Use GNUtar as default on windows
args.push('--force-local')
if (args.length > 0) {
Comment thread
Phantsure marked this conversation as resolved.
Outdated
args.push('--force-local')
}
return gnuTar
} else if (existsSync(systemTar)) {
return systemTar
Expand All @@ -25,7 +28,9 @@ async function getTarPath(args: string[]): Promise<string> {
const gnuTar = await io.which('gtar', false)
if (gnuTar) {
// fix permission denied errors when extracting BSD tar archive with GNU tar - https://github.com/actions/cache/issues/527
args.push('--delay-directory-restore')
if (args.length > 0) {
args.push('--delay-directory-restore')
}
return gnuTar
}
break
Expand All @@ -49,18 +54,28 @@ function getWorkingDirectory(): string {
}

// Common function for extractTar and listTar to get the compression method
function getCompressionProgram(compressionMethod: CompressionMethod): string[] {
async function getCompressionProgram(
compressionMethod: CompressionMethod
): Promise<string[]> {
// -d: Decompress.
// unzstd is equivalent to 'zstd -d'
// --long=#: Enables long distance matching with # bits. Maximum is 30 (1GB) on 32-bit OS and 31 (2GB) on 64-bit.
// Using 30 here because we also support 32-bit self-hosted runners.
const tarPath = await getTarPath([])

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

getTarPath is only used for getting to know if SystemTarPathOnWindows is used or not. This can be simplified so that we do not need to fit args array.
For example either:

  1. Create a separate function to just get tar path
  2. Or move settings the args to getCompressionProgram where anyways we are setting more arguments

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

args set in getTarPath are only the ones related to particular os which also helps in deciding which tar to use.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In that case we can break into two function - one to get tar path and another to to get archive command.

const BSD_TAR_ZSTD = IS_WINDOWS && tarPath === SystemTarPathOnWindows
switch (compressionMethod) {
case CompressionMethod.Zstd:
if (BSD_TAR_ZSTD) {
return ['-a'] // auto-detect compression
Comment thread
Phantsure marked this conversation as resolved.
Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Will auto-detection take care of --long=30 part?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It seems to be used for compression where information is taken from tar file extension. So I think it can't take care of that

}
return [
'--use-compress-program',
IS_WINDOWS ? 'zstd -d --long=30' : 'unzstd --long=30'
]
case CompressionMethod.ZstdWithoutLong:
if (BSD_TAR_ZSTD) {
return ['a'] // auto-detect compression
}
return ['--use-compress-program', IS_WINDOWS ? 'zstd -d' : 'unzstd']
default:
return ['-z']
Expand All @@ -72,7 +87,7 @@ export async function listTar(
compressionMethod: CompressionMethod
): Promise<void> {
const args = [
...getCompressionProgram(compressionMethod),
...(await getCompressionProgram(compressionMethod)),
'-tf',
archivePath.replace(new RegExp(`\\${path.sep}`, 'g'), '/'),
'-P'
Expand All @@ -88,7 +103,7 @@ export async function extractTar(
const workingDirectory = getWorkingDirectory()
await io.mkdirP(workingDirectory)
const args = [
...getCompressionProgram(compressionMethod),
...(await getCompressionProgram(compressionMethod)),
'-xf',
archivePath.replace(new RegExp(`\\${path.sep}`, 'g'), '/'),
'-P',
Expand Down Expand Up @@ -117,31 +132,43 @@ export async function createTar(
// --long=#: Enables long distance matching with # bits. Maximum is 30 (1GB) on 32-bit OS and 31 (2GB) on 64-bit.
// Using 30 here because we also support 32-bit self-hosted runners.
// Long range mode is added to zstd in v1.3.2 release, so we will not use --long in older version of zstd.
function getCompressionProgram(): string[] {
async function getCompressionProgram(): Promise<string[]> {
const tarPath = await getTarPath([])
const BSD_TAR_ZSTD = IS_WINDOWS && tarPath === SystemTarPathOnWindows

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Naming it BSD_TAR_WINDOWS may be better as it coveys that this applies only for windows and uses bsdtar. zstd is already evident from switch case below.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Makes sense

switch (compressionMethod) {
case CompressionMethod.Zstd:
if (BSD_TAR_ZSTD) {
return ['-O', '|', 'zstd -T0 --long=30 -o']
}
return [
'--use-compress-program',
IS_WINDOWS ? 'zstd -T0 --long=30' : 'zstdmt --long=30'
IS_WINDOWS ? 'zstd -T0 --long=30' : 'zstdmt --long=30',
'-cf'
]
case CompressionMethod.ZstdWithoutLong:
return ['--use-compress-program', IS_WINDOWS ? 'zstd -T0' : 'zstdmt']
if (BSD_TAR_ZSTD) {
return ['-O', '|', 'zstd -T0 -o']
}
return [
'--use-compress-program',
IS_WINDOWS ? 'zstd -T0' : 'zstdmt',
'-cf'
]
default:
return ['-z']
return ['-z', '-cf']
}
}
const args = [
'--posix',
...getCompressionProgram(),
'-cf',
cacheFileName.replace(new RegExp(`\\${path.sep}`, 'g'), '/'),
'--exclude',
cacheFileName.replace(new RegExp(`\\${path.sep}`, 'g'), '/'),
'-P',
'-C',
workingDirectory.replace(new RegExp(`\\${path.sep}`, 'g'), '/'),
'--files-from',
manifestFilename
manifestFilename,
...(await getCompressionProgram()),
cacheFileName.replace(new RegExp(`\\${path.sep}`, 'g'), '/')

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Does moving this args order have any negative effect on gnutar case or bsdtar on macos case? Essentially it would be good to idea to test all these existing scenarios for regression.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Generally it shouldn't affect. In windows case due to pipe symbol it is needed. I would take a look at it

]
await execTar(args, archiveFolder)
}