From 0743c1262ec9123d4eb590e4747032d9a36aa102 Mon Sep 17 00:00:00 2001 From: Igor Matuszewski Date: Fri, 24 Aug 2018 15:55:21 +0200 Subject: [PATCH 1/4] Implement external build plan This adds a `BuildGraph` trait which aims to abstract away the API for a build plan and it implements it for the in-process Cargo build plan as well as the new, external one (for `cargo build --build-plan` format). In addition to that, since save-analysis (since rls-data 0.18.1) includes the invocation used to compile a given crate, we try to recreate the external build plan from the passed save-analysis files via `build_command` config option. --- src/actions/post_build.rs | 2 + src/build/cargo.rs | 45 +-- src/build/cargo_plan.rs | 584 ++++++++++++++++++++++++++++++++++++ src/build/external.rs | 519 ++++++++++++++++++++++++++++++-- src/build/mod.rs | 66 +++-- src/build/plan.rs | 601 +++++--------------------------------- src/build/rustc.rs | 39 ++- src/config.rs | 4 - 8 files changed, 1233 insertions(+), 627 deletions(-) create mode 100644 src/build/cargo_plan.rs diff --git a/src/actions/post_build.rs b/src/actions/post_build.rs index 25b53bb9689..31fdd9acd4c 100644 --- a/src/actions/post_build.rs +++ b/src/actions/post_build.rs @@ -380,8 +380,10 @@ impl Job { self.cwd ); if self.analysis.is_empty() { + trace!("reloading from disk: {:?}", self.cwd); self.handler.reload_analysis_from_disk(&self.cwd); } else { + trace!("reloading from memory: {:?}", self.cwd); self.handler .reload_analysis_from_memory(&self.cwd, self.analysis); } diff --git a/src/build/cargo.rs b/src/build/cargo.rs index 949a1fda8e0..b68efd7a7e5 100644 --- a/src/build/cargo.rs +++ b/src/build/cargo.rs @@ -20,11 +20,12 @@ use failure::{self, format_err}; use serde_json; use crate::actions::progress::ProgressUpdate; -use crate::build::environment::{self, Environment, EnvironmentLock}; -use crate::build::plan::Plan; use crate::build::{BufWriter, BuildResult, CompilationContext, Internals, PackageArg}; +use crate::build::cargo_plan::CargoPlan; +use crate::build::environment::{self, Environment, EnvironmentLock}; +use crate::build::plan::BuildPlan; use crate::config::Config; -use log::{debug, error, trace, warn}; +use log::{debug, trace, warn}; use rls_data::Analysis; use rls_vfs::Vfs; @@ -34,7 +35,6 @@ use std::ffi::OsString; use std::fmt::Write; use std::fs::{read_dir, remove_file}; use std::path::{Path, PathBuf}; -use std::process::Command; use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::mpsc::Sender; use std::sync::{Arc, Mutex}; @@ -191,7 +191,8 @@ fn run_cargo( // Since Cargo build routine will try to regenerate the unit dep graph, // we need to clear the existing dep graph. - compilation_cx.lock().unwrap().build_plan = Plan::for_packages(pkg_names); + compilation_cx.lock().unwrap().build_plan = + BuildPlan::Cargo(CargoPlan::with_packages(&manifest_path, pkg_names)); let compile_opts = CompileOptions { spec, @@ -336,12 +337,12 @@ impl Executor for RlsExecutor { /// is fresh and won't be compiled. fn init(&self, cx: &Context<'_, '_>, unit: &Unit<'_>) { let mut compilation_cx = self.compilation_cx.lock().unwrap(); - let plan = &mut compilation_cx.build_plan; + let plan = compilation_cx.build_plan.as_cargo_mut() + .expect("Build plan should be properly initialized before running Cargo"); + let only_primary = |unit: &Unit<'_>| self.is_primary_crate(unit.pkg.package_id()); - if let Err(err) = plan.emplace_dep_with_filter(unit, cx, &only_primary) { - error!("{:?}", err); - } + plan.emplace_dep_with_filter(unit, cx, &only_primary); } fn force_rebuild(&self, unit: &Unit<'_>) -> bool { @@ -439,8 +440,8 @@ impl Executor for RlsExecutor { .collect(); let envs = cargo_cmd.get_envs().clone(); - let sysroot = - current_sysroot().expect("need to specify SYSROOT env var or use rustup or multirust"); + let sysroot = super::rustc::current_sysroot() + .expect("need to specify SYSROOT env var or use rustup or multirust"); { let config = self.config.lock().unwrap(); @@ -502,7 +503,8 @@ impl Executor for RlsExecutor { // Cache executed command for the build plan { let mut cx = self.compilation_cx.lock().unwrap(); - cx.build_plan.cache_compiler_job(id, target, mode, &cmd); + let plan = cx.build_plan.as_cargo_mut().unwrap(); + plan.cache_compiler_job(id, target, mode, &cmd); } // Prepare modified cargo-generated args/envs for future rustc calls @@ -654,25 +656,6 @@ fn parse_arg(args: &[OsString], arg: &str) -> Option { None } -fn current_sysroot() -> Option { - let home = env::var("RUSTUP_HOME").or_else(|_| env::var("MULTIRUST_HOME")); - let toolchain = env::var("RUSTUP_TOOLCHAIN").or_else(|_| env::var("MULTIRUST_TOOLCHAIN")); - if let (Ok(home), Ok(toolchain)) = (home, toolchain) { - Some(format!("{}/toolchains/{}", home, toolchain)) - } else { - let rustc_exe = env::var("RUSTC").unwrap_or_else(|_| "rustc".to_owned()); - env::var("SYSROOT").map(|s| s.to_owned()).ok().or_else(|| { - Command::new(rustc_exe) - .arg("--print") - .arg("sysroot") - .output() - .ok() - .and_then(|out| String::from_utf8(out.stdout).ok()) - .map(|s| s.trim().to_owned()) - }) - } -} - /// `flag_str` is a string of command line args for Rust. This function removes any /// duplicate flags. fn dedup_flags(flag_str: &str) -> String { diff --git a/src/build/cargo_plan.rs b/src/build/cargo_plan.rs new file mode 100644 index 00000000000..cf70ec62188 --- /dev/null +++ b/src/build/cargo_plan.rs @@ -0,0 +1,584 @@ +// Copyright 2017 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +//! This contains a build plan that is created during the Cargo build routine +//! and stored afterwards, which can be later queried, given a list of dirty +//! files, to retrieve a queue of compiler calls to be invoked (including +//! appropriate arguments and env variables). +//! The underlying structure is a dependency graph between simplified units +//! (package id and crate target kind), as opposed to Cargo units (package with +//! a target info, including crate target kind, profile and host/target kind). +//! This will be used for a quick check recompilation and does not aim to +//! reimplement all the intricacies of Cargo. +//! The unit dependency graph in Cargo also distinguishes between compiling the +//! build script and running it and collecting the build script output to modify +//! the subsequent compilations etc. Since build script executions (not building) +//! are not exposed via `Executor` trait in Cargo, we simply coalesce every unit +//! with a same package and crate target kind (e.g. both building and running +//! build scripts). + +use std::collections::{HashMap, HashSet}; +use std::fmt; +use std::path::{Path, PathBuf}; +use std::sync::Mutex; + +use cargo::core::compiler::{CompileMode, Context, Kind, Unit}; +use cargo::core::profiles::Profile; +use cargo::core::{PackageId, Target, TargetKind}; +use cargo::util::ProcessBuilder; +use cargo_metadata; +use log::{error, trace}; +use url::Url; + +use crate::build::PackageArg; +use crate::build::plan::{BuildKey, BuildGraph, JobQueue, WorkStatus}; +use crate::lsp_data::parse_file_path; + +/// Main key type by which `Unit`s will be distinguished in the build plan. +/// In Target we're mostly interested in TargetKind (Lib, Bin, ...) and name +/// (e.g. we can have 2 binary targets with different names). +crate type UnitKey = (PackageId, Target, CompileMode); + +/// Holds the information how exactly the build will be performed for a given +/// workspace with given, specified features. +#[derive(Debug, Default)] +crate struct CargoPlan { + /// Stores a full Cargo `Unit` data for a first processed unit with a given key. + crate units: HashMap, + /// Main dependency graph between the simplified units. + crate dep_graph: HashMap>, + /// Reverse dependency graph that's used to construct a dirty compiler call queue. + crate rev_dep_graph: HashMap>, + /// Cached compiler calls used when creating a compiler call queue. + crate compiler_jobs: HashMap, + // An object for finding the package which a file belongs to and this inferring + // a package argument. + package_map: Option, + /// Packages (names) for which this build plan was prepared. + /// Used to detect if the plan can reused when building certain packages. + built_packages: HashSet, +} + +impl CargoPlan { + crate fn with_manifest(manifest_path: &Path) -> CargoPlan { + CargoPlan { + package_map: Some(PackageMap::new(manifest_path)), + ..Default::default() + } + } + + crate fn with_packages(manifest_path: &Path, pkgs: HashSet) -> CargoPlan { + CargoPlan { + built_packages: pkgs, + ..Self::with_manifest(manifest_path) + } + } + + /// Returns whether a build plan has cached compiler invocations and dep + /// graph so it's at all able to return a job queue via `prepare_work`. + crate fn is_ready(&self) -> bool { + !self.compiler_jobs.is_empty() + } + + /// Cache a given compiler invocation in `ProcessBuilder` for a given + /// `PackageId` and `TargetKind` in `Target`, to be used when processing + /// cached build plan. + crate fn cache_compiler_job( + &mut self, + id: &PackageId, + target: &Target, + mode: CompileMode, + cmd: &ProcessBuilder, + ) { + let unit_key = (id.clone(), target.clone(), mode); + self.compiler_jobs.insert(unit_key, cmd.clone()); + } + + /// Emplace a given `Unit`, along with its `Unit` dependencies (recursively) + /// into the dependency graph as long as the passed `Unit` isn't filtered + /// out by the `filter` closure. + crate fn emplace_dep_with_filter( + &mut self, + unit: &Unit<'_>, + cx: &Context<'_, '_>, + filter: &Filter, + ) + where + Filter: Fn(&Unit<'_>) -> bool, + { + if !filter(unit) { + return; + } + + let key = key_from_unit(unit); + self.units.entry(key.clone()).or_insert_with(|| (*unit).into()); + // Process only those units, which are not yet in the dep graph. + if self.dep_graph.get(&key).is_some() { + return; + } + + // Keep all the additional Unit information for a given unit (It's + // worth remembering, that the units are only discriminated by a + // pair of (PackageId, TargetKind), so only first occurrence will be saved. + self.units.insert(key.clone(), (*unit).into()); + + // Fetch and insert relevant unit dependencies to the forward dep graph. + let units = cx.dep_targets(unit); + let dep_keys: HashSet = units.iter() + // We might not want certain deps to be added transitively (e.g. + // when creating only a sub-dep-graph, limiting the scope). + .filter(|unit| filter(unit)) + .map(key_from_unit) + // Units can depend on others with different Targets or Profiles + // (e.g. different `run_custom_build`) despite having the same UnitKey. + // We coalesce them here while creating the UnitKey dep graph. + .filter(|dep| key != *dep) + .collect(); + self.dep_graph.insert(key.clone(), dep_keys.clone()); + + // We also need to track reverse dependencies here, as it's needed + // to quickly construct a work sub-graph from a set of dirty units. + self.rev_dep_graph + .entry(key.clone()) + .or_insert_with(HashSet::new); + for unit in dep_keys { + let revs = self.rev_dep_graph.entry(unit).or_insert_with(HashSet::new); + revs.insert(key.clone()); + } + + // Recursively process other remaining forward dependencies. + for unit in units { + self.emplace_dep_with_filter(&unit, cx, filter); + } + } + + /// TODO: Improve detecting dirty crate targets for a set of dirty file paths. + /// This uses a lousy heuristic of checking path prefix for a given crate + /// target to determine whether a given unit (crate target) is dirty. This + /// can easily backfire, e.g. when build script is under src/. Any change + /// to a file under src/ would imply the build script is always dirty, so we + /// never do work and always offload to Cargo in such case. + /// Because of that, build scripts are checked separately and only other + /// crate targets are checked with path prefixes. + fn fetch_dirty_units>(&self, files: &[T]) -> HashSet { + let mut result = HashSet::new(); + + let build_scripts: HashMap<&Path, UnitKey> = self + .units + .iter() + .filter(|&(&(_, ref target, _), _)| { + *target.kind() == TargetKind::CustomBuild && target.src_path().is_path() + }) + .map(|(key, unit)| (unit.target.src_path().path(), key.clone())) + .collect(); + let other_targets: HashMap = self + .units + .iter() + .filter(|&(&(_, ref target, _), _)| *target.kind() != TargetKind::CustomBuild) + .map(|(key, unit)| { + ( + key.clone(), + unit.target + .src_path() + .path() + .parent() + .expect("no parent for src_path"), + ) + }).collect(); + + for modified in files.iter().map(|x| x.as_ref()) { + if let Some(unit) = build_scripts.get(modified) { + result.insert(unit.clone()); + } else { + // Not a build script, so we associate a dirty file with a + // package by finding longest (most specified) path prefix. + let matching_prefix_components = |a: &Path, b: &Path| -> usize { + assert!(a.is_absolute() && b.is_absolute()); + a.components().zip(b.components()) + .skip(1) // Skip RootDir + .take_while(|&(x, y)| x == y) + .count() + }; + // Since a package can correspond to many units (e.g. compiled + // as a regular binary or a test harness for unit tests), we + // collect every unit having the longest path prefix. + let max_matching_prefix = other_targets + .values() + .map(|src_dir| matching_prefix_components(modified, src_dir)) + .max(); + + match max_matching_prefix { + Some(0) => error!( + "Modified file {} didn't correspond to any buildable unit!", + modified.display() + ), + Some(max) => { + let dirty_units = other_targets + .iter() + .filter(|(_, dir)| max == matching_prefix_components(modified, dir)) + .map(|(unit, _)| unit); + + result.extend(dirty_units.cloned()); + } + None => {} // Possible that only build scripts were modified + } + } + } + result + } + + /// For a given set of select dirty units, returns a set of all the + /// dependencies that has to be rebuilt transitively. + fn transitive_dirty_units(&self, dirties: &HashSet) -> HashSet { + let mut transitive = dirties.clone(); + // Walk through a rev dep graph using a stack of nodes to collect + // transitively every dirty node + let mut to_process: Vec<_> = dirties.iter().cloned().collect(); + while let Some(top) = to_process.pop() { + if transitive.get(&top).is_some() { + continue; + } + transitive.insert(top.clone()); + + // Process every dirty rev dep of the processed node + let dirty_rev_deps = self + .rev_dep_graph + .get(&top) + .expect("missing key in rev_dep_graph") + .iter() + .filter(|dep| dirties.contains(dep)); + for rev_dep in dirty_rev_deps { + to_process.push(rev_dep.clone()); + } + } + transitive + } + + /// Creates a dirty reverse dependency graph using a set of given dirty units. + fn dirty_rev_dep_graph( + &self, + dirties: &HashSet, + ) -> HashMap> { + let dirties = self.transitive_dirty_units(dirties); + trace!("transitive_dirty_units: {:?}", dirties); + + self.rev_dep_graph.iter() + // Remove nodes that are not dirty + .filter(|&(unit, _)| dirties.contains(unit)) + // Retain only dirty dependencies of the ones that are dirty + .map(|(k, deps)| (k.clone(), deps.iter().cloned().filter(|d| dirties.contains(d)).collect())) + .collect() + } + + /// Returns a topological ordering of a connected DAG of rev deps. The + /// output is a stack of units that can be linearly rebuilt, starting from + /// the last element. + fn topological_sort(&self, dirties: &HashMap>) -> Vec { + let mut visited = HashSet::new(); + let mut output = vec![]; + + for k in dirties.keys() { + if !visited.contains(k) { + dfs(k, &self.rev_dep_graph, &mut visited, &mut output); + } + } + + return output; + + // Process graph depth-first recursively. A node needs to be pushed + // after processing every other before to ensure topological ordering. + fn dfs( + unit: &UnitKey, + graph: &HashMap>, + visited: &mut HashSet, + output: &mut Vec, + ) { + if visited.contains(unit) { + return; + } else { + visited.insert(unit.clone()); + for neighbour in graph.get(unit).into_iter().flat_map(|nodes| nodes) { + dfs(neighbour, graph, visited, output); + } + output.push(unit.clone()); + } + } + } + + crate fn prepare_work + fmt::Debug>( + &self, + modified: &[T], + ) -> WorkStatus { + if !self.is_ready() || self.package_map.is_none() { + return WorkStatus::NeedsCargo(PackageArg::Default); + } + + let dirty_packages = self + .package_map + .as_ref() + .unwrap() + .compute_dirty_packages(modified); + + let needs_more_packages = dirty_packages + .difference(&self.built_packages) + .next() + .is_some(); + + let needed_packages = self + .built_packages + .union(&dirty_packages) + .cloned() + .collect(); + + // We modified a file from a packages, that are not included in the + // cached build plan - run Cargo to recreate the build plan including them + if needs_more_packages { + return WorkStatus::NeedsCargo(PackageArg::Packages(needed_packages)); + } + + let dirties = self.fetch_dirty_units(modified); + trace!( + "fetch_dirty_units: for files {:?}, these units are dirty: {:?}", + modified, + dirties, + ); + + if dirties + .iter() + .any(|&(_, ref target, _)| *target.kind() == TargetKind::CustomBuild) + { + WorkStatus::NeedsCargo(PackageArg::Packages(needed_packages)) + } else { + let graph = self.dirty_rev_dep_graph(&dirties); + trace!("Constructed dirty rev dep graph: {:?}", graph); + + if graph.is_empty() { + return WorkStatus::NeedsCargo(PackageArg::Default); + } + + let queue = self.topological_sort(&graph); + trace!( + "Topologically sorted dirty graph: {:?} {}", + queue, + self.is_ready() + ); + let jobs: Option> = queue + .iter() + .map(|x| self.compiler_jobs.get(x).cloned()) + .collect(); + + // It is possible that we want a job which is not in our cache (compiler_jobs), + // for example we might be building a workspace with an error in a crate and later + // crates within the crate that depend on the error-ing one have never been built. + // In that case we need to build from scratch so that everything is in our cache, or + // we cope with the error. In the error case, jobs will be None. + match jobs { + None => WorkStatus::NeedsCargo(PackageArg::Default), + Some(jobs) => { + assert!(!jobs.is_empty()); + WorkStatus::Execute(JobQueue::with_commands(jobs)) + } + } + } + } +} + +/// Maps paths to packages. +/// +/// The point of the PackageMap is detect if additional packages need to be +/// included in the cached build plan. The cache can represent only a subset of +/// the entire workspace, hence why we need to detect if a package was modified +/// that's outside the cached build plan - if so, we need to recreate it, +/// including the new package. +#[derive(Debug)] +struct PackageMap { + // A map from a manifest directory to the package name. + package_paths: HashMap, + // A map from a file's path, to the package it belongs to. + map_cache: Mutex>, +} + +impl PackageMap { + fn new(manifest_path: &Path) -> PackageMap { + PackageMap { + package_paths: Self::discover_package_paths(manifest_path), + map_cache: Mutex::new(HashMap::new()), + } + } + + // Find each package in the workspace and record the root directory and package name. + fn discover_package_paths(manifest_path: &Path) -> HashMap { + trace!("read metadata {:?}", manifest_path); + let metadata = match cargo_metadata::metadata(Some(manifest_path)) { + Ok(metadata) => metadata, + Err(_) => return HashMap::new(), + }; + metadata + .workspace_members + .into_iter() + .map(|wm| { + assert!(wm.url().starts_with("path+")); + let url = Url::parse(&wm.url()[5..]).expect("Bad URL"); + let path = parse_file_path(&url).expect("URL not a path"); + (path, wm.name().into()) + }).collect() + } + + /// Given modified set of files, returns a set of corresponding dirty packages. + fn compute_dirty_packages + fmt::Debug>( + &self, + modified_files: &[T], + ) -> HashSet { + modified_files + .iter() + .filter_map(|p| self.map(p.as_ref())) + .collect() + } + + // Map a file to the package which it belongs to. + // We do this by walking up the directory tree from `path` until we get to + // one of the recorded package root directories. + fn map(&self, path: &Path) -> Option { + if self.package_paths.is_empty() { + return None; + } + + let mut map_cache = self.map_cache.lock().unwrap(); + if map_cache.contains_key(path) { + return Some(map_cache[path].clone()); + } + + let result = Self::map_uncached(path, &self.package_paths)?; + + map_cache.insert(path.to_owned(), result.clone()); + Some(result) + } + + fn map_uncached(path: &Path, package_paths: &HashMap) -> Option { + if package_paths.is_empty() { + return None; + } + + match package_paths.get(path) { + Some(package) => Some(package.clone()), + None => Self::map_uncached(path.parent()?, package_paths), + } + } +} + +fn key_from_unit(unit: &Unit<'_>) -> UnitKey { + ( + unit.pkg.package_id().clone(), + unit.target.clone(), + unit.mode, + ) +} + +#[derive(Hash, PartialEq, Eq, Debug, Clone)] +/// An owned version of `cargo::core::Unit`. +crate struct OwnedUnit { + crate id: PackageId, + crate target: Target, + crate profile: Profile, + crate kind: Kind, + crate mode: CompileMode, +} + +impl<'a> From> for OwnedUnit { + fn from(unit: Unit<'a>) -> OwnedUnit { + OwnedUnit { + id: unit.pkg.package_id().to_owned(), + target: unit.target.clone(), + profile: unit.profile, + kind: unit.kind, + mode: unit.mode, + } + } +} + +impl BuildKey for OwnedUnit { + type Key = UnitKey; + + fn key(&self) -> UnitKey { + (self.id.clone(), self.target.clone(), self.mode) + } +} + +impl BuildGraph for CargoPlan { + type Unit = OwnedUnit; + + fn units(&self) -> Vec<&Self::Unit> { + self.units.values().collect() + } + fn get(&self, key: ::Key) -> Option<&Self::Unit> { + self.units.get(&key) + } + fn get_mut(&mut self, key: ::Key) -> Option<&mut Self::Unit> { + self.units.get_mut(&key) + } + fn deps(&self, key: ::Key) -> Vec<&Self::Unit> { + self.dep_graph + .get(&key) + .map(|d| d.iter().map(|d| &self.units[d]).collect()) + .unwrap_or_default() + } + + fn add>(&mut self, unit: T, deps: Vec) { + let unit = unit.into(); + // Units can depend on others with different Targets or Profiles + // (e.g. different `run_custom_build`) despite having the same UnitKey. + // We coalesce them here while creating the UnitKey dep graph. + // TODO: Are we sure? Can we figure that out? + let deps = deps.into_iter().map(|d| d.into()).filter(|dep| unit.key() != dep.key()); + + for dep in deps { + self.dep_graph.entry(unit.key()).or_insert_with(HashSet::new).insert(dep.key()); + self.rev_dep_graph.entry(dep.key()).or_insert_with(HashSet::new).insert(unit.key()); + + self.units.entry(dep.key()).or_insert(dep); + } + + // We expect these entries to be present for each unit in the graph + self.dep_graph.entry(unit.key()).or_insert_with(HashSet::new); + self.rev_dep_graph.entry(unit.key()).or_insert_with(HashSet::new); + + self.units.entry(unit.key()).or_insert(unit); + } + + fn dirties>(&self, files: &[T]) -> Vec<&Self::Unit> { + self.fetch_dirty_units(files) + .iter() + .map(|key| self.units.get(key).expect("dirties")) + .collect() + } + + fn dirties_transitive>(&self, files: &[T]) -> Vec<&Self::Unit> { + let dirties = self.fetch_dirty_units(files); + + self.transitive_dirty_units(&dirties) + .iter() + .map(|key| self.units.get(key).expect("dirties_transitive")) + .collect() + } + + fn topological_sort(&self, units: Vec<&Self::Unit>) -> Vec<&Self::Unit> { + let keys = units.into_iter().map(|u| u.key()).collect(); + let graph = self.dirty_rev_dep_graph(&keys); + + CargoPlan::topological_sort(self, &graph) + .iter() + .map(|key| self.units.get(key).expect("topological_sort")) + .collect() + } + + fn prepare_work + std::fmt::Debug>(&self, files: &[T]) -> WorkStatus { + CargoPlan::prepare_work(self, files) + } +} diff --git a/src/build/external.rs b/src/build/external.rs index 9841cb5d8d4..6cad7346b1e 100644 --- a/src/build/external.rs +++ b/src/build/external.rs @@ -8,21 +8,50 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -//! Performs a build using a provided black-box build command, which ought to -//! return a list of save-analysis JSON files to be reloaded by the RLS. -//! Please note that since the command is ran externally (at a file/OS level) -//! this doesn't work with files that are not saved. +//! Contains data and logic for executing builds specified externally (rather +//! than executing and intercepting Cargo calls). +//! +//! Provides deserialization structs for the build plan format as it is output +//! by `cargo build --build-plan` and means to execute that plan as part of the +//! RLS build to retrieve diagnostics and analysis data. +//! +//! Additionally, we allow to build the analysis data with an external command, +//! which should return a list of save-analysis JSON files to be reloaded by RLS. +//! From these we construct an internal build plan that is used to rebuild +//! the project incrementally ourselves. +use std::collections::{BTreeMap, HashMap, HashSet}; +use std::collections::hash_map::DefaultHasher; use std::fs::File; +use std::hash::{Hash, Hasher}; use std::io::BufRead; use std::io::Read; use std::path::{Path, PathBuf}; use std::process::{Command, Stdio}; -use super::BuildResult; +use crate::build::BuildResult; +use crate::build::plan::{BuildKey, BuildGraph, JobQueue, WorkStatus}; +use cargo::util::{process, ProcessBuilder}; use log::trace; -use rls_data::Analysis; +use rls_data::{Analysis, CompilationOptions}; +use serde_derive::Deserialize; + +fn cmd_line_to_command>(cmd_line: &S, cwd: &Path) -> Result { + let cmd_line = cmd_line.as_ref(); + let (cmd, args) = { + let mut words = cmd_line.split_whitespace(); + let cmd = words.next().ok_or(())?; + (cmd, words) + }; + + let mut cmd = Command::new(cmd); + cmd.args(args) + .current_dir(cwd) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()); + Ok(cmd) +} /// Performs a build using an external command and interprets the results. /// The command should output on stdout a list of save-analysis .json files @@ -32,31 +61,22 @@ use rls_data::Analysis; pub(super) fn build_with_external_cmd>( cmd_line: S, build_dir: PathBuf, -) -> BuildResult { +) -> (BuildResult, Result) { let cmd_line = cmd_line.as_ref(); - let (cmd, args) = { - let mut words = cmd_line.split_whitespace(); - let cmd = match words.next() { - Some(cmd) => cmd, - None => { - return BuildResult::Err("Specified build_command is empty".into(), None); - } - }; - (cmd, words) + + let mut cmd = match cmd_line_to_command(&cmd_line, &build_dir) { + Ok(cmd) => cmd, + Err(_) => { + let err_msg = format!("Couldn't treat {} as command", cmd_line); + return (BuildResult::Err(err_msg, Some(cmd_line.to_owned())), Err(())); + } }; - let spawned = Command::new(&cmd) - .args(args) - .current_dir(&build_dir) - .stdout(Stdio::piped()) - .stderr(Stdio::null()) - .spawn(); - let child = match spawned { + let child = match cmd.spawn() { Ok(child) => child, Err(io) => { let err_msg = format!("Couldn't execute: {} ({:?})", cmd_line, io.kind()); - trace!("{}", err_msg); - return BuildResult::Err(err_msg, Some(cmd_line.to_owned())); + return (BuildResult::Err(err_msg, Some(cmd_line.to_owned())), Err(())); } }; @@ -71,11 +91,12 @@ pub(super) fn build_with_external_cmd>( Ok(analyses) => analyses, Err(cause) => { let err_msg = format!("Couldn't read analysis data: {}", cause); - return BuildResult::Err(err_msg, Some(cmd_line.to_owned())); + return (BuildResult::Err(err_msg, Some(cmd_line.to_owned())), Err(())); } }; - BuildResult::Success(build_dir.clone(), vec![], analyses, false) + let plan = plan_from_analysis(&analyses, &build_dir); + (BuildResult::Success(build_dir, vec![], analyses, false), plan) } /// Reads and deserializes given save-analysis JSON files into corresponding @@ -105,3 +126,447 @@ where Ok(analyses) } + +fn plan_from_analysis(analysis: &[Analysis], build_dir: &Path) -> Result { + let indices: HashMap<_, usize> = analysis + .iter() + .enumerate() + .map(|(idx, a)| (a.prelude.as_ref().unwrap().crate_id.disambiguator, idx)) + .collect(); + + let invocations: Vec = analysis.into_iter() + .map(|a| { + let CompilationOptions { ref directory, ref program, ref arguments, ref output } = + a.compilation.as_ref().ok_or(())?; + + let deps: Vec = a.prelude.as_ref().unwrap() + .external_crates + .iter() + .filter_map(|c| indices.get(&c.id.disambiguator)) + .cloned() + .collect(); + + let cwd = match directory.is_relative() { + true => build_dir.join(directory), + false => directory.to_owned(), + }; + + Ok(RawInvocation { + deps, + outputs: vec![output.clone()], + program: program.clone(), + args: arguments.clone(), + env: Default::default(), + links: Default::default(), + cwd: Some(cwd) + + }) + }) + .collect::, ()>>()?; + + ExternalPlan::try_from_raw(RawPlan { invocations }) +} + +#[derive(Debug, Deserialize)] +/// Build plan as emitted by `cargo build --build-plan -Zunstable-options` +crate struct RawPlan { + crate invocations: Vec, +} + +#[derive(Debug, Deserialize)] +crate struct RawInvocation { + crate deps: Vec, + crate outputs: Vec, + #[serde(default)] + crate links: BTreeMap, + crate program: String, + crate args: Vec, + crate env: BTreeMap, + #[serde(default)] + crate cwd: Option, +} + +#[derive(Clone, Debug)] +crate struct Invocation { + deps: Vec, // FIXME: Use arena and store refs instead for ergonomics + outputs: Vec, + links: BTreeMap, + command: ProcessBuilder, + // Parsed data + src_path: Option, +} + +/// Safe build plan type, invocation dependencies are guaranteed to be inside +/// the plan. +#[derive(Debug, Default)] +crate struct ExternalPlan { + units: HashMap, + deps: HashMap>, + rev_deps: HashMap>, +} + +impl BuildKey for Invocation { + type Key = u64; + + // Invocation key is the hash of the program, its arguments and environment. + fn key(&self) -> u64 { + let mut hash = DefaultHasher::new(); + + self.command.get_program().hash(&mut hash); + let /*mut*/ args = self.command.get_args().to_owned(); + // args.sort(); // TODO: Parse 2-part args (e.g. ["--extern", "a=b"]) + args.hash(&mut hash); + let mut envs: Vec<_> = self.command.get_envs().iter().collect(); + envs.sort(); + envs.hash(&mut hash); + + hash.finish() + } +} + +impl From for Invocation { + fn from(raw: RawInvocation) -> Invocation { + let mut command = process(&raw.program); + command.args(&raw.args); + for (k, v) in &raw.env { + command.env(&k, v); + } + if let Some(cwd) = &raw.cwd { + command.cwd(cwd); + } + + Invocation { + deps: raw.deps.to_owned(), + outputs: raw.outputs.to_owned(), + links: raw.links.to_owned(), + src_path: guess_rustc_src_path(&command), + command, + } + } +} + +impl ExternalPlan { + crate fn new() -> ExternalPlan { + Default::default() + } + + crate fn with_units(units: Vec) -> ExternalPlan { + let mut plan = ExternalPlan::new(); + for unit in &units { + for &dep in &unit.deps { + plan.add_dep(unit.key(), units[dep].key()); + } + } + + ExternalPlan { + units: units.into_iter().map(|u| (u.key(), u)).collect(), + ..plan + } + } + + #[rustfmt::skip] + fn add_dep(&mut self, key: u64, dep: u64) { + self.deps.entry(key).or_insert_with(HashSet::new).insert(dep); + self.rev_deps.entry(dep).or_insert_with(HashSet::new).insert(key); + } + + crate fn try_from_raw(raw: RawPlan) -> Result { + // Sanity check, each dependency (index) has to be inside the build plan + if raw + .invocations + .iter() + .flat_map(|inv| &inv.deps) + .any(|idx| raw.invocations.get(*idx).is_none()) + { + return Err(()); + } + + let units: Vec = raw.invocations.into_iter().map(|x| x.into()).collect(); + + Ok(ExternalPlan::with_units(units)) + } +} + +impl BuildGraph for ExternalPlan { + type Unit = Invocation; + + fn units(&self) -> Vec<&Self::Unit> { + self.units.values().collect() + } + + fn get(&self, key: u64) -> Option<&Self::Unit> { + self.units.get(&key) + } + + fn get_mut(&mut self, key: u64) -> Option<&mut Self::Unit> { + self.units.get_mut(&key) + } + + fn deps(&self, key: u64) -> Vec<&Self::Unit> { + self.deps + .get(&key) + .map(|d| d.iter().map(|d| &self.units[d]).collect()) + .unwrap_or_default() + } + + fn add(&mut self, unit: T, deps: Vec) + where + T: Into + { + let unit = unit.into(); + + for dep in deps.into_iter().map(|d| d.into()) { + self.add_dep(unit.key(), dep.key()); + + self.units.entry(dep.key()).or_insert(dep); + } + + self.rev_deps.entry(unit.key()).or_insert_with(HashSet::new); + self.units.entry(unit.key()).or_insert(unit); + } + + // FIXME: Change associating files with units by their path but rather + // include file inputs in the build plan or call rustc with --emit=dep-info + fn dirties>(&self, modified: &[T]) -> Vec<&Self::Unit> { + let mut results = HashSet::::new(); + + for modified in modified.iter().map(|x| x.as_ref()) { + // We associate a dirty file with a + // package by finding longest (most specified) path prefix. + let matching_prefix_components = |a: &Path, b: &Path| -> usize { + assert!(a.is_absolute() && b.is_absolute()); + a.components() + .zip(b.components()) + .take_while(|&(x, y)| x == y) + .count() + }; + // Since a package can correspond to many units (e.g. compiled + // as a regular binary or a test harness for unit tests), we + // collect every unit having the longest path prefix. + let matching_units: Vec<(&_, usize)> = self.units.values() + // For `rustc dir/some.rs` we'll consider every changed files + // under dir/ as relevant + .map(|unit| (unit, unit.src_path.as_ref().and_then(|src| src.parent()))) + .filter_map(|(unit, src)| src.map(|src| (unit, src))) + // Discard units that are in a different directory subtree + .filter_map(|(unit, src)| { + let matching = matching_prefix_components(modified, &src); + if matching >= src.components().count() { + Some((unit, matching)) + } else { + None + } + }) + .collect(); + + // Changing files in the same directory might affect multiple units + // (e.g. multiple crate binaries, their unit test harness), so + // treat all of them as dirty. + if let Some(max_prefix) = matching_units.iter().map(|(_, p)| p).max() { + let dirty_keys = matching_units + .iter() + .filter(|(_, prefix)| prefix == max_prefix) + .map(|(unit, _)| unit.key()); + + results.extend(dirty_keys); + } + } + + results.iter().map(|key| &self.units[key]).collect() + } + + fn dirties_transitive>(&self, files: &[T]) -> Vec<&Self::Unit> { + let mut results = HashSet::new(); + + let mut stack = self.dirties(files); + + while let Some(key) = stack.pop().map(|u| u.key()) { + if results.insert(key) { + if let Some(rdeps) = self.rev_deps.get(&key) { + for rdep in rdeps { + stack.push(&self.units[rdep]); + } + } + } + } + + results.into_iter().map(|key| &self.units[&key]).collect() + } + + fn topological_sort(&self, units: Vec<&Self::Unit>) -> Vec<&Self::Unit> { + let dirties: HashSet<_> = units.into_iter().map(|u| u.key()).collect(); + + let mut visited: HashSet<_> = HashSet::new(); + let mut output = vec![]; + + for k in dirties { + if !visited.contains(&k) { + dfs(k, &self.rev_deps, &mut visited, &mut output); + } + } + + return output.iter().map(|key| &self.units[key]).collect(); + + // Process graph depth-first recursively. A node needs to be pushed + // after processing every other before to ensure topological ordering. + fn dfs( + unit: u64, + graph: &HashMap>, + visited: &mut HashSet, + output: &mut Vec, + ) { + if visited.insert(unit) { + for &neighbour in graph.get(&unit).iter().flat_map(|&edges| edges) { + dfs(neighbour, graph, visited, output); + } + output.push(unit); + } + } + } + + fn prepare_work>(&self, files: &[T]) -> WorkStatus { + let dirties = self.dirties_transitive(files); + let topo = self.topological_sort(dirties); + + let cmds = topo.into_iter().map(|unit| unit.command.clone()).collect(); + + WorkStatus::Execute(JobQueue::with_commands(cmds)) + } +} + +fn guess_rustc_src_path(cmd: &ProcessBuilder) -> Option { + if !Path::new(cmd.get_program()).ends_with("rustc") { + return None; + } + + let file = cmd + .get_args() + .iter() + .find(|&a| Path::new(a).extension().map(|e| e == "rs").unwrap_or(false))?; + let file_path = PathBuf::from(file); + + Some(match (cmd.get_cwd(), file_path.is_absolute()) { + (_, true) => file_path, + (Some(cwd), _) => cwd.join(file_path), + // TODO: is cwd correct here? + (None, _) => std::env::current_dir().ok()?.join(file_path), + }) +} + +#[cfg(test)] +mod tests { + use super::*; + + trait Sorted { + fn sorted(self) -> Self; + } + + impl Sorted for Vec { + fn sorted(mut self: Self) -> Self { + self.sort(); + self + } + } + + /// Helper struct that prints sorted unit source directories in a given plan. + #[derive(Debug)] + struct SrcPaths<'a>(Vec<&'a PathBuf>); + impl<'a> SrcPaths<'a> { + fn from(plan: &ExternalPlan) -> SrcPaths<'_> { + SrcPaths( + plan.units() + .iter() + .filter_map(|u| u.src_path.as_ref()) + .collect(), + ) + } + } + + fn paths<'a>(invocations: &Vec<&'a Invocation>) -> Vec<&'a str> { + invocations + .iter() + .filter_map(|d| d.src_path.as_ref()) + .map(|p| p.to_str().unwrap()) + .collect() + } + + #[test] + fn dirty_units_path_heuristics() { + let plan = r#"{"invocations": [ + { "deps": [], "program": "rustc", "args": ["--crate-name", "build_script_build", "/my/repo/build.rs"], "env": {}, "outputs": [] }, + { "deps": [0], "program": "rustc", "args": ["--crate-name", "repo", "/my/repo/src/lib.rs"], "env": {}, "outputs": [] } + ]}"#; + let plan = serde_json::from_str::(&plan).unwrap(); + let plan = ExternalPlan::try_from_raw(plan).unwrap(); + + eprintln!("src_paths: {:#?}", &SrcPaths::from(&plan)); + + let dirties = |file: &str| -> Vec<&str> { + plan.dirties(&[file]) + .iter() + .filter_map(|d| d.src_path.as_ref()) + .map(|p| p.to_str().unwrap()) + .collect() + }; + + assert_eq!(dirties("/my/dummy.rs"), Vec::<&str>::new()); + assert_eq!(dirties("/my/repo/dummy.rs"), vec!["/my/repo/build.rs"]); + assert_eq!(dirties("/my/repo/src/c.rs"), vec!["/my/repo/src/lib.rs"]); + assert_eq!(dirties("/my/repo/src/a/b.rs"), vec!["/my/repo/src/lib.rs"]); + } + + #[test] + fn dirties_transitive() { + let plan = r#"{"invocations": [ + { "deps": [], "program": "rustc", "args": ["--crate-name", "build_script_build", "/my/repo/build.rs"], "env": {}, "outputs": [] }, + { "deps": [0], "program": "rustc", "args": ["--crate-name", "repo", "/my/repo/src/lib.rs"], "env": {}, "outputs": [] } + ]}"#; + let plan = serde_json::from_str::(&plan).unwrap(); + let plan = ExternalPlan::try_from_raw(plan).unwrap(); + + eprintln!("src_paths: {:#?}", &SrcPaths::from(&plan)); + eprintln!("plan: {:?}", &plan); + + assert_eq!( + paths(&plan.dirties(&["/my/repo/src/a/b.rs"])), + vec!["/my/repo/src/lib.rs"] + ); + + assert_eq!( + paths(&plan.dirties_transitive(&["/my/repo/file.rs"])).sorted(), + vec!["/my/repo/build.rs", "/my/repo/src/lib.rs"].sorted(), + ); + assert_eq!( + paths(&plan.dirties_transitive(&["/my/repo/src/file.rs"])).sorted(), + vec!["/my/repo/src/lib.rs"].sorted(), + ); + } + + #[test] + fn topological_sort() { + let plan = r#"{"invocations": [ + { "deps": [], "program": "rustc", "args": ["--crate-name", "build_script_build", "/my/repo/build.rs"], "env": {}, "outputs": [] }, + { "deps": [0], "program": "rustc", "args": ["--crate-name", "repo", "/my/repo/src/lib.rs"], "env": {}, "outputs": [] } + ]}"#; + let plan = serde_json::from_str::(&plan).unwrap(); + let plan = ExternalPlan::try_from_raw(plan).unwrap(); + + eprintln!("src_paths: {:#?}", &SrcPaths::from(&plan)); + eprintln!("plan: {:?}", &plan); + + let units_to_rebuild = plan.dirties_transitive(&["/my/repo/file.rs"]); + assert_eq!( + paths(&units_to_rebuild).sorted(), + vec!["/my/repo/build.rs", "/my/repo/src/lib.rs"].sorted(), + ); + + // TODO: Test on non-trivial input, use Iterator::position if + // nondeterminate order wrt hashing is a problem + // Jobs that have to run first are *last* in the topological sorting here + let topo_units = plan.topological_sort(units_to_rebuild); + assert_eq!( + paths(&topo_units), + vec!["/my/repo/src/lib.rs", "/my/repo/build.rs"], + ) + } +} diff --git a/src/build/mod.rs b/src/build/mod.rs index 6f13bdf7943..b9ff8ed2d3d 100644 --- a/src/build/mod.rs +++ b/src/build/mod.rs @@ -10,12 +10,10 @@ //! Running builds as-needed for the server to answer questions. -pub use self::cargo::make_cargo_config; - use self::environment::EnvironmentLock; -use self::plan::{Plan as BuildPlan, WorkStatus}; +use self::plan::{BuildGraph, BuildPlan, WorkStatus}; -use ::cargo::util::{CargoError, important_paths}; +use ::cargo::util::CargoError; use crate::actions::post_build::PostBuildHandler; use crate::actions::progress::{ProgressNotifier, ProgressUpdate}; use crate::config::Config; @@ -34,6 +32,7 @@ use std::thread; use std::time::{Duration, Instant}; mod cargo; +mod cargo_plan; pub mod environment; mod external; mod plan; @@ -147,7 +146,7 @@ struct CompilationContext { /// Whether needs to perform a Cargo rebuild needs_rebuild: bool, /// Build plan, which should know all the inter-package/target dependencies - /// along with args/envs. Only contains inter-package dep-graph for now. + /// along with args/envs. build_plan: BuildPlan, } @@ -523,37 +522,56 @@ impl Internals { // do this so we can load changed code from the VFS, rather than from // disk). - // Check if an override setting was provided and execute that, instead. - if let Some(ref cmd) = self.config.lock().unwrap().build_command { - let build_dir = self.compilation_cx.lock().unwrap().build_dir.clone(); - return external::build_with_external_cmd(cmd, build_dir.unwrap()); - } - // If the build plan has already been cached, use it, unless Cargo // has to be specifically rerun (e.g. when build scripts changed) let work = { let modified: Vec<_> = self.dirty_files.lock().unwrap().keys().cloned().collect(); let mut cx = self.compilation_cx.lock().unwrap(); - let needs_to_run_cargo = cx.needs_rebuild; - let build_dir = cx.build_dir.as_ref().unwrap(); - - match important_paths::find_root_manifest_for_wd(build_dir) { - Ok(manifest_path) => { - cx.build_plan - .prepare_work(&manifest_path, &modified, needs_to_run_cargo) + let build_dir = cx.build_dir.clone().unwrap(); + let needs_rebuild = cx.needs_rebuild; + + // Check if an external build command was provided and execute that, instead. + if let Some(cmd) = self.config.lock().unwrap().build_command.clone() { + match (needs_rebuild, &cx.build_plan) { + (false, BuildPlan::External(ref plan)) => { + plan.prepare_work(&modified) + }, + // We need to rebuild; regenerate the build plan if possible. + _ => match external::build_with_external_cmd(cmd, build_dir) { + (result, Err(_)) => return result, + (result, Ok(plan)) => { + cx.needs_rebuild = false; + cx.build_plan = BuildPlan::External(plan); + // Since we don't support diagnostics in external + // builds it might be worth rerunning the commands + // ourselves again to get both analysis *and* diagnostics + return result; + } + }, } - Err(e) => { - let msg = format!("Error reading manifest path: {:?}", e); - return BuildResult::Err(msg, None); + // Fall back to Cargo + } else { + // Cargo plan is recreated and `needs_rebuild` reset if we run cargo::cargo(). + match cx.build_plan { + BuildPlan::External(_) => { + WorkStatus::NeedsCargo(PackageArg::Default) + }, + BuildPlan::Cargo(ref plan) => { + match plan.prepare_work(&modified) { + // Don't reuse the plan if we need to rebuild + WorkStatus::Execute(_) if needs_rebuild => { + WorkStatus::NeedsCargo(PackageArg::Default) + }, + work @ _ => work, + } + } } } }; - trace!("Specified work: {:?}", work); + trace!("Specified work: {:#?}", work); let result = match work { - // Cargo performs the full build and returns - // appropriate diagnostics/analysis data WorkStatus::NeedsCargo(package_arg) => cargo::cargo(self, package_arg, progress_sender), WorkStatus::Execute(job_queue) => job_queue.execute(self, progress_sender), }; diff --git a/src/build/plan.rs b/src/build/plan.rs index 1f9851732c8..f6aa9ce298c 100644 --- a/src/build/plan.rs +++ b/src/build/plan.rs @@ -1,411 +1,49 @@ -// Copyright 2017 The Rust Project Developers. See the COPYRIGHT -// file at the top-level directory of this distribution and at -// http://rust-lang.org/COPYRIGHT. -// -// Licensed under the Apache License, Version 2.0 or the MIT license -// , at your -// option. This file may not be copied, modified, or distributed -// except according to those terms. - -//! This contains a build plan that is created during the Cargo build routine -//! and stored afterwards, which can be later queried, given a list of dirty -//! files, to retrieve a queue of compiler calls to be invoked (including -//! appropriate arguments and env variables). -//! The underlying structure is a dependency graph between simplified units -//! (package id and crate target kind), as opposed to Cargo units (package with -//! a target info, including crate target kind, profile and host/target kind). -//! This will be used for a quick check recompilation and does not aim to -//! reimplement all the intricacies of Cargo. -//! The unit dependency graph in Cargo also distinguishes between compiling the -//! build script and running it and collecting the build script output to modify -//! the subsequent compilations etc. Since build script executions (not building) -//! are not exposed via `Executor` trait in Cargo, we simply coalesce every unit -//! with a same package and crate target kind (e.g. both building and running -//! build scripts). - -use std::collections::{HashMap, HashSet}; -use std::ffi::OsStr; -use std::fmt; +//! Specified a notion of a build graph, which ultimately can be queried as to +//! what work is required (either a list of `rustc` invocations or a rebuild +//! request) for a given set of dirty files. +//! Currently, there are 2 types of build plans: +//! * Cargo - used when we run Cargo in-process and intercept it +//! * External - dependency graph between invocations + +use std::hash::Hash; use std::path::{Path, PathBuf}; +use std::ffi::OsStr; use std::sync::mpsc::Sender; -use std::sync::{Arc, Mutex}; +use std::sync::Arc; -use cargo::core::compiler::{CompileMode, Context, Kind, Unit}; -use cargo::core::profiles::Profile; -use cargo::core::{PackageId, Target, TargetKind}; -use cargo::util::{CargoResult, ProcessBuilder}; -use cargo_metadata; -use crate::build::PackageArg; -use crate::lsp_data::parse_file_path; -use log::{error, trace}; -use url::Url; +use cargo::util::ProcessBuilder; +use log::trace; -use super::{BuildResult, Internals}; use crate::actions::progress::ProgressUpdate; +use crate::build::{BuildResult, Internals, PackageArg}; +use crate::build::cargo_plan::CargoPlan; +use crate::build::external::ExternalPlan; -/// Main key type by which `Unit`s will be distinguished in the build plan. -/// In Target we're mostly interested in TargetKind (Lib, Bin, ...) and name -/// (e.g. we can have 2 binary targets with different names). -crate type UnitKey = (PackageId, Target, CompileMode); - -/// Holds the information how exactly the build will be performed for a given -/// workspace with given, specified features. -crate struct Plan { - /// Stores a full Cargo `Unit` data for a first processed unit with a given key. - crate units: HashMap, - /// Main dependency graph between the simplified units. - crate dep_graph: HashMap>, - /// Reverse dependency graph that's used to construct a dirty compiler call queue. - crate rev_dep_graph: HashMap>, - /// Cached compiler calls used when creating a compiler call queue. - crate compiler_jobs: HashMap, - // An object for finding the package which a file belongs to and this inferring - // a package argument. - package_map: Option, - /// Packages (names) for which this build plan was prepared. - /// Used to detect if the plan can reused when building certain packages. - built_packages: HashSet, +crate trait BuildKey { + type Key: Eq + Hash; + fn key(&self) -> Self::Key; } -impl Plan { - crate fn new() -> Plan { - Self::for_packages(HashSet::new()) - } - - crate fn for_packages(pkgs: HashSet) -> Plan { - Plan { - units: HashMap::new(), - dep_graph: HashMap::new(), - rev_dep_graph: HashMap::new(), - compiler_jobs: HashMap::new(), - package_map: None, - built_packages: pkgs, - } - } - - /// Returns whether a build plan has cached compiler invocations and dep - /// graph so it's at all able to return a job queue via `prepare_work`. - crate fn is_ready(&self) -> bool { - !self.compiler_jobs.is_empty() - } - - /// Cache a given compiler invocation in `ProcessBuilder` for a given - /// `PackageId` and `TargetKind` in `Target`, to be used when processing - /// cached build plan. - crate fn cache_compiler_job( - &mut self, - id: &PackageId, - target: &Target, - mode: CompileMode, - cmd: &ProcessBuilder, - ) { - let unit_key = (id.clone(), target.clone(), mode); - self.compiler_jobs.insert(unit_key, cmd.clone()); - } - - /// Emplace a given `Unit`, along with its `Unit` dependencies (recursively) - /// into the dependency graph. - #[allow(dead_code)] - crate fn emplace_dep(&mut self, unit: &Unit<'_>, cx: &Context<'_, '_>) -> CargoResult<()> { - let null_filter = |_unit: &Unit<'_>| true; - self.emplace_dep_with_filter(unit, cx, &null_filter) - } - - /// Emplace a given `Unit`, along with its `Unit` dependencies (recursively) - /// into the dependency graph as long as the passed `Unit` isn't filtered - /// out by the `filter` closure. - crate fn emplace_dep_with_filter( - &mut self, - unit: &Unit<'_>, - cx: &Context<'_, '_>, - filter: &Filter, - ) -> CargoResult<()> - where - Filter: Fn(&Unit<'_>) -> bool, - { - if !filter(unit) { - return Ok(()); - } - - let key = key_from_unit(unit); - self.units.entry(key.clone()).or_insert_with(|| unit.into()); - // Process only those units, which are not yet in the dep graph. - if self.dep_graph.get(&key).is_some() { - return Ok(()); - } - - // Keep all the additional Unit information for a given unit (It's - // worth remembering, that the units are only discriminated by a - // pair of (PackageId, TargetKind), so only first occurrence will be saved. - self.units.insert(key.clone(), unit.into()); +crate trait BuildGraph { + type Unit: BuildKey; - // Fetch and insert relevant unit dependencies to the forward dep graph. - let units = cx.dep_targets(unit); - let dep_keys: HashSet = units.iter() - // We might not want certain deps to be added transitively (e.g. - // when creating only a sub-dep-graph, limiting the scope). - .filter(|unit| filter(unit)) - .map(key_from_unit) - // Units can depend on others with different Targets or Profiles - // (e.g. different `run_custom_build`) despite having the same UnitKey. - // We coalesce them here while creating the UnitKey dep graph. - .filter(|dep| key != *dep) - .collect(); - self.dep_graph.insert(key.clone(), dep_keys.clone()); + fn units(&self) -> Vec<&Self::Unit>; + fn get(&self, key: ::Key) -> Option<&Self::Unit>; + fn get_mut(&mut self, key: ::Key) -> Option<&mut Self::Unit>; + fn deps(&self, key: ::Key) -> Vec<&Self::Unit>; - // We also need to track reverse dependencies here, as it's needed - // to quickly construct a work sub-graph from a set of dirty units. - self.rev_dep_graph - .entry(key.clone()) - .or_insert_with(HashSet::new); - for unit in dep_keys { - let revs = self.rev_dep_graph.entry(unit).or_insert_with(HashSet::new); - revs.insert(key.clone()); - } - - // Recursively process other remaining forward dependencies. - for unit in units { - self.emplace_dep_with_filter(&unit, cx, filter)?; - } - Ok(()) - } - - /// TODO: Improve detecting dirty crate targets for a set of dirty file paths. - /// This uses a lousy heuristic of checking path prefix for a given crate - /// target to determine whether a given unit (crate target) is dirty. This - /// can easily backfire, e.g. when build script is under src/. Any change - /// to a file under src/ would imply the build script is always dirty, so we - /// never do work and always offload to Cargo in such case. - /// Because of that, build scripts are checked separately and only other - /// crate targets are checked with path prefixes. - fn fetch_dirty_units>(&self, files: &[T]) -> HashSet { - let mut result = HashSet::new(); - - let build_scripts: HashMap<&Path, UnitKey> = self - .units - .iter() - .filter(|&(&(_, ref target, _), _)| { - *target.kind() == TargetKind::CustomBuild && target.src_path().is_path() - }) - .map(|(key, unit)| (unit.target.src_path().path(), key.clone())) - .collect(); - let other_targets: HashMap = self - .units - .iter() - .filter(|&(&(_, ref target, _), _)| *target.kind() != TargetKind::CustomBuild) - .map(|(key, unit)| { - ( - key.clone(), - unit.target - .src_path() - .path() - .parent() - .expect("no parent for src_path"), - ) - }).collect(); - - for modified in files.iter().map(|x| x.as_ref()) { - if let Some(unit) = build_scripts.get(modified) { - result.insert(unit.clone()); - } else { - // Not a build script, so we associate a dirty file with a - // package by finding longest (most specified) path prefix. - let matching_prefix_components = |a: &Path, b: &Path| -> usize { - assert!(a.is_absolute() && b.is_absolute()); - a.components().zip(b.components()) - .skip(1) // Skip RootDir - .take_while(|&(x, y)| x == y) - .count() - }; - // Since a package can correspond to many units (e.g. compiled - // as a regular binary or a test harness for unit tests), we - // collect every unit having the longest path prefix. - let max_matching_prefix = other_targets - .values() - .map(|src_dir| matching_prefix_components(modified, src_dir)) - .max(); - - match max_matching_prefix { - Some(0) => error!( - "Modified file {} didn't correspond to any buildable unit!", - modified.display() - ), - Some(max) => { - let dirty_units = other_targets - .iter() - .filter(|(_, dir)| max == matching_prefix_components(modified, dir)) - .map(|(unit, _)| unit); - - result.extend(dirty_units.cloned()); - } - None => {} // Possible that only build scripts were modified - } - } - } - result - } + fn add>(&mut self, unit: T, deps: Vec); + fn dirties>(&self, files: &[T]) -> Vec<&Self::Unit>; /// For a given set of select dirty units, returns a set of all the /// dependencies that has to be rebuilt transitively. - fn transitive_dirty_units(&self, dirties: &HashSet) -> HashSet { - let mut transitive = dirties.clone(); - // Walk through a rev dep graph using a stack of nodes to collect - // transitively every dirty node - let mut to_process: Vec<_> = dirties.iter().cloned().collect(); - while let Some(top) = to_process.pop() { - if transitive.get(&top).is_some() { - continue; - } - transitive.insert(top.clone()); - - // Process every dirty rev dep of the processed node - let dirty_rev_deps = self - .rev_dep_graph - .get(&top) - .expect("missing key in rev_dep_graph") - .iter() - .filter(|dep| dirties.contains(dep)); - for rev_dep in dirty_rev_deps { - to_process.push(rev_dep.clone()); - } - } - transitive - } - - /// Creates a dirty reverse dependency graph using a set of given dirty units. - fn dirty_rev_dep_graph( - &self, - dirties: &HashSet, - ) -> HashMap> { - let dirties = self.transitive_dirty_units(dirties); - trace!("transitive_dirty_units: {:?}", dirties); - - self.rev_dep_graph.iter() - // Remove nodes that are not dirty - .filter(|&(unit, _)| dirties.contains(unit)) - // Retain only dirty dependencies of the ones that are dirty - .map(|(k, deps)| (k.clone(), deps.iter().cloned().filter(|d| dirties.contains(d)).collect())) - .collect() - } - - /// Returns a topological ordering of a connected DAG of rev deps. The - /// output is a stack of units that can be linearly rebuilt, starting from - /// the last element. - fn topological_sort(&self, dirties: &HashMap>) -> Vec { - let mut visited: HashSet = HashSet::new(); - let mut output = vec![]; - - for k in dirties.keys() { - if !visited.contains(k) { - dfs(k, &self.rev_dep_graph, &mut visited, &mut output); - } - } - - return output; - - // Process graph depth-first recursively. A node needs to be pushed - // after processing every other before to ensure topological ordering. - fn dfs( - unit: &UnitKey, - graph: &HashMap>, - visited: &mut HashSet, - output: &mut Vec, - ) { - if visited.contains(unit) { - return; - } else { - visited.insert(unit.clone()); - for neighbour in &graph[unit] { - dfs(neighbour, graph, visited, output); - } - output.push(unit.clone()); - } - } - } - - crate fn prepare_work + fmt::Debug>( - &mut self, - manifest_path: &Path, - modified: &[T], - requested_cargo: bool, - ) -> WorkStatus { - if self.package_map.is_none() || requested_cargo { - self.package_map = Some(PackageMap::new(manifest_path)); - } - - if !self.is_ready() || requested_cargo { - return WorkStatus::NeedsCargo(PackageArg::Default); - } - - let dirty_packages = self - .package_map - .as_ref() - .unwrap() - .compute_dirty_packages(modified); - - let needs_more_packages = dirty_packages - .difference(&self.built_packages) - .next() - .is_some(); - - let needed_packages = self - .built_packages - .union(&dirty_packages) - .cloned() - .collect(); - - // We modified a file from a packages, that are not included in the - // cached build plan - run Cargo to recreate the build plan including them - if needs_more_packages { - return WorkStatus::NeedsCargo(PackageArg::Packages(needed_packages)); - } - - let dirties = self.fetch_dirty_units(modified); - trace!( - "fetch_dirty_units: for files {:?}, these units are dirty: {:?}", - modified, - dirties, - ); - - if dirties - .iter() - .any(|&(_, ref target, _)| *target.kind() == TargetKind::CustomBuild) - { - WorkStatus::NeedsCargo(PackageArg::Packages(needed_packages)) - } else { - let graph = self.dirty_rev_dep_graph(&dirties); - trace!("Constructed dirty rev dep graph: {:?}", graph); - - if graph.is_empty() { - return WorkStatus::NeedsCargo(PackageArg::Default); - } - - let queue = self.topological_sort(&graph); - trace!( - "Topologically sorted dirty graph: {:?} {}", - queue, - self.is_ready() - ); - let jobs: Option> = queue - .iter() - .map(|x| self.compiler_jobs.get(x).cloned()) - .collect(); - - // It is possible that we want a job which is not in our cache (compiler_jobs), - // for example we might be building a workspace with an error in a crate and later - // crates within the crate that depend on the error-ing one have never been built. - // In that case we need to build from scratch so that everything is in our cache, or - // we cope with the error. In the error case, jobs will be None. - match jobs { - None => WorkStatus::NeedsCargo(PackageArg::Default), - Some(jobs) => { - assert!(!jobs.is_empty()); - WorkStatus::Execute(JobQueue(jobs)) - } - } - } - } + fn dirties_transitive>(&self, files: &[T]) -> Vec<&Self::Unit>; + /// Returns a topological ordering of units with regards to reverse + /// dependencies. + /// The output is a stack of units that can be linearly rebuilt, starting + /// from the last element. + fn topological_sort(&self, units: Vec<&Self::Unit>) -> Vec<&Self::Unit>; + fn prepare_work + std::fmt::Debug>(&self, files: &[T]) -> WorkStatus; } #[derive(Debug)] @@ -414,89 +52,26 @@ crate enum WorkStatus { Execute(JobQueue), } -/// Maps paths to packages. -/// -/// The point of the PackageMap is detect if additional packages need to be -/// included in the cached build plan. The cache can represent only a subset of -/// the entire workspace, hence why we need to detect if a package was modified -/// that's outside the cached build plan - if so, we need to recreate it, -/// including the new package. #[derive(Debug)] -struct PackageMap { - // A map from a manifest directory to the package name. - package_paths: HashMap, - // A map from a file's path, to the package it belongs to. - map_cache: Mutex>, +crate enum BuildPlan { + External(ExternalPlan), + Cargo(CargoPlan) } -impl PackageMap { - fn new(manifest_path: &Path) -> PackageMap { - PackageMap { - package_paths: Self::discover_package_paths(manifest_path), - map_cache: Mutex::new(HashMap::new()), - } - } - - // Find each package in the workspace and record the root directory and package name. - fn discover_package_paths(manifest_path: &Path) -> HashMap { - trace!("read metadata {:?}", manifest_path); - let metadata = match cargo_metadata::metadata(Some(manifest_path)) { - Ok(metadata) => metadata, - Err(_) => return HashMap::new(), - }; - metadata - .workspace_members - .into_iter() - .map(|wm| { - assert!(wm.url().starts_with("path+")); - let url = Url::parse(&wm.url()[5..]).expect("Bad URL"); - let path = parse_file_path(&url).expect("URL not a path"); - (path, wm.name().into()) - }).collect() - } - - /// Given modified set of files, returns a set of corresponding dirty packages. - fn compute_dirty_packages + fmt::Debug>( - &self, - modified_files: &[T], - ) -> HashSet { - modified_files - .iter() - .filter_map(|p| self.map(p.as_ref())) - .collect() - } - - // Map a file to the package which it belongs to. - // We do this by walking up the directory tree from `path` until we get to - // one of the recorded package root directories. - fn map(&self, path: &Path) -> Option { - if self.package_paths.is_empty() { - return None; - } - - let mut map_cache = self.map_cache.lock().unwrap(); - if map_cache.contains_key(path) { - return Some(map_cache[path].clone()); - } - - let result = Self::map_uncached(path, &self.package_paths)?; - - map_cache.insert(path.to_owned(), result.clone()); - Some(result) +impl BuildPlan { + pub fn new() -> BuildPlan { + BuildPlan::Cargo(Default::default()) } - fn map_uncached(path: &Path, package_paths: &HashMap) -> Option { - if package_paths.is_empty() { - return None; - } - - match package_paths.get(path) { - Some(package) => Some(package.clone()), - None => Self::map_uncached(path.parent()?, package_paths), + pub fn as_cargo_mut(&mut self) -> Option<&mut CargoPlan> { + match self { + BuildPlan::Cargo(plan) => Some(plan), + _ => None, } } } +#[derive(Debug)] crate struct JobQueue(Vec); /// Returns an immediately next argument to the one specified in a given @@ -515,21 +90,11 @@ fn proc_argument_value>(prc: &ProcessBuilder, key: T) -> Option< Some(args.get(idx + 1)?.as_os_str()) } -impl fmt::Debug for JobQueue { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "JobQueue: [")?; - for prog in self.0.iter().rev() { - let name = proc_argument_value(prog, "--crate-name").unwrap(); - let typ_ = proc_argument_value(prog, "--crate-type") - .unwrap_or_else(|| OsStr::new("")); - write!(f, "{:?} ({:?}), ", name, typ_); - } - write!(f, "]")?; - Ok(()) +impl JobQueue { + crate fn with_commands(jobs: Vec) -> JobQueue { + JobQueue(jobs) } -} -impl JobQueue { crate fn dequeue(&mut self) -> Option { self.0.pop() } @@ -559,7 +124,7 @@ impl JobQueue { // Go through cached compiler invocations sequentially, collecting each // invocation's compiler messages for diagnostics and analysis data while let Some(job) = self.dequeue() { - trace!("Executing: {:?}", job); + trace!("Executing: {:#?}", job); let mut args: Vec<_> = job .get_args() .iter() @@ -574,6 +139,22 @@ impl JobQueue { .expect("cannot stringify job program"); args.insert(0, program.clone()); + // Needed to parse rustc diagnostics + if args.iter().find(|x| x.as_str() == "--error-format=json").is_none() { + args.push("--error-format=json".to_owned()); + } + + if args.iter().find(|x| x.as_str() == "--sysroot").is_none() { + let sysroot = super::rustc::current_sysroot() + .expect("need to specify SYSROOT env var or use rustup or multirust"); + + let config = internals.config.lock().unwrap(); + if config.sysroot.is_none() { + args.push("--sysroot".to_owned()); + args.push(sysroot); + } + } + // Send a window/progress notification. { let crate_name = proc_argument_value(&job, "--crate-name").and_then(|x| x.to_str()); @@ -602,7 +183,7 @@ impl JobQueue { &internals.vfs, &args, job.get_envs(), - cwd.as_ref().map(|p| &**p), + job.get_cwd().or_else(|| cwd.as_ref().map(|p| &**p)), &build_dir, Arc::clone(&internals.config), &internals.env_lock.as_facade(), @@ -639,55 +220,3 @@ impl JobQueue { ) } } - -fn key_from_unit(unit: &Unit<'_>) -> UnitKey { - ( - unit.pkg.package_id().clone(), - unit.target.clone(), - unit.mode, - ) -} - -macro_rules! print_dep_graph { - ($name: expr, $graph: expr, $f: expr) => { - $f.write_str(&format!("{}:\n", $name))?; - for (key, deps) in &$graph { - $f.write_str(&format!("{:?}\n", key))?; - for dep in deps { - $f.write_str(&format!("- {:?}\n", dep))?; - } - } - }; -} - -impl fmt::Debug for Plan { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.write_str(&format!("Units: {:?}\n", self.units))?; - print_dep_graph!("Dependency graph", self.dep_graph, f); - print_dep_graph!("Reverse dependency graph", self.rev_dep_graph, f); - f.write_str(&format!("Compiler jobs: {:?}\n", self.compiler_jobs))?; - Ok(()) - } -} - -#[derive(Hash, PartialEq, Eq, Debug)] -/// An owned version of `cargo::core::Unit`. -crate struct OwnedUnit { - crate id: PackageId, - crate target: Target, - crate profile: Profile, - crate kind: Kind, - crate mode: CompileMode, -} - -impl<'a> From<&'a Unit<'a>> for OwnedUnit { - fn from(unit: &Unit<'a>) -> OwnedUnit { - OwnedUnit { - id: unit.pkg.package_id().to_owned(), - target: unit.target.clone(), - profile: unit.profile, - kind: unit.kind, - mode: unit.mode, - } - } -} diff --git a/src/build/rustc.rs b/src/build/rustc.rs index f0cd1b2ef99..db9c121f291 100644 --- a/src/build/rustc.rs +++ b/src/build/rustc.rs @@ -49,10 +49,12 @@ use crate::build::{BufWriter, BuildResult}; use crate::config::{ClippyPreference, Config}; use std::collections::HashMap; +use std::env; use std::ffi::OsString; use std::io; use std::path::{Path, PathBuf}; use std::sync::{Arc, Mutex}; +use std::process::Command; // Runs a single instance of rustc. Runs in-process. crate fn rustc( @@ -65,9 +67,10 @@ crate fn rustc( env_lock: &EnvironmentLockFacade, ) -> BuildResult { trace!( - "rustc - args: `{:?}`, envs: {:?}, build dir: {:?}", + "rustc - args: `{:?}`, envs: {:?}, cwd: {:?}, build dir: {:?}", args, envs, + cwd, build_dir ); @@ -75,11 +78,17 @@ crate fn rustc( let mut local_envs = envs.clone(); - if rls_config.lock().unwrap().clear_env_rust_log { - local_envs.insert(String::from("RUST_LOG"), None); - } + let clippy_pref = { + let config = rls_config.lock().unwrap(); + if config.clear_env_rust_log { + local_envs.insert(String::from("RUST_LOG"), None); + } + + config.clippy_preference + }; + // Required for Clippy not to crash when running outside Cargo? + local_envs.entry("CARGO_MANIFEST_DIR".into()).or_insert(Some(build_dir.clone().into())); - let clippy_pref = rls_config.lock().unwrap().clippy_preference; let (guard, _) = env_lock.lock(); let restore_env = Environment::push_with_lock(&local_envs, cwd, guard); @@ -132,6 +141,7 @@ crate fn rustc( let analysis = analysis .map(|analysis| vec![analysis]) .unwrap_or_else(Vec::new); + log::debug!("rustc: analysis read successfully?: {}", !analysis.is_empty()); let cwd = cwd .unwrap_or_else(|| restore_env.get_old_cwd()) @@ -340,3 +350,22 @@ impl FileLoader for ReplacedFileLoader { self.real_file_loader.read_file(path) } } + +pub(super) fn current_sysroot() -> Option { + let home = env::var("RUSTUP_HOME").or_else(|_| env::var("MULTIRUST_HOME")); + let toolchain = env::var("RUSTUP_TOOLCHAIN").or_else(|_| env::var("MULTIRUST_TOOLCHAIN")); + if let (Ok(home), Ok(toolchain)) = (home, toolchain) { + Some(format!("{}/toolchains/{}", home, toolchain)) + } else { + let rustc_exe = env::var("RUSTC").unwrap_or_else(|_| "rustc".to_owned()); + env::var("SYSROOT").map(|s| s.to_owned()).ok().or_else(|| { + Command::new(rustc_exe) + .arg("--print") + .arg("sysroot") + .output() + .ok() + .and_then(|out| String::from_utf8(out.stdout).ok()) + .map(|s| s.trim().to_owned()) + }) + } +} diff --git a/src/config.rs b/src/config.rs index 49bed14f85b..25941bc75ee 100644 --- a/src/config.rs +++ b/src/config.rs @@ -162,8 +162,6 @@ pub struct Config { /// If set, executes a given program responsible for rebuilding save-analysis /// to be loaded by the RLS. The program given should output a list of /// resulting .json files on stdout. - /// - /// Implies `build_on_save`: true. pub build_command: Option, } @@ -233,8 +231,6 @@ impl Config { self.cfg_test = false; self.rustfmt_path = None; self.build_command = None; - } else if self.build_command.is_some() { - self.build_on_save = true; } } From aeb21bc3f3850145c66b1ab60c8724f78d6c11f0 Mon Sep 17 00:00:00 2001 From: Igor Matuszewski Date: Sun, 30 Sep 2018 12:23:10 +0200 Subject: [PATCH 2/4] Fix Windows tests --- src/build/external.rs | 71 +++++++++++++++++++++++++------------------ 1 file changed, 42 insertions(+), 29 deletions(-) diff --git a/src/build/external.rs b/src/build/external.rs index 6cad7346b1e..d8ff66f84ea 100644 --- a/src/build/external.rs +++ b/src/build/external.rs @@ -443,13 +443,17 @@ fn guess_rustc_src_path(cmd: &ProcessBuilder) -> Option { .get_args() .iter() .find(|&a| Path::new(a).extension().map(|e| e == "rs").unwrap_or(false))?; - let file_path = PathBuf::from(file); - Some(match (cmd.get_cwd(), file_path.is_absolute()) { - (_, true) => file_path, - (Some(cwd), _) => cwd.join(file_path), - // TODO: is cwd correct here? - (None, _) => std::env::current_dir().ok()?.join(file_path), + src_path(cmd.get_cwd(), file) +} + +fn src_path(cwd: Option<&Path>, path: impl AsRef) -> Option { + let path = path.as_ref(); + + Some(match (cwd, path.is_absolute()) { + (_, true) => path.to_owned(), + (Some(cwd), _) => cwd.join(path), + (None, _) => std::env::current_dir().ok()?.join(path) }) } @@ -482,14 +486,17 @@ mod tests { } } - fn paths<'a>(invocations: &Vec<&'a Invocation>) -> Vec<&'a str> { + fn paths(invocations: &Vec<&Invocation>) -> Vec { invocations .iter() - .filter_map(|d| d.src_path.as_ref()) - .map(|p| p.to_str().unwrap()) + .filter_map(|d| d.src_path.clone()) .collect() } + fn to_paths(cwd: Option<&Path>, paths: &[&str]) -> Vec { + paths.iter().map(|p| src_path(cwd, p).unwrap()).collect() + } + #[test] fn dirty_units_path_heuristics() { let plan = r#"{"invocations": [ @@ -500,19 +507,21 @@ mod tests { let plan = ExternalPlan::try_from_raw(plan).unwrap(); eprintln!("src_paths: {:#?}", &SrcPaths::from(&plan)); + eprintln!("plan: {:?}", &plan); - let dirties = |file: &str| -> Vec<&str> { - plan.dirties(&[file]) - .iter() - .filter_map(|d| d.src_path.as_ref()) - .map(|p| p.to_str().unwrap()) - .collect() - }; + let check_dirties = |files: &[&str], expected: &[&str]| { + let to_paths = |x| to_paths(None, x); + let (files, expected) = (to_paths(files), to_paths(expected)); - assert_eq!(dirties("/my/dummy.rs"), Vec::<&str>::new()); - assert_eq!(dirties("/my/repo/dummy.rs"), vec!["/my/repo/build.rs"]); - assert_eq!(dirties("/my/repo/src/c.rs"), vec!["/my/repo/src/lib.rs"]); - assert_eq!(dirties("/my/repo/src/a/b.rs"), vec!["/my/repo/src/lib.rs"]); + assert_eq!( + plan.dirties(&files).iter().filter_map(|d| d.src_path.clone()).collect::>(), + expected + ); + }; + check_dirties(&["/my/dummy.rs"], &[]); + check_dirties(&["/my/repo/dummy.rs"], &["/my/repo/build.rs"]); + check_dirties(&["/my/repo/src/c.rs"], &["/my/repo/src/lib.rs"]); + check_dirties(&["/my/repo/src/a/b.rs"], &["/my/repo/src/lib.rs"]); } #[test] @@ -527,18 +536,20 @@ mod tests { eprintln!("src_paths: {:#?}", &SrcPaths::from(&plan)); eprintln!("plan: {:?}", &plan); + let to_paths = |x| to_paths(None, x); + assert_eq!( - paths(&plan.dirties(&["/my/repo/src/a/b.rs"])), - vec!["/my/repo/src/lib.rs"] + paths(&plan.dirties(&to_paths(&["/my/repo/src/a/b.rs"]))), + to_paths(&["/my/repo/src/lib.rs"]) ); assert_eq!( - paths(&plan.dirties_transitive(&["/my/repo/file.rs"])).sorted(), - vec!["/my/repo/build.rs", "/my/repo/src/lib.rs"].sorted(), + paths(&plan.dirties_transitive(&to_paths(&["/my/repo/file.rs"]))).sorted(), + to_paths(&["/my/repo/build.rs", "/my/repo/src/lib.rs"]).sorted(), ); assert_eq!( - paths(&plan.dirties_transitive(&["/my/repo/src/file.rs"])).sorted(), - vec!["/my/repo/src/lib.rs"].sorted(), + paths(&plan.dirties_transitive(&to_paths(&["/my/repo/src/file.rs"]))).sorted(), + to_paths(&["/my/repo/src/lib.rs"]).sorted(), ); } @@ -554,10 +565,12 @@ mod tests { eprintln!("src_paths: {:#?}", &SrcPaths::from(&plan)); eprintln!("plan: {:?}", &plan); - let units_to_rebuild = plan.dirties_transitive(&["/my/repo/file.rs"]); + let to_paths = |x| to_paths(None, x); + + let units_to_rebuild = plan.dirties_transitive(&to_paths(&["/my/repo/file.rs"])); assert_eq!( paths(&units_to_rebuild).sorted(), - vec!["/my/repo/build.rs", "/my/repo/src/lib.rs"].sorted(), + to_paths(&["/my/repo/build.rs", "/my/repo/src/lib.rs"]).sorted(), ); // TODO: Test on non-trivial input, use Iterator::position if @@ -566,7 +579,7 @@ mod tests { let topo_units = plan.topological_sort(units_to_rebuild); assert_eq!( paths(&topo_units), - vec!["/my/repo/src/lib.rs", "/my/repo/build.rs"], + to_paths(&["/my/repo/src/lib.rs", "/my/repo/build.rs"]), ) } } From 18718402cdee077fbac9b5c59a9df077ed129de2 Mon Sep 17 00:00:00 2001 From: Igor Matuszewski Date: Sun, 30 Sep 2018 12:39:44 +0200 Subject: [PATCH 3/4] Respect build_dir when creating external build plans --- src/build/external.rs | 40 ++++++++++++++++++++++++---------------- 1 file changed, 24 insertions(+), 16 deletions(-) diff --git a/src/build/external.rs b/src/build/external.rs index d8ff66f84ea..d0bc8ebce54 100644 --- a/src/build/external.rs +++ b/src/build/external.rs @@ -164,7 +164,7 @@ fn plan_from_analysis(analysis: &[Analysis], build_dir: &Path) -> Result, ()>>()?; - ExternalPlan::try_from_raw(RawPlan { invocations }) + ExternalPlan::try_from_raw(build_dir, RawPlan { invocations }) } #[derive(Debug, Deserialize)] @@ -224,8 +224,8 @@ impl BuildKey for Invocation { } } -impl From for Invocation { - fn from(raw: RawInvocation) -> Invocation { +impl Invocation { + fn from_raw(build_dir: &Path, raw: RawInvocation) -> Invocation { let mut command = process(&raw.program); command.args(&raw.args); for (k, v) in &raw.env { @@ -239,7 +239,7 @@ impl From for Invocation { deps: raw.deps.to_owned(), outputs: raw.outputs.to_owned(), links: raw.links.to_owned(), - src_path: guess_rustc_src_path(&command), + src_path: guess_rustc_src_path(build_dir, &command), command, } } @@ -270,7 +270,7 @@ impl ExternalPlan { self.rev_deps.entry(dep).or_insert_with(HashSet::new).insert(key); } - crate fn try_from_raw(raw: RawPlan) -> Result { + crate fn try_from_raw(build_dir: &Path, raw: RawPlan) -> Result { // Sanity check, each dependency (index) has to be inside the build plan if raw .invocations @@ -281,7 +281,10 @@ impl ExternalPlan { return Err(()); } - let units: Vec = raw.invocations.into_iter().map(|x| x.into()).collect(); + let units = raw.invocations + .into_iter() + .map(|raw| Invocation::from_raw(build_dir, raw)) + .collect(); Ok(ExternalPlan::with_units(units)) } @@ -434,17 +437,19 @@ impl BuildGraph for ExternalPlan { } } -fn guess_rustc_src_path(cmd: &ProcessBuilder) -> Option { +fn guess_rustc_src_path(build_dir: &Path, cmd: &ProcessBuilder) -> Option { if !Path::new(cmd.get_program()).ends_with("rustc") { return None; } + let cwd = cmd.get_cwd().or(Some(build_dir)); + let file = cmd .get_args() .iter() .find(|&a| Path::new(a).extension().map(|e| e == "rs").unwrap_or(false))?; - src_path(cmd.get_cwd(), file) + src_path(cwd, file) } fn src_path(cwd: Option<&Path>, path: impl AsRef) -> Option { @@ -493,8 +498,8 @@ mod tests { .collect() } - fn to_paths(cwd: Option<&Path>, paths: &[&str]) -> Vec { - paths.iter().map(|p| src_path(cwd, p).unwrap()).collect() + fn to_paths(cwd: &Path, paths: &[&str]) -> Vec { + paths.iter().map(|p| src_path(Some(cwd), p).unwrap()).collect() } #[test] @@ -503,14 +508,15 @@ mod tests { { "deps": [], "program": "rustc", "args": ["--crate-name", "build_script_build", "/my/repo/build.rs"], "env": {}, "outputs": [] }, { "deps": [0], "program": "rustc", "args": ["--crate-name", "repo", "/my/repo/src/lib.rs"], "env": {}, "outputs": [] } ]}"#; + let build_dir = std::env::temp_dir(); let plan = serde_json::from_str::(&plan).unwrap(); - let plan = ExternalPlan::try_from_raw(plan).unwrap(); + let plan = ExternalPlan::try_from_raw(&build_dir, plan).unwrap(); eprintln!("src_paths: {:#?}", &SrcPaths::from(&plan)); eprintln!("plan: {:?}", &plan); let check_dirties = |files: &[&str], expected: &[&str]| { - let to_paths = |x| to_paths(None, x); + let to_paths = |x| to_paths(&build_dir, x); let (files, expected) = (to_paths(files), to_paths(expected)); assert_eq!( @@ -530,13 +536,14 @@ mod tests { { "deps": [], "program": "rustc", "args": ["--crate-name", "build_script_build", "/my/repo/build.rs"], "env": {}, "outputs": [] }, { "deps": [0], "program": "rustc", "args": ["--crate-name", "repo", "/my/repo/src/lib.rs"], "env": {}, "outputs": [] } ]}"#; + let build_dir = std::env::temp_dir(); let plan = serde_json::from_str::(&plan).unwrap(); - let plan = ExternalPlan::try_from_raw(plan).unwrap(); + let plan = ExternalPlan::try_from_raw(&build_dir, plan).unwrap(); eprintln!("src_paths: {:#?}", &SrcPaths::from(&plan)); eprintln!("plan: {:?}", &plan); - let to_paths = |x| to_paths(None, x); + let to_paths = |x| to_paths(&build_dir, x); assert_eq!( paths(&plan.dirties(&to_paths(&["/my/repo/src/a/b.rs"]))), @@ -559,13 +566,14 @@ mod tests { { "deps": [], "program": "rustc", "args": ["--crate-name", "build_script_build", "/my/repo/build.rs"], "env": {}, "outputs": [] }, { "deps": [0], "program": "rustc", "args": ["--crate-name", "repo", "/my/repo/src/lib.rs"], "env": {}, "outputs": [] } ]}"#; + let build_dir = std::env::temp_dir(); let plan = serde_json::from_str::(&plan).unwrap(); - let plan = ExternalPlan::try_from_raw(plan).unwrap(); + let plan = ExternalPlan::try_from_raw(&build_dir, plan).unwrap(); eprintln!("src_paths: {:#?}", &SrcPaths::from(&plan)); eprintln!("plan: {:?}", &plan); - let to_paths = |x| to_paths(None, x); + let to_paths = |x| to_paths(&build_dir, x); let units_to_rebuild = plan.dirties_transitive(&to_paths(&["/my/repo/file.rs"])); assert_eq!( From 538989e419eef1f455db5dabe9ab39b92be95710 Mon Sep 17 00:00:00 2001 From: Igor Matuszewski Date: Mon, 1 Oct 2018 00:27:39 +0200 Subject: [PATCH 4/4] Apply Clippy lints --- src/build/external.rs | 11 ++++++----- src/build/mod.rs | 2 +- src/build/rustc.rs | 3 ++- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/build/external.rs b/src/build/external.rs index d0bc8ebce54..37c1950adf8 100644 --- a/src/build/external.rs +++ b/src/build/external.rs @@ -146,9 +146,10 @@ fn plan_from_analysis(analysis: &[Analysis], build_dir: &Path) -> Result build_dir.join(directory), - false => directory.to_owned(), + let cwd = if directory.is_relative() { + build_dir.join(directory) + } else { + directory.to_owned() }; Ok(RawInvocation { @@ -442,7 +443,7 @@ fn guess_rustc_src_path(build_dir: &Path, cmd: &ProcessBuilder) -> Option) -> Vec { + fn paths(invocations: &[&Invocation]) -> Vec { invocations .iter() .filter_map(|d| d.src_path.clone()) diff --git a/src/build/mod.rs b/src/build/mod.rs index b9ff8ed2d3d..5c84098901f 100644 --- a/src/build/mod.rs +++ b/src/build/mod.rs @@ -563,7 +563,7 @@ impl Internals { WorkStatus::Execute(_) if needs_rebuild => { WorkStatus::NeedsCargo(PackageArg::Default) }, - work @ _ => work, + work => work, } } } diff --git a/src/build/rustc.rs b/src/build/rustc.rs index db9c121f291..0faa1c7039e 100644 --- a/src/build/rustc.rs +++ b/src/build/rustc.rs @@ -87,7 +87,8 @@ crate fn rustc( config.clippy_preference }; // Required for Clippy not to crash when running outside Cargo? - local_envs.entry("CARGO_MANIFEST_DIR".into()).or_insert(Some(build_dir.clone().into())); + local_envs.entry("CARGO_MANIFEST_DIR".into()) + .or_insert_with(|| Some(build_dir.into())); let (guard, _) = env_lock.lock(); let restore_env = Environment::push_with_lock(&local_envs, cwd, guard);