From 43d72f5923815174494fbb9a2d430fa79fd87526 Mon Sep 17 00:00:00 2001 From: Sampark Sharma Date: Mon, 7 Nov 2022 13:39:07 +0000 Subject: [PATCH 01/14] Add gnuTar as default in windows --- src/internal/tar.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/internal/tar.ts b/src/internal/tar.ts index 2e28ca1..3b35141 100644 --- a/src/internal/tar.ts +++ b/src/internal/tar.ts @@ -14,7 +14,11 @@ async function getTarPath( switch (process.platform) { case 'win32': { const systemTar = `${process.env['windir']}\\System32\\tar.exe` - if (compressionMethod !== CompressionMethod.Gzip) { + const gnuTar = `${process.env['windir']}\\Program Files\\Git\\usr\\bin\\tar.exe` + if (compressionMethod !== CompressionMethod.Gzip && existsSync(gnuTar)) { + args.push('--force-local') + return gnuTar + } else 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') From 802b275be8f9b66906015c9f648b5c76ecb7ba8b Mon Sep 17 00:00:00 2001 From: Sampark Sharma Date: Mon, 7 Nov 2022 13:43:54 +0000 Subject: [PATCH 02/14] Fix gnutar check on windows --- src/internal/cacheUtils.ts | 3 ++- src/internal/tar.ts | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/internal/cacheUtils.ts b/src/internal/cacheUtils.ts index c2ace52..4dd3a61 100644 --- a/src/internal/cacheUtils.ts +++ b/src/internal/cacheUtils.ts @@ -117,8 +117,9 @@ export function getCacheFileName(compressionMethod: CompressionMethod): string { } export async function isGnuTarInstalled(): Promise { + const gnuTar = `${process.env['windir']}\\Program Files\\Git\\usr\\bin\\tar.exe` const versionOutput = await getVersion('tar') - return versionOutput.toLowerCase().includes('gnu tar') + return versionOutput.toLowerCase().includes('gnu tar') || fs.existsSync(gnuTar) } export function assertDefined(name: string, value?: T): T { diff --git a/src/internal/tar.ts b/src/internal/tar.ts index 3b35141..9d5add7 100644 --- a/src/internal/tar.ts +++ b/src/internal/tar.ts @@ -15,7 +15,7 @@ async function getTarPath( case 'win32': { const systemTar = `${process.env['windir']}\\System32\\tar.exe` const gnuTar = `${process.env['windir']}\\Program Files\\Git\\usr\\bin\\tar.exe` - if (compressionMethod !== CompressionMethod.Gzip && existsSync(gnuTar)) { + if (existsSync(gnuTar)) { args.push('--force-local') return gnuTar } else if (compressionMethod !== CompressionMethod.Gzip) { From cc292001ebad5e6dd019b13abe09d053f8fa894b Mon Sep 17 00:00:00 2001 From: Sampark Sharma Date: Tue, 8 Nov 2022 06:43:28 +0000 Subject: [PATCH 03/14] Fix tests --- __tests__/tar.test.ts | 4 ++-- src/internal/cacheUtils.ts | 4 +++- src/internal/tar.ts | 1 + 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/__tests__/tar.test.ts b/__tests__/tar.test.ts index e4233bc..31af266 100644 --- a/__tests__/tar.test.ts +++ b/__tests__/tar.test.ts @@ -92,9 +92,9 @@ test('gzip extract tar', async () => { ) }) -test('gzip extract GNU tar on windows', async () => { +test('gzip extract GNU tar on windows with GNUtar in path', async () => { if (IS_WINDOWS) { - jest.spyOn(fs, 'existsSync').mockReturnValueOnce(false) + jest.spyOn(fs, 'existsSync').mockReturnValue(false) const isGnuMock = jest .spyOn(utils, 'isGnuTarInstalled') diff --git a/src/internal/cacheUtils.ts b/src/internal/cacheUtils.ts index 4dd3a61..e6c09ee 100644 --- a/src/internal/cacheUtils.ts +++ b/src/internal/cacheUtils.ts @@ -119,7 +119,9 @@ export function getCacheFileName(compressionMethod: CompressionMethod): string { export async function isGnuTarInstalled(): Promise { const gnuTar = `${process.env['windir']}\\Program Files\\Git\\usr\\bin\\tar.exe` const versionOutput = await getVersion('tar') - return versionOutput.toLowerCase().includes('gnu tar') || fs.existsSync(gnuTar) + return ( + versionOutput.toLowerCase().includes('gnu tar') || fs.existsSync(gnuTar) + ) } export function assertDefined(name: string, value?: T): T { diff --git a/src/internal/tar.ts b/src/internal/tar.ts index 9d5add7..b650f4d 100644 --- a/src/internal/tar.ts +++ b/src/internal/tar.ts @@ -16,6 +16,7 @@ async function getTarPath( const systemTar = `${process.env['windir']}\\System32\\tar.exe` const gnuTar = `${process.env['windir']}\\Program Files\\Git\\usr\\bin\\tar.exe` if (existsSync(gnuTar)) { + // Making GNUtar + zstd as default on windows args.push('--force-local') return gnuTar } else if (compressionMethod !== CompressionMethod.Gzip) { From e8091e7b94fd5bc9cabd45b57687556cc3d6e7c5 Mon Sep 17 00:00:00 2001 From: Sampark Sharma Date: Tue, 8 Nov 2022 08:29:31 +0000 Subject: [PATCH 04/14] Fix GNUtar path --- src/internal/cacheUtils.ts | 2 +- src/internal/tar.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/internal/cacheUtils.ts b/src/internal/cacheUtils.ts index e6c09ee..da4b8d8 100644 --- a/src/internal/cacheUtils.ts +++ b/src/internal/cacheUtils.ts @@ -117,7 +117,7 @@ export function getCacheFileName(compressionMethod: CompressionMethod): string { } export async function isGnuTarInstalled(): Promise { - const gnuTar = `${process.env['windir']}\\Program Files\\Git\\usr\\bin\\tar.exe` + const gnuTar = `${process.env['PROGRAMFILES']}\\Git\\usr\\bin\\tar.exe` const versionOutput = await getVersion('tar') return ( versionOutput.toLowerCase().includes('gnu tar') || fs.existsSync(gnuTar) diff --git a/src/internal/tar.ts b/src/internal/tar.ts index b650f4d..9e140fe 100644 --- a/src/internal/tar.ts +++ b/src/internal/tar.ts @@ -14,7 +14,7 @@ async function getTarPath( switch (process.platform) { case 'win32': { const systemTar = `${process.env['windir']}\\System32\\tar.exe` - const gnuTar = `${process.env['windir']}\\Program Files\\Git\\usr\\bin\\tar.exe` + const gnuTar = `${process.env['PROGRAMFILES']}\\Git\\usr\\bin\\tar.exe` if (existsSync(gnuTar)) { // Making GNUtar + zstd as default on windows args.push('--force-local') From 77aef0ed4c4e898c1dfdd339ae4082cde4cebb66 Mon Sep 17 00:00:00 2001 From: Sampark Sharma Date: Tue, 8 Nov 2022 08:47:35 +0000 Subject: [PATCH 05/14] Fix tests --- __tests__/tar.test.ts | 43 +++++++++++++++++++++++++++++--------- src/internal/cacheUtils.ts | 2 +- src/internal/tar.ts | 9 ++++---- 3 files changed, 39 insertions(+), 15 deletions(-) diff --git a/__tests__/tar.test.ts b/__tests__/tar.test.ts index 31af266..6983d5f 100644 --- a/__tests__/tar.test.ts +++ b/__tests__/tar.test.ts @@ -28,6 +28,10 @@ beforeAll(async () => { await jest.requireActual('@actions/io').rmRF(getTempDir()) }) +beforeEach(async () => { + jest.restoreAllMocks() +}) + afterAll(async () => { delete process.env['GITHUB_WORKSPACE'] await jest.requireActual('@actions/io').rmRF(getTempDir()) @@ -41,13 +45,16 @@ test('zstd extract tar', async () => { ? `${process.env['windir']}\\fakepath\\cache.tar` : 'cache.tar' const workspace = process.env['GITHUB_WORKSPACE'] + const tarPath = IS_WINDOWS + ? `${process.env['PROGRAMFILES']}\\Git\\usr\\bin\\tar.exe` + : defaultTarPath await tar.extractTar(archivePath, CompressionMethod.Zstd) expect(mkdirMock).toHaveBeenCalledWith(workspace) expect(execMock).toHaveBeenCalledTimes(1) expect(execMock).toHaveBeenCalledWith( - `"${defaultTarPath}"`, + `"${tarPath}"`, [ '--use-compress-program', IS_WINDOWS ? 'zstd -d --long=30' : 'unzstd --long=30', @@ -75,7 +82,7 @@ test('gzip extract tar', async () => { expect(mkdirMock).toHaveBeenCalledWith(workspace) const tarPath = IS_WINDOWS - ? `${process.env['windir']}\\System32\\tar.exe` + ? `${process.env['PROGRAMFILES']}\\Git\\usr\\bin\\tar.exe` : defaultTarPath expect(execMock).toHaveBeenCalledTimes(1) expect(execMock).toHaveBeenCalledWith( @@ -87,7 +94,9 @@ test('gzip extract tar', async () => { '-P', '-C', IS_WINDOWS ? workspace?.replace(/\\/g, '/') : workspace - ].concat(IS_MAC ? ['--delay-directory-restore'] : []), + ] + .concat(IS_WINDOWS ? ['--force-local'] : []) + .concat(IS_MAC ? ['--delay-directory-restore'] : []), {cwd: undefined} ) }) @@ -134,9 +143,13 @@ test('zstd create tar', async () => { await tar.createTar(archiveFolder, sourceDirectories, CompressionMethod.Zstd) + const tarPath = IS_WINDOWS + ? `${process.env['PROGRAMFILES']}\\Git\\usr\\bin\\tar.exe` + : defaultTarPath + expect(execMock).toHaveBeenCalledTimes(1) expect(execMock).toHaveBeenCalledWith( - `"${defaultTarPath}"`, + `"${tarPath}"`, [ '--posix', '--use-compress-program', @@ -171,7 +184,7 @@ test('gzip create tar', async () => { await tar.createTar(archiveFolder, sourceDirectories, CompressionMethod.Gzip) const tarPath = IS_WINDOWS - ? `${process.env['windir']}\\System32\\tar.exe` + ? `${process.env['PROGRAMFILES']}\\Git\\usr\\bin\\tar.exe` : defaultTarPath expect(execMock).toHaveBeenCalledTimes(1) @@ -189,7 +202,9 @@ test('gzip create tar', async () => { IS_WINDOWS ? workspace?.replace(/\\/g, '/') : workspace, '--files-from', 'manifest.txt' - ].concat(IS_MAC ? ['--delay-directory-restore'] : []), + ] + .concat(IS_WINDOWS ? ['--force-local'] : []) + .concat(IS_MAC ? ['--delay-directory-restore'] : []), { cwd: archiveFolder } @@ -205,9 +220,12 @@ test('zstd list tar', async () => { await tar.listTar(archivePath, CompressionMethod.Zstd) + const tarPath = IS_WINDOWS + ? `${process.env['PROGRAMFILES']}\\Git\\usr\\bin\\tar.exe` + : defaultTarPath expect(execMock).toHaveBeenCalledTimes(1) expect(execMock).toHaveBeenCalledWith( - `"${defaultTarPath}"`, + `"${tarPath}"`, [ '--use-compress-program', IS_WINDOWS ? 'zstd -d --long=30' : 'unzstd --long=30', @@ -230,9 +248,12 @@ test('zstdWithoutLong list tar', async () => { await tar.listTar(archivePath, CompressionMethod.ZstdWithoutLong) + const tarPath = IS_WINDOWS + ? `${process.env['PROGRAMFILES']}\\Git\\usr\\bin\\tar.exe` + : defaultTarPath expect(execMock).toHaveBeenCalledTimes(1) expect(execMock).toHaveBeenCalledWith( - `"${defaultTarPath}"`, + `"${tarPath}"`, [ '--use-compress-program', IS_WINDOWS ? 'zstd -d' : 'unzstd', @@ -255,7 +276,7 @@ test('gzip list tar', async () => { await tar.listTar(archivePath, CompressionMethod.Gzip) const tarPath = IS_WINDOWS - ? `${process.env['windir']}\\System32\\tar.exe` + ? `${process.env['PROGRAMFILES']}\\Git\\usr\\bin\\tar.exe` : defaultTarPath expect(execMock).toHaveBeenCalledTimes(1) expect(execMock).toHaveBeenCalledWith( @@ -265,7 +286,9 @@ test('gzip list tar', async () => { '-tf', IS_WINDOWS ? archivePath.replace(/\\/g, '/') : archivePath, '-P' - ].concat(IS_MAC ? ['--delay-directory-restore'] : []), + ] + .concat(IS_WINDOWS ? ['--force-local'] : []) + .concat(IS_MAC ? ['--delay-directory-restore'] : []), {cwd: undefined} ) }) diff --git a/src/internal/cacheUtils.ts b/src/internal/cacheUtils.ts index da4b8d8..30538e2 100644 --- a/src/internal/cacheUtils.ts +++ b/src/internal/cacheUtils.ts @@ -67,7 +67,7 @@ export async function unlinkFile(filePath: fs.PathLike): Promise { return util.promisify(fs.unlink)(filePath) } -async function getVersion(app: string): Promise { +export async function getVersion(app: string): Promise { core.debug(`Checking ${app} --version`) let versionOutput = '' try { diff --git a/src/internal/tar.ts b/src/internal/tar.ts index 9e140fe..1941afd 100644 --- a/src/internal/tar.ts +++ b/src/internal/tar.ts @@ -16,17 +16,18 @@ async function getTarPath( const systemTar = `${process.env['windir']}\\System32\\tar.exe` const gnuTar = `${process.env['PROGRAMFILES']}\\Git\\usr\\bin\\tar.exe` if (existsSync(gnuTar)) { - // Making GNUtar + zstd as default on windows + // Use GNUtar as default on windows args.push('--force-local') return gnuTar - } else if (compressionMethod !== CompressionMethod.Gzip) { + } 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)) { return systemTar - } else if (await utils.isGnuTarInstalled()) { - args.push('--force-local') } break } From af8783ee87077963c27437612d9dbc9b737ed5ac Mon Sep 17 00:00:00 2001 From: Sampark Sharma Date: Mon, 14 Nov 2022 05:24:09 +0000 Subject: [PATCH 06/14] Add logs for cache version on miss --- src/internal/cacheHttpClient.ts | 18 +++++++++++++++++- src/internal/contracts.d.ts | 6 ++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/src/internal/cacheHttpClient.ts b/src/internal/cacheHttpClient.ts index c66d1a7..922c275 100644 --- a/src/internal/cacheHttpClient.ts +++ b/src/internal/cacheHttpClient.ts @@ -17,7 +17,8 @@ import { CommitCacheRequest, ReserveCacheRequest, ReserveCacheResponse, - ITypedResponseWithError + ITypedResponseWithError, + ArtifactCacheList } from './contracts' import {downloadCacheHttpClient, downloadCacheStorageSDK} from './downloadUtils' import { @@ -113,6 +114,21 @@ export async function getCacheEntry( const cacheResult = response.result const cacheDownloadUrl = cacheResult?.archiveLocation if (!cacheDownloadUrl) { + // List cache for primary key only if cache miss occurs + const resource = `caches?key=${encodeURIComponent(keys[0])}` + const response = await httpClient.getJson(getCacheApiUrl(resource)) + if(response.statusCode === 204) { + const cacheListResult = response.result + const totalCount = cacheListResult?.totalCount + if(totalCount && totalCount > 0) { + core.info(`Cache miss occurred on the cache key '${keys[0]}' and version '${version} but there is ${totalCount} existing version of the cache for this key. More info on versioning can be found here: https://github.com/actions/cache#cache-version`) + core.debug(`Other versions are as follows:`) + cacheListResult?.artifactCaches?.forEach(cacheEntry => { + core.debug(`Cache Key: ${cacheEntry?.cacheKey}, Cache Version: ${cacheEntry?.cacheVersion}, Cache Scope: ${cacheEntry?.scope}, Cache Created: ${cacheEntry?.creationTime}`) + }) + } + } + throw new Error('Cache not found.') } core.setSecret(cacheDownloadUrl) diff --git a/src/internal/contracts.d.ts b/src/internal/contracts.d.ts index 1b2a13a..b5f53bd 100644 --- a/src/internal/contracts.d.ts +++ b/src/internal/contracts.d.ts @@ -9,10 +9,16 @@ export interface ITypedResponseWithError extends TypedResponse { export interface ArtifactCacheEntry { cacheKey?: string scope?: string + cacheVersion?: string creationTime?: string archiveLocation?: string } +export interface ArtifactCacheList { + totalCount: number + artifactCaches?: ArtifactCacheEntry[] +} + export interface CommitCacheRequest { size: number } From 54d762b585d040c6fff9f98b1bf97d81518afb12 Mon Sep 17 00:00:00 2001 From: Sampark Sharma Date: Mon, 14 Nov 2022 06:33:53 +0000 Subject: [PATCH 07/14] Revert "Add logs for cache version on miss" This reverts commit af8783ee87077963c27437612d9dbc9b737ed5ac. --- src/internal/cacheHttpClient.ts | 18 +----------------- src/internal/contracts.d.ts | 6 ------ 2 files changed, 1 insertion(+), 23 deletions(-) diff --git a/src/internal/cacheHttpClient.ts b/src/internal/cacheHttpClient.ts index 922c275..c66d1a7 100644 --- a/src/internal/cacheHttpClient.ts +++ b/src/internal/cacheHttpClient.ts @@ -17,8 +17,7 @@ import { CommitCacheRequest, ReserveCacheRequest, ReserveCacheResponse, - ITypedResponseWithError, - ArtifactCacheList + ITypedResponseWithError } from './contracts' import {downloadCacheHttpClient, downloadCacheStorageSDK} from './downloadUtils' import { @@ -114,21 +113,6 @@ export async function getCacheEntry( const cacheResult = response.result const cacheDownloadUrl = cacheResult?.archiveLocation if (!cacheDownloadUrl) { - // List cache for primary key only if cache miss occurs - const resource = `caches?key=${encodeURIComponent(keys[0])}` - const response = await httpClient.getJson(getCacheApiUrl(resource)) - if(response.statusCode === 204) { - const cacheListResult = response.result - const totalCount = cacheListResult?.totalCount - if(totalCount && totalCount > 0) { - core.info(`Cache miss occurred on the cache key '${keys[0]}' and version '${version} but there is ${totalCount} existing version of the cache for this key. More info on versioning can be found here: https://github.com/actions/cache#cache-version`) - core.debug(`Other versions are as follows:`) - cacheListResult?.artifactCaches?.forEach(cacheEntry => { - core.debug(`Cache Key: ${cacheEntry?.cacheKey}, Cache Version: ${cacheEntry?.cacheVersion}, Cache Scope: ${cacheEntry?.scope}, Cache Created: ${cacheEntry?.creationTime}`) - }) - } - } - throw new Error('Cache not found.') } core.setSecret(cacheDownloadUrl) diff --git a/src/internal/contracts.d.ts b/src/internal/contracts.d.ts index b5f53bd..1b2a13a 100644 --- a/src/internal/contracts.d.ts +++ b/src/internal/contracts.d.ts @@ -9,16 +9,10 @@ export interface ITypedResponseWithError extends TypedResponse { export interface ArtifactCacheEntry { cacheKey?: string scope?: string - cacheVersion?: string creationTime?: string archiveLocation?: string } -export interface ArtifactCacheList { - totalCount: number - artifactCaches?: ArtifactCacheEntry[] -} - export interface CommitCacheRequest { size: number } From 1765c0fd16ea2494c8208dcdad9b66678246f48f Mon Sep 17 00:00:00 2001 From: Sampark Sharma Date: Mon, 14 Nov 2022 06:34:18 +0000 Subject: [PATCH 08/14] Remove export --- src/internal/cacheUtils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/internal/cacheUtils.ts b/src/internal/cacheUtils.ts index 30538e2..da4b8d8 100644 --- a/src/internal/cacheUtils.ts +++ b/src/internal/cacheUtils.ts @@ -67,7 +67,7 @@ export async function unlinkFile(filePath: fs.PathLike): Promise { return util.promisify(fs.unlink)(filePath) } -export async function getVersion(app: string): Promise { +async function getVersion(app: string): Promise { core.debug(`Checking ${app} --version`) let versionOutput = '' try { From be8b56eaf5a76a2a0b3f062124832b5231fd301d Mon Sep 17 00:00:00 2001 From: Sampark Sharma Date: Mon, 14 Nov 2022 08:12:15 +0000 Subject: [PATCH 09/14] Address review comments --- __tests__/tar.test.ts | 15 ++++++++------- src/internal/tar.ts | 11 ++++++----- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/__tests__/tar.test.ts b/__tests__/tar.test.ts index 6983d5f..6579cb3 100644 --- a/__tests__/tar.test.ts +++ b/__tests__/tar.test.ts @@ -12,6 +12,7 @@ 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' @@ -46,7 +47,7 @@ test('zstd extract tar', async () => { : 'cache.tar' const workspace = process.env['GITHUB_WORKSPACE'] const tarPath = IS_WINDOWS - ? `${process.env['PROGRAMFILES']}\\Git\\usr\\bin\\tar.exe` + ? windowsGnuTar : defaultTarPath await tar.extractTar(archivePath, CompressionMethod.Zstd) @@ -82,7 +83,7 @@ test('gzip extract tar', async () => { expect(mkdirMock).toHaveBeenCalledWith(workspace) const tarPath = IS_WINDOWS - ? `${process.env['PROGRAMFILES']}\\Git\\usr\\bin\\tar.exe` + ? windowsGnuTar : defaultTarPath expect(execMock).toHaveBeenCalledTimes(1) expect(execMock).toHaveBeenCalledWith( @@ -144,7 +145,7 @@ test('zstd create tar', async () => { await tar.createTar(archiveFolder, sourceDirectories, CompressionMethod.Zstd) const tarPath = IS_WINDOWS - ? `${process.env['PROGRAMFILES']}\\Git\\usr\\bin\\tar.exe` + ? windowsGnuTar : defaultTarPath expect(execMock).toHaveBeenCalledTimes(1) @@ -184,7 +185,7 @@ test('gzip create tar', async () => { await tar.createTar(archiveFolder, sourceDirectories, CompressionMethod.Gzip) const tarPath = IS_WINDOWS - ? `${process.env['PROGRAMFILES']}\\Git\\usr\\bin\\tar.exe` + ? windowsGnuTar : defaultTarPath expect(execMock).toHaveBeenCalledTimes(1) @@ -221,7 +222,7 @@ test('zstd list tar', async () => { await tar.listTar(archivePath, CompressionMethod.Zstd) const tarPath = IS_WINDOWS - ? `${process.env['PROGRAMFILES']}\\Git\\usr\\bin\\tar.exe` + ? windowsGnuTar : defaultTarPath expect(execMock).toHaveBeenCalledTimes(1) expect(execMock).toHaveBeenCalledWith( @@ -249,7 +250,7 @@ test('zstdWithoutLong list tar', async () => { await tar.listTar(archivePath, CompressionMethod.ZstdWithoutLong) const tarPath = IS_WINDOWS - ? `${process.env['PROGRAMFILES']}\\Git\\usr\\bin\\tar.exe` + ? windowsGnuTar : defaultTarPath expect(execMock).toHaveBeenCalledTimes(1) expect(execMock).toHaveBeenCalledWith( @@ -276,7 +277,7 @@ test('gzip list tar', async () => { await tar.listTar(archivePath, CompressionMethod.Gzip) const tarPath = IS_WINDOWS - ? `${process.env['PROGRAMFILES']}\\Git\\usr\\bin\\tar.exe` + ? windowsGnuTar : defaultTarPath expect(execMock).toHaveBeenCalledTimes(1) expect(execMock).toHaveBeenCalledWith( diff --git a/src/internal/tar.ts b/src/internal/tar.ts index 1941afd..fab9a92 100644 --- a/src/internal/tar.ts +++ b/src/internal/tar.ts @@ -11,14 +11,15 @@ async function getTarPath( args: string[], compressionMethod: CompressionMethod ): Promise { + var tarPath = await io.which('tar', true) switch (process.platform) { case 'win32': { - const systemTar = `${process.env['windir']}\\System32\\tar.exe` const gnuTar = `${process.env['PROGRAMFILES']}\\Git\\usr\\bin\\tar.exe` + const systemTar = `${process.env['windir']}\\System32\\tar.exe` if (existsSync(gnuTar)) { // Use GNUtar as default on windows args.push('--force-local') - return gnuTar + tarPath = gnuTar } else if ( compressionMethod !== CompressionMethod.Gzip || (await utils.isGnuTarInstalled()) @@ -27,7 +28,7 @@ async function getTarPath( // a bug with compressing large files with bsdtar + zstd args.push('--force-local') } else if (existsSync(systemTar)) { - return systemTar + tarPath = systemTar } break } @@ -36,14 +37,14 @@ async function getTarPath( 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') - return gnuTar + tarPath = gnuTar } break } default: break } - return await io.which('tar', true) + return tarPath } async function execTar( From 10f0f130f28a548c827b28069f88728aa8fa6aee Mon Sep 17 00:00:00 2001 From: Sampark Sharma Date: Mon, 14 Nov 2022 10:11:46 +0000 Subject: [PATCH 10/14] Fix tests --- __tests__/tar.test.ts | 28 +++++++--------------------- 1 file changed, 7 insertions(+), 21 deletions(-) diff --git a/__tests__/tar.test.ts b/__tests__/tar.test.ts index 6579cb3..bbd542a 100644 --- a/__tests__/tar.test.ts +++ b/__tests__/tar.test.ts @@ -46,9 +46,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 ? windowsGnuTar : defaultTarPath await tar.extractTar(archivePath, CompressionMethod.Zstd) @@ -82,9 +80,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 ? windowsGnuTar : defaultTarPath expect(execMock).toHaveBeenCalledTimes(1) expect(execMock).toHaveBeenCalledWith( `"${tarPath}"`, @@ -144,9 +140,7 @@ test('zstd create tar', async () => { await tar.createTar(archiveFolder, sourceDirectories, CompressionMethod.Zstd) - const tarPath = IS_WINDOWS - ? windowsGnuTar - : defaultTarPath + const tarPath = IS_WINDOWS ? windowsGnuTar : defaultTarPath expect(execMock).toHaveBeenCalledTimes(1) expect(execMock).toHaveBeenCalledWith( @@ -184,9 +178,7 @@ test('gzip create tar', async () => { await tar.createTar(archiveFolder, sourceDirectories, CompressionMethod.Gzip) - const tarPath = IS_WINDOWS - ? windowsGnuTar - : defaultTarPath + const tarPath = IS_WINDOWS ? windowsGnuTar : defaultTarPath expect(execMock).toHaveBeenCalledTimes(1) expect(execMock).toHaveBeenCalledWith( @@ -221,9 +213,7 @@ test('zstd list tar', async () => { await tar.listTar(archivePath, CompressionMethod.Zstd) - const tarPath = IS_WINDOWS - ? windowsGnuTar - : defaultTarPath + const tarPath = IS_WINDOWS ? windowsGnuTar : defaultTarPath expect(execMock).toHaveBeenCalledTimes(1) expect(execMock).toHaveBeenCalledWith( `"${tarPath}"`, @@ -249,9 +239,7 @@ test('zstdWithoutLong list tar', async () => { await tar.listTar(archivePath, CompressionMethod.ZstdWithoutLong) - const tarPath = IS_WINDOWS - ? windowsGnuTar - : defaultTarPath + const tarPath = IS_WINDOWS ? windowsGnuTar : defaultTarPath expect(execMock).toHaveBeenCalledTimes(1) expect(execMock).toHaveBeenCalledWith( `"${tarPath}"`, @@ -276,9 +264,7 @@ test('gzip list tar', async () => { await tar.listTar(archivePath, CompressionMethod.Gzip) - const tarPath = IS_WINDOWS - ? windowsGnuTar - : defaultTarPath + const tarPath = IS_WINDOWS ? windowsGnuTar : defaultTarPath expect(execMock).toHaveBeenCalledTimes(1) expect(execMock).toHaveBeenCalledWith( `"${tarPath}"`, From 1c598e6f900e57eb5571ba236bf57cd28894b86b Mon Sep 17 00:00:00 2001 From: Sampark Sharma Date: Mon, 14 Nov 2022 10:25:17 +0000 Subject: [PATCH 11/14] Fix tests --- src/internal/tar.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/internal/tar.ts b/src/internal/tar.ts index fab9a92..8f548bd 100644 --- a/src/internal/tar.ts +++ b/src/internal/tar.ts @@ -11,7 +11,7 @@ async function getTarPath( args: string[], compressionMethod: CompressionMethod ): Promise { - var tarPath = await io.which('tar', true) + let tarPath = await io.which('tar', true) switch (process.platform) { case 'win32': { const gnuTar = `${process.env['PROGRAMFILES']}\\Git\\usr\\bin\\tar.exe` From cb8659f414c1009df86c1bad03166af47dc93dc9 Mon Sep 17 00:00:00 2001 From: Sampark Sharma Date: Tue, 15 Nov 2022 08:41:04 +0000 Subject: [PATCH 12/14] Address comments --- __tests__/tar.test.ts | 28 +++++++++++++++------------- src/internal/cacheUtils.ts | 18 +++++++++++------- src/internal/constants.ts | 3 +++ src/internal/tar.ts | 11 ++--------- 4 files changed, 31 insertions(+), 29 deletions(-) diff --git a/__tests__/tar.test.ts b/__tests__/tar.test.ts index bbd542a..61338bc 100644 --- a/__tests__/tar.test.ts +++ b/__tests__/tar.test.ts @@ -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 @@ -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' @@ -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) @@ -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}"`, @@ -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'] @@ -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( @@ -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( @@ -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}"`, @@ -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}"`, @@ -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}"`, diff --git a/src/internal/cacheUtils.ts b/src/internal/cacheUtils.ts index da4b8d8..64fef08 100644 --- a/src/internal/cacheUtils.ts +++ b/src/internal/cacheUtils.ts @@ -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 { @@ -90,7 +94,7 @@ async function getVersion(app: string): Promise { // Use zstandard if possible to maximize cache performance export async function getCompressionMethod(): Promise { - 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 } @@ -116,12 +120,12 @@ export function getCacheFileName(compressionMethod: CompressionMethod): string { : CacheFilename.Zstd } -export async function isGnuTarInstalled(): Promise { - const gnuTar = `${process.env['PROGRAMFILES']}\\Git\\usr\\bin\\tar.exe` +export async function getGnuTarPathOnWindows(): Promise { + 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(name: string, value?: T): T { diff --git a/src/internal/constants.ts b/src/internal/constants.ts index 2f78d32..cc58a0b 100644 --- a/src/internal/constants.ts +++ b/src/internal/constants.ts @@ -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' \ No newline at end of file diff --git a/src/internal/tar.ts b/src/internal/tar.ts index 8f548bd..db2f635 100644 --- a/src/internal/tar.ts +++ b/src/internal/tar.ts @@ -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 } From a4e0ab9aed3d6066cf0f025c098d0ed15f3b4eb7 Mon Sep 17 00:00:00 2001 From: Sampark Sharma Date: Tue, 15 Nov 2022 08:48:29 +0000 Subject: [PATCH 13/14] Fix tests --- __tests__/tar.test.ts | 2 +- src/internal/constants.ts | 2 +- src/internal/tar.ts | 5 +---- 3 files changed, 3 insertions(+), 6 deletions(-) diff --git a/__tests__/tar.test.ts b/__tests__/tar.test.ts index 61338bc..31f9bd5 100644 --- a/__tests__/tar.test.ts +++ b/__tests__/tar.test.ts @@ -106,7 +106,7 @@ test('gzip extract GNU tar on windows with GNUtar in path', async () => { // GNU tar present in path but not at default location const isGnuMock = jest .spyOn(utils, 'getGnuTarPathOnWindows') - .mockReturnValue(Promise.resolve('C:\\Program Files\\gnutar\\tar.exe')) + .mockReturnValue(Promise.resolve('tar')) const execMock = jest.spyOn(exec, 'exec') const archivePath = `${process.env['windir']}\\fakepath\\cache.tar` const workspace = process.env['GITHUB_WORKSPACE'] diff --git a/src/internal/constants.ts b/src/internal/constants.ts index cc58a0b..f4da6e6 100644 --- a/src/internal/constants.ts +++ b/src/internal/constants.ts @@ -23,4 +23,4 @@ export const DefaultRetryDelay = 5000 export const SocketTimeout = 5000 // The default path of GNUtar on hosted Windows runners -export const GnuTarPathOnWindows = 'C:\\Program Files\\Git\\usr\\bin\\tar.exe' \ No newline at end of file +export const GnuTarPathOnWindows = 'C:\\Program Files\\Git\\usr\\bin\\tar.exe' diff --git a/src/internal/tar.ts b/src/internal/tar.ts index db2f635..b192161 100644 --- a/src/internal/tar.ts +++ b/src/internal/tar.ts @@ -7,10 +7,7 @@ import {CompressionMethod} from './constants' const IS_WINDOWS = process.platform === 'win32' -async function getTarPath( - args: string[], - compressionMethod: CompressionMethod -): Promise { +async function getTarPath(args: string[]): Promise { let tarPath = await io.which('tar', true) switch (process.platform) { case 'win32': { From db0e76705dd5869203b169a17321cc863d3dd01e Mon Sep 17 00:00:00 2001 From: Sampark Sharma Date: Tue, 15 Nov 2022 10:18:46 +0000 Subject: [PATCH 14/14] Address review comments --- src/internal/constants.ts | 2 +- src/internal/tar.ts | 23 +++++++++-------------- 2 files changed, 10 insertions(+), 15 deletions(-) diff --git a/src/internal/constants.ts b/src/internal/constants.ts index f4da6e6..e61b46f 100644 --- a/src/internal/constants.ts +++ b/src/internal/constants.ts @@ -23,4 +23,4 @@ export const DefaultRetryDelay = 5000 export const SocketTimeout = 5000 // The default path of GNUtar on hosted Windows runners -export const GnuTarPathOnWindows = 'C:\\Program Files\\Git\\usr\\bin\\tar.exe' +export const GnuTarPathOnWindows = `${process.env['PROGRAMFILES']}\\Git\\usr\\bin\\tar.exe` diff --git a/src/internal/tar.ts b/src/internal/tar.ts index b192161..4269200 100644 --- a/src/internal/tar.ts +++ b/src/internal/tar.ts @@ -8,7 +8,6 @@ import {CompressionMethod} from './constants' const IS_WINDOWS = process.platform === 'win32' async function getTarPath(args: string[]): Promise { - let tarPath = await io.which('tar', true) switch (process.platform) { case 'win32': { const gnuTar = await utils.getGnuTarPathOnWindows() @@ -16,9 +15,9 @@ async function getTarPath(args: string[]): Promise { if (gnuTar) { // Use GNUtar as default on windows args.push('--force-local') - tarPath = gnuTar + return gnuTar } else if (existsSync(systemTar)) { - tarPath = systemTar + return systemTar } break } @@ -27,23 +26,19 @@ async function getTarPath(args: string[]): Promise { 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') - tarPath = gnuTar + return gnuTar } break } default: break } - return tarPath + return await io.which('tar', true) } -async function execTar( - args: string[], - compressionMethod: CompressionMethod, - cwd?: string -): Promise { +async function execTar(args: string[], cwd?: string): Promise { try { - await exec(`"${await getTarPath(args, compressionMethod)}"`, args, {cwd}) + await exec(`"${await getTarPath(args)}"`, args, {cwd}) } catch (error) { throw new Error(`Tar failed with error: ${error?.message}`) } @@ -82,7 +77,7 @@ export async function listTar( archivePath.replace(new RegExp(`\\${path.sep}`, 'g'), '/'), '-P' ] - await execTar(args, compressionMethod) + await execTar(args) } export async function extractTar( @@ -100,7 +95,7 @@ export async function extractTar( '-C', workingDirectory.replace(new RegExp(`\\${path.sep}`, 'g'), '/') ] - await execTar(args, compressionMethod) + await execTar(args) } export async function createTar( @@ -148,5 +143,5 @@ export async function createTar( '--files-from', manifestFilename ] - await execTar(args, compressionMethod, archiveFolder) + await execTar(args, archiveFolder) }