Skip to content

perf(test): try putting tmp rustup installations in OS' tmp directory #5212

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

Draft
rami3l wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

perf(test): try putting tmp rustup installations in OS' tmp directory #5212

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ed5185c
fix(test): restore old directory for tmp rustup installations
rami3l Nov 13, 2026
32f410d
perf(test): try putting tmp rustup installations in OS' tmp directory
rami3l Nov 13, 2026
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
53 changes: 46 additions & 7 deletions src/test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,11 @@ use std::{

use sha2::{Digest, Sha256};

use crate::{dist::TargetTuple, process::TestProcess};
use crate::{dist::TargetTuple, process::TestProcess, utils::raw};

#[cfg(all(windows, any(test, feature = "test")))]
pub(crate) fn test_id() -> String {
crate::utils::raw::random_string(8)
raw::random_string(8)
}

#[cfg(windows)]
Expand Down Expand Up @@ -75,12 +75,51 @@ impl Env for HashMap<String, String> {

/// The path to a dir for this test binaries state
fn exe_test_dir() -> io::Result<PathBuf> {
let current_exe_path = env::current_exe().unwrap();
let mut exe_dir = current_exe_path.parent().unwrap();
if exe_dir.ends_with("deps") {
exe_dir = exe_dir.parent().unwrap();
let curr_exe = env::current_exe().unwrap();
let curr_exe_dir = curr_exe.parent().unwrap();

// Make an empty dir and try to rename it to under the OS' tmp dir to check if they are on the
// same device. If they are, we can conveniently use the renamed dir as the base for our test
// directories. If not, we try to put the base dir in `./target/` as before.
let probe_dir = curr_exe_dir.join(format!("rustup-test-{}", raw::random_string(8)));
raw::ensure_dir_exists(&probe_dir, |_| ())?;
let tmp_test_dir = env::temp_dir().join("rustup-test");

let Err(e) = fs::rename(&probe_dir, &tmp_test_dir) else {
return Ok(tmp_test_dir);
};

fs::remove_dir(&probe_dir)?;
match e.kind() {
io::ErrorKind::DirectoryNotEmpty => return Ok(tmp_test_dir),
io::ErrorKind::CrossesDevices => (),
_ => return Err(e),
}
Ok(exe_dir.parent().unwrap().to_owned())

let mut comps = curr_exe_dir.components().collect::<Vec<_>>();
let retained_comps = match &comps[..] {

@ChrisDenton ChrisDenton Nov 14, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we do something that's less dependent on output directory layout? E.g. can we use CARGO_TARGET_TMPDIR or so?

View changes since the review

@rami3l rami3l Nov 14, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@ChrisDenton This is complicated, because neither CARGO_TARGET_TMPDIR nor CARGO_BIN_EXE_rustup-init is guaranteed to be set on both integration tests and unit tests, and this function is a common infrastructure of both sides, meaning if we need a fallback anyway on one side, it might just as well be the actual implementation :( Maybe you have a better idea?

@ChrisDenton ChrisDenton Nov 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Well my better idea would be "maybe we should ask the cargo team" even if as a feature request. Because I know we aren't the only ones that need some scratch space for test infra.

But short of that, manually implementing it is fine. I just don't love it.

// New `cargo` output directory layout since Rust v1.100.0:
// <base>/(debug|release)/build/rustup/<unit-hash>/out/<current_exe>
[base @ .., _profile, build, rustup, _hash, out]
if build.as_os_str() == "build"
&& rustup.as_os_str() == "rustup"
&& out.as_os_str() == "out" =>
{
base.len()
}
// Legacy layout before Rust v1.100.0:
// <base>/(debug|release)/deps/<current_exe>
[base @ .., _profile, deps] if deps.as_os_str() == "deps" => base.len(),
_ => {
return Err(io::Error::other(format!(
"unexpected current exe path format: {}",
curr_exe.display()
)));
}
};

comps.truncate(retained_comps);
Ok(PathBuf::from_iter(comps))
}

/// Returns a tempdir for running tests in
Expand Down
3 changes: 1 addition & 2 deletions tests/suite/cli_rustup.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4315,10 +4315,9 @@ async fn only_toml_in_rust_toolchain_toml() {
cx.config
.expect(["rustc", "--version"])
.await
.extend_redactions([("[CWD]", &cwd)])
.with_stderr(snapbox::str![[r#"
...
error: could not parse override file: '[CWD]/rust-toolchain.toml'[..]
error: could not parse override file: '[..]/rust-toolchain.toml'[..]
...
"#]])
.is_err();
Expand Down
Loading