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

sccache-action usage: Use GHA as local cache #2142

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

Conversation

Alphare
Copy link
Contributor

@Alphare Alphare commented Apr 3, 2024

This is a draft PR to implement the base for a fix for Mozilla-Actions/sccache-action#81.

Some users have been implementing their own fix by going around the sccache-action and making their own cache in GHAc (Mozilla-Actions/sccache-action#50), but I think that this allows us to be more generic by moving more of the complexity into sccache itself, which makes it easier for end-users and allows us to re-use the same principle for other remote storage systems in the future.

This will need to be accompanied by a separate PR in sccache-action to make use of the new config option(s) and the new --update-cache command.

I've listed TODOs in the code as questions for reviewers, hence the draft state.

@codecov-commenter
Copy link

codecov-commenter commented Apr 3, 2024

Codecov Report

Attention: Patch coverage is 9.32203% with 107 lines in your changes missing coverage. Please review.

Project coverage is 30.22%. Comparing base (0cc0c62) to head (464138d).
Report is 71 commits behind head on main.

Files Patch % Lines
src/cache/gha.rs 0.00% 69 Missing ⚠️
src/cache/cache.rs 18.91% 25 Missing and 5 partials ⚠️
src/config.rs 25.00% 4 Missing and 2 partials ⚠️
src/server.rs 50.00% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2142      +/-   ##
==========================================
- Coverage   30.91%   30.22%   -0.69%     
==========================================
  Files          53       53              
  Lines       20112    20590     +478     
  Branches     9755    10029     +274     
==========================================
+ Hits         6217     6224       +7     
- Misses       7922     8281     +359     
- Partials     5973     6085     +112     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Alphare Alphare force-pushed the gha-local branch 2 times, most recently from 9a8cc59 to 37bf416 Compare April 4, 2024 07:22
@Xuanwo
Copy link
Collaborator

Xuanwo commented Apr 9, 2024

Fascinating idea! Actually, I believe it could be a broader solution applicable to all storage services. For instance, downloading from S3/GCS/AZBlob should also be useful.

What do you think?

@Alphare
Copy link
Contributor Author

Alphare commented Apr 16, 2024

Fascinating idea! Actually, I believe it could be a broader solution applicable to all storage services. For instance, downloading from S3/GCS/AZBlob should also be useful.

What do you think?

Indeed, I even mentioned this briefly in this PR's first message. I think this can make sense, but I didn't want to start with a more generic implementation before I could get a sense of how correct my solution is to this problem.

Is there any chance you could answer the few questions I've left as comments in the code?

@Alphare
Copy link
Contributor Author

Alphare commented Apr 22, 2024

I think this is ready for a first review. I'll try to somehow get sccache-action to test this in the meantime.

@sylvestre sylvestre marked this pull request as ready for review April 22, 2024 15:31
@sylvestre sylvestre changed the title WIP GHA as local cache sccache-action usage: Use GHA as local cache Apr 22, 2024
@sylvestre
Copy link
Collaborator

I guess you saw that some jobs are failing

Cargo.toml Outdated Show resolved Hide resolved
@Alphare Alphare force-pushed the gha-local branch 2 times, most recently from 80ac943 to ae6430a Compare April 23, 2024 11:53
@Alphare
Copy link
Contributor Author

Alphare commented Apr 23, 2024

After a long battle I finally managed in a (very very) hacky way to exercise this path¹.

I'll have to make adjustments here when debugging.

[1] https://github.com/Mozilla-Actions/sccache-action/actions/runs/8803242248/job/24160865310

@Alphare Alphare force-pushed the gha-local branch 2 times, most recently from 33b7587 to 7b240de Compare April 24, 2024 08:45
src/cmdline.rs Outdated
@@ -274,6 +282,15 @@ pub fn try_parse() -> Result<Command> {
Ok(Command::DistAuth)
} else if matches.get_flag("dist-status") {
Ok(Command::DistStatus)
} else if matches.get_flag("upload-cache") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi, I'm considering whether we can add this feature without altering the existing workflow. For instance, we could introduce a new configuration value in sccache, such as remote_cache. When the sccache daemon starts, it would load from the remote cache and then write back to it upon shutdown or at regular intervals.

This approach would eliminate the need for users to manually download or upload the cache.

What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

In this design, we just need to add a new input in sccache-action without changing the workflow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can definitely see the "upon shutdown" version work and I could possibly already use it in this PR. I don't think it could work for regular intervals since GHA for example does not allow re-uploading to the same cache version+path.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can definitely see the "upon shutdown" version work and I could possibly already use it in this PR. I don't think it could work for regular intervals since GHA for example does not allow re-uploading to the same cache version+path.

You are right. I'm guessing regular intervals is not that useful too. We can remove this part.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can definitely see the "upon shutdown" version work and I could possibly already use it in this PR.

The key point I want to emphasize is not to add new command lines or alter the existing workflow that utilizes sccache. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, agreed, I think it's a better design anyway. :)

@Alphare Alphare force-pushed the gha-local branch 10 times, most recently from 5056ed1 to 464138d Compare April 24, 2024 16:42
This is the first implementation of uploading the local cache
as a single file to a remote cache for reuse in a future build.

Right now it is only done for GHA as that was the intended scope¹,
but one could adapt this system to other remote caches.

Because of the immutability of GHACache, this commit only adds support
for re-using the cache for the same version (as defined by the user
through the `SCCACHE_GHA_VERSION` environment variable).
A way of reusing incremental build within a given version or even
across versions could be devised, but it falls outside the scope of
this particular effort, and it's probably not trivial.

[1] Mozilla-Actions/sccache-action#81
@Alphare
Copy link
Contributor Author

Alphare commented Apr 24, 2024

I have failed to fully get this to a working state because of an unknown error (in the gha-as-local integration test), and because I've now truly ran out of time on this, but I feel like this is very close to working for a single version.

I have detailed my thoughts about the remaining work in a TODO inside the code to make it easier to contextualize for a further effort, as agreed to with @sylvestre in a private chat.

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.

5 participants