Skip to content

Fall back to direct cache write if tempfile creation on the same fs fails #2369

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

myzhang1029
Copy link

Please see #2288 for the discussion.

@codecov-commenter
Copy link

codecov-commenter commented Apr 15, 2025

Codecov Report

Attention: Patch coverage is 97.14286% with 2 lines in your changes missing coverage. Please review.

Project coverage is 67.23%. Comparing base (7025295) to head (0bd6c96).

Files with missing lines Patch % Lines
src/cache/cache.rs 97.14% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2369      +/-   ##
==========================================
+ Coverage   67.06%   67.23%   +0.17%     
==========================================
  Files          64       64              
  Lines       34718    34783      +65     
==========================================
+ Hits        23285    23388     +103     
+ Misses      11433    11395      -38     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sylvestre
Copy link
Collaborator

would it be possible to add a test to make sure we don't regress in the future?
thanks

@myzhang1029
Copy link
Author

@sylvestre
I am not familiar with a similar concept under Windows, so is it fine if I hardcode a test for something under /dev and make this test case cfg(unix)?

@myzhang1029
Copy link
Author

myzhang1029 commented May 26, 2025

Rebased on main and added two tests. Both tests succeed on the branch and fail before the patch.

I am not entirely sure if the use of /dev/fd is portable enough to all the unices that Rust and sccache intend to support. Please feel free to skip that one if not.

@sylvestre
Copy link
Collaborator

could you please add high level tests in https://github.com/mozilla/sccache/blob/main/tests/system.rs too ? thanks

like

fn test_sccache_command(preprocessor_cache_mode: bool) {

@myzhang1029
Copy link
Author

myzhang1029 commented Jun 16, 2025

@sylvestre Sorry for the delayed response. I am planning to test this by adding two functions that are similar to test_basic_compile in system.rs.

One of them should test /dev/null and the other for /dev/stdout (AssertCmd::assert seems to capture stdout already --- better approach maybe?) They would:

  1. Compiles a file
  2. Repeats the compilation command
  3. Check that both commands succeed and that #miss and #hit are both 1.

@myzhang1029
Copy link
Author

myzhang1029 commented Jun 16, 2025

On a second thought, I have a question:

I think I named the PR a bit too narrow. Maybe like

Fall back to direct cache write if parent directory is not writable

I was think of reframing the PR this way, because this way, we don't have to hardcode /dev anymore. We can instead:

  1. run the first compilation command
  2. chmod u-w the destination dir
  3. run the second (cached) pass

@@ -207,22 +208,41 @@ impl CacheRead {
optional,
} in objects
{
if path == Path::new("/dev/null") {
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be NUL for Windows with MSVC, isn't it?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry I do not have a MSVC environment to test this so I wasn't sure.

For gcc, it seems to be complicated by gcc appending .exe automatically:

  • -o NUL: it gives me NUL.exe
  • -o NUL.: ld.exe: cannot open output file NUL.: Invalid argument
  • -Wl,-o,NUL: ld.exe: final link failed: file truncated
  • -c -o NUL: nothing (good)
  • -c -o NUL.o: NUL.o (as expected)

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we should support -c -o NUL as a base one, isn't it?

P.S. I don't think we really support -o NUL variant with the compiling & linking together: @Xuanwo , do we?

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.

4 participants