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 two issues related to using zstd compression #469

Merged
merged 5 commits into from May 19, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/cache/__tests__/tar.test.ts
Expand Up @@ -95,7 +95,7 @@ test('gzip extract GNU tar on windows', async () => {
jest.spyOn(fs, 'existsSync').mockReturnValueOnce(false)

const isGnuMock = jest
.spyOn(utils, 'useGnuTar')
.spyOn(utils, 'isGnuTarInstalled')
.mockReturnValue(Promise.resolve(true))
const execMock = jest.spyOn(exec, 'exec')
const archivePath = `${process.env['windir']}\\fakepath\\cache.tar`
Expand Down
11 changes: 11 additions & 0 deletions packages/cache/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions packages/cache/package.json
Expand Up @@ -41,10 +41,12 @@
"@actions/glob": "^0.1.0",
"@actions/http-client": "^1.0.8",
"@actions/io": "^1.0.1",
"semver": "^6.1.0",
"uuid": "^3.3.3"
},
"devDependencies": {
"typescript": "^3.8.3",
"@types/semver": "^6.0.0",
"@types/uuid": "^3.4.5"
}
}
4 changes: 3 additions & 1 deletion packages/cache/src/internal/cacheHttpClient.ts
Expand Up @@ -96,7 +96,9 @@ export function getCacheVersion(
compressionMethod?: CompressionMethod
): string {
const components = paths.concat(
compressionMethod === CompressionMethod.Zstd ? [compressionMethod] : []
!compressionMethod || compressionMethod === CompressionMethod.Gzip
? []
: [compressionMethod]
)

// Add salt to cache version to support breaking changes in cache entry
Expand Down
25 changes: 17 additions & 8 deletions packages/cache/src/internal/cacheUtils.ts
Expand Up @@ -4,6 +4,7 @@ import * as glob from '@actions/glob'
import * as io from '@actions/io'
import * as fs from 'fs'
import * as path from 'path'
import * as semver from 'semver'
import * as util from 'util'
import {v4 as uuidV4} from 'uuid'
import {CacheFilename, CompressionMethod} from './constants'
Expand Down Expand Up @@ -82,19 +83,27 @@ async function getVersion(app: string): Promise<string> {

// Use zstandard if possible to maximize cache performance
export async function getCompressionMethod(): Promise<CompressionMethod> {
const versionOutput = await getVersion('zstd')
return versionOutput.toLowerCase().includes('zstd command line interface')
? CompressionMethod.Zstd
: CompressionMethod.Gzip
if (process.platform === 'win32' && !isGnuTarInstalled()) {
aiqiaoy marked this conversation as resolved.
Show resolved Hide resolved
// Disable zstd due to bug https://github.com/actions/cache/issues/301
return CompressionMethod.Gzip
} else {
const versionOutput = await getVersion('zstd')
const version = semver.clean(versionOutput)
return !versionOutput.toLowerCase().includes('zstd command line interface')
? CompressionMethod.Gzip
: !version || semver.lt(version, 'v1.3.2')
? CompressionMethod.ZstdOld
: CompressionMethod.Zstd
Copy link
Member

Choose a reason for hiding this comment

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

This block of quote is a little difficult to parse. It might be better to break this up and add some comments about why we care about v1.3.2

Suggested change
return !versionOutput.toLowerCase().includes('zstd command line interface')
? CompressionMethod.Gzip
: !version || semver.lt(version, 'v1.3.2')
? CompressionMethod.ZstdOld
: CompressionMethod.Zstd
if (!versionOutput.toLowerCase().includes('zstd command line interface')) {
return CompressionMethod.Gzip
}
// v1.3.2 is required to use the `--long` options in ZSTD
if (!version || semver.lt(version, 'v1.3.2') {
return CompressionMethod.ZstdOld
}
return CompressionMethod.Zstd

}
}

export function getCacheFileName(compressionMethod: CompressionMethod): string {
return compressionMethod === CompressionMethod.Zstd
? CacheFilename.Zstd
: CacheFilename.Gzip
return compressionMethod === CompressionMethod.Gzip
? CacheFilename.Gzip
: CacheFilename.Zstd
}

export async function useGnuTar(): Promise<boolean> {
export async function isGnuTarInstalled(): Promise<boolean> {
const versionOutput = await getVersion('tar')
return versionOutput.toLowerCase().includes('gnu tar')
}
1 change: 1 addition & 0 deletions packages/cache/src/internal/constants.ts
Expand Up @@ -5,6 +5,7 @@ export enum CacheFilename {

export enum CompressionMethod {
Gzip = 'gzip',
ZstdOld = 'zstd-old',
Copy link
Member

Choose a reason for hiding this comment

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

Could we add a comment or be more descriptive in the enum name for what's considered an "Old" version of zstd?

The latest version of zstd is v1.4.4, so both of the versions we're dealing with could be considered old.

Zstd = 'zstd'
}

Expand Down
58 changes: 43 additions & 15 deletions packages/cache/src/internal/tar.ts
Expand Up @@ -5,23 +5,33 @@ import * as path from 'path'
import * as utils from './cacheUtils'
import {CompressionMethod} from './constants'

async function getTarPath(args: string[]): Promise<string> {
// Explicitly use BSD Tar on Windows
async function getTarPath(
args: string[],
compressionMethod: CompressionMethod
): Promise<string> {
const IS_WINDOWS = process.platform === 'win32'
if (IS_WINDOWS) {
const systemTar = `${process.env['windir']}\\System32\\tar.exe`
if (existsSync(systemTar)) {
if (compressionMethod !== CompressionMethod.Gzip) {
// We only use zstandard compression on windows when gnu tar is installed due to
// a bug with compressing large files with bsdtar + zstd
args.push('--force-local')
} else if (existsSync(systemTar)) {
return systemTar
} else if (await utils.useGnuTar()) {
} else if (await utils.isGnuTarInstalled()) {
args.push('--force-local')
}
}
return await io.which('tar', true)
}

async function execTar(args: string[], cwd?: string): Promise<void> {
async function execTar(
args: string[],
compressionMethod: CompressionMethod,
cwd?: string
): Promise<void> {
try {
await exec(`"${await getTarPath(args)}"`, args, {cwd})
await exec(`"${await getTarPath(args, compressionMethod)}"`, args, {cwd})
} catch (error) {
throw new Error(`Tar failed with error: ${error?.message}`)
}
Expand All @@ -41,17 +51,25 @@ export async function extractTar(
// --d: Decompress.
// --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.
function getProg(): string[] {
switch (compressionMethod) {
case CompressionMethod.Zstd:
return ['--use-compress-program', 'zstd -d --long=30']
case CompressionMethod.ZstdOld:
return ['--use-compress-program', 'zstd -d']
default:
return ['-z']
}
}
const args = [
...(compressionMethod === CompressionMethod.Zstd
? ['--use-compress-program', 'zstd -d --long=30']
: ['-z']),
...getProg(),
'-xf',
archivePath.replace(new RegExp(`\\${path.sep}`, 'g'), '/'),
'-P',
'-C',
workingDirectory.replace(new RegExp(`\\${path.sep}`, 'g'), '/')
]
await execTar(args)
await execTar(args, compressionMethod)
}

export async function createTar(
Expand All @@ -66,14 +84,24 @@ export async function createTar(
path.join(archiveFolder, manifestFilename),
sourceDirectories.join('\n')
)
const workingDirectory = getWorkingDirectory()

// -T#: Compress using # working thread. If # is 0, attempt to detect and use the number of physical CPU cores.
// --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 workingDirectory = getWorkingDirectory()
// Long range mode is added to zstd in v1.3.2 release, so we will not use --long in older version of zstd.
function getProg(): string[] {
aiqiaoy marked this conversation as resolved.
Show resolved Hide resolved
switch (compressionMethod) {
case CompressionMethod.Zstd:
return ['--use-compress-program', 'zstd -T0 --long=30']
case CompressionMethod.ZstdOld:
return ['--use-compress-program', 'zstd -T0']
default:
return ['-z']
}
}
const args = [
...(compressionMethod === CompressionMethod.Zstd
? ['--use-compress-program', 'zstd -T0 --long=30']
: ['-z']),
...getProg(),
'-cf',
cacheFileName.replace(new RegExp(`\\${path.sep}`, 'g'), '/'),
'-P',
Expand All @@ -82,5 +110,5 @@ export async function createTar(
'--files-from',
manifestFilename
]
await execTar(args, archiveFolder)
await execTar(args, compressionMethod, archiveFolder)
}