diff --git a/Cargo.toml b/Cargo.toml index 6bdb354e03..100bf0abf4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -131,7 +131,7 @@ members = ["download"] [workspace.package] version = "1.28.0" -edition = "2021" +edition = "2024" license = "MIT OR Apache-2.0" [workspace.dependencies] diff --git a/download/tests/read-proxy-env.rs b/download/tests/read-proxy-env.rs index 7858fd4b6e..1ec8998b05 100644 --- a/download/tests/read-proxy-env.rs +++ b/download/tests/read-proxy-env.rs @@ -15,24 +15,30 @@ use url::Url; static SERIALISE_TESTS: LazyLock> = LazyLock::new(|| Mutex::new(())); -fn scrub_env() { - remove_var("http_proxy"); - remove_var("https_proxy"); - remove_var("HTTPS_PROXY"); - remove_var("ftp_proxy"); - remove_var("FTP_PROXY"); - remove_var("all_proxy"); - remove_var("ALL_PROXY"); - remove_var("no_proxy"); - remove_var("NO_PROXY"); +unsafe fn scrub_env() { + unsafe { + remove_var("http_proxy"); + remove_var("https_proxy"); + remove_var("HTTPS_PROXY"); + remove_var("ftp_proxy"); + remove_var("FTP_PROXY"); + remove_var("all_proxy"); + remove_var("ALL_PROXY"); + remove_var("no_proxy"); + remove_var("NO_PROXY"); + } } // Tests for correctly retrieving the proxy (host, port) tuple from $https_proxy #[tokio::test] async fn read_basic_proxy_params() { let _guard = SERIALISE_TESTS.lock().await; - scrub_env(); - set_var("https_proxy", "http://proxy.example.com:8080"); + // SAFETY: We are setting environment variables when `SERIALISE_TESTS` is locked, + // and those environment variables in question are not relevant elsewhere in the test suite. + unsafe { + scrub_env(); + set_var("https_proxy", "http://proxy.example.com:8080"); + } let u = Url::parse("https://www.example.org").ok().unwrap(); assert_eq!( for_url(&u).host_port(), @@ -46,8 +52,12 @@ async fn socks_proxy_request() { static CALL_COUNT: AtomicUsize = AtomicUsize::new(0); let _guard = SERIALISE_TESTS.lock().await; - scrub_env(); - set_var("all_proxy", "socks5://127.0.0.1:1080"); + // SAFETY: We are setting environment variables when `SERIALISE_TESTS` is locked, + // and those environment variables in question are not relevant elsewhere in the test suite. + unsafe { + scrub_env(); + set_var("all_proxy", "socks5://127.0.0.1:1080"); + } thread::spawn(move || { let listener = TcpListener::bind("127.0.0.1:1080").unwrap(); diff --git a/rustfmt.toml b/rustfmt.toml deleted file mode 100644 index 3501136812..0000000000 --- a/rustfmt.toml +++ /dev/null @@ -1 +0,0 @@ -style_edition = "2024" diff --git a/src/cli/job.rs b/src/cli/job.rs index f31e2973f8..9552c031b8 100644 --- a/src/cli/job.rs +++ b/src/cli/job.rs @@ -58,47 +58,49 @@ mod imp { } pub(crate) unsafe fn setup() -> Option { - // Creates a new job object for us to use and then adds ourselves to it. - // Note that all errors are basically ignored in this function, - // intentionally. Job objects are "relatively new" in Windows, - // particularly the ability to support nested job objects. Older - // Windows installs don't support this ability. We probably don't want - // to force Cargo to abort in this situation or force others to *not* - // use job objects, so we instead just ignore errors and assume that - // we're otherwise part of someone else's job object in this case. - - let job = CreateJobObjectW(ptr::null_mut(), ptr::null()); - if job.is_null() { - return None; - } - let job = Handle { inner: job }; - - // Indicate that when all handles to the job object are gone that all - // process in the object should be killed. Note that this includes our - // entire process tree by default because we've added ourselves and - // our children will reside in the job once we spawn a process. - let mut info: JOBOBJECT_EXTENDED_LIMIT_INFORMATION; - info = mem::zeroed(); - info.BasicLimitInformation.LimitFlags = JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE; - let r = SetInformationJobObject( - job.inner, - JobObjectExtendedLimitInformation, - &mut info as *mut _ as *const std::ffi::c_void, - mem::size_of_val(&info) as u32, - ); - if r == 0 { - return None; - } + unsafe { + // Creates a new job object for us to use and then adds ourselves to it. + // Note that all errors are basically ignored in this function, + // intentionally. Job objects are "relatively new" in Windows, + // particularly the ability to support nested job objects. Older + // Windows installs don't support this ability. We probably don't want + // to force Cargo to abort in this situation or force others to *not* + // use job objects, so we instead just ignore errors and assume that + // we're otherwise part of someone else's job object in this case. + + let job = CreateJobObjectW(ptr::null_mut(), ptr::null()); + if job.is_null() { + return None; + } + let job = Handle { inner: job }; + + // Indicate that when all handles to the job object are gone that all + // process in the object should be killed. Note that this includes our + // entire process tree by default because we've added ourselves and + // our children will reside in the job once we spawn a process. + let mut info: JOBOBJECT_EXTENDED_LIMIT_INFORMATION; + info = mem::zeroed(); + info.BasicLimitInformation.LimitFlags = JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE; + let r = SetInformationJobObject( + job.inner, + JobObjectExtendedLimitInformation, + &mut info as *mut _ as *const std::ffi::c_void, + mem::size_of_val(&info) as u32, + ); + if r == 0 { + return None; + } - // Assign our process to this job object, meaning that our children will - // now live or die based on our existence. - let me = GetCurrentProcess(); - let r = AssignProcessToJobObject(job.inner, me); - if r == 0 { - return None; - } + // Assign our process to this job object, meaning that our children will + // now live or die based on our existence. + let me = GetCurrentProcess(); + let r = AssignProcessToJobObject(job.inner, me); + if r == 0 { + return None; + } - Some(Setup { job }) + Some(Setup { job }) + } } impl Drop for Setup { diff --git a/src/cli/log.rs b/src/cli/log.rs index 532dda3c20..15c4d8ab33 100644 --- a/src/cli/log.rs +++ b/src/cli/log.rs @@ -20,7 +20,7 @@ use crate::{process::Process, utils::notify::NotificationLevel}; pub fn tracing_subscriber( process: &Process, ) -> ( - impl tracing::Subscriber, + impl tracing::Subscriber + use<>, reload::Handle, ) { #[cfg(feature = "otel")] @@ -45,7 +45,7 @@ pub fn tracing_subscriber( /// When the `RUSTUP_LOG` environment variable is present, a standard [`tracing_subscriber`] /// formatter will be used according to the filtering directives set in its value. /// Otherwise, this logger will use [`EventFormatter`] to mimic "classic" Rustup `stderr` output. -fn console_logger(process: &Process) -> (impl Layer, reload::Handle) +fn console_logger(process: &Process) -> (impl Layer + use, reload::Handle) where S: Subscriber + for<'span> LookupSpan<'span>, { @@ -129,7 +129,7 @@ impl NotificationLevel { /// A [`tracing::Subscriber`] [`Layer`][`tracing_subscriber::Layer`] that corresponds to Rustup's /// optional `opentelemetry` (a.k.a. `otel`) feature. #[cfg(feature = "otel")] -fn telemetry(process: &Process) -> impl Layer +fn telemetry(process: &Process) -> impl Layer + use where S: Subscriber + for<'span> LookupSpan<'span>, { diff --git a/src/diskio/immediate.rs b/src/diskio/immediate.rs index 2bc2a5d415..991248bc16 100644 --- a/src/diskio/immediate.rs +++ b/src/diskio/immediate.rs @@ -69,8 +69,8 @@ impl Executor for ImmediateUnpacker { fn dispatch(&self, mut item: Item) -> Box + '_> { item.result = match &mut item.kind { super::Kind::Directory => super::create_dir(&item.full_path), - super::Kind::File(ref contents) => { - if let super::FileBuffer::Immediate(ref contents) = &contents { + super::Kind::File(contents) => { + if let super::FileBuffer::Immediate(contents) = &contents { super::write_file(&item.full_path, contents, item.mode) } else { unreachable!() @@ -81,21 +81,20 @@ impl Executor for ImmediateUnpacker { // If there is a pending error, return it, otherwise stash the // Item for eventual return when the file is finished. let mut guard = self.incremental_state.lock().unwrap(); - if let Some(ref mut state) = *guard { - if state.err.is_some() { - let err = state.err.take().unwrap(); - item.result = err; - item.finish = item - .start - .map(|s| Instant::now().saturating_duration_since(s)); - *guard = None; - Box::new(Some(CompletedIo::Item(item)).into_iter()) - } else { - state.item = Some(item); - Box::new(None.into_iter()) - } + let Some(ref mut state) = *guard else { + unreachable!() + }; + if state.err.is_some() { + let err = state.err.take().unwrap(); + item.result = err; + item.finish = item + .start + .map(|s| Instant::now().saturating_duration_since(s)); + *guard = None; + Box::new(Some(CompletedIo::Item(item)).into_iter()) } else { - unreachable!(); + state.item = Some(item); + Box::new(None.into_iter()) } }; } @@ -181,9 +180,7 @@ impl IncrementalFileWriter { if (self.state.lock().unwrap()).is_none() { return false; } - let chunk = if let FileBuffer::Immediate(v) = chunk { - v - } else { + let FileBuffer::Immediate(chunk) = chunk else { unreachable!() }; match self.write(chunk) { @@ -203,25 +200,23 @@ impl IncrementalFileWriter { fn write(&mut self, chunk: Vec) -> std::result::Result { let mut state = self.state.lock().unwrap(); - if let Some(ref mut state) = *state { - if let Some(ref mut file) = self.file.as_mut() { - // Length 0 vector is used for clean EOF signalling. - if chunk.is_empty() { - trace_scoped!("close", "name:": self.path_display); - drop(std::mem::take(&mut self.file)); - state.finished = true; - } else { - trace_scoped!("write_segment", "name": self.path_display, "len": chunk.len()); - file.write_all(&chunk)?; - - state.completed_chunks.push(chunk.len()); - } - Ok(true) - } else { - Ok(false) - } + let Some(ref mut state) = *state else { + unreachable!() + }; + let Some(ref mut file) = self.file.as_mut() else { + return Ok(false); + }; + // Length 0 vector is used for clean EOF signalling. + if chunk.is_empty() { + trace_scoped!("close", "name:": self.path_display); + drop(std::mem::take(&mut self.file)); + state.finished = true; } else { - unreachable!(); + trace_scoped!("write_segment", "name": self.path_display, "len": chunk.len()); + file.write_all(&chunk)?; + + state.completed_chunks.push(chunk.len()); } + Ok(true) } } diff --git a/src/diskio/mod.rs b/src/diskio/mod.rs index 49e9793909..0fea84dd7d 100644 --- a/src/diskio/mod.rs +++ b/src/diskio/mod.rs @@ -81,14 +81,14 @@ pub(crate) enum FileBuffer { impl FileBuffer { /// All the buffers space to be re-used when the last reference to it is dropped. pub(crate) fn clear(&mut self) { - if let FileBuffer::Threaded(ref mut contents) = self { + if let FileBuffer::Threaded(contents) = self { contents.clear() } } pub(crate) fn len(&self) -> usize { match self { - FileBuffer::Immediate(ref vec) => vec.len(), + FileBuffer::Immediate(vec) => vec.len(), FileBuffer::Threaded(PoolReference::Owned(owned, _)) => owned.len(), FileBuffer::Threaded(PoolReference::Mut(mutable, _)) => mutable.len(), } @@ -109,7 +109,7 @@ impl Deref for FileBuffer { fn deref(&self) -> &Self::Target { match self { - FileBuffer::Immediate(ref vec) => vec, + FileBuffer::Immediate(vec) => vec, FileBuffer::Threaded(PoolReference::Owned(owned, _)) => owned, FileBuffer::Threaded(PoolReference::Mut(mutable, _)) => mutable, } @@ -119,7 +119,7 @@ impl Deref for FileBuffer { impl DerefMut for FileBuffer { fn deref_mut(&mut self) -> &mut Self::Target { match self { - FileBuffer::Immediate(ref mut vec) => vec, + FileBuffer::Immediate(vec) => vec, FileBuffer::Threaded(PoolReference::Owned(_, _)) => { unimplemented!() } @@ -337,15 +337,11 @@ pub(crate) fn perform(item: &mut Item, chunk_complete_callback: F) // Files, write them. item.result = match &mut item.kind { Kind::Directory => create_dir(&item.full_path), - Kind::File(ref mut contents) => { + Kind::File(contents) => { contents.clear(); match contents { - FileBuffer::Immediate(ref contents) => { - write_file(&item.full_path, contents, item.mode) - } - FileBuffer::Threaded(ref mut contents) => { - write_file(&item.full_path, contents, item.mode) - } + FileBuffer::Immediate(contents) => write_file(&item.full_path, contents, item.mode), + FileBuffer::Threaded(contents) => write_file(&item.full_path, contents, item.mode), } } Kind::IncrementalFile(incremental_file) => write_file_incremental( diff --git a/src/dist/component/package.rs b/src/dist/component/package.rs index a3759eb388..82bc823862 100644 --- a/src/dist/component/package.rs +++ b/src/dist/component/package.rs @@ -229,10 +229,9 @@ fn filter_result(op: &mut CompletedIo) -> io::Result<()> { // mkdir of e.g. ~/.rustup already existing is just fine; // for others it would be better to know whether it is // expected to exist or not -so put a flag in the state. - if let Kind::Directory = op.kind { - Ok(()) - } else { - Err(e) + match op.kind { + Kind::Directory => Ok(()), + _ => Err(e), } } _ => Err(e), @@ -454,40 +453,40 @@ fn unpack_without_first_dir( let item = loop { // Create the full path to the entry if it does not exist already - if let Some(parent) = item.full_path.to_owned().parent() { - match directories.get_mut(parent) { - None => { - // Tar has item before containing directory - // Complain about this so we can see if these exist. - writeln!( - process.stderr().lock(), - "Unexpected: missing parent '{}' for '{}'", - parent.display(), - entry.path()?.display() - )?; - directories.insert(parent.to_owned(), DirStatus::Pending(vec![item])); - item = Item::make_dir(parent.to_owned(), 0o755); - // Check the parent's parent - continue; - } - Some(DirStatus::Exists) => { - break Some(item); - } - Some(DirStatus::Pending(pending)) => { - // Parent dir is being made - pending.push(item); - if incremental_file_sender.is_none() { - // take next item from tar - continue 'entries; - } else { - // don't submit a new item for processing, but do be ready to feed data to the incremental file. - break None; - } + let full_path = item.full_path.to_owned(); + let Some(parent) = full_path.parent() else { + // We should never see a path with no parent. + unreachable!() + }; + match directories.get_mut(parent) { + None => { + // Tar has item before containing directory + // Complain about this so we can see if these exist. + writeln!( + process.stderr().lock(), + "Unexpected: missing parent '{}' for '{}'", + parent.display(), + entry.path()?.display() + )?; + directories.insert(parent.to_owned(), DirStatus::Pending(vec![item])); + item = Item::make_dir(parent.to_owned(), 0o755); + // Check the parent's parent + continue; + } + Some(DirStatus::Exists) => { + break Some(item); + } + Some(DirStatus::Pending(pending)) => { + // Parent dir is being made + pending.push(item); + if incremental_file_sender.is_none() { + // take next item from tar + continue 'entries; + } else { + // don't submit a new item for processing, but do be ready to feed data to the incremental file. + break None; } } - } else { - // We should never see a path with no parent. - panic!(); } }; diff --git a/src/dist/mod.rs b/src/dist/mod.rs index bfcfdb9585..ef6cd95a8b 100644 --- a/src/dist/mod.rs +++ b/src/dist/mod.rs @@ -335,10 +335,9 @@ impl FromStr for ParsedToolchainDesc { } }); - if let Some(d) = d { - Ok(d) - } else { - Err(RustupError::InvalidToolchainName(desc.to_string()).into()) + match d { + Some(d) => Ok(d), + None => Err(RustupError::InvalidToolchainName(desc.to_string()).into()), } } } diff --git a/src/test/mock/clitools.rs b/src/test/mock/clitools.rs index c6c7f44122..80f2095126 100644 --- a/src/test/mock/clitools.rs +++ b/src/test/mock/clitools.rs @@ -192,19 +192,33 @@ impl ConstState { /// State a test can interact and mutate pub async fn setup_test_state(test_dist_dir: tempfile::TempDir) -> (tempfile::TempDir, Config) { - // Unset env variables that will break our testing - env::remove_var("RUSTUP_UPDATE_ROOT"); - env::remove_var("RUSTUP_TOOLCHAIN"); - env::remove_var("SHELL"); - env::remove_var("ZDOTDIR"); - // clap does its own terminal colour probing, and that isn't - // trait-controllable, but it does honour the terminal. To avoid testing - // claps code, lie about whatever terminal this process was started under. - env::set_var("TERM", "dumb"); - - match env::var("RUSTUP_BACKTRACE") { - Ok(val) => env::set_var("RUST_BACKTRACE", val), - _ => env::remove_var("RUST_BACKTRACE"), + // SAFETY: This is probably not the best way of doing such a thing, but it should be + // okay since we are setting the environment variables for the integration tests only. + // There are two types of integration test in rustup: in-process and subprocess. + // For the former, the environment variables are 100% injected via [`TestContext::vars`]; + // for the latter, the environment variables in question are only relevant in the + // corresponding subprocesses. Thus, it should be safe to assume that the following won't + // cause inconsistencies as far as **this** particular process is concerned, as long as + // **each subprocess gets the same value for every environment variable listed below when + // it spins off**. To do so, we will have to ensure that: + // - The following `unsafe` block is idempotent, making its output absolutely stable. + // - The environment variables listed below are never modified to anything else + // **in this process** when the tests are still running. + unsafe { + // Unset env variables that will break our testing + env::remove_var("RUSTUP_UPDATE_ROOT"); + env::remove_var("RUSTUP_TOOLCHAIN"); + env::remove_var("SHELL"); + env::remove_var("ZDOTDIR"); + // clap does its own terminal colour probing, and that isn't + // trait-controllable, but it does honour the terminal. To avoid testing + // claps code, lie about whatever terminal this process was started under. + env::set_var("TERM", "dumb"); + + match env::var("RUSTUP_BACKTRACE") { + Ok(val) => env::set_var("RUST_BACKTRACE", val), + _ => env::remove_var("RUST_BACKTRACE"), + } } let current_exe_path = env::current_exe().unwrap(); diff --git a/src/toolchain.rs b/src/toolchain.rs index 2f0bde1e87..e65d9f443d 100644 --- a/src/toolchain.rs +++ b/src/toolchain.rs @@ -87,11 +87,11 @@ impl<'a> Toolchain<'a> { ActiveReason::CommandLine => { "the +toolchain on the command line specifies an uninstalled toolchain".to_string() } - ActiveReason::OverrideDB(ref path) => format!( + ActiveReason::OverrideDB(path) => format!( "the directory override for '{}' specifies an uninstalled toolchain", utils::canonicalize_path(path, cfg.notify_handler.as_ref()).display(), ), - ActiveReason::ToolchainFile(ref path) => format!( + ActiveReason::ToolchainFile(path) => format!( "the toolchain file at '{}' specifies an uninstalled toolchain", utils::canonicalize_path(path, cfg.notify_handler.as_ref()).display(), ),