Bsdtar + zstd fallback to prevent zstd hanging issue on windows by lvpx · Pull Request #1237 · actions/toolkit

lvpx · 2022-11-16T21:11:49Z

This PR implements the bsdtar + zstd fallback for windows to prevent the bsdtar + zstd hanging on windows issue highlighted in #301

Context and Motivation

bsdtar on windows runners tends to hang when called with --use-compress-program zstd. During testing we found the issue can be circumvented by calling bsdtar and zstd as separate processes such as tar [OPTIONS] && zstd [OPTIONS]. If tar fails then zstd won't run.

For compression, command would be similar to tar [OPTIONS] && zstd [OPTIONS] and for decompression, command would be similar to zstd -d [OPTIONS] && tar [OPTIONS].

This PR implements this approach as a fallback method since from #1239 gnutar will be default on windows where this problem does not exist.

Requesting Feedback

The ordering of arguments while forming the tar command that executes the archiving and compression is what has changed, so the change has larger exposure than required. Feel free to suggest better ways to implement this fallback where the impact can be minimized.

Relevant Documentation

Parent Public issue: Cache compression - cross OS support cache#984

packages/cache/src/internal/tar.ts

… unless with GNUtar

packages/cache/__tests__/tar.test.ts

bishal-pdMSFT · 2022-11-21T07:13:31Z

packages/cache/src/internal/tar.ts

+    ...(await getCompressionProgram()),
+    cacheFileName.replace(new RegExp(`\\${path.sep}`, 'g'), '/')


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.

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

bishal-pdMSFT · 2022-11-21T07:18:13Z

packages/cache/src/internal/tar.ts

-  function getCompressionProgram(): string[] {
+  async function getCompressionProgram(): Promise<string[]> {
+    const tarPath = await getTarPath([])
+    const BSD_TAR_ZSTD = IS_WINDOWS && tarPath === SystemTarPathOnWindows


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.

Makes sense

bishal-pdMSFT · 2022-11-21T07:21:07Z

packages/cache/src/internal/tar.ts

  switch (compressionMethod) {
    case CompressionMethod.Zstd:
+      if (BSD_TAR_ZSTD) {
+        return ['-a'] // auto-detect compression


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

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

bishal-pdMSFT · 2022-11-21T08:23:44Z

packages/cache/src/internal/tar.ts

  // 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([])


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:

Create a separate function to just get tar path

Or move settings the args to getCompressionProgram where anyways we are setting more arguments

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

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

bishal-pdMSFT · 2022-11-21T08:31:13Z

@Phantsure @pdotl overall I feel that the logic to find right command for various combinations is very spread-out and hence difficult to maintain. Should we think about simplifying it (in this or a separate PR)?

Decide tar and its path
For that tar decide compression
A map for create/extract command which has three pivots - tar, compression and OS.

Phantsure · 2022-11-21T11:38:15Z

I agree with you how the logic is spread at different places.

A map for create/extract command which has three pivots - tar, compression and OS.

@pdotl also pointed this out. Let me try to work on reducing the number of pivots.

packages/cache/src/internal/tar.ts

lvpx added 5 commits November 16, 2022 20:18

bsd + zstd fallback implementation

6349c3c

bsd + zstd fallback implementation

db3517f

Fix tar operations

27bf830

Add -v option for testing

e9e146b

Fix order of args for tar

5f89653

Phantsure reviewed Nov 17, 2022

View reviewed changes

packages/cache/src/internal/tar.ts Outdated Show resolved Hide resolved

Phantsure reviewed Nov 17, 2022

View reviewed changes

packages/cache/src/internal/tar.ts Outdated Show resolved Hide resolved

Phantsure reviewed Nov 17, 2022

View reviewed changes

packages/cache/src/internal/tar.ts Outdated Show resolved Hide resolved

Phantsure changed the base branch from main to releases/cache-v3-beta November 17, 2022 08:21

lvpx added 2 commits November 17, 2022 08:53

Fix tar tests

ea98560

Fix lint issues

4fa5b7d

lvpx marked this pull request as ready for review November 17, 2022 09:21

lvpx requested a review from a team as a code owner November 17, 2022 09:21

lvpx requested a review from bishal-pdMSFT November 17, 2022 09:22

lvpx changed the title ~~Bsdtar + zstd fallback to prevent zstd hanging issue on windows~~ [WIP] Bsdtar + zstd fallback to prevent zstd hanging issue on windows Nov 17, 2022

lvpx added 2 commits November 17, 2022 09:46

Fix windows gnutar test case

b3bd482

Temporarily remove thhe condition that prevents zstd usage on windows…

f9dfb05

… unless with GNUtar

Phantsure reviewed Nov 18, 2022

View reviewed changes

packages/cache/__tests__/tar.test.ts Outdated Show resolved Hide resolved

bishal-pdMSFT reviewed Nov 21, 2022

View reviewed changes

Phantsure self-assigned this Nov 21, 2022

Phantsure added 4 commits November 21, 2022 12:05

Address some comments and correct compression commands

54eb9b8

Add windows bsdtar test

32b9582

Fix windows test

32f5381

Fix test

ffde3e4

Phantsure marked this pull request as draft November 22, 2022 10:58

Phantsure added 2 commits November 23, 2022 07:39

Separate args

2f73afa

Fix old tests

39b7a86

Phantsure force-pushed the pdotl/win-bsdtar-zstd branch from c5abe60 to 2163245 Compare November 23, 2022 10:51

Add new tests

1f33717

Phantsure force-pushed the pdotl/win-bsdtar-zstd branch from 2163245 to 1f33717 Compare November 23, 2022 11:20

Phantsure added 2 commits November 23, 2022 11:40

Fix tests

187781e

Fix lint test

0822441

Phantsure marked this pull request as ready for review November 23, 2022 11:52

Phantsure changed the title ~~[WIP] Bsdtar + zstd fallback to prevent zstd hanging issue on windows~~ Bsdtar + zstd fallback to prevent zstd hanging issue on windows Nov 23, 2022

Refactor code

0fd856d

bishal-pdMSFT reviewed Nov 29, 2022

View reviewed changes

packages/cache/src/internal/tar.ts Show resolved Hide resolved

bishal-pdMSFT reviewed Nov 29, 2022

View reviewed changes

packages/cache/src/internal/tar.ts Outdated Show resolved Hide resolved

bishal-pdMSFT reviewed Nov 29, 2022

View reviewed changes

packages/cache/src/internal/tar.ts Outdated Show resolved Hide resolved

bishal-pdMSFT approved these changes Nov 29, 2022

View reviewed changes

Phantsure added 4 commits November 29, 2022 10:35

Address review comments

34f0143

Fix test

2e5a517

Fix tar test

424ae62

Add await to async function calls

afbc5c0

Phantsure force-pushed the pdotl/win-bsdtar-zstd branch from e4ce959 to 98fe6a2 Compare November 30, 2022 10:17

Fix test

8595831

Phantsure force-pushed the pdotl/win-bsdtar-zstd branch from 98fe6a2 to 8595831 Compare November 30, 2022 10:37

Phantsure merged commit 785599d into releases/cache-v3-beta Dec 1, 2022

		...(await getCompressionProgram()),
		cacheFileName.replace(new RegExp(`\\${path.sep}`, 'g'), '/')

Conversation

lvpx commented Nov 16, 2022 • edited by Phantsure Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context and Motivation

Requesting Feedback

Relevant Documentation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bishal-pdMSFT commented Nov 21, 2022

Uh oh!

Phantsure commented Nov 21, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lvpx commented Nov 16, 2022 •

edited by Phantsure

Loading