Skip to content

Commit

Permalink
Auto merge of #7008 - alexcrichton:less-pipelining, r=ehuss
Browse files Browse the repository at this point in the history
Handle pipelined tests of libraries

The previous implementation of pipelining accidentally forgot to account
for rlibs being compiled in `--test` mode. The compilations would be
pipelined to where all the dependencies of an rlib might not be
available yet, but `--test` actually performs linking in rustc!

This commit fixes the issue by refactoring slightly and removing
`Target::requires_upstream_objects` (moving it to `TargetKind`) and then
making the source of truth a `Unit::requires_upstream_objects` method
which takes into account the value from `TargetKind` as well as the
`CompileMode`.

Closes #6993
  • Loading branch information
bors committed Jun 5, 2019
2 parents 0e38712 + 0c51d71 commit afcd5ef
Show file tree
Hide file tree
Showing 6 changed files with 31 additions and 19 deletions.
2 changes: 1 addition & 1 deletion src/cargo/core/compiler/context/compilation_files.rs
Original file line number Diff line number Diff line change
Expand Up @@ -420,7 +420,7 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> {
)?;
}
let path = out_dir.join(format!("lib{}.rmeta", file_stem));
if !unit.target.requires_upstream_objects() {
if !unit.requires_upstream_objects() {
ret.push(OutputFile {
path,
hardlink: None,
Expand Down
4 changes: 2 additions & 2 deletions src/cargo/core/compiler/context/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -475,11 +475,11 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
self.pipelining
// We're only a candidate for requiring an `rmeta` file if we
// ourselves are building an rlib,
&& !parent.target.requires_upstream_objects()
&& !parent.requires_upstream_objects()
&& parent.mode == CompileMode::Build
// Our dependency must also be built as an rlib, otherwise the
// object code must be useful in some fashion
&& !dep.target.requires_upstream_objects()
&& !dep.requires_upstream_objects()
&& dep.mode == CompileMode::Build
}

Expand Down
2 changes: 1 addition & 1 deletion src/cargo/core/compiler/job_queue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ impl<'a, 'cfg> JobQueue<'a, 'cfg> {
// the target as well. This should ensure that edges changed to
// `Metadata` propagate upwards `All` dependencies to anything that
// transitively contains the `Metadata` edge.
if unit.target.requires_upstream_objects() {
if unit.requires_upstream_objects() {
for dep in dependencies.iter() {
depend_on_deps_of_deps(cx, &mut queue_deps, dep);
}
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/core/compiler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -827,7 +827,7 @@ fn build_base_args<'a, 'cfg>(

if unit.mode.is_check() {
cmd.arg("--emit=dep-info,metadata");
} else if !unit.target.requires_upstream_objects() {
} else if !unit.requires_upstream_objects() {
// Always produce metdata files for rlib outputs. Metadata may be used
// in this session for a pipelined compilation, or it may be used in a
// future Cargo session as part of a pipelined compile.
Expand Down
12 changes: 12 additions & 0 deletions src/cargo/core/compiler/unit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,18 @@ pub struct UnitInner<'a> {
pub mode: CompileMode,
}

impl UnitInner<'_> {
/// Returns whether compilation of this unit requires all upstream artifacts
/// to be available.
///
/// This effectively means that this unit is a synchronization point (if the
/// return value is `true`) that all previously pipelined units need to
/// finish in their entirety before this one is started.
pub fn requires_upstream_objects(&self) -> bool {
self.mode.is_any_test() || self.target.kind().requires_upstream_objects()
}
}

impl<'a> Unit<'a> {
pub fn buildkey(&self) -> String {
format!("{}-{}", self.pkg.name(), short_hash(self))
Expand Down
28 changes: 14 additions & 14 deletions src/cargo/core/manifest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,20 @@ impl TargetKind {
TargetKind::CustomBuild => "build-script",
}
}

/// Returns whether production of this artifact requires the object files
/// from dependencies to be available.
///
/// This only returns `false` when all we're producing is an rlib, otherwise
/// it will return `true`.
pub fn requires_upstream_objects(&self) -> bool {
match self {
TargetKind::Lib(kinds) | TargetKind::ExampleLib(kinds) => {
kinds.iter().any(|k| k.requires_upstream_objects())
}
_ => true,
}
}
}

/// Information about a binary, a library, an example, etc. that is part of the
Expand Down Expand Up @@ -821,20 +835,6 @@ impl Target {
}
}

/// Returns whether production of this artifact requires the object files
/// from dependencies to be available.
///
/// This only returns `false` when all we're producing is an rlib, otherwise
/// it will return `true`.
pub fn requires_upstream_objects(&self) -> bool {
match &self.kind {
TargetKind::Lib(kinds) | TargetKind::ExampleLib(kinds) => {
kinds.iter().any(|k| k.requires_upstream_objects())
}
_ => true,
}
}

pub fn is_bin(&self) -> bool {
self.kind == TargetKind::Bin
}
Expand Down

0 comments on commit afcd5ef

Please sign in to comment.