Skip to content

Commit

Permalink
Auto merge of #7857 - ehuss:fix-build-script-dupe, r=alexcrichton
Browse files Browse the repository at this point in the history
Fix BuildScriptOutput when a build script is run multiple times.

When I implemented profile build overrides, I created a scenario where a build script could run more than once for different scenarios.  See the test for an example.

However, the `BuildScriptOutputs` map did not take this into consideration.  This caused multiple build script runs to stomp on the output of previous runs.  This is further exacerbated by the new feature resolver in #7820 where build scripts can run with different features enabled.

The solution is to make the map key unique for the Unit it is running for.  Since this map must be updated on different threads, `Unit` cannot be used as a key, so I chose just using the metadata hash which is hopefully unique.  Most of this patch is involved with the fussiness of getting the correct metadata hash (we want the RunCustomBuild unit's hash).  I also added some checks to avoid collisions and assert assumptions.
  • Loading branch information
bors committed Feb 7, 2020
2 parents 1bc7f53 + 2296af2 commit 3c53211
Show file tree
Hide file tree
Showing 9 changed files with 296 additions and 81 deletions.
1 change: 1 addition & 0 deletions src/cargo/core/compiler/compilation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use crate::core::compiler::CompileKind;
use crate::core::{Edition, Package, PackageId, Target};
use crate::util::{self, config, join_paths, process, CargoResult, Config, ProcessBuilder};

/// Structure with enough information to run `rustdoc --test`.
pub struct Doctest {
/// The package being doc-tested.
pub package: Package,
Expand Down
9 changes: 8 additions & 1 deletion src/cargo/core/compiler/context/compilation_files.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ use crate::util::{self, CargoResult};
///
/// Note that the `Fingerprint` is in charge of tracking everything needed to determine if a
/// rebuild is needed.
#[derive(Clone, Hash, Eq, PartialEq, Ord, PartialOrd)]
#[derive(Copy, Clone, Hash, Eq, PartialEq, Ord, PartialOrd)]
pub struct Metadata(u64);

impl fmt::Display for Metadata {
Expand All @@ -50,6 +50,12 @@ impl fmt::Display for Metadata {
}
}

impl fmt::Debug for Metadata {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "Metadata({:016x})", self.0)
}
}

/// Collection of information about the files emitted by the compiler, and the
/// output directory structure.
pub struct CompilationFiles<'a, 'cfg> {
Expand Down Expand Up @@ -213,6 +219,7 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> {
pub fn build_script_dir(&self, unit: &Unit<'a>) -> PathBuf {
assert!(unit.target.is_custom_build());
assert!(!unit.mode.is_run_custom_build());
assert!(self.metas.contains_key(unit));
let dir = self.pkg_dir(unit);
self.layout(CompileKind::Host).build().join(dir)
}
Expand Down
43 changes: 38 additions & 5 deletions src/cargo/core/compiler/context/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,8 +122,8 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
})
}

// Returns a mapping of the root package plus its immediate dependencies to
// where the compiled libraries are all located.
/// Starts compilation, waits for it to finish, and returns information
/// about the result of compilation.
pub fn compile(
mut self,
units: &[Unit<'a>],
Expand Down Expand Up @@ -245,16 +245,16 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
super::output_depinfo(&mut self, unit)?;
}

for (&(ref pkg, _), output) in self.build_script_outputs.lock().unwrap().iter() {
for (pkg_id, output) in self.build_script_outputs.lock().unwrap().iter() {
self.compilation
.cfgs
.entry(pkg.clone())
.entry(pkg_id)
.or_insert_with(HashSet::new)
.extend(output.cfgs.iter().cloned());

self.compilation
.extra_env
.entry(pkg.clone())
.entry(pkg_id)
.or_insert_with(Vec::new)
.extend(output.env.iter().cloned());

Expand Down Expand Up @@ -347,6 +347,39 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
&self.unit_dependencies[unit]
}

/// Returns the RunCustomBuild Unit associated with the given Unit.
///
/// If the package does not have a build script, this returns None.
pub fn find_build_script_unit(&self, unit: Unit<'a>) -> Option<Unit<'a>> {
if unit.mode.is_run_custom_build() {
return Some(unit);
}
self.unit_dependencies[&unit]
.iter()
.find(|unit_dep| {
unit_dep.unit.mode.is_run_custom_build()
&& unit_dep.unit.pkg.package_id() == unit.pkg.package_id()
})
.map(|unit_dep| unit_dep.unit)
}

/// Returns the metadata hash for the RunCustomBuild Unit associated with
/// the given unit.
///
/// If the package does not have a build script, this returns None.
pub fn find_build_script_metadata(&self, unit: Unit<'a>) -> Option<Metadata> {
let script_unit = self.find_build_script_unit(unit)?;
Some(self.get_run_build_script_metadata(&script_unit))
}

/// Returns the metadata hash for a RunCustomBuild unit.
pub fn get_run_build_script_metadata(&self, unit: &Unit<'a>) -> Metadata {
assert!(unit.mode.is_run_custom_build());
self.files()
.metadata(unit)
.expect("build script should always have hash")
}

pub fn is_primary_package(&self, unit: &Unit<'a>) -> bool {
self.primary_packages.contains(&unit.pkg.package_id())
}
Expand Down
136 changes: 98 additions & 38 deletions src/cargo/core/compiler/custom_build.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use super::job::{Freshness, Job, Work};
use super::{fingerprint, CompileKind, Context, Unit};
use super::{fingerprint, Context, Unit};
use crate::core::compiler::context::Metadata;
use crate::core::compiler::job_queue::JobState;
use crate::core::{profiles::ProfileRoot, PackageId};
use crate::util::errors::{CargoResult, CargoResultExt};
Expand Down Expand Up @@ -41,37 +42,49 @@ pub struct BuildOutput {
/// This initially starts out as empty. Overridden build scripts get
/// inserted during `build_map`. The rest of the entries are added
/// immediately after each build script runs.
pub type BuildScriptOutputs = HashMap<(PackageId, CompileKind), BuildOutput>;
///
/// The `Metadata` is the unique metadata hash for the RunCustomBuild Unit of
/// the package. It needs a unique key, since the build script can be run
/// multiple times with different profiles or features. We can't embed a
/// `Unit` because this structure needs to be shareable between threads.
#[derive(Default)]
pub struct BuildScriptOutputs {
outputs: HashMap<(PackageId, Metadata), BuildOutput>,
}

/// Linking information for a `Unit`.
///
/// See `build_map` for more details.
#[derive(Default)]
pub struct BuildScripts {
/// List of build script outputs this Unit needs to include for linking. Each
/// element is an index into `BuildScriptOutputs`.
///
/// Cargo will use this `to_link` vector to add `-L` flags to compiles as we
/// propagate them upwards towards the final build. Note, however, that we
/// need to preserve the ordering of `to_link` to be topologically sorted.
/// This will ensure that build scripts which print their paths properly will
/// correctly pick up the files they generated (if there are duplicates
/// elsewhere).
///
/// To preserve this ordering, the (id, kind) is stored in two places, once
/// To preserve this ordering, the (id, metadata) is stored in two places, once
/// in the `Vec` and once in `seen_to_link` for a fast lookup. We maintain
/// this as we're building interactively below to ensure that the memory
/// usage here doesn't blow up too much.
///
/// For more information, see #2354.
pub to_link: Vec<(PackageId, CompileKind)>,
pub to_link: Vec<(PackageId, Metadata)>,
/// This is only used while constructing `to_link` to avoid duplicates.
seen_to_link: HashSet<(PackageId, CompileKind)>,
/// Host-only dependencies that have build scripts.
seen_to_link: HashSet<(PackageId, Metadata)>,
/// Host-only dependencies that have build scripts. Each element is an
/// index into `BuildScriptOutputs`.
///
/// This is the set of transitive dependencies that are host-only
/// (proc-macro, plugin, build-dependency) that contain a build script.
/// Any `BuildOutput::library_paths` path relative to `target` will be
/// added to LD_LIBRARY_PATH so that the compiler can find any dynamic
/// libraries a build script may have generated.
pub plugins: BTreeSet<PackageId>,
pub plugins: BTreeSet<(PackageId, Metadata)>,
}

/// Dependency information as declared by a build script.
Expand All @@ -94,9 +107,13 @@ pub fn prepare<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> CargoRe
unit.target.name()
));

let key = (unit.pkg.package_id(), unit.kind);

if cx.build_script_outputs.lock().unwrap().contains_key(&key) {
let metadata = cx.get_run_build_script_metadata(unit);
if cx
.build_script_outputs
.lock()
.unwrap()
.contains_key(unit.pkg.package_id(), metadata)
{
// The output is already set, thus the build script is overridden.
fingerprint::prepare_target(cx, unit, false)
} else {
Expand Down Expand Up @@ -231,9 +248,11 @@ fn build_work<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> CargoRes
.iter()
.filter_map(|dep| {
if dep.unit.mode.is_run_custom_build() {
let dep_metadata = cx.get_run_build_script_metadata(&dep.unit);
Some((
dep.unit.pkg.manifest().links().unwrap().to_string(),
dep.unit.pkg.package_id(),
dep_metadata,
))
} else {
None
Expand All @@ -255,10 +274,10 @@ fn build_work<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> CargoRes
script_out_dir.clone(),
);
let build_scripts = cx.build_scripts.get(unit).cloned();
let kind = unit.kind;
let json_messages = bcx.build_config.emit_json();
let extra_verbose = bcx.config.extra_verbose();
let (prev_output, prev_script_out_dir) = prev_build_output(cx, unit);
let metadata_hash = cx.get_run_build_script_metadata(unit);

paths::create_dir_all(&script_dir)?;
paths::create_dir_all(&script_out_dir)?;
Expand Down Expand Up @@ -286,15 +305,16 @@ fn build_work<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> CargoRes
// native dynamic libraries.
if !build_plan {
let build_script_outputs = build_script_outputs.lock().unwrap();
for (name, id) in lib_deps {
let key = (id, kind);
let script_output = build_script_outputs.get(&key).ok_or_else(|| {
internal(format!(
"failed to locate build state for env \
vars: {}/{:?}",
id, kind
))
})?;
for (name, dep_id, dep_metadata) in lib_deps {
let script_output =
build_script_outputs
.get(dep_id, dep_metadata)
.ok_or_else(|| {
internal(format!(
"failed to locate build state for env vars: {}/{}",
dep_id, dep_metadata
))
})?;
let data = &script_output.metadata;
for &(ref key, ref value) in data.iter() {
cmd.env(
Expand Down Expand Up @@ -360,7 +380,7 @@ fn build_work<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> CargoRes
build_script_outputs
.lock()
.unwrap()
.insert((id, kind), parsed_output);
.insert(id, metadata_hash, parsed_output);
Ok(())
});

Expand All @@ -386,7 +406,7 @@ fn build_work<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> CargoRes
build_script_outputs
.lock()
.unwrap()
.insert((id, kind), output);
.insert(id, metadata_hash, output);
Ok(())
});

Expand Down Expand Up @@ -614,7 +634,7 @@ impl BuildDeps {
/// scripts.
///
/// The important one here is `build_scripts`, which for each `(package,
/// kind)` stores a `BuildScripts` object which contains a list of
/// metadata)` stores a `BuildScripts` object which contains a list of
/// dependencies with build scripts that the unit should consider when
/// linking. For example this lists all dependencies' `-L` flags which need to
/// be propagated transitively.
Expand Down Expand Up @@ -644,20 +664,27 @@ pub fn build_map<'b, 'cfg>(cx: &mut Context<'b, 'cfg>, units: &[Unit<'b>]) -> Ca
}

// If there is a build script override, pre-fill the build output.
if let Some(links) = unit.pkg.manifest().links() {
if let Some(output) = cx.bcx.script_override(links, unit.kind) {
let key = (unit.pkg.package_id(), unit.kind);
cx.build_script_outputs
.lock()
.unwrap()
.insert(key, output.clone());
if unit.mode.is_run_custom_build() {
if let Some(links) = unit.pkg.manifest().links() {
if let Some(output) = cx.bcx.script_override(links, unit.kind) {
let metadata = cx.get_run_build_script_metadata(unit);
cx.build_script_outputs.lock().unwrap().insert(
unit.pkg.package_id(),
metadata,
output.clone(),
);
}
}
}

let mut ret = BuildScripts::default();

// If a package has a build script, add itself as something to inspect for linking.
if !unit.target.is_custom_build() && unit.pkg.has_custom_build() {
add_to_link(&mut ret, unit.pkg.package_id(), unit.kind);
let script_meta = cx
.find_build_script_metadata(*unit)
.expect("has_custom_build should have RunCustomBuild");
add_to_link(&mut ret, unit.pkg.package_id(), script_meta);
}

// Load any dependency declarations from a previous run.
Expand All @@ -676,11 +703,10 @@ pub fn build_map<'b, 'cfg>(cx: &mut Context<'b, 'cfg>, units: &[Unit<'b>]) -> Ca
let dep_scripts = build(out, cx, dep_unit)?;

if dep_unit.target.for_host() {
ret.plugins
.extend(dep_scripts.to_link.iter().map(|p| &p.0).cloned());
ret.plugins.extend(dep_scripts.to_link.iter().cloned());
} else if dep_unit.target.linkable() {
for &(pkg, kind) in dep_scripts.to_link.iter() {
add_to_link(&mut ret, pkg, kind);
for &(pkg, metadata) in dep_scripts.to_link.iter() {
add_to_link(&mut ret, pkg, metadata);
}
}
}
Expand All @@ -693,9 +719,9 @@ pub fn build_map<'b, 'cfg>(cx: &mut Context<'b, 'cfg>, units: &[Unit<'b>]) -> Ca

// When adding an entry to 'to_link' we only actually push it on if the
// script hasn't seen it yet (e.g., we don't push on duplicates).
fn add_to_link(scripts: &mut BuildScripts, pkg: PackageId, kind: CompileKind) {
if scripts.seen_to_link.insert((pkg, kind)) {
scripts.to_link.push((pkg, kind));
fn add_to_link(scripts: &mut BuildScripts, pkg: PackageId, metadata: Metadata) {
if scripts.seen_to_link.insert((pkg, metadata)) {
scripts.to_link.push((pkg, metadata));
}
}

Expand Down Expand Up @@ -741,3 +767,37 @@ fn prev_build_output<'a, 'cfg>(
prev_script_out_dir,
)
}

impl BuildScriptOutputs {
/// Inserts a new entry into the map.
fn insert(&mut self, pkg_id: PackageId, metadata: Metadata, parsed_output: BuildOutput) {
match self.outputs.entry((pkg_id, metadata)) {
Entry::Vacant(entry) => {
entry.insert(parsed_output);
}
Entry::Occupied(entry) => panic!(
"build script output collision for {}/{}\n\
old={:?}\nnew={:?}",
pkg_id,
metadata,
entry.get(),
parsed_output
),
}
}

/// Returns `true` if the given key already exists.
fn contains_key(&self, pkg_id: PackageId, metadata: Metadata) -> bool {
self.outputs.contains_key(&(pkg_id, metadata))
}

/// Gets the build output for the given key.
pub fn get(&self, pkg_id: PackageId, meta: Metadata) -> Option<&BuildOutput> {
self.outputs.get(&(pkg_id, meta))
}

/// Returns an iterator over all entries.
pub fn iter(&self) -> impl Iterator<Item = (PackageId, &BuildOutput)> {
self.outputs.iter().map(|(key, value)| (key.0, value))
}
}
Loading

0 comments on commit 3c53211

Please sign in to comment.