Skip to content

Commit

Permalink
Address comments
Browse files Browse the repository at this point in the history
  • Loading branch information
Phantsure authored Nov 15, 2022
1 parent 7441dc5 commit 0e707ae
Show file tree
Hide file tree
Showing 4 changed files with 31 additions and 29 deletions.
28 changes: 15 additions & 13 deletions packages/cache/__tests__/tar.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
import * as exec from '@actions/exec'
import * as io from '@actions/io'
import * as path from 'path'
import {CacheFilename, CompressionMethod} from '../src/internal/constants'
import {
CacheFilename,
CompressionMethod,
GnuTarPathOnWindows
} from '../src/internal/constants'
import * as tar from '../src/internal/tar'
import * as utils from '../src/internal/cacheUtils'
// eslint-disable-next-line @typescript-eslint/no-require-imports
Expand All @@ -12,7 +16,6 @@ jest.mock('@actions/io')

const IS_WINDOWS = process.platform === 'win32'
const IS_MAC = process.platform === 'darwin'
const windowsGnuTar = `${process.env['PROGRAMFILES']}\\Git\\usr\\bin\\tar.exe`

const defaultTarPath = process.platform === 'darwin' ? 'gtar' : 'tar'

Expand Down Expand Up @@ -46,7 +49,7 @@ test('zstd extract tar', async () => {
? `${process.env['windir']}\\fakepath\\cache.tar`
: 'cache.tar'
const workspace = process.env['GITHUB_WORKSPACE']
const tarPath = IS_WINDOWS ? windowsGnuTar : defaultTarPath
const tarPath = IS_WINDOWS ? GnuTarPathOnWindows : defaultTarPath

await tar.extractTar(archivePath, CompressionMethod.Zstd)

Expand Down Expand Up @@ -80,7 +83,7 @@ test('gzip extract tar', async () => {
await tar.extractTar(archivePath, CompressionMethod.Gzip)

expect(mkdirMock).toHaveBeenCalledWith(workspace)
const tarPath = IS_WINDOWS ? windowsGnuTar : defaultTarPath
const tarPath = IS_WINDOWS ? GnuTarPathOnWindows : defaultTarPath
expect(execMock).toHaveBeenCalledTimes(1)
expect(execMock).toHaveBeenCalledWith(
`"${tarPath}"`,
Expand All @@ -100,11 +103,10 @@ test('gzip extract tar', async () => {

test('gzip extract GNU tar on windows with GNUtar in path', async () => {
if (IS_WINDOWS) {
jest.spyOn(fs, 'existsSync').mockReturnValue(false)

// GNU tar present in path but not at default location
const isGnuMock = jest
.spyOn(utils, 'isGnuTarInstalled')
.mockReturnValue(Promise.resolve(true))
.spyOn(utils, 'getGnuTarPathOnWindows')
.mockReturnValue(Promise.resolve('C:\\Program Files\\gnutar\\tar.exe'))
const execMock = jest.spyOn(exec, 'exec')
const archivePath = `${process.env['windir']}\\fakepath\\cache.tar`
const workspace = process.env['GITHUB_WORKSPACE']
Expand Down Expand Up @@ -140,7 +142,7 @@ test('zstd create tar', async () => {

await tar.createTar(archiveFolder, sourceDirectories, CompressionMethod.Zstd)

const tarPath = IS_WINDOWS ? windowsGnuTar : defaultTarPath
const tarPath = IS_WINDOWS ? GnuTarPathOnWindows : defaultTarPath

expect(execMock).toHaveBeenCalledTimes(1)
expect(execMock).toHaveBeenCalledWith(
Expand Down Expand Up @@ -178,7 +180,7 @@ test('gzip create tar', async () => {

await tar.createTar(archiveFolder, sourceDirectories, CompressionMethod.Gzip)

const tarPath = IS_WINDOWS ? windowsGnuTar : defaultTarPath
const tarPath = IS_WINDOWS ? GnuTarPathOnWindows : defaultTarPath

expect(execMock).toHaveBeenCalledTimes(1)
expect(execMock).toHaveBeenCalledWith(
Expand Down Expand Up @@ -213,7 +215,7 @@ test('zstd list tar', async () => {

await tar.listTar(archivePath, CompressionMethod.Zstd)

const tarPath = IS_WINDOWS ? windowsGnuTar : defaultTarPath
const tarPath = IS_WINDOWS ? GnuTarPathOnWindows : defaultTarPath
expect(execMock).toHaveBeenCalledTimes(1)
expect(execMock).toHaveBeenCalledWith(
`"${tarPath}"`,
Expand All @@ -239,7 +241,7 @@ test('zstdWithoutLong list tar', async () => {

await tar.listTar(archivePath, CompressionMethod.ZstdWithoutLong)

const tarPath = IS_WINDOWS ? windowsGnuTar : defaultTarPath
const tarPath = IS_WINDOWS ? GnuTarPathOnWindows : defaultTarPath
expect(execMock).toHaveBeenCalledTimes(1)
expect(execMock).toHaveBeenCalledWith(
`"${tarPath}"`,
Expand All @@ -264,7 +266,7 @@ test('gzip list tar', async () => {

await tar.listTar(archivePath, CompressionMethod.Gzip)

const tarPath = IS_WINDOWS ? windowsGnuTar : defaultTarPath
const tarPath = IS_WINDOWS ? GnuTarPathOnWindows : defaultTarPath
expect(execMock).toHaveBeenCalledTimes(1)
expect(execMock).toHaveBeenCalledWith(
`"${tarPath}"`,
Expand Down
18 changes: 11 additions & 7 deletions packages/cache/src/internal/cacheUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,11 @@ 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'
import {
CacheFilename,
CompressionMethod,
GnuTarPathOnWindows
} from './constants'

// From https://github.com/actions/toolkit/blob/main/packages/tool-cache/src/tool-cache.ts#L23
export async function createTempDirectory(): Promise<string> {
Expand Down Expand Up @@ -90,7 +94,7 @@ 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 isGnuTarInstalled())) {
if (process.platform === 'win32' && !(await getGnuTarPathOnWindows())) {
// Disable zstd due to bug https://github.com/actions/cache/issues/301
return CompressionMethod.Gzip
}
Expand All @@ -116,12 +120,12 @@ export function getCacheFileName(compressionMethod: CompressionMethod): string {
: CacheFilename.Zstd
}

export async function isGnuTarInstalled(): Promise<boolean> {
const gnuTar = `${process.env['PROGRAMFILES']}\\Git\\usr\\bin\\tar.exe`
export async function getGnuTarPathOnWindows(): Promise<string> {
if (fs.existsSync(GnuTarPathOnWindows)) {
return GnuTarPathOnWindows
}
const versionOutput = await getVersion('tar')
return (
versionOutput.toLowerCase().includes('gnu tar') || fs.existsSync(gnuTar)
)
return versionOutput.toLowerCase().includes('gnu tar') ? io.which('tar') : ''
}

export function assertDefined<T>(name: string, value?: T): T {
Expand Down
3 changes: 3 additions & 0 deletions packages/cache/src/internal/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,6 @@ export const DefaultRetryDelay = 5000
// over the socket during this period, the socket is destroyed and the download
// is aborted.
export const SocketTimeout = 5000

// The default path of GNUtar on hosted Windows runners
export const GnuTarPathOnWindows = 'C:\\Program Files\\Git\\usr\\bin\\tar.exe'
11 changes: 2 additions & 9 deletions packages/cache/src/internal/tar.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,19 +14,12 @@ async function getTarPath(
let tarPath = await io.which('tar', true)
switch (process.platform) {
case 'win32': {
const gnuTar = `${process.env['PROGRAMFILES']}\\Git\\usr\\bin\\tar.exe`
const gnuTar = await utils.getGnuTarPathOnWindows()
const systemTar = `${process.env['windir']}\\System32\\tar.exe`
if (existsSync(gnuTar)) {
if (gnuTar) {
// Use GNUtar as default on windows
args.push('--force-local')
tarPath = gnuTar
} else if (
compressionMethod !== CompressionMethod.Gzip ||
(await utils.isGnuTarInstalled())
) {
// 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)) {
tarPath = systemTar
}
Expand Down

0 comments on commit 0e707ae

Please sign in to comment.