Auto merge of #6688 - ehuss:incremental-cleanup, r=alexcrichton

Incremental profile cleanup.

Some minor code cleanup, and doc updates regarding incremental compilation.

Move incremental logic into the profile computation, which makes it a little more consistent and helps simplify things a little (such as removing the fingerprint special-case).

This introduces a small change in behavior with the `build.incremental` config variable. Previously `build.incremental = true` was completely ignored. Now, it is treated the same as `CARGO_INCREMENTAL=1` environment variable.

Moves config profile validation to the workspace so that warnings do not appear multiple times (once for each manifest).

Split from #6643.

Author: bors

Diff for: src/cargo/core/compiler/build_context/mod.rs

@@ -38,7 +38,6 @@ pub struct BuildContext<'a, 'cfg: 'a> {
     pub target_config: TargetConfig,
     pub target_info: TargetInfo,
     pub host_info: TargetInfo,
-    pub incremental_env: Option<bool>,
 }
 
 impl<'a, 'cfg> BuildContext<'a, 'cfg> {
@@ -51,11 +50,6 @@ impl<'a, 'cfg> BuildContext<'a, 'cfg> {
         profiles: &'a Profiles,
         extra_compiler_args: HashMap<Unit<'a>, Vec<String>>,
     ) -> CargoResult<BuildContext<'a, 'cfg>> {
-        let incremental_env = match env::var("CARGO_INCREMENTAL") {
-            Ok(v) => Some(v == "1"),
-            Err(_) => None,
-        };
-
         let rustc = config.rustc(Some(ws))?;
         let host_config = TargetConfig::new(config, &rustc.host)?;
         let target_config = match build_config.requested_target.as_ref() {
@@ -84,7 +78,6 @@ impl<'a, 'cfg> BuildContext<'a, 'cfg> {
             host_info,
             build_config,
             profiles,
-            incremental_env,
             extra_compiler_args,
         })
     }

Diff for: src/cargo/core/compiler/context/mod.rs

@@ -386,61 +386,6 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
         deps
     }
 
-    pub fn incremental_args(&self, unit: &Unit<'_>) -> CargoResult<Vec<String>> {
-        // There's a number of ways to configure incremental compilation right
-        // now. In order of descending priority (first is highest priority) we
-        // have:
-        //
-        // * `CARGO_INCREMENTAL` - this is blanket used unconditionally to turn
-        //   on/off incremental compilation for any cargo subcommand. We'll
-        //   respect this if set.
-        // * `build.incremental` - in `.cargo/config` this blanket key can
-        //   globally for a system configure whether incremental compilation is
-        //   enabled. Note that setting this to `true` will not actually affect
-        //   all builds though. For example a `true` value doesn't enable
-        //   release incremental builds, only dev incremental builds. This can
-        //   be useful to globally disable incremental compilation like
-        //   `CARGO_INCREMENTAL`.
-        // * `profile.dev.incremental` - in `Cargo.toml` specific profiles can
-        //   be configured to enable/disable incremental compilation. This can
-        //   be primarily used to disable incremental when buggy for a package.
-        // * Finally, each profile has a default for whether it will enable
-        //   incremental compilation or not. Primarily development profiles
-        //   have it enabled by default while release profiles have it disabled
-        //   by default.
-        let global_cfg = self
-            .bcx
-            .config
-            .get_bool("build.incremental")?
-            .map(|c| c.val);
-        let incremental = match (
-            self.bcx.incremental_env,
-            global_cfg,
-            unit.profile.incremental,
-        ) {
-            (Some(v), _, _) => v,
-            (None, Some(false), _) => false,
-            (None, _, other) => other,
-        };
-
-        if !incremental {
-            return Ok(Vec::new());
-        }
-
-        // Only enable incremental compilation for sources the user can
-        // modify (aka path sources). For things that change infrequently,
-        // non-incremental builds yield better performance in the compiler
-        // itself (aka crates.io / git dependencies)
-        //
-        // (see also https://github.com/rust-lang/cargo/issues/3972)
-        if !unit.pkg.package_id().source_id().is_path() {
-            return Ok(Vec::new());
-        }
-
-        let dir = self.files().layout(unit.kind).incremental().display();
-        Ok(vec!["-C".to_string(), format!("incremental={}", dir)])
-    }
-
     pub fn is_primary_package(&self, unit: &Unit<'a>) -> bool {
         self.primary_packages.contains(&unit.pkg.package_id())
     }

Diff for: src/cargo/core/compiler/fingerprint.rs

@@ -492,12 +492,7 @@ fn calculate<'a, 'cfg>(
     } else {
         bcx.rustflags_args(unit)?
     };
-    let profile_hash = util::hash_u64(&(
-        &unit.profile,
-        unit.mode,
-        bcx.extra_args_for(unit),
-        cx.incremental_args(unit)?,
-    ));
+    let profile_hash = util::hash_u64(&(&unit.profile, unit.mode, bcx.extra_args_for(unit)));
     let fingerprint = Arc::new(Fingerprint {
         rustc: util::hash_u64(&bcx.rustc.verbose_version),
         target: util::hash_u64(&unit.target),

Diff for: src/cargo/core/compiler/mod.rs

@@ -760,6 +760,7 @@ fn build_base_args<'a, 'cfg>(
         overflow_checks,
         rpath,
         ref panic,
+        incremental,
         ..
     } = unit.profile;
     let test = unit.mode.is_any_test();
@@ -902,8 +903,10 @@ fn build_base_args<'a, 'cfg>(
         "linker=",
         bcx.linker(unit.kind).map(|s| s.as_ref()),
     );
-    cmd.args(&cx.incremental_args(unit)?);
-
+    if incremental {
+        let dir = cx.files().layout(unit.kind).incremental().as_os_str();
+        opt(cmd, "-C", "incremental=", Some(dir));
+    }
     Ok(())
 }
 

Diff for: src/cargo/core/manifest.rs

@@ -66,6 +66,7 @@ pub struct VirtualManifest {
     workspace: WorkspaceConfig,
     profiles: Profiles,
     warnings: Warnings,
+    features: Features,
 }
 
 /// General metadata about a package which is just blindly uploaded to the
@@ -541,13 +542,15 @@ impl VirtualManifest {
         patch: HashMap<Url, Vec<Dependency>>,
         workspace: WorkspaceConfig,
         profiles: Profiles,
+        features: Features,
     ) -> VirtualManifest {
         VirtualManifest {
             replace,
             patch,
             workspace,
             profiles,
             warnings: Warnings::new(),
+            features,
         }
     }
 
@@ -574,6 +577,10 @@ impl VirtualManifest {
     pub fn warnings(&self) -> &Warnings {
         &self.warnings
     }
+
+    pub fn features(&self) -> &Features {
+        &self.features
+    }
 }
 
 impl Target {

Diff for: src/cargo/core/profiles.rs

@@ -1,5 +1,5 @@
 use std::collections::HashSet;
-use std::{cmp, fmt, hash};
+use std::{cmp, env, fmt, hash};
 
 use serde::Deserialize;
 
@@ -19,6 +19,10 @@ pub struct Profiles {
     test: ProfileMaker,
     bench: ProfileMaker,
     doc: ProfileMaker,
+    /// Incremental compilation can be overridden globally via:
+    /// - `CARGO_INCREMENTAL` environment variable.
+    /// - `build.incremental` config value.
+    incremental: Option<bool>,
 }
 
 impl Profiles {
@@ -33,7 +37,11 @@ impl Profiles {
         }
 
         let config_profiles = config.profiles()?;
-        config_profiles.validate(features, warnings)?;
+
+        let incremental = match env::var_os("CARGO_INCREMENTAL") {
+            Some(v) => Some(v == "1"),
+            None => config.get::<Option<bool>>("build.incremental")?,
+        };
 
         Ok(Profiles {
             dev: ProfileMaker {
@@ -61,6 +69,7 @@ impl Profiles {
                 toml: profiles.and_then(|p| p.doc.clone()),
                 config: None,
             },
+            incremental,
         })
     }
 
@@ -100,10 +109,25 @@ impl Profiles {
             CompileMode::Doc { .. } => &self.doc,
         };
         let mut profile = maker.get_profile(Some(pkg_id), is_member, unit_for);
-        // `panic` should not be set for tests/benches, or any of their dependencies.
+        // `panic` should not be set for tests/benches, or any of their
+        // dependencies.
         if !unit_for.is_panic_ok() || mode.is_any_test() {
             profile.panic = None;
         }
+
+        // Incremental can be globally overridden.
+        if let Some(v) = self.incremental {
+            profile.incremental = v;
+        }
+        // Only enable incremental compilation for sources the user can
+        // modify (aka path sources). For things that change infrequently,
+        // non-incremental builds yield better performance in the compiler
+        // itself (aka crates.io / git dependencies)
+        //
+        // (see also https://github.com/rust-lang/cargo/issues/3972)
+        if !pkg_id.source_id().is_path() {
+            profile.incremental = false;
+        }
         profile
     }
 

Diff for: src/cargo/core/workspace.rs

@@ -237,13 +237,9 @@ impl<'cfg> Workspace<'cfg> {
     }
 
     pub fn profiles(&self) -> &Profiles {
-        let root = self
-            .root_manifest
-            .as_ref()
-            .unwrap_or(&self.current_manifest);
-        match *self.packages.get(root) {
-            MaybePackage::Package(ref p) => p.manifest().profiles(),
-            MaybePackage::Virtual(ref vm) => vm.profiles(),
+        match self.root_maybe() {
+            MaybePackage::Package(p) => p.manifest().profiles(),
+            MaybePackage::Virtual(vm) => vm.profiles(),
         }
     }
 
@@ -260,6 +256,15 @@ impl<'cfg> Workspace<'cfg> {
         .unwrap()
     }
 
+    /// Returns the root Package or VirtualManifest.
+    fn root_maybe(&self) -> &MaybePackage {
+        let root = self
+            .root_manifest
+            .as_ref()
+            .unwrap_or(&self.current_manifest);
+        self.packages.get(root)
+    }
+
     pub fn target_dir(&self) -> Filesystem {
         self.target_dir
             .clone()
@@ -270,27 +275,19 @@ impl<'cfg> Workspace<'cfg> {
     ///
     /// This may be from a virtual crate or an actual crate.
     pub fn root_replace(&self) -> &[(PackageIdSpec, Dependency)] {
-        let path = match self.root_manifest {
-            Some(ref p) => p,
-            None => &self.current_manifest,
-        };
-        match *self.packages.get(path) {
-            MaybePackage::Package(ref p) => p.manifest().replace(),
-            MaybePackage::Virtual(ref vm) => vm.replace(),
+        match self.root_maybe() {
+            MaybePackage::Package(p) => p.manifest().replace(),
+            MaybePackage::Virtual(vm) => vm.replace(),
         }
     }
 
     /// Returns the root [patch] section of this workspace.
     ///
     /// This may be from a virtual crate or an actual crate.
     pub fn root_patch(&self) -> &HashMap<Url, Vec<Dependency>> {
-        let path = match self.root_manifest {
-            Some(ref p) => p,
-            None => &self.current_manifest,
-        };
-        match *self.packages.get(path) {
-            MaybePackage::Package(ref p) => p.manifest().patch(),
-            MaybePackage::Virtual(ref vm) => vm.patch(),
+        match self.root_maybe() {
+            MaybePackage::Package(p) => p.manifest().patch(),
+            MaybePackage::Virtual(vm) => vm.patch(),
         }
     }
 
@@ -524,6 +521,18 @@ impl<'cfg> Workspace<'cfg> {
     /// 2. All workspace members agree on this one root as the root.
     /// 3. The current crate is a member of this workspace.
     fn validate(&mut self) -> CargoResult<()> {
+        // Validate config profiles only once per workspace.
+        let features = match self.root_maybe() {
+            MaybePackage::Package(p) => p.manifest().features(),
+            MaybePackage::Virtual(vm) => vm.features(),
+        };
+        let mut warnings = Vec::new();
+        self.config.profiles()?.validate(features, &mut warnings)?;
+        for warning in warnings {
+            self.config.shell().warn(&warning)?;
+        }
+
+        // The rest of the checks require a VirtualManifest or multiple members.
         if self.root_manifest.is_none() {
             return Ok(());
         }

Diff for: src/cargo/util/toml/mod.rs

@@ -1137,7 +1137,7 @@ impl TomlManifest {
             }
         };
         Ok((
-            VirtualManifest::new(replace, patch, workspace_config, profiles),
+            VirtualManifest::new(replace, patch, workspace_config, profiles, features),
             nested_paths,
         ))
     }

Diff for: src/doc/src/reference/config.md

@@ -129,6 +129,8 @@ target-dir = "target"     # path of where to place all generated artifacts
 rustflags = ["..", ".."]  # custom flags to pass to all compiler invocations
 rustdocflags = ["..", ".."]  # custom flags to pass to rustdoc
 incremental = true        # whether or not to enable incremental compilation
+                          # If `incremental` is not set, then the value from
+                          # the profile is used.
 dep-info-basedir = ".."   # full path for the base directory for targets in depfiles
 
 [term]

Diff for: src/doc/src/reference/manifest.md

@@ -374,6 +374,9 @@ codegen-units = 16 # if > 1 enables parallel code generation which improves
                    # Passes `-C codegen-units`.
 panic = 'unwind'   # panic strategy (`-C panic=...`), can also be 'abort'
 incremental = true # whether or not incremental compilation is enabled
+                   # This can be overridden globally with the CARGO_INCREMENTAL
+                   # environment variable or `build.incremental` config
+                   # variable. Incremental is only used for path sources.
 overflow-checks = true # use overflow checks for integer arithmetic.
                    # Passes the `-C overflow-checks=...` flag to the compiler.
 

Diff for: tests/testsuite/build.rs

@@ -37,7 +37,7 @@ fn cargo_fail_with_no_stderr() {
 }
 
 /// Checks that the `CARGO_INCREMENTAL` environment variable results in
-/// `rustc` getting `-Zincremental` passed to it.
+/// `rustc` getting `-C incremental` passed to it.
 #[test]
 fn cargo_compile_incremental() {
     let p = project()