From f9f8855aa7a70399243e4ed677d3df3ffa178a8f Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Thu, 7 Feb 2019 13:37:21 -0800 Subject: [PATCH] Ignore incremental when lto is set. --- src/cargo/core/compiler/build_context/mod.rs | 7 --- src/cargo/core/compiler/context/mod.rs | 55 -------------------- src/cargo/core/compiler/fingerprint.rs | 7 +-- src/cargo/core/compiler/mod.rs | 7 ++- src/cargo/core/manifest.rs | 7 +++ src/cargo/core/profiles.rs | 32 +++++++++++- src/cargo/core/workspace.rs | 51 ++++++++++-------- src/cargo/util/toml/mod.rs | 12 ++++- src/doc/src/reference/config.md | 2 + src/doc/src/reference/manifest.md | 4 ++ tests/testsuite/build.rs | 30 ++++++++++- tests/testsuite/profile_config.rs | 10 ++-- 12 files changed, 123 insertions(+), 101 deletions(-) diff --git a/src/cargo/core/compiler/build_context/mod.rs b/src/cargo/core/compiler/build_context/mod.rs index 03aa4e58514..5db08f4307e 100644 --- a/src/cargo/core/compiler/build_context/mod.rs +++ b/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, } impl<'a, 'cfg> BuildContext<'a, 'cfg> { @@ -51,11 +50,6 @@ impl<'a, 'cfg> BuildContext<'a, 'cfg> { profiles: &'a Profiles, extra_compiler_args: HashMap, Vec>, ) -> CargoResult> { - 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 --git a/src/cargo/core/compiler/context/mod.rs b/src/cargo/core/compiler/context/mod.rs index 6285fa19673..f0e058955a8 100644 --- a/src/cargo/core/compiler/context/mod.rs +++ b/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> { - // 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 --git a/src/cargo/core/compiler/fingerprint.rs b/src/cargo/core/compiler/fingerprint.rs index a7c21ffee7a..bc696cd845f 100644 --- a/src/cargo/core/compiler/fingerprint.rs +++ b/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 --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index a3bf609ad2a..3f3dee55985 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -763,6 +763,7 @@ fn build_base_args<'a, 'cfg>( overflow_checks, rpath, ref panic, + incremental, .. } = unit.profile; let test = unit.mode.is_any_test(); @@ -905,8 +906,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 --git a/src/cargo/core/manifest.rs b/src/cargo/core/manifest.rs index 55ee9e6a16d..d92141b3aaf 100644 --- a/src/cargo/core/manifest.rs +++ b/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 @@ -539,6 +540,7 @@ impl VirtualManifest { patch: HashMap>, workspace: WorkspaceConfig, profiles: Profiles, + features: Features, ) -> VirtualManifest { VirtualManifest { replace, @@ -546,6 +548,7 @@ impl VirtualManifest { workspace, profiles, warnings: Warnings::new(), + features, } } @@ -572,6 +575,10 @@ impl VirtualManifest { pub fn warnings(&self) -> &Warnings { &self.warnings } + + pub fn features(&self) -> &Features { + &self.features + } } impl Target { diff --git a/src/cargo/core/profiles.rs b/src/cargo/core/profiles.rs index 6a4214c90ae..f8586852c15 100644 --- a/src/cargo/core/profiles.rs +++ b/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, } 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::>("build.incremental")?, + }; Ok(Profiles { dev: ProfileMaker { @@ -61,6 +69,7 @@ impl Profiles { toml: profiles.and_then(|p| p.doc.clone()), config: None, }, + incremental, }) } @@ -100,11 +109,30 @@ 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. 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; + } + // Incremental is incompatible with LTO. + if profile.lto != Lto::Bool(false) { + profile.incremental = false; + } + // 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 --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index 647eed0292c..6226abe7113 100644 --- a/src/cargo/core/workspace.rs +++ b/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() } + /// Return 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,13 +275,9 @@ 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(), } } @@ -284,13 +285,9 @@ impl<'cfg> Workspace<'cfg> { /// /// This may be from a virtual crate or an actual crate. pub fn root_patch(&self) -> &HashMap> { - 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 --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index a57a09dfa61..30df214a5a2 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -473,6 +473,16 @@ impl TomlProfile { } _ => {} } + + if self.incremental == Some(true) + && self.lto != None + && self.lto != Some(StringOrBool::Bool(false)) + { + warnings.push(format!( + "`incremental` setting is ignored for the `{}` profile when `lto` is enabled", + name + )); + } Ok(()) } @@ -1144,7 +1154,7 @@ impl TomlManifest { } }; Ok(( - VirtualManifest::new(replace, patch, workspace_config, profiles), + VirtualManifest::new(replace, patch, workspace_config, profiles, features), nested_paths, )) } diff --git a/src/doc/src/reference/config.md b/src/doc/src/reference/config.md index da6b3c5d649..2810142414e 100644 --- a/src/doc/src/reference/config.md +++ b/src/doc/src/reference/config.md @@ -123,6 +123,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 --git a/src/doc/src/reference/manifest.md b/src/doc/src/reference/manifest.md index 3788037a6de..02ca3d68904 100644 --- a/src/doc/src/reference/manifest.md +++ b/src/doc/src/reference/manifest.md @@ -361,6 +361,10 @@ 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 CARGO_INCREMENTAL + # environment variable or `build.incremental` config + # variable. Incremental cannot be used with `lto`. + # 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 --git a/tests/testsuite/build.rs b/tests/testsuite/build.rs index 892b7c9e277..2f5ceddb1c5 100644 --- a/tests/testsuite/build.rs +++ b/tests/testsuite/build.rs @@ -37,7 +37,7 @@ fn cargo_fail_with_no_stderr() { } /// Check 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() @@ -102,6 +102,34 @@ fn incremental_profile() { .run(); } +#[test] +fn incremental_profile_lto() { + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + authors = [] + + [profile.release] + incremental = true + lto = true + "#, + ) + .file("src/main.rs", "fn main() {}") + .build(); + + p.cargo("build --release -v") + .env_remove("CARGO_INCREMENTAL") + .with_stderr_does_not_contain("[..]C incremental=[..]") + .with_stderr_contains( + "[WARNING] `incremental` setting is ignored for the `release` profile when `lto` is enabled" + ) + .run(); +} + #[test] fn incremental_config() { let p = project() diff --git a/tests/testsuite/profile_config.rs b/tests/testsuite/profile_config.rs index 8a46cff70a5..d837d228c6b 100644 --- a/tests/testsuite/profile_config.rs +++ b/tests/testsuite/profile_config.rs @@ -138,10 +138,7 @@ fn profile_config_validate_errors() { .with_status(101) .with_stderr( "\ -[ERROR] failed to parse manifest at `[CWD]/Cargo.toml` - -Caused by: - config profile `profile.dev` is not valid +[ERROR] config profile `profile.dev` is not valid Caused by: `panic` may not be specified in a profile override. @@ -235,8 +232,7 @@ found profile override specs: bar, bar:0.5.0", fn profile_config_all_options() { // Ensure all profile options are supported. let p = project() - .file("Cargo.toml", &basic_lib_manifest("foo")) - .file("src/lib.rs", "") + .file("src/main.rs", "fn main() {}") .file( ".cargo/config", r#" @@ -258,10 +254,12 @@ fn profile_config_all_options() { .masquerade_as_nightly_cargo() .with_stderr( "\ +[WARNING] `incremental` setting is ignored [..] [COMPILING] foo [..] [RUNNING] `rustc --crate-name foo [..] \ -C opt-level=1 \ -C panic=abort \ + -C lto \ -C codegen-units=2 \ -C debuginfo=2 \ -C debug-assertions=on \