-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 package release for compression change in windows with symlink fix #1291
Cache package release for compression change in windows with symlink fix #1291
Conversation
208618d
to
da162e4
Compare
!compressionMethod || compressionMethod === CompressionMethod.Gzip | ||
? [] | ||
: [compressionMethod] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove this as we are not doing any special casing for gzip?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will cause cache invalidation, but that will get masked by invalidation due to windows-only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is there so that cache created by gzip is only restorable by gzip. If self-hosted runners don't have zstd then it falls back to gzip.
We can remove this if we callout that having zstd in path is a requirement for cache to work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then in that case we can remove gzip from our code. And that seems like a big change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't the fallback logic take care of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bishal-pdMSFT Yes, but you were asking if we should remove it. Right? We need to keep some component of compression algo in version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So just to understand, if we remove this statement, cache invalidation will happen because the newer versions will include gzip
in their version hash and that won't match for older gzip caches. So even in this case, shouldn't gzip caches only be restorable by using gzip. Previously they were identified by not having gzip
in version but moving forward will have gzip
in version. I think this is what @bishal-pdMSFT is referring to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously they were identified by not having gzip in version but moving forward will have gzip in version.
Yes, this is what I meant. We need not put empty compression method here for gzip and let it be handled just like any other compression method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously they were identified by not having gzip in version but moving forward will have gzip in version.
Then we can do that. Remove empty compression case here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty string still remains if no compression method is passed. Removed gzip case. This was present since initial commit. Let me check if it is really needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added comments
!compressionMethod || compressionMethod === CompressionMethod.Gzip | ||
? [] | ||
: [compressionMethod] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So just to understand, if we remove this statement, cache invalidation will happen because the newer versions will include gzip
in their version hash and that won't match for older gzip caches. So even in this case, shouldn't gzip caches only be restorable by using gzip. Previously they were identified by not having gzip
in version but moving forward will have gzip
in version. I think this is what @bishal-pdMSFT is referring to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
…-1289-revert-1281-phantsure/compression-changes-release
|
||
const IS_WINDOWS = process.platform === 'win32' | ||
exportVariable('MSYS', 'winsymlinks:nativestrict') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a grave mistake. This code is supposed to be a library and not leave any remnants in the environment after it ran.
This can cause failures in GitHub workflow runs (and indeed, it did over here). You basically changed the behavior of every workflow run that involves MSYS2 (or for that matter, Git for Windows).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks like it would address the issue, if in any release. I was pretty certain that I was at the current @actions/cache
version when I raised the concern, though, so I am fairly confident that no released version fixes this? Am I wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are correct. We were in the process of getting it merged, will be creating a release to get it live.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you.
// Add compression method to cache version to restore | ||
// compressed cache as per compression method | ||
if (compressionMethod) { | ||
components.push(compressionMethod) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bug, it mutates the user-provided argument paths
, fix in #1378
Please check commits after first commit to understand the changes done
Original PR: #1281