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

Remove save-always usage #934

Merged
merged 9 commits into from
Nov 6, 2024
Merged

Conversation

DavisVaughan
Copy link
Member

Closes #928

I don't think this is quite right yet but it may be as far as i can take it

When the GitHub team deprecated save-always, they provided some guidance on how to update:
https://github.com/actions/cache/tree/main/save#always-save-cache

The gist is that we have to split our usage into separate explicit save/restore steps.

I am quite uncertain of how to use the magic always() if conditional, what we want is:

  • Save the cache if inputs.cache = 'always' || inputs.cache = 'true' and the workflow hasn't failed so far
  • Save the cache if inputs.cache = 'always', regardless of whether or not the workflow has failed

I tried encoding this using always() but it feels fishy. I'm not sure if always() needs to be at the very beginning of the if: or not. Here are the docs on it:
https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/evaluate-expressions-in-workflows-and-actions#always

Copy link
Member

@gaborcsardi gaborcsardi left a comment

Choose a reason for hiding this comment

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

Thanks! I modified it a bit, so the two code paths are completely independent now. I think it is easier to understand and implement it this way.

@jeroen
Copy link
Member

jeroen commented Nov 6, 2024

Ran into some errors running a test but maybe these are unrelated:

On windows R-4.1

Run actions/cache/save@v4
"C:\Program Files\Git\usr\bin\tar.exe" --posix -cf cache.tzst --exclude cache.tzst -P -C D:/a/curl/curl --files-from manifest.txt --force-local --use-compress-program "zstd -T0"
zstd: error 26 : Read error : I/O error 
/usr/bin/tar: cache.tzst: Cannot write: Broken pipe
/usr/bin/tar: Child returned status 26
/usr/bin/tar: Error is not recoverable: exiting now

If we have concurrent jobs on the same OS, only the first one finishing saves the cache, the other ones error, but perhaps that is okay. It does not fail the job.

Failed to save: Unable to reserve cache with key Windows Server 2022 x64 (build 20348)-R version 4.4.2 (2024-10-31 ucrt)-1-4b9c62d9f8670a99ac6f570a1b9aed10418af9e1ee363075564aecf65ae30455, another job may be creating this cache. More details: Cache already exists. 

@gaborcsardi
Copy link
Member

gaborcsardi commented Nov 6, 2024

Not sure this works: jeroen/curl/actions/runs/11702329781/job/32590296640

What am I supposed to look at there? I am fairly sure that it works, see the links in #934 (comment)

If we have concurrent jobs on the same OS, only the first one finishing saves the cache, the other ones error

What can we do with that?

@jeroen
Copy link
Member

jeroen commented Nov 6, 2024

Sorry I think the cache: always feature works. The missing caches were due to the other things mentioned above.

@gaborcsardi
Copy link
Member

I don't think it is setup-r-dependencies's job to handle concurrency. But even if it was, what can it do?

@gaborcsardi gaborcsardi merged commit 108c7a6 into r-lib:v2-branch Nov 6, 2024
26 checks passed
@gaborcsardi
Copy link
Member

Thanks!

@jeroen
Copy link
Member

jeroen commented Nov 6, 2024

I don't think it is setup-r-dependencies's job to handle concurrency. But even if it was, what can it do?

You are right, I was considering if we should add the job-id to the caching key but this unavailable during the run anyway. And it works fine the way it does now, the first one of the concurrent jobs to finish gets to save the cache and the other ones silently skip saving.

Should I open a separate issue for the above zstd issue on Windows ?

@gaborcsardi
Copy link
Member

You are right, I was considering if we should add the job-id to the caching key

Then you'd never get a cache hit, no? You'd also get a lot of (useless) caches, if every job saves its own cache.

Should I open a separate issue for the above zstd issue on Windows ?

Seems to be coming from the cache action, so maybe there? But isn't that just because of the concurrency?

@jeroen
Copy link
Member

jeroen commented Nov 6, 2024

Seems to be coming from the cache action, so maybe there? But isn't that just because of the concurrency?

It happens earlier and only for R < 4.2. Maybe an issue with zstd in rtools40 I don't know.

@gaborcsardi
Copy link
Member

gaborcsardi commented Nov 6, 2024

That's possible, but they should really set the PATH properly instead of picking up whatever binary they find. Anyway, I can take a look and adjust the PATH I guess.

#940

Copy link

This pull request has been automatically locked. If you believe you have found a related problem, please file a new issue and include a link to this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

save-always deprecation warning from actions/cache
3 participants