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 - getCacheVersion - dup paths array #1378

Merged
merged 1 commit into from Jan 10, 2024

Conversation

MSP-Greg
Copy link
Contributor

@MSP-Greg MSP-Greg commented Mar 20, 2023

Previous to commit b2d865f, code used components = paths.concat(...), which left paths unchanged.

The commit changed to using components = paths, then pushing onto components later. This changes the paths array.

Closes #1377

@MSP-Greg
Copy link
Contributor Author

MSP-Greg commented Mar 21, 2023

Not sure how deep the copy/clone should be. Maybe a better choice would be the below, or something similar:

JSON.parse(JSON.stringify(arr))

EDIT: js isn't my 'primary' langauge, at least for several years. I think .slice() should work...

@MSP-Greg
Copy link
Contributor Author

MSP-Greg commented Mar 21, 2023

I think the below is a fairly common pattern using this code is:

// full_key is often generated from dependency lock file
restoreKey = restoreCache(paths, full_key, partialKeys)
if (restoreKey !== full_key) {
  // update dependencies
  saveCache(paths, full_key)
}

So, if paths is being changed by the code here, the version from getCacheVersion will change.

Consumers of the code can replace restoreCache(paths, full_key, partialKeys) with restoreCache(paths.slice(), full_key, partialKeys) or an equivalent. Regardless, the code here should not be changing parameters passed to its functions...

@WebeWizard
Copy link

bump. I think a lot of people have been patiently waiting for this fix.

@deemp
Copy link

deemp commented Jul 17, 2023

@kotewar, @rentziass, please, review this PR.

The fix suggested in #1378 (comment) worked for me.

After nix-community/cache-nix-action@93041f2 cache was restored successfully in the second run https://github.com/deemp/cache-nix-too/actions/runs/5573652888/jobs/10181692942#step:4:19

xfangfang added a commit to xfangfang/flatpak-github-actions that referenced this pull request Jul 24, 2023
For more information, please refer to: actions/toolkit#1378 (comment)
xfangfang added a commit to xfangfang/flatpak-github-actions that referenced this pull request Jul 24, 2023
Bertg added a commit to Bertg/npm-install that referenced this pull request Aug 1, 2023
An issue in action/toolkit modifies the inputPath values passed to cache function. These changes work around this issue. 

For more details see: actions/toolkit#1378

Co-authored-by: Justin Leis <justin.leis@webewizard.com>
Bertg added a commit to Bertg/npm-install that referenced this pull request Aug 1, 2023
An issue in action/toolkit modifies the inputPath values passed to cache function. These changes work around this issue.

For more details see: actions/toolkit#1378

Co-authored-by: Justin Leis <justin.leis@webewizard.com>
bahmutov pushed a commit to bahmutov/npm-install that referenced this pull request Aug 1, 2023
- closes #175 

An issue in action/toolkit modifies the inputPath values passed to cache function. These changes work around this issue.

For more details see: actions/toolkit#1378

Co-authored-by: Justin Leis <justin.leis@webewizard.com>
garbas added a commit to flox/install-flox-action that referenced this pull request Oct 20, 2023
@threeal
Copy link

threeal commented Dec 6, 2023

When are we going to merge this? I just stumble upon this issue too

@ali-kafel
Copy link

I have run into this issue as well can we merge and release please?

@ali-kafel
Copy link

ali-kafel commented Dec 20, 2023

@robherley @bethanyj28 @konradpabjan Sorry for mentioning you but it would be great if we could get a review on this from the actions team

@robherley robherley merged commit cab491a into actions:main Jan 10, 2024
@MSP-Greg MSP-Greg deleted the 00-cache-paths-dup branch January 10, 2024 23:17
@robherley
Copy link
Member

👋 The @actions/cache package now has this fix within v3.2.3, thanks for the contribution!

deemp added a commit to nix-community/cache-nix-action that referenced this pull request Mar 10, 2024
…since we use the latest @cache-nix-action/cache which is a mod of @actions/cache which has this bug fixed actions/cache#1302
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.

@actions/cache not restoring cache, version mismatch?
6 participants