Auto merge of #7215 - ehuss:build-script-cleanup, r=alexcrichton

Clean up build script stuff and documentation.

I often get lost trying to understand some of this build script stuff, so this is an attempt to make it a little easier to follow. Overview:

- Remove `Context::build_script_overridden`.
    - Add `BuildContext::script_override` to query if something is overridden.
    - Do not bother to compute unit dependencies for overridden build scripts. This is the most risky change, since there are some subtle assumptions when connecting up the various build script maps. However, as best as I can reason through, it seems correct. The intent here is to avoid all the situations where it attempts to filter out these extra deps.
- Rename `Context::build_state` to `Context::build_script_outputs` to try to be clearer about what it is. Similarly rename `BuildState` to `BuildScriptOutputs`.
- Remove trivial 1-line function `load_build_deps`.
- Add some documentation.

Author: bors

src/cargo/core/compiler/build_context/mod.rs

@@ -16,6 +16,11 @@ mod target_info;
 pub use self::target_info::{FileFlavor, TargetInfo};
 
 /// The build context, containing all information about a build task.
+///
+/// It is intended that this is mostly static information. Stuff that mutates
+/// during the build can be found in the parent `Context`. (I say mostly,
+/// because this has internal caching, but nothing that should be observable
+/// or require &mut.)
 pub struct BuildContext<'a, 'cfg> {
     /// The workspace the build is for.
     pub ws: &'a Workspace<'cfg>,
@@ -183,6 +188,17 @@ impl<'a, 'cfg> BuildContext<'a, 'cfg> {
     pub fn extra_args_for(&self, unit: &Unit<'a>) -> Option<&Vec<String>> {
         self.extra_compiler_args.get(unit)
     }
+
+    /// If a build script is overridden, this returns the `BuildOutput` to use.
+    ///
+    /// `lib_name` is the `links` library name and `kind` is whether it is for
+    /// Host or Target.
+    pub fn script_override(&self, lib_name: &str, kind: Kind) -> Option<&BuildOutput> {
+        match kind {
+            Kind::Host => self.host_config.overrides.get(lib_name),
+            Kind::Target => self.target_config.overrides.get(lib_name),
+        }
+    }
 }
 
 /// Information required to build for a target.
@@ -192,7 +208,11 @@ pub struct TargetConfig {
     pub ar: Option<PathBuf>,
     /// The path of the linker for this target.
     pub linker: Option<PathBuf>,
-    /// Special build options for any necessary input files (filename -> options).
+    /// Build script override for the given library name.
+    ///
+    /// Any package with a `links` value for the given library name will skip
+    /// running its build script and instead use the given output from the
+    /// config file.
     pub overrides: HashMap<String, BuildOutput>,
 }
 

src/cargo/core/compiler/context/mod.rs

@@ -3,7 +3,7 @@ use std::collections::{HashMap, HashSet};
 use std::ffi::OsStr;
 use std::fmt::Write;
 use std::path::PathBuf;
-use std::sync::Arc;
+use std::sync::{Arc, Mutex};
 
 use filetime::FileTime;
 use jobserver::Client;
@@ -15,7 +15,7 @@ use crate::util::errors::{CargoResult, CargoResultExt};
 use crate::util::{internal, profile, Config};
 
 use super::build_plan::BuildPlan;
-use super::custom_build::{self, BuildDeps, BuildScripts, BuildState};
+use super::custom_build::{self, BuildDeps, BuildScriptOutputs, BuildScripts};
 use super::fingerprint::Fingerprint;
 use super::job_queue::JobQueue;
 use super::layout::Layout;
@@ -28,21 +28,45 @@ mod compilation_files;
 use self::compilation_files::CompilationFiles;
 pub use self::compilation_files::{Metadata, OutputFile};
 
+/// Collection of all the stuff that is needed to perform a build.
 pub struct Context<'a, 'cfg> {
+    /// Mostly static information about the build task.
     pub bcx: &'a BuildContext<'a, 'cfg>,
+    /// A large collection of information about the result of the entire compilation.
     pub compilation: Compilation<'cfg>,
-    pub build_state: Arc<BuildState>,
-    pub build_script_overridden: HashSet<(PackageId, Kind)>,
+    /// Output from build scripts, updated after each build script runs.
+    pub build_script_outputs: Arc<Mutex<BuildScriptOutputs>>,
+    /// Dependencies (like rerun-if-changed) declared by a build script.
+    /// This is *only* populated from the output from previous runs.
+    /// If the build script hasn't ever been run, then it must be run.
     pub build_explicit_deps: HashMap<Unit<'a>, BuildDeps>,
+    /// Fingerprints used to detect if a unit is out-of-date.
     pub fingerprints: HashMap<Unit<'a>, Arc<Fingerprint>>,
+    /// Cache of file mtimes to reduce filesystem hits.
     pub mtime_cache: HashMap<PathBuf, FileTime>,
+    /// A set used to track which units have been compiled.
+    /// A unit may appear in the job graph multiple times as a dependency of
+    /// multiple packages, but it only needs to run once.
     pub compiled: HashSet<Unit<'a>>,
+    /// Linking information for each `Unit`.
+    /// See `build_map` for details.
     pub build_scripts: HashMap<Unit<'a>, Arc<BuildScripts>>,
+    /// Used to check the `links` field in the manifest is not duplicated and
+    /// is used correctly.
     pub links: Links,
+    /// Job server client to manage concurrency with other processes.
     pub jobserver: Client,
+    /// "Primary" packages are the ones the user selected on the command-line
+    /// with `-p` flags. If no flags are specified, then it is the defaults
+    /// based on the current directory and the default workspace members.
     primary_packages: HashSet<PackageId>,
+    /// The dependency graph of units to compile.
     unit_dependencies: HashMap<Unit<'a>, Vec<Unit<'a>>>,
+    /// An abstraction of the files and directories that will be generated by
+    /// the compilation. This is `None` until after `unit_dependencies` has
+    /// been computed.
     files: Option<CompilationFiles<'a, 'cfg>>,
+    /// Cache of packages, populated when they are downloaded.
     package_cache: HashMap<PackageId, &'a Package>,
 
     /// A flag indicating whether pipelining is enabled for this compilation
@@ -82,16 +106,14 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
         Ok(Self {
             bcx,
             compilation: Compilation::new(bcx)?,
-            build_state: Arc::new(BuildState::new(&bcx.host_config, &bcx.target_config)),
+            build_script_outputs: Arc::new(Mutex::new(BuildScriptOutputs::default())),
             fingerprints: HashMap::new(),
             mtime_cache: HashMap::new(),
             compiled: HashSet::new(),
             build_scripts: HashMap::new(),
             build_explicit_deps: HashMap::new(),
             links: Links::new(),
             jobserver,
-            build_script_overridden: HashSet::new(),
-
             primary_packages: HashSet::new(),
             unit_dependencies: HashMap::new(),
             files: None,
@@ -228,7 +250,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
             super::output_depinfo(&mut self, unit)?;
         }
 
-        for (&(ref pkg, _), output) in self.build_state.outputs.lock().unwrap().iter() {
+        for (&(ref pkg, _), output) in self.build_script_outputs.lock().unwrap().iter() {
             self.compilation
                 .cfgs
                 .entry(pkg.clone())
@@ -338,22 +360,6 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
     //
     // TODO: this ideally should be `-> &[Unit<'a>]`.
     pub fn dep_targets(&self, unit: &Unit<'a>) -> Vec<Unit<'a>> {
-        // If this build script's execution has been overridden then we don't
-        // actually depend on anything, we've reached the end of the dependency
-        // chain as we've got all the info we're gonna get.
-        //
-        // Note there's a subtlety about this piece of code! The
-        // `build_script_overridden` map here is populated in
-        // `custom_build::build_map` which you need to call before inspecting
-        // dependencies. However, that code itself calls this method and
-        // gets a full pre-filtered set of dependencies. This is not super
-        // obvious, and clear, but it does work at the moment.
-        if unit.target.is_custom_build() {
-            let key = (unit.pkg.package_id(), unit.kind);
-            if self.build_script_overridden.contains(&key) {
-                return Vec::new();
-            }
-        }
         self.unit_dependencies[unit].clone()
     }
 

src/cargo/core/compiler/context/unit_dependencies.rs

@@ -258,6 +258,13 @@ fn compute_deps_custom_build<'a, 'cfg>(
     unit: &Unit<'a>,
     bcx: &BuildContext<'a, 'cfg>,
 ) -> CargoResult<Vec<(Unit<'a>, UnitFor)>> {
+    if let Some(links) = unit.pkg.manifest().links() {
+        if bcx.script_override(links, unit.kind).is_some() {
+            // Overridden build scripts don't have any dependencies.
+            return Ok(Vec::new());
+        }
+    }
+
     // When not overridden, then the dependencies to run a build script are:
     //
     // 1. Compiling the build script itself.