From a78ff4bb104b3996bb2405831a6c169f27e80460 Mon Sep 17 00:00:00 2001 From: Chris Manchester Date: Wed, 6 Nov 2019 15:17:55 -0800 Subject: [PATCH] Take a toolchain archive file into account when deciding to re-build rust. One source of issues with our custom toolchains is that swapping out archives will sometimes require a clobber: if a mis-matched version of a rust toolchain is specified and compiles sucessfully, future compiles may fail due to the presence of that compiled artifact, and updating the specified archive will not be enough to fix things, because the build system will not know to re-build. This change takes advantage of the distributed output re-writer we already use to modify cargo dep files during distributed builds to also add the distributed archive (if any) as a dependency. --- src/compiler/compiler.rs | 44 +++++++++++++++++++++++++++++----------- src/compiler/rust.rs | 7 +++++++ src/dist/cache.rs | 36 ++++++++++++++++++-------------- src/dist/http.rs | 2 +- src/dist/mod.rs | 2 +- 5 files changed, 62 insertions(+), 29 deletions(-) diff --git a/src/compiler/compiler.rs b/src/compiler/compiler.rs index 17f81dcd1f..7a4706d290 100644 --- a/src/compiler/compiler.rs +++ b/src/compiler/compiler.rs @@ -481,13 +481,15 @@ where debug!("[{}]: Identifying dist toolchain for {:?}", compile_out_pretty2, local_executable); dist_client.put_toolchain(&local_executable, &weak_toolchain_key, toolchain_packager) .and_then(|(dist_toolchain, maybe_dist_compile_executable)| { - if let Some(dist_compile_executable) = maybe_dist_compile_executable { + let mut tc_archive = None; + if let Some((dist_compile_executable, archive_path)) = maybe_dist_compile_executable { dist_compile_cmd.executable = dist_compile_executable; + tc_archive = Some(archive_path); } - Ok((dist_client, dist_compile_cmd, dist_toolchain, inputs_packager, outputs_rewriter, dist_output_paths)) + Ok((dist_client, dist_compile_cmd, dist_toolchain, inputs_packager, outputs_rewriter, dist_output_paths, tc_archive)) }) }) - .and_then(move |(dist_client, dist_compile_cmd, dist_toolchain, inputs_packager, outputs_rewriter, dist_output_paths)| { + .and_then(move |(dist_client, dist_compile_cmd, dist_toolchain, inputs_packager, outputs_rewriter, dist_output_paths, tc_archive)| { debug!("[{}]: Requesting allocation", compile_out_pretty3); dist_client.do_alloc_job(dist_toolchain.clone()) .and_then(move |jares| { @@ -564,7 +566,11 @@ where assert!(count == len); } - try_or_cleanup!(outputs_rewriter.handle_outputs(&path_transformer, &output_paths) + let extra_inputs = match tc_archive { + Some(p) => vec![p], + None => vec![], + }; + try_or_cleanup!(outputs_rewriter.handle_outputs(&path_transformer, &output_paths, &extra_inputs) .chain_err(|| "failed to rewrite outputs from compile")); Ok((DistType::Ok(server_id), jc.output.into())) }) @@ -631,6 +637,7 @@ pub trait OutputsRewriter { self: Box, path_transformer: &dist::PathTransformer, output_paths: &[PathBuf], + extra_inputs: &[PathBuf], ) -> Result<()>; } @@ -642,6 +649,7 @@ impl OutputsRewriter for NoopOutputsRewriter { self: Box, _path_transformer: &dist::PathTransformer, _output_paths: &[PathBuf], + _extra_inputs: &[PathBuf], ) -> Result<()> { Ok(()) } @@ -1727,7 +1735,7 @@ mod test_dist { SubmitToolchainResult, Toolchain, }; use std::cell::Cell; - use std::path::Path; + use std::path::{Path, PathBuf}; use std::sync::Arc; use crate::errors::*; @@ -1762,7 +1770,7 @@ mod test_dist { _: &Path, _: &str, _: Box, - ) -> SFuture<(Toolchain, Option)> { + ) -> SFuture<(Toolchain, Option<(String, PathBuf)>)> { f_err("put toolchain failure") } fn rewrite_includes_only(&self) -> bool { @@ -1807,7 +1815,7 @@ mod test_dist { _: &Path, _: &str, _: Box, - ) -> SFuture<(Toolchain, Option)> { + ) -> SFuture<(Toolchain, Option<(String, PathBuf)>)> { f_ok((self.tc.clone(), None)) } fn rewrite_includes_only(&self) -> bool { @@ -1868,7 +1876,7 @@ mod test_dist { _: &Path, _: &str, _: Box, - ) -> SFuture<(Toolchain, Option)> { + ) -> SFuture<(Toolchain, Option<(String, PathBuf)>)> { f_ok((self.tc.clone(), None)) } fn rewrite_includes_only(&self) -> bool { @@ -1931,8 +1939,14 @@ mod test_dist { _: &Path, _: &str, _: Box, - ) -> SFuture<(Toolchain, Option)> { - f_ok((self.tc.clone(), Some("/overridden/compiler".to_owned()))) + ) -> SFuture<(Toolchain, Option<(String, PathBuf)>)> { + f_ok(( + self.tc.clone(), + Some(( + "/overridden/compiler".to_owned(), + PathBuf::from("somearchiveid"), + )), + )) } fn rewrite_includes_only(&self) -> bool { false @@ -2015,8 +2029,14 @@ mod test_dist { _: &Path, _: &str, _: Box, - ) -> SFuture<(Toolchain, Option)> { - f_ok((self.tc.clone(), Some("/overridden/compiler".to_owned()))) + ) -> SFuture<(Toolchain, Option<(String, PathBuf)>)> { + f_ok(( + self.tc.clone(), + Some(( + "/overridden/compiler".to_owned(), + PathBuf::from("somearchiveid"), + )), + )) } fn rewrite_includes_only(&self) -> bool { false diff --git a/src/compiler/rust.rs b/src/compiler/rust.rs index 21f430b91d..46e1a6663e 100644 --- a/src/compiler/rust.rs +++ b/src/compiler/rust.rs @@ -1699,12 +1699,16 @@ impl OutputsRewriter for RustOutputsRewriter { self: Box, path_transformer: &dist::PathTransformer, output_paths: &[PathBuf], + extra_inputs: &[PathBuf], ) -> Result<()> { use std::io::Write; // Outputs in dep files (the files at the beginning of lines) are untransformed at this point - // remap-path-prefix is documented to only apply to 'inputs'. trace!("Pondering on rewriting dep file {:?}", self.dep_info); + let extra_input_str = extra_inputs + .into_iter() + .fold(String::new(), |s, p| s + " " + &p.to_string_lossy()); if let Some(dep_info) = self.dep_info { for dep_info_local_path in output_paths { trace!("Comparing with {}", dep_info_local_path.display()); @@ -1731,6 +1735,9 @@ impl OutputsRewriter for RustOutputsRewriter { let re = regex::Regex::new(&re_str).expect("Invalid regex"); deps = re.replace_all(&deps, local_path_str).into_owned(); } + if extra_input_str.len() > 0 { + deps = deps.replace(": ", &format!(":{} ", extra_input_str)); + } // Write the depinfo file let f = fs::File::create(&dep_info) .chain_err(|| "Failed to recreate dep info file")?; diff --git a/src/dist/cache.rs b/src/dist/cache.rs index 65f17b104f..aa995a52d5 100644 --- a/src/dist/cache.rs +++ b/src/dist/cache.rs @@ -184,17 +184,17 @@ mod client { compiler_path: &Path, weak_key: &str, toolchain_packager: Box, - ) -> Result<(Toolchain, Option)> { + ) -> Result<(Toolchain, Option<(String, PathBuf)>)> { if self.disabled_toolchains.contains(compiler_path) { bail!( "Toolchain distribution for {} is disabled", compiler_path.display() ) } - if let Some(tc_and_compiler_path) = self.get_custom_toolchain(compiler_path) { + if let Some(tc_and_paths) = self.get_custom_toolchain(compiler_path) { debug!("Using custom toolchain for {:?}", compiler_path); - let (tc, compiler_path) = tc_and_compiler_path?; - return Ok((tc, Some(compiler_path))); + let (tc, compiler_path, archive) = tc_and_paths?; + return Ok((tc, Some((compiler_path, archive)))); } // Only permit one toolchain creation at a time. Not an issue if there are multiple attempts // to create the same toolchain, just a waste of time @@ -216,16 +216,18 @@ mod client { fn get_custom_toolchain( &self, compiler_path: &Path, - ) -> Option> { + ) -> Option> { return match self .custom_toolchain_paths .lock() .unwrap() .get_mut(compiler_path) { - Some((custom_tc, Some(tc))) => { - Some(Ok((tc.clone(), custom_tc.compiler_executable.clone()))) - } + Some((custom_tc, Some(tc))) => Some(Ok(( + tc.clone(), + custom_tc.compiler_executable.clone(), + custom_tc.archive.clone(), + ))), Some((custom_tc, maybe_tc @ None)) => { let archive_id = match path_key(&custom_tc.archive) { Ok(archive_id) => archive_id, @@ -250,7 +252,11 @@ mod client { ) } } - Some(Ok((tc, custom_tc.compiler_executable.clone()))) + Some(Ok(( + tc, + custom_tc.compiler_executable.clone(), + custom_tc.archive.clone(), + ))) } None => None, }; @@ -308,7 +314,7 @@ mod client { 1024, &[config::DistToolchainConfig::PathOverride { compiler_executable: "/my/compiler".into(), - archive: ct1, + archive: ct1.clone(), archive_compiler_executable: "/my/compiler/in_archive".into(), }], ) @@ -321,7 +327,7 @@ mod client { PanicToolchainPackager::new(), ) .unwrap(); - assert!(newpath.unwrap() == "/my/compiler/in_archive"); + assert!(newpath.unwrap() == ("/my/compiler/in_archive".to_string(), ct1)); } #[test] @@ -349,7 +355,7 @@ mod client { // Uses the same archive, but a maps a different external compiler to the same achive compiler as the first config::DistToolchainConfig::PathOverride { compiler_executable: "/my/compiler3".into(), - archive: ct1, + archive: ct1.clone(), archive_compiler_executable: "/my/compiler/in_archive".into(), }, ], @@ -363,7 +369,7 @@ mod client { PanicToolchainPackager::new(), ) .unwrap(); - assert!(newpath.unwrap() == "/my/compiler/in_archive"); + assert!(newpath.unwrap() == ("/my/compiler/in_archive".to_string(), ct1.clone())); let (_tc, newpath) = client_toolchains .put_toolchain( "/my/compiler2".as_ref(), @@ -371,7 +377,7 @@ mod client { PanicToolchainPackager::new(), ) .unwrap(); - assert!(newpath.unwrap() == "/my/compiler2/in_archive"); + assert!(newpath.unwrap() == ("/my/compiler2/in_archive".to_string(), ct1.clone())); let (_tc, newpath) = client_toolchains .put_toolchain( "/my/compiler3".as_ref(), @@ -379,7 +385,7 @@ mod client { PanicToolchainPackager::new(), ) .unwrap(); - assert!(newpath.unwrap() == "/my/compiler/in_archive"); + assert!(newpath.unwrap() == ("/my/compiler/in_archive".to_string(), ct1)); } #[test] diff --git a/src/dist/http.rs b/src/dist/http.rs index 2d13de423f..069be20874 100644 --- a/src/dist/http.rs +++ b/src/dist/http.rs @@ -1295,7 +1295,7 @@ mod client { compiler_path: &Path, weak_key: &str, toolchain_packager: Box, - ) -> SFuture<(Toolchain, Option)> { + ) -> SFuture<(Toolchain, Option<(String, std::path::PathBuf)>)> { let compiler_path = compiler_path.to_owned(); let weak_key = weak_key.to_owned(); let tc_cache = self.tc_cache.clone(); diff --git a/src/dist/mod.rs b/src/dist/mod.rs index 2bc40d7207..694c68f58f 100644 --- a/src/dist/mod.rs +++ b/src/dist/mod.rs @@ -746,6 +746,6 @@ pub trait Client { compiler_path: &Path, weak_key: &str, toolchain_packager: Box, - ) -> SFuture<(Toolchain, Option)>; + ) -> SFuture<(Toolchain, Option<(String, std::path::PathBuf)>)>; fn rewrite_includes_only(&self) -> bool; }