Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
166 changes: 157 additions & 9 deletions src/compiler/compiler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -877,15 +877,18 @@ where
let dist_client = match dist_compile_cmd.clone().and(dist_client) {
Some(dc) => dc,
None => {
debug!("[{}]: Compiling locally", out_pretty);
info!(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

should be done in a different pr

"[{}]: Compiling locally (not eligible for distributed compilation)",
out_pretty
);
return compile_cmd
.execute(service, &creator)
.await
.map(move |o| (cacheable, DistType::NoDist, o));
}
};

debug!("[{}]: Attempting distributed compilation", out_pretty);
info!("[{}]: Attempting distributed compilation", out_pretty);
let out_pretty2 = out_pretty.clone();

let local_executable = compile_cmd.get_executable();
Expand All @@ -900,6 +903,14 @@ where
.map(|output| path_transformer.as_dist_abs(&cwd.join(output.path)))
.collect::<Option<_>>()
.context("Failed to adapt an output path for distributed compile")?;
// The local paths every declared output must occupy once the compile
// finishes. Captured before `compilation` is moved into the packagers
// below so a remote compile that drops a declared output can be detected
// and salvaged (see the missing-output check after the run completes).
let expected_output_paths: Vec<PathBuf> = compilation
.outputs()
.map(|output| cwd.join(output.path))
.collect();
let (inputs_packager, toolchain_packager, outputs_rewriter) =
compilation.into_dist_packagers(path_transformer)?;

Expand Down Expand Up @@ -970,7 +981,7 @@ where
)
})?;

let mut jc = match jres {
let jc = match jres {
dist::RunJobResult::Complete(jc) => jc,
dist::RunJobResult::JobNotFound => bail!("Job {} not found on server", job_id),
};
Expand Down Expand Up @@ -1036,14 +1047,42 @@ where
);

if jc.output.code != 0 {
// Add server info to help diagnose host-specific failures, e.g. due to flaky hardware.
// Failed builds are not cached so this tampering should not cause too much trouble.
let server_info = format!("sccache: Job failed on server {}:\n", server_id.addr());
jc.output
.stderr
.splice(0..0, server_info.as_bytes().to_vec());
// A non-zero remote result is frequently a distribution artifact
// rather than a genuine compiler error: e.g. an object that
// .incbin's a binary the inputs packager did not ship (the kernel's
// vdso/dtb/embedded-config wrappers), which the build-server cannot
// assemble. Fall back to a local recompile via the or_else below - it
// either succeeds (confirming a dist-only artifact) or reproduces the
// real error locally, so a remote failure never breaks a build that
// would compile fine locally. This only affects failing dist
// compiles; successful ones are returned unchanged above.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks like a fix for #2700.

IMO, please, do a dedicated PR with that fix & test it to mitigate possible future regressions

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

And it implements #2745, isn't it?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

To implement #2745, it probably(?) makes sense to wait until #2735 is merged, then reuse some of its code.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe yes. IMO, your code part should resolve that problem

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I actually haven't written any code in that area (yet, at least)

bail!(
"distributed compile on {} returned exit code {}; recompiling locally",
server_id.addr(),
jc.output.code
);
}

// The remote compile returned exit 0 but may have dropped a declared
// output: glibc's ldconfig.o/sprof.o omit their `.o.dt` dep file, which
// the build-server does not return. Zipping the outputs later would then
// fail fatally with no recourse. Treat a missing declared output as a
// distribution artifact and fall back to a local recompile via the
// or_else below, which reproduces the full output set. Compiles whose
// dist output set is complete are unaffected.
if let Some(missing) = expected_output_paths.iter().find(|p| !p.exists()) {
bail!(
"distributed compile on {} did not return expected output {}; recompiling locally",
server_id.addr(),
missing.display()
);
}

info!(
"[{}]: Distributed compilation finished on {}",
out_pretty,
server_id.addr()
);
Ok((DistType::Ok(server_id), jc.output.into()))
};

Expand Down Expand Up @@ -3209,6 +3248,7 @@ LLVM version: 6.0",
test_dist::ErrorAllocJobClient::new(),
test_dist::ErrorSubmitToolchainClient::new(),
test_dist::ErrorRunJobClient::new(),
test_dist::IncompleteOutputsClient::new(),
];
// Write a dummy input file so the preprocessor cache mode can work
std::fs::write(f.tempdir.path().join("foo.c"), "whatever").unwrap();
Expand Down Expand Up @@ -3677,4 +3717,112 @@ mod test_dist {
None
}
}

/// A dist client whose remote compile succeeds (exit 0) but whose returned
/// output set drops a declared output - the glibc `.o.dt` failure mode,
/// where the build-server runs the compile fine yet does not return every
/// declared output. The client must detect the missing output and fall back
/// to a local recompile rather than failing the build.
pub struct IncompleteOutputsClient {
has_started: AtomicBool,
tc: Toolchain,
output: ProcessOutput,
}

impl IncompleteOutputsClient {
#[allow(clippy::new_ret_no_self)]
pub fn new() -> Arc<dyn dist::Client> {
Arc::new(Self {
has_started: AtomicBool::default(),
tc: Toolchain {
archive_id: "somearchiveid".to_owned(),
},
output: ProcessOutput::fake_output(0, vec![], vec![]),
})
}
}

#[async_trait]
impl dist::Client for IncompleteOutputsClient {
async fn do_alloc_job(&self, tc: Toolchain) -> Result<AllocJobResult> {
assert!(
!self
.has_started
.swap(true, std::sync::atomic::Ordering::AcqRel)
);
assert_eq!(self.tc, tc);

Ok(AllocJobResult::Success {
job_alloc: JobAlloc {
auth: "abcd".to_owned(),
job_id: JobId(0),
server_id: ServerId::new(([0, 0, 0, 0], 1).into()),
},
need_toolchain: true,
})
}
async fn do_get_status(&self) -> Result<SchedulerStatusResult> {
unreachable!("fn do_get_status is not used for this test. qed")
}
async fn do_submit_toolchain(
&self,
job_alloc: JobAlloc,
tc: Toolchain,
) -> Result<SubmitToolchainResult> {
assert_eq!(job_alloc.job_id, JobId(0));
assert_eq!(self.tc, tc);

Ok(SubmitToolchainResult::Success)
}
async fn do_run_job(
&self,
job_alloc: JobAlloc,
command: CompileCommand,
outputs: Vec<String>,
inputs_packager: Box<dyn pkg::InputsPackager>,
) -> Result<(RunJobResult, PathTransformer)> {
assert_eq!(job_alloc.job_id, JobId(0));
assert_eq!(command.executable, "/overridden/compiler");

let mut inputs = vec![];
let path_transformer = inputs_packager.write_inputs(&mut inputs).unwrap();
// Drop one declared output to mimic a build-server that returned a
// successful compile with an incomplete output set.
let mut outputs = outputs;
outputs.pop();
let outputs = outputs
.into_iter()
.map(|name| {
let data = format!("some data in {}", name);
let data = OutputData::try_from_reader(data.as_bytes()).unwrap();
(name, data)
})
.collect();
let result = RunJobResult::Complete(JobComplete {
output: self.output.clone(),
outputs,
});
Ok((result, path_transformer))
}
async fn put_toolchain(
&self,
_: PathBuf,
_: String,
_: Box<dyn pkg::ToolchainPackager>,
) -> Result<(Toolchain, Option<(String, PathBuf)>)> {
Ok((
self.tc.clone(),
Some((
"/overridden/compiler".to_owned(),
PathBuf::from("somearchiveid"),
)),
))
}
fn rewrite_includes_only(&self) -> bool {
false
}
fn get_custom_toolchain(&self, _exe: &Path) -> Option<PathBuf> {
None
}
}
}
69 changes: 68 additions & 1 deletion src/compiler/gcc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1021,7 +1021,14 @@ where
}
arguments.extend(vec![
parsed_args.compilation_flag.clone().into_string().ok()?,
path_transformer.as_dist(&parsed_args.input)?,
// Match the path the inputs packager ships the preprocessed
// content at (CInputsPackager uses cwd.join(input) + simplify_path).
// parsed_args.input is relative for out-of-tree builds (e.g. OE's
// `../sources/foo.c`); passing it raw made the server look for the
// input at a path the package never placed it, failing with
// "No such file or directory". Absolutize+simplify so both agree.
path_transformer
.as_dist(&dist::pkg::simplify_path(&cwd.join(&parsed_args.input)).ok()?)?,
"-o".into(),
path_transformer.as_dist(out_file)?,
]);
Expand Down Expand Up @@ -2478,6 +2485,66 @@ mod test {
assert_eq!(0, creator.lock().unwrap().children.len());
}

#[test]
#[cfg(feature = "dist-client")]
fn test_compile_relative_input_dist_command_is_absolute() {
// Regression: the dist command must reference the same absolute, simplified
// input path the inputs packager ships the preprocessed content at
// (CInputsPackager uses cwd.join(input) + simplify_path). For out-of-tree
// builds the input is relative (e.g. OE's `../sources/foo.c`); passing it
// raw made the build-server look where the input was never placed, failing
// with "No such file or directory" (OE xz/glibc out-of-tree compiles).
let f = TestFixture::new();
let parsed_args = ParsedArguments {
input: "../foo.c".into(),
double_dash_input: false,
language: Language::C,
compilation_flag: "-c".into(),
depfile: None,
outputs: vec![(
"obj",
ArtifactDescriptor {
path: "foo.o".into(),
optional: false,
},
)]
.into_iter()
.collect(),
dependency_args: vec![],
preprocessor_args: vec![],
common_args: vec![],
arch_args: vec![],
unhashed_args: vec![],
extra_dist_files: vec![],
extra_hash_files: vec![],
msvc_show_includes: false,
profile_generate: false,
color_mode: ColorMode::Auto,
suppress_rewrite_includes_only: false,
too_hard_for_preprocessor_cache_mode: None,
};
let mut path_transformer = dist::PathTransformer::new();
let (_command, dist_command, _cacheable) = generate_compile_commands(
&mut path_transformer,
&f.bins[0],
&parsed_args,
f.tempdir.path(),
&[],
CCompilerKind::Gcc,
false,
language_to_gcc_arg,
)
.unwrap();
let dist_command = dist_command.expect("relative input must still produce a dist command");
// No argument may carry an unresolved `..`: the input must be the simplified
// absolute path the packager ships, not the raw relative one.
assert!(
!dist_command.arguments.iter().any(|a| a.contains("..")),
"dist command still references a relative input: {:?}",
dist_command.arguments
);
}

#[test]
fn test_compile_simple_verbose_short() {
let creator = new_creator();
Expand Down
Loading
Loading