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.

Sign up for GitHub

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
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c3175e9
Skip cache output to /dev/null
myzhang1029 Mar 20, 2025
13e7863
Fall back to direct write if tmp creation fails
myzhang1029 Mar 20, 2025
28e5c00
Fix typo
myzhang1029 Apr 10, 2025
93e7b18
Test for cache extraction to /dev/null
myzhang1029 May 26, 2025
62fb7c7
Test for cache extraction to /dev/fd/{n}
myzhang1029 May 26, 2025
99afb88
Fix rustfmt
myzhang1029 May 26, 2025
4306c7c
High-level tests into /dev/(null|stdout)
myzhang1029 Jun 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 87 additions & 5 deletions src/cache/cache.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ use fs_err as fs;

use serde::{Deserialize, Serialize};
use std::fmt;
use std::fs::File;
use std::io::{self, Cursor, Read, Seek, Write};
use std::path::{Path, PathBuf};
use std::sync::Arc;
Expand Down Expand Up @@ -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?

debug!("Skipping output to /dev/null");
continue;
}
let dir = match path.parent() {
Some(d) => d,
None => bail!("Output file without a parent directory!"),
};
// Write the cache entry to a tempfile and then atomically
// move it to its final location so that other rustc invocations
// happening in parallel don't see a partially-written file.
let mut tmp = NamedTempFile::new_in(dir)?;
match (self.get_object(&key, &mut tmp), optional) {
(Ok(mode), _) => {
tmp.persist(&path)?;
match (NamedTempFile::new_in(dir), optional) {
(Ok(mut tmp), _) => {
match (self.get_object(&key, &mut tmp), optional) {
(Ok(mode), _) => {
tmp.persist(&path)?;
if let Some(mode) = mode {
set_file_mode(&path, mode)?;
}
}
(Err(e), false) => return Err(e),
// skip if no object found and it's optional
(Err(_), true) => continue,
}
}
(Err(e), false) => {
// Fall back to writing directly to the final location
warn!("Failed to create temp file on the same file system: {e}");
let mut f = File::create(&path)?;
// `optional` is false in this branch, so do not ignore errors
let mode = self.get_object(&key, &mut f)?;
if let Some(mode) = mode {
set_file_mode(&path, mode)?;
}
}
(Err(e), false) => return Err(e),
// skip if no object found and it's optional
(Err(_), true) => continue,
}
Expand Down Expand Up @@ -823,4 +843,66 @@ mod test {
});
}
}

#[cfg(unix)]
#[test]
fn test_extract_object_to_devnull_works() {
let runtime = tokio::runtime::Builder::new_current_thread()
.enable_all()
.worker_threads(1)
.build()
.unwrap();

let pool = runtime.handle();

let cache_data = CacheWrite::new();
let cache_read = CacheRead::from(io::Cursor::new(cache_data.finish().unwrap())).unwrap();

let objects = vec![FileObjectSource {
key: "test_key".to_string(),
path: PathBuf::from("/dev/null"),
optional: false,
}];

let result = runtime.block_on(cache_read.extract_objects(objects, pool));
assert!(result.is_ok(), "Extracting to /dev/null should succeed");
}

#[cfg(unix)]
#[test]
fn test_extract_object_to_dev_fd_something() {
// Open a pipe, write to `/dev/fd/{fd}` and check the other end that the correct data was written.
use std::os::fd::AsRawFd;
use tokio::io::AsyncReadExt;
let runtime = tokio::runtime::Builder::new_current_thread()
.enable_all()
.worker_threads(1)
.build()
.unwrap();
let pool = runtime.handle();
let mut cache_data = CacheWrite::new();
let data = b"test data";
cache_data.put_bytes("test_key", data).unwrap();
let cache_read = CacheRead::from(io::Cursor::new(cache_data.finish().unwrap())).unwrap();
runtime.block_on(async {
let (sender, mut receiver) = tokio::net::unix::pipe::pipe().unwrap();
let sender_fd = sender.into_blocking_fd().unwrap();
let raw_fd = sender_fd.as_raw_fd();
let objects = vec![FileObjectSource {
key: "test_key".to_string(),
path: PathBuf::from(format!("/dev/fd/{}", raw_fd)),
optional: false,
}];
let result = cache_read.extract_objects(objects, pool).await;
assert!(
result.is_ok(),
"Extracting to /dev/fd/{} should succeed",
raw_fd
);
let mut buf = vec![0; data.len()];
let n = receiver.read_exact(&mut buf).await.unwrap();
assert_eq!(n, data.len(), "Read the correct number of bytes");
assert_eq!(buf, data, "Read the correct data from /dev/fd/{}", raw_fd);
});
}
}
118 changes: 118 additions & 0 deletions tests/system.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,8 @@ const INPUT_FOR_HIP_A: &str = "test_a.hip";
const INPUT_FOR_HIP_B: &str = "test_b.hip";
const INPUT_FOR_HIP_C: &str = "test_c.hip";
const OUTPUT: &str = "test.o";
const DEV_NULL: &str = "/dev/null";
const DEV_STDOUT: &str = "/dev/stdout";

// Copy the source files into the tempdir so we can compile with relative paths, since the commandline winds up in the hash key.
fn copy_to_tempdir(inputs: &[&str], tempdir: &Path) {
Expand Down Expand Up @@ -263,6 +265,120 @@ fn test_basic_compile(compiler: Compiler, tempdir: &Path) {
});
}

#[cfg(unix)]
fn test_basic_compile_into_dev_null(compiler: Compiler, tempdir: &Path) {
let Compiler {
name,
exe,
env_vars,
} = compiler;
println!("test_basic_compile: {}", name);
// Compile a source file.
copy_to_tempdir(&[INPUT, INPUT_ERR], tempdir);

let out_file = tempdir.join(OUTPUT);
trace!("compile");
sccache_command()
.args(compile_cmdline(name, &exe, INPUT, DEV_NULL, Vec::new()))
.current_dir(tempdir)
.envs(env_vars.clone())
.assert()
.success();
trace!("request stats");
get_stats(|info| {
assert_eq!(1, info.stats.compile_requests);
assert_eq!(1, info.stats.requests_executed);
assert_eq!(0, info.stats.cache_hits.all());
assert_eq!(1, info.stats.cache_misses.all());
assert_eq!(&1, info.stats.cache_misses.get("C/C++").unwrap());
let adv_key = adv_key_kind("c", compiler.name);
assert_eq!(&1, info.stats.cache_misses.get_adv(&adv_key).unwrap());
});
trace!("compile");
fs::remove_file(&out_file).unwrap();
sccache_command()
.args(compile_cmdline(name, &exe, INPUT, DEV_NULL, Vec::new()))
.current_dir(tempdir)
.envs(env_vars)
.assert()
.success();
assert!(fs::metadata(&out_file).map(|m| m.len() > 0).unwrap());
trace!("request stats");
get_stats(|info| {
assert_eq!(2, info.stats.compile_requests);
assert_eq!(2, info.stats.requests_executed);
assert_eq!(1, info.stats.cache_hits.all());
assert_eq!(1, info.stats.cache_misses.all());
assert_eq!(&1, info.stats.cache_hits.get("C/C++").unwrap());
assert_eq!(&1, info.stats.cache_misses.get("C/C++").unwrap());
let adv_key = adv_key_kind("c", compiler.name);
assert_eq!(&1, info.stats.cache_hits.get_adv(&adv_key).unwrap());
assert_eq!(&1, info.stats.cache_misses.get_adv(&adv_key).unwrap());
});
}

#[cfg(not(unix))]
fn test_basic_compile_into_dev_null(_: Compiler, _: &Path) {
warn!("Not unix, skipping /dev tests");
}

#[cfg(unix)]
fn test_basic_compile_into_dev_stdout(compiler: Compiler, tempdir: &Path) {
let Compiler {
name,
exe,
env_vars,
} = compiler;
println!("test_basic_compile: {}", name);
// Compile a source file.
copy_to_tempdir(&[INPUT, INPUT_ERR], tempdir);

let out_file = tempdir.join(OUTPUT);
trace!("compile");
sccache_command()
.args(compile_cmdline(name, &exe, INPUT, DEV_STDOUT, Vec::new()))
.current_dir(tempdir)
.envs(env_vars.clone())
.assert()
.success();
trace!("request stats");
get_stats(|info| {
assert_eq!(1, info.stats.compile_requests);
assert_eq!(1, info.stats.requests_executed);
assert_eq!(0, info.stats.cache_hits.all());
assert_eq!(1, info.stats.cache_misses.all());
assert_eq!(&1, info.stats.cache_misses.get("C/C++").unwrap());
let adv_key = adv_key_kind("c", compiler.name);
assert_eq!(&1, info.stats.cache_misses.get_adv(&adv_key).unwrap());
});
trace!("compile");
fs::remove_file(&out_file).unwrap();
sccache_command()
.args(compile_cmdline(name, &exe, INPUT, DEV_STDOUT, Vec::new()))
.current_dir(tempdir)
.envs(env_vars)
.assert()
.success();
assert!(fs::metadata(&out_file).map(|m| m.len() > 0).unwrap());
trace!("request stats");
get_stats(|info| {
assert_eq!(2, info.stats.compile_requests);
assert_eq!(2, info.stats.requests_executed);
assert_eq!(1, info.stats.cache_hits.all());
assert_eq!(1, info.stats.cache_misses.all());
assert_eq!(&1, info.stats.cache_hits.get("C/C++").unwrap());
assert_eq!(&1, info.stats.cache_misses.get("C/C++").unwrap());
let adv_key = adv_key_kind("c", compiler.name);
assert_eq!(&1, info.stats.cache_hits.get_adv(&adv_key).unwrap());
assert_eq!(&1, info.stats.cache_misses.get_adv(&adv_key).unwrap());
});
}

#[cfg(not(unix))]
fn test_basic_compile_into_dev_stdout(_: Compiler, _: &Path) {
warn!("Not unix, skipping /dev tests");
}

fn test_noncacheable_stats(compiler: Compiler, tempdir: &Path) {
let Compiler {
name,
Expand Down Expand Up @@ -629,6 +745,8 @@ fn run_sccache_command_tests(compiler: Compiler, tempdir: &Path, preprocessor_ca
test_basic_compile(compiler.clone(), tempdir);
}
test_compile_with_define(compiler.clone(), tempdir);
test_basic_compile_into_dev_null(compiler.clone(), tempdir);
test_basic_compile_into_dev_stdout(compiler.clone(), tempdir);
if compiler.name == "cl.exe" {
test_msvc_deps(compiler.clone(), tempdir);
test_msvc_responsefile(compiler.clone(), tempdir);
Expand Down
Loading