Skip to content

Commit

Permalink
Take a toolchain archive file into account when deciding to re-build …
Browse files Browse the repository at this point in the history 
…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.
  • Loading branch information
@chmanchester
chmanchester committed Jan 15, 2020
1 parent 1a3c0de commit a78ff4b
Show file tree
Hide file tree
Showing 5 changed files with 62 additions and 29 deletions.
44 changes: 32 additions & 12 deletions src/compiler/compiler.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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| {
Expand Down Expand Up @@ -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()))
})
Expand Down Expand Up @@ -631,6 +637,7 @@ pub trait OutputsRewriter {
self: Box<Self>,
path_transformer: &dist::PathTransformer,
output_paths: &[PathBuf],
extra_inputs: &[PathBuf],
) -> Result<()>;
}

Expand All @@ -642,6 +649,7 @@ impl OutputsRewriter for NoopOutputsRewriter {
self: Box<Self>,
_path_transformer: &dist::PathTransformer,
_output_paths: &[PathBuf],
_extra_inputs: &[PathBuf],
) -> Result<()> {
Ok(())
}
Expand Down Expand Up @@ -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::*;
Expand Down Expand Up @@ -1762,7 +1770,7 @@ mod test_dist {
_: &Path,
_: &str,
_: Box<dyn pkg::ToolchainPackager>,
) -> SFuture<(Toolchain, Option<String>)> {
) -> SFuture<(Toolchain, Option<(String, PathBuf)>)> {
f_err("put toolchain failure")
}
fn rewrite_includes_only(&self) -> bool {
Expand Down Expand Up @@ -1807,7 +1815,7 @@ mod test_dist {
_: &Path,
_: &str,
_: Box<dyn pkg::ToolchainPackager>,
) -> SFuture<(Toolchain, Option<String>)> {
) -> SFuture<(Toolchain, Option<(String, PathBuf)>)> {
f_ok((self.tc.clone(), None))
}
fn rewrite_includes_only(&self) -> bool {
Expand Down Expand Up @@ -1868,7 +1876,7 @@ mod test_dist {
_: &Path,
_: &str,
_: Box<dyn pkg::ToolchainPackager>,
) -> SFuture<(Toolchain, Option<String>)> {
) -> SFuture<(Toolchain, Option<(String, PathBuf)>)> {
f_ok((self.tc.clone(), None))
}
fn rewrite_includes_only(&self) -> bool {
Expand Down Expand Up @@ -1931,8 +1939,14 @@ mod test_dist {
_: &Path,
_: &str,
_: Box<dyn pkg::ToolchainPackager>,
) -> SFuture<(Toolchain, Option<String>)> {
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
Expand Down Expand Up @@ -2015,8 +2029,14 @@ mod test_dist {
_: &Path,
_: &str,
_: Box<dyn pkg::ToolchainPackager>,
) -> SFuture<(Toolchain, Option<String>)> {
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
Expand Down
7 changes: 7 additions & 0 deletions src/compiler/rust.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1699,12 +1699,16 @@ impl OutputsRewriter for RustOutputsRewriter {
self: Box<Self>,
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());
Expand All @@ -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")?;
Expand Down
36 changes: 21 additions & 15 deletions src/dist/cache.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -184,17 +184,17 @@ mod client {
compiler_path: &Path,
weak_key: &str,
toolchain_packager: Box<dyn ToolchainPackager>,
) -> Result<(Toolchain, Option<String>)> {
) -> 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
Expand All @@ -216,16 +216,18 @@ mod client {
fn get_custom_toolchain(
&self,
compiler_path: &Path,
) -> Option<Result<(Toolchain, String)>> {
) -> Option<Result<(Toolchain, String, PathBuf)>> {
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,
Expand All @@ -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,
};
Expand Down Expand Up @@ -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(),
}],
)
Expand All @@ -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]
Expand Down Expand Up @@ -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(),
},
],
Expand All @@ -363,23 +369,23 @@ 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(),
"weak_key2",
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(),
"weak_key2",
PanicToolchainPackager::new(),
)
.unwrap();
assert!(newpath.unwrap() == "/my/compiler/in_archive");
assert!(newpath.unwrap() == ("/my/compiler/in_archive".to_string(), ct1));
}

#[test]
Expand Down
2 changes: 1 addition & 1 deletion src/dist/http.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1295,7 +1295,7 @@ mod client {
compiler_path: &Path,
weak_key: &str,
toolchain_packager: Box<dyn ToolchainPackager>,
) -> SFuture<(Toolchain, Option<String>)> {
) -> 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();
Expand Down
2 changes: 1 addition & 1 deletion src/dist/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -746,6 +746,6 @@ pub trait Client {
compiler_path: &Path,
weak_key: &str,
toolchain_packager: Box<dyn pkg::ToolchainPackager>,
) -> SFuture<(Toolchain, Option<String>)>;
) -> SFuture<(Toolchain, Option<(String, std::path::PathBuf)>)>;
fn rewrite_includes_only(&self) -> bool;
}

0 comments on commit a78ff4b

Please sign in to comment.