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

cache: add support for zstd --adapt #1772

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

smorimoto
Copy link
Contributor

@smorimoto smorimoto commented Jul 23, 2024

If I remember correctly, we had that support before, but somehow it seems to have been removed at some point. This PR not only brings it back to keep the cache toolkit fast, but also "actually" enables long mode as described below.

At least the official GitHub Actions images have fairly new zstd installed on all platforms, so once merged we get all the benefits.

Comment on lines -105 to +112
if (versionOutput === '') {
if (version === null) {
return CompressionMethod.Gzip
} else {
} else if (semver.lt(version, '1.3.2')) {
return CompressionMethod.ZstdWithoutLong
} else if (semver.lt(version, '1.3.6')) {
return CompressionMethod.ZstdWithoutAdapt
} else {
return CompressionMethod.Zstd
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also fix this bug, as it seems we have not actually been able to use long mode yet.

@@ -207,6 +207,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'
// --adapt: Dynamically adapt compression level to I/O conditions.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smorimoto smorimoto marked this pull request as ready for review July 23, 2024 13:28
@smorimoto smorimoto requested a review from a team as a code owner July 23, 2024 13:28
@smorimoto
Copy link
Contributor Author

Ready to merge!

@smorimoto
Copy link
Contributor Author

Gentle ping @robherley

@smorimoto
Copy link
Contributor Author

@robherley ping

@smorimoto
Copy link
Contributor Author

@nebuk89 How do we move this forward?

@smorimoto
Copy link
Contributor Author

Gentle reminder

@smorimoto
Copy link
Contributor Author

@joshmgross ping

@smorimoto
Copy link
Contributor Author

ping @joshmgross

@joshmgross
Copy link
Member

👋 Hey @smorimoto,

Thank you for this change!

I don't think we have capacity to fully validate and test this change at the moment, but I'll keep in mind in the future when we're ready to invest more in this cache package.

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