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 zstd failing on windows when used with the gnu tar workaround #1152

Merged
merged 10 commits into from
Aug 18, 2022

Conversation

lvpx
Copy link
Contributor

@lvpx lvpx commented Aug 13, 2022

This PR aims to resolve the issues #888 and #891 in actions/cache.

Context

We had introduced zstd aliases in actions/cache v3.0.6 to fix issues with zstd failing for tar version < 1.30. That change was made in understanding that we have zstd disabled on windows(bsd tar). However, due to dismal gzip performance on windows runners, developers have been using a workaround to use gnu tar along with zstd. The above release broke this workaround.

Approach

We have switched to using short flag versions(ex. zstd -d) of zstd command for windows on gnu tar instead of the alias versions(ex. unzstd). This restores the workaround functionality same as before the release v3.0.6. This approach has been implemented in this PR.

The downsides of using this approach:

  • The change is quite specific in nature and doesn't cover other potential issues we might face with using zstd aliases.

Alternative Approach

We also considered reverting the change done in v3.0.6 and instead implement zstd aliases based on tar versions. This would also work correctly as it has been working before. We only use zstd aliases for tar version < 1.30.

The downside of using this approach:

  • Extra system call to tar version to fetch version for every cache call.
  • Parsing of tar version stdout output. The tar version is not in semantic versioning format so could be brittle to parse and test.
  • Also tar version parsing needs to be implemented.

Feedback is welcome for choosing between both approaches and code review as well. 👀

@lvpx lvpx force-pushed the pdotl-zstd-win-patch branch from c1a7558 to 98a4069 Compare August 17, 2022 08:33
@lvpx lvpx force-pushed the pdotl-zstd-win-patch branch from a3159a1 to ce68daa Compare August 18, 2022 11:02
@lvpx lvpx merged commit bc4be50 into main Aug 18, 2022
@lvpx lvpx deleted the pdotl-zstd-win-patch branch August 26, 2022 05:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants