Address review comments

Co-authored-by: Bishal Prasad <[email protected]>

.github/workflows/cache-windows-test.yml

@@ -0,0 +1,91 @@
+name: cache-windows-bsd-unit-tests
+on:
+  push:
+    branches:
+      - main
+    paths-ignore:
+      - '**.md'
+  pull_request:
+    paths-ignore:
+      - '**.md'
+
+jobs:
+  build:
+    name: Build
+
+    runs-on: windows-latest
+
+    steps:
+    - name: Checkout
+      uses: actions/checkout@v2
+
+    - shell: bash
+      run: |
+        rm "C:\Program Files\Git\usr\bin\tar.exe"
+
+    - name: Set Node.js 12.x
+      uses: actions/setup-node@v1
+      with:
+        node-version: 12.x
+
+    # In order to save & restore cache from a shell script, certain env variables need to be set that are only available in the
+    # node context. This runs a local action that gets and sets the necessary env variables that are needed
+    - name: Set env variables
+      uses: ./packages/cache/__tests__/__fixtures__/
+
+    # Need root node_modules because certain npm packages like jest are configured for the entire repository and it won't be possible
+    # without these to just compile the cache package
+    - name: Install root npm packages
+      run: npm ci
+
+    - name: Compile cache package
+      run: |
+        npm ci
+        npm run tsc
+      working-directory: packages/cache
+    
+    - name: Generate files in working directory
+      shell: bash
+      run: packages/cache/__tests__/create-cache-files.sh ${{ runner.os }} test-cache
+
+    - name: Generate files outside working directory
+      shell: bash
+      run: packages/cache/__tests__/create-cache-files.sh ${{ runner.os }} ~/test-cache
+
+    # We're using node -e to call the functions directly available in the @actions/cache package
+    - name: Save cache using saveCache()
+      run: |
+        node -e "Promise.resolve(require('./packages/cache/lib/cache').saveCache(['test-cache','~/test-cache'],'test-${{ runner.os }}-${{ github.run_id }}'))"
+
+    - name: Delete cache folders before restoring
+      shell: bash
+      run: |
+        rm -rf test-cache
+        rm -rf ~/test-cache
+
+    - name: Restore cache using restoreCache() with http-client
+      run: |
+        node -e "Promise.resolve(require('./packages/cache/lib/cache').restoreCache(['test-cache','~/test-cache'],'test-${{ runner.os }}-${{ github.run_id }}',[],{useAzureSdk: false}))"
+
+    - name: Verify cache restored with http-client
+      shell: bash
+      run: |
+        packages/cache/__tests__/verify-cache-files.sh ${{ runner.os }} test-cache
+        packages/cache/__tests__/verify-cache-files.sh ${{ runner.os }} ~/test-cache
+
+    - name: Delete cache folders before restoring
+      shell: bash
+      run: |
+        rm -rf test-cache
+        rm -rf ~/test-cache
+        rm -f cache.tar
+
+    - name: Restore cache using restoreCache() with Azure SDK
+      run: |
+        node -e "Promise.resolve(require('./packages/cache/lib/cache').restoreCache(['test-cache','~/test-cache'],'test-${{ runner.os }}-${{ github.run_id }}'))"
+
+    - name: Verify cache restored with Azure SDK
+      shell: bash
+      run: |
+        packages/cache/__tests__/verify-cache-files.sh ${{ runner.os }} test-cache
+        packages/cache/__tests__/verify-cache-files.sh ${{ runner.os }} ~/test-cache

packages/cache/RELEASES.md

@@ -91,3 +91,6 @@
 
 ### 3.0.6
 - Added `@azure/abort-controller` to dependencies to fix compatibility issue with ESM [#1208](https://github.com/actions/toolkit/issues/1208)
+
+### 3.1.0-beta.1
+- Update actions/cache on windows to use gnu tar and zstd by default and fallback to bsdtar and zstd if gnu tar is not available. ([issue](https://github.com/actions/cache/issues/984))

packages/cache/__tests__/restoreCache.test.ts

@@ -161,6 +161,81 @@ test('restore with gzip compressed cache found', async () => {
   expect(getCompressionMock).toHaveBeenCalledTimes(1)
 })
 
+test('restore with zstd as default but gzip compressed cache found on windows', async () => {
+  if (process.platform === 'win32') {
+    const paths = ['node_modules']
+    const key = 'node-test'
+
+    const cacheEntry: ArtifactCacheEntry = {
+      cacheKey: key,
+      scope: 'refs/heads/main',
+      archiveLocation: 'www.actionscache.test/download'
+    }
+    const getCacheMock = jest.spyOn(cacheHttpClient, 'getCacheEntry')
+    getCacheMock
+      .mockImplementationOnce(async () => {
+        return Promise.resolve(null)
+      })
+      .mockImplementationOnce(async () => {
+        return Promise.resolve(cacheEntry)
+      })
+
+    const tempPath = '/foo/bar'
+
+    const createTempDirectoryMock = jest.spyOn(
+      cacheUtils,
+      'createTempDirectory'
+    )
+    createTempDirectoryMock.mockImplementation(async () => {
+      return Promise.resolve(tempPath)
+    })
+
+    const archivePath = path.join(tempPath, CacheFilename.Gzip)
+    const downloadCacheMock = jest.spyOn(cacheHttpClient, 'downloadCache')
+
+    const fileSize = 142
+    const getArchiveFileSizeInBytesMock = jest
+      .spyOn(cacheUtils, 'getArchiveFileSizeInBytes')
+      .mockReturnValue(fileSize)
+
+    const extractTarMock = jest.spyOn(tar, 'extractTar')
+    const unlinkFileMock = jest.spyOn(cacheUtils, 'unlinkFile')
+
+    const compression = CompressionMethod.Zstd
+    const getCompressionMock = jest
+      .spyOn(cacheUtils, 'getCompressionMethod')
+      .mockReturnValue(Promise.resolve(compression))
+
+    const cacheKey = await restoreCache(paths, key)
+
+    expect(cacheKey).toBe(key)
+    expect(getCacheMock).toHaveBeenNthCalledWith(1, [key], paths, {
+      compressionMethod: compression
+    })
+    expect(getCacheMock).toHaveBeenNthCalledWith(2, [key], paths, {
+      compressionMethod: CompressionMethod.Gzip
+    })
+    expect(createTempDirectoryMock).toHaveBeenCalledTimes(1)
+    expect(downloadCacheMock).toHaveBeenCalledWith(
+      cacheEntry.archiveLocation,
+      archivePath,
+      undefined
+    )
+    expect(getArchiveFileSizeInBytesMock).toHaveBeenCalledWith(archivePath)
+
+    expect(extractTarMock).toHaveBeenCalledTimes(1)
+    expect(extractTarMock).toHaveBeenCalledWith(
+      archivePath,
+      CompressionMethod.Gzip
+    )
+
+    expect(unlinkFileMock).toHaveBeenCalledTimes(1)
+    expect(unlinkFileMock).toHaveBeenCalledWith(archivePath)
+
+    expect(getCompressionMock).toHaveBeenCalledTimes(1)
+  }
+})
+
 test('restore with zstd compressed cache found', async () => {
   const paths = ['node_modules']
   const key = 'node-test'

packages/cache/__tests__/tar.test.ts

@@ -73,7 +73,9 @@ test('zstd extract tar', async () => {
         '--use-compress-program',
         IS_WINDOWS ? '"zstd -d --long=30"' : 'unzstd --long=30'
       ])
-      .join(' ')
+      .join(' '),
+    undefined,
+    {cwd: undefined}
   )
 })
 
@@ -92,20 +94,31 @@ test('zstd extract tar with windows BSDtar', async () => {
     await tar.extractTar(archivePath, CompressionMethod.Zstd)
 
     expect(mkdirMock).toHaveBeenCalledWith(workspace)
-    expect(execMock).toHaveBeenCalledTimes(1)
+    expect(execMock).toHaveBeenCalledTimes(2)
-    expect(execMock).toHaveBeenCalledWith(
+
+    expect(execMock).toHaveBeenNthCalledWith(
+      1,
       [
         'zstd -d --long=30 -o',
         TarFilename.replace(new RegExp(`\\${path.sep}`, 'g'), '/'),
-        archivePath.replace(new RegExp(`\\${path.sep}`, 'g'), '/'),
+        archivePath.replace(new RegExp(`\\${path.sep}`, 'g'), '/')
-        '&&',
+      ].join(' '),
+      undefined,
+      {cwd: undefined}
+    )
+
+    expect(execMock).toHaveBeenNthCalledWith(
+      2,
+      [
         `"${tarPath}"`,
         '-xf',
         TarFilename.replace(new RegExp(`\\${path.sep}`, 'g'), '/'),
         '-P',
         '-C',
         workspace?.replace(/\\/g, '/')
-      ].join(' ')
+      ].join(' '),
+      undefined,
+      {cwd: undefined}
     )
   }
 })
@@ -135,7 +148,9 @@ test('gzip extract tar', async () => {
       .concat(IS_WINDOWS ? ['--force-local'] : [])
       .concat(IS_MAC ? ['--delay-directory-restore'] : [])
       .concat(['-z'])
-      .join(' ')
+      .join(' '),
+    undefined,
+    {cwd: undefined}
   )
 })
 
@@ -162,7 +177,9 @@ test('gzip extract GNU tar on windows with GNUtar in path', async () => {
         workspace?.replace(/\\/g, '/'),
         '--force-local',
         '-z'
-      ].join(' ')
+      ].join(' '),
+      undefined,
+      {cwd: undefined}
     )
   }
 })
@@ -230,8 +247,10 @@ test('zstd create tar with windows BSDtar', async () => {
 
     const tarPath = SystemTarPathOnWindows
 
-    expect(execMock).toHaveBeenCalledTimes(1)
+    expect(execMock).toHaveBeenCalledTimes(2)
-    expect(execMock).toHaveBeenCalledWith(
+
+    expect(execMock).toHaveBeenNthCalledWith(
+      1,
       [
         `"${tarPath}"`,
         '--posix',
@@ -243,8 +262,17 @@ test('zstd create tar with windows BSDtar', async () => {
         '-C',
         workspace?.replace(/\\/g, '/'),
         '--files-from',
-        ManifestFilename,
+        ManifestFilename
-        '&&',
+      ].join(' '),
+      undefined, // args
+      {
+        cwd: archiveFolder
+      }
+    )
+
+    expect(execMock).toHaveBeenNthCalledWith(
+      2,
+      [
         'zstd -T0 --long=30 -o',
         CacheFilename.Zstd.replace(/\\/g, '/'),
         TarFilename.replace(/\\/g, '/')
@@ -320,7 +348,9 @@ test('zstd list tar', async () => {
         '--use-compress-program',
         IS_WINDOWS ? '"zstd -d --long=30"' : 'unzstd --long=30'
       ])
-      .join(' ')
+      .join(' '),
+    undefined,
+    {cwd: undefined}
   )
 })
 
@@ -335,18 +365,29 @@ test('zstd list tar with windows BSDtar', async () => {
     await tar.listTar(archivePath, CompressionMethod.Zstd)
 
     const tarPath = SystemTarPathOnWindows
-    expect(execMock).toHaveBeenCalledTimes(1)
+    expect(execMock).toHaveBeenCalledTimes(2)
-    expect(execMock).toHaveBeenCalledWith(
+
+    expect(execMock).toHaveBeenNthCalledWith(
+      1,
       [
         'zstd -d --long=30 -o',
         TarFilename.replace(new RegExp(`\\${path.sep}`, 'g'), '/'),
-        archivePath.replace(new RegExp(`\\${path.sep}`, 'g'), '/'),
+        archivePath.replace(new RegExp(`\\${path.sep}`, 'g'), '/')
-        '&&',
+      ].join(' '),
+      undefined,
+      {cwd: undefined}
+    )
+
+    expect(execMock).toHaveBeenNthCalledWith(
+      2,
+      [
         `"${tarPath}"`,
         '-tf',
         TarFilename.replace(new RegExp(`\\${path.sep}`, 'g'), '/'),
         '-P'
-      ].join(' ')
+      ].join(' '),
+      undefined,
+      {cwd: undefined}
     )
   }
 })
@@ -372,7 +413,9 @@ test('zstdWithoutLong list tar', async () => {
       .concat(IS_WINDOWS ? ['--force-local'] : [])
       .concat(IS_MAC ? ['--delay-directory-restore'] : [])
       .concat(['--use-compress-program', IS_WINDOWS ? '"zstd -d"' : 'unzstd'])
-      .join(' ')
+      .join(' '),
+    undefined,
+    {cwd: undefined}
   )
 })
 
@@ -396,6 +439,8 @@ test('gzip list tar', async () => {
       .concat(IS_WINDOWS ? ['--force-local'] : [])
       .concat(IS_MAC ? ['--delay-directory-restore'] : [])
       .concat(['-z'])
-      .join(' ')
+      .join(' '),
+    undefined,
+    {cwd: undefined}
   )
 })

packages/cache/package-lock.json

@@ -1,12 +1,12 @@
 {
   "name": "@actions/cache",
-  "version": "3.0.6",
+  "version": "3.1.0-beta.1",
   "lockfileVersion": 2,
   "requires": true,
   "packages": {
     "": {
       "name": "@actions/cache",
-      "version": "3.0.6",
+      "version": "3.1.0-beta.1",
       "license": "MIT",
       "dependencies": {
         "@actions/core": "^1.10.0",
@@ -457,9 +457,9 @@
       }
     },
     "node_modules/minimatch": {
-      "version": "3.0.4",
+      "version": "3.1.2",
-      "resolved": "https://registry.npmjs.org/minimatch/-/minimatch-3.0.4.tgz",
+      "resolved": "https://registry.npmjs.org/minimatch/-/minimatch-3.1.2.tgz",
-      "integrity": "sha512-yJHVQEhyqPLUTgt9B83PXu6W3rx4MvvHvSUvToogpwoGDOUQ+yDrR0HRot+yOCdCO7u4hX3pWft6kWBBcqh0UA==",
+      "integrity": "sha512-J7p63hRiAjw1NDEww1W7i37+ByIrOWO5XQQAzZ3VOcL0PNybwpfmV/N05zFAzwQ9USyEcX6t3UO+K5aqBQOIHw==",
       "dependencies": {
         "brace-expansion": "^1.1.7"
       },
@@ -998,9 +998,9 @@
       }
     },
     "minimatch": {
-      "version": "3.0.4",
+      "version": "3.1.2",
-      "resolved": "https://registry.npmjs.org/minimatch/-/minimatch-3.0.4.tgz",
+      "resolved": "https://registry.npmjs.org/minimatch/-/minimatch-3.1.2.tgz",
-      "integrity": "sha512-yJHVQEhyqPLUTgt9B83PXu6W3rx4MvvHvSUvToogpwoGDOUQ+yDrR0HRot+yOCdCO7u4hX3pWft6kWBBcqh0UA==",
+      "integrity": "sha512-J7p63hRiAjw1NDEww1W7i37+ByIrOWO5XQQAzZ3VOcL0PNybwpfmV/N05zFAzwQ9USyEcX6t3UO+K5aqBQOIHw==",
       "requires": {
         "brace-expansion": "^1.1.7"
       }

packages/cache/package.json

@@ -1,6 +1,6 @@
 {
   "name": "@actions/cache",
-  "version": "3.0.6",
+  "version": "3.1.0-beta.1",
   "preview": true,
   "description": "Actions cache lib",
   "keywords": [

packages/cache/src/cache.ts

@@ -4,6 +4,8 @@ import * as utils from './internal/cacheUtils'
 import * as cacheHttpClient from './internal/cacheHttpClient'
 import {createTar, extractTar, listTar} from './internal/tar'
 import {DownloadOptions, UploadOptions} from './options'
+import {CompressionMethod} from './internal/constants'
+import {ArtifactCacheEntry} from './internal/contracts'
 
 export class ValidationError extends Error {
   constructor(message: string) {
@@ -85,17 +87,35 @@ export async function restoreCache(
     checkKey(key)
   }
 
-  const compressionMethod = await utils.getCompressionMethod()
+  let cacheEntry: ArtifactCacheEntry | null
+  let compressionMethod = await utils.getCompressionMethod()
   let archivePath = ''
   try {
     // path are needed to compute version
-    const cacheEntry = await cacheHttpClient.getCacheEntry(keys, paths, {
+    cacheEntry = await cacheHttpClient.getCacheEntry(keys, paths, {
       compressionMethod
     })
-
     if (!cacheEntry?.archiveLocation) {
-      // Cache not found
+      // This is to support the old cache entry created by gzip on windows.
-      return undefined
+      if (
+        process.platform === 'win32' &&
+        compressionMethod !== CompressionMethod.Gzip
+      ) {
+        compressionMethod = CompressionMethod.Gzip
+        cacheEntry = await cacheHttpClient.getCacheEntry(keys, paths, {
+          compressionMethod
+        })
+        if (!cacheEntry?.archiveLocation) {
+          return undefined
+        }
+
+        core.debug(
+          "Couldn't find cache entry with zstd compression, falling back to gzip compression."
+        )
+      } else {
+        // Cache not found
+        return undefined
+      }
     }
 
     archivePath = path.join(

packages/cache/src/internal/cacheHttpClient.ts

@@ -104,6 +104,7 @@ export async function getCacheEntry(
     httpClient.getJson<ArtifactCacheEntry>(getCacheApiUrl(resource))
   )
   if (response.statusCode === 204) {
+    // Cache not found
     return null
   }
   if (!isSuccessStatusCode(response.statusCode)) {
@@ -113,6 +114,7 @@ export async function getCacheEntry(
   const cacheResult = response.result
   const cacheDownloadUrl = cacheResult?.archiveLocation
   if (!cacheDownloadUrl) {
+    // Cache achiveLocation not found. This should never happen, and hence bail out.
     throw new Error('Cache not found.')
   }
   core.setSecret(cacheDownloadUrl)

packages/cache/src/internal/tar.ts

@@ -14,7 +14,7 @@ import {
 
 const IS_WINDOWS = process.platform === 'win32'
 
-// Function also mutates the args array. For non-mutation call with passing an empty array.
+// Returns tar path and type: BSD or GNU
 async function getTarPath(): Promise<ArchiveTool> {
   switch (process.platform) {
     case 'win32': {
@@ -43,6 +43,7 @@ async function getTarPath(): Promise<ArchiveTool> {
     default:
       break
   }
+  // Default assumption is GNU tar is present in path
   return <ArchiveTool>{
     path: await io.which('tar', true),
     type: ArchiveToolType.GNU
@@ -60,6 +61,7 @@ async function getTarArgs(
   const cacheFileName = utils.getCacheFileName(compressionMethod)
   const tarFile = 'cache.tar'
   const workingDirectory = getWorkingDirectory()
+  // Speficic args for BSD tar on windows for workaround
   const BSD_TAR_ZSTD =
     tarPath.type === ArchiveToolType.BSD &&
     compressionMethod !== CompressionMethod.Gzip &&
@@ -122,11 +124,14 @@ async function getTarArgs(
   return args
 }
 
-async function getArgs(
+// Returns commands to run tar and compression program
+async function getCommands(
   compressionMethod: CompressionMethod,
   type: string,
   archivePath = ''
-): Promise<string> {
+): Promise<string[]> {
+  let args
+
   const tarPath = await getTarPath()
   const tarArgs = await getTarArgs(
     tarPath,
@@ -142,11 +147,18 @@ async function getArgs(
     tarPath.type === ArchiveToolType.BSD &&
     compressionMethod !== CompressionMethod.Gzip &&
     IS_WINDOWS
+
   if (BSD_TAR_ZSTD && type !== 'create') {
-    return [...compressionArgs, ...tarArgs].join(' ')
+    args = [[...compressionArgs].join(' '), [...tarArgs].join(' ')]
   } else {
-    return [...tarArgs, ...compressionArgs].join(' ')
+    args = [[...tarArgs].join(' '), [...compressionArgs].join(' ')]
   }
+
+  if (BSD_TAR_ZSTD) {
+    return args
+  }
+
+  return [args.join(' ')]
 }
 
 function getWorkingDirectory(): string {
@@ -173,8 +185,7 @@ async function getDecompressionProgram(
         ? [
             'zstd -d --long=30 -o',
             TarFilename,
-            archivePath.replace(new RegExp(`\\${path.sep}`, 'g'), '/'),
+            archivePath.replace(new RegExp(`\\${path.sep}`, 'g'), '/')
-            '&&'
           ]
         : [
             '--use-compress-program',
@@ -185,8 +196,7 @@ async function getDecompressionProgram(
         ? [
             'zstd -d -o',
             TarFilename,
-            archivePath.replace(new RegExp(`\\${path.sep}`, 'g'), '/'),
+            archivePath.replace(new RegExp(`\\${path.sep}`, 'g'), '/')
-            '&&'
           ]
         : ['--use-compress-program', IS_WINDOWS ? '"zstd -d"' : 'unzstd']
     default:
@@ -194,6 +204,7 @@ async function getDecompressionProgram(
   }
 }
 
+// Used for creating the archive
 // -T#: Compress using # working thread. If # is 0, attempt to detect and use the number of physical CPU cores.
 // zstdmt is equivalent to 'zstd -T0'
 // --long=#: Enables long distance matching with # bits. Maximum is 30 (1GB) on 32-bit OS and 31 (2GB) on 64-bit.
@@ -212,7 +223,6 @@ async function getCompressionProgram(
     case CompressionMethod.Zstd:
       return BSD_TAR_ZSTD
         ? [
-            '&&',
             'zstd -T0 --long=30 -o',
             cacheFileName.replace(new RegExp(`\\${path.sep}`, 'g'), '/'),
             TarFilename
@@ -224,7 +234,6 @@ async function getCompressionProgram(
     case CompressionMethod.ZstdWithoutLong:
       return BSD_TAR_ZSTD
         ? [
-            '&&',
             'zstd -T0 -o',
             cacheFileName.replace(new RegExp(`\\${path.sep}`, 'g'), '/'),
             TarFilename
@@ -235,18 +244,29 @@ async function getCompressionProgram(
   }
 }
 
+// Executes all commands as separate processes
+async function execCommands(commands: string[], cwd?: string): Promise<void> {
+  for (const command of commands) {
+    try {
+      await exec(command, undefined, {cwd})
+    } catch (error) {
+      throw new Error(
+        `${command.split(' ')[0]} failed with error: ${error?.message}`
+      )
+    }
+  }
+}
+
+// List the contents of a tar
 export async function listTar(
   archivePath: string,
   compressionMethod: CompressionMethod
 ): Promise<void> {
-  const args = await getArgs(compressionMethod, 'list', archivePath)
+  const commands = await getCommands(compressionMethod, 'list', archivePath)
-  try {
+  await execCommands(commands)
-    await exec(args)
-  } catch (error) {
-    throw new Error(`Tar failed with error: ${error?.message}`)
-  }
 }
 
+// Extract a tar
 export async function extractTar(
   archivePath: string,
   compressionMethod: CompressionMethod
@@ -254,14 +274,11 @@ export async function extractTar(
   // Create directory to extract tar into
   const workingDirectory = getWorkingDirectory()
   await io.mkdirP(workingDirectory)
-  const args = await getArgs(compressionMethod, 'extract', archivePath)
+  const commands = await getCommands(compressionMethod, 'extract', archivePath)
-  try {
+  await execCommands(commands)
-    await exec(args)
-  } catch (error) {
-    throw new Error(`Tar failed with error: ${error?.message}`)
-  }
 }
 
+// Create a tar
 export async function createTar(
   archiveFolder: string,
   sourceDirectories: string[],
@@ -272,10 +289,6 @@ export async function createTar(
     path.join(archiveFolder, ManifestFilename),
     sourceDirectories.join('\n')
   )
-  const args = await getArgs(compressionMethod, 'create')
+  const commands = await getCommands(compressionMethod, 'create')
-  try {
+  await execCommands(commands, archiveFolder)
-    await exec(args, undefined, {cwd: archiveFolder})
-  } catch (error) {
-    throw new Error(`Tar failed with error: ${error?.message}`)
-  }
 }