Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable incremental by default #4817

Merged
merged 1 commit into from
Dec 21, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,7 @@ matrix:

- env: TARGET=x86_64-unknown-linux-gnu
ALT=i686-unknown-linux-gnu
# FIXME(rust-lang/rust#46271) should use just `nightly`
rust: nightly-2017-11-20
rust: nightly
install:
- mdbook --help || cargo install mdbook --force
script:
Expand Down
4 changes: 4 additions & 0 deletions src/cargo/core/manifest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,8 @@ pub struct Profile {
pub check: bool,
#[serde(skip_serializing)]
pub panic: Option<String>,
#[serde(skip_serializing)]
pub incremental: bool,
}

#[derive(Default, Clone, Debug, PartialEq, Eq)]
Expand Down Expand Up @@ -631,6 +633,7 @@ impl Profile {
debuginfo: Some(2),
debug_assertions: true,
overflow_checks: true,
incremental: true,
..Profile::default()
}
}
Expand Down Expand Up @@ -712,6 +715,7 @@ impl Default for Profile {
run_custom_build: false,
check: false,
panic: None,
incremental: false,
}
}
}
Expand Down
84 changes: 50 additions & 34 deletions src/cargo/ops/cargo_rustc/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ pub struct Context<'a, 'cfg: 'a> {
target_info: TargetInfo,
host_info: TargetInfo,
profiles: &'a Profiles,
incremental_enabled: bool,
incremental_env: Option<bool>,

/// For each Unit, a list all files produced as a triple of
///
Expand Down Expand Up @@ -154,24 +154,11 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
None => None,
};

// Enable incremental builds if the user opts in. For now,
// this is an environment variable until things stabilize a
// bit more.
let incremental_enabled = match env::var("CARGO_INCREMENTAL") {
Ok(v) => v == "1",
Err(_) => false,
let incremental_env = match env::var("CARGO_INCREMENTAL") {
Ok(v) => Some(v == "1"),
Err(_) => None,
};

// -Z can only be used on nightly builds; other builds complain loudly.
// Since incremental builds only work on nightly anyway, we silently
// ignore CARGO_INCREMENTAL on anything but nightly. This allows users
// to always have CARGO_INCREMENTAL set without getting unexpected
// errors on stable/beta builds.
let is_nightly =
config.rustc()?.verbose_version.contains("-nightly") ||
config.rustc()?.verbose_version.contains("-dev");
let incremental_enabled = incremental_enabled && is_nightly;

// Load up the jobserver that we'll use to manage our parallelism. This
// is the same as the GNU make implementation of a jobserver, and
// intentionally so! It's hoped that we can interact with GNU make and
Expand Down Expand Up @@ -206,7 +193,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
build_explicit_deps: HashMap::new(),
links: Links::new(),
used_in_plugin: HashSet::new(),
incremental_enabled: incremental_enabled,
incremental_env,
jobserver: jobserver,
build_script_overridden: HashSet::new(),

Expand Down Expand Up @@ -1082,24 +1069,53 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
}

pub fn incremental_args(&self, unit: &Unit) -> CargoResult<Vec<String>> {
if self.incremental_enabled {
if unit.pkg.package_id().source_id().is_path() {
// Only enable incremental compilation for sources the user can modify.
// For things that change infrequently, non-incremental builds yield
// better performance.
// (see also https://github.com/rust-lang/cargo/issues/3972)
return Ok(vec![format!("-Zincremental={}",
self.layout(unit.kind).incremental().display())]);
} else if unit.profile.codegen_units.is_none() {
// For non-incremental builds we set a higher number of
// codegen units so we get faster compiles. It's OK to do
// so because the user has already opted into slower
// runtime code by setting CARGO_INCREMENTAL.
return Ok(vec![format!("-Ccodegen-units={}", ::num_cpus::get())]);
}
// 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`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, so the value true for build.incremental does not change anything, because true && default is just default.

So, the only usage for this flag is to disable incremental compilation. So perhaps we can rename the flag to build.disable_incremental and allow only true value for it?

Should this setting override even explicit profile incremental setting? If the primary use-case here is to work-around the possible bugs in incremental compilation, then I think a reliable way to turn off all incremental would be more valuable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this was mostly just meant to mirror CARGO_INCREMENTAL as a "global configuration option" to disable it everywhere (I don't think enabling it everywhere makes much sense for release profiles for example).

I personally prefer incremental at least though to disable-incremental in that it's a bit more clear (even if incremental = true doesn't actually do anything).

I think that makes sense as well yeah for .cargo/config to override profiles, I'll tweak that

// * `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 project.
// * 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.config.get_bool("build.incremental")?.map(|c| c.val);
let incremental = match (self.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())
}

Ok(vec![])
let dir = self.layout(unit.kind).incremental().display();
Ok(vec![
"-C".to_string(),
format!("incremental={}", dir),
])
}

pub fn rustflags_args(&self, unit: &Unit) -> CargoResult<Vec<String>> {
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/ops/cargo_rustc/fingerprint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,7 @@ fn calculate<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>)
let fingerprint = Arc::new(Fingerprint {
rustc: util::hash_u64(&cx.config.rustc()?.verbose_version),
target: util::hash_u64(&unit.target),
profile: util::hash_u64(&unit.profile),
profile: util::hash_u64(&(&unit.profile, cx.incremental_args(unit)?)),
// Note that .0 is hashed here, not .1 which is the cwd. That doesn't
// actually affect the output artifact so there's no need to hash it.
path: util::hash_u64(&super::path_args(cx, unit).0),
Expand Down
10 changes: 6 additions & 4 deletions src/cargo/ops/cargo_rustc/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,6 @@ fn rustc<'a, 'cfg>(cx: &mut Context<'a, 'cfg>,
}.with_extension("d");
let dep_info_loc = fingerprint::dep_info_loc(cx, unit);

rustc.args(&cx.incremental_args(unit)?);
rustc.args(&cx.rustflags_args(unit)?);
let json_messages = cx.build_config.json_messages;
let package_id = unit.pkg.package_id().clone();
Expand Down Expand Up @@ -651,7 +650,7 @@ fn prepare_rustc<'a, 'cfg>(cx: &mut Context<'a, 'cfg>,
unit: &Unit<'a>) -> CargoResult<ProcessBuilder> {
let mut base = cx.compilation.rustc_process(unit.pkg)?;
base.inherit_jobserver(&cx.jobserver);
build_base_args(cx, &mut base, unit, crate_types);
build_base_args(cx, &mut base, unit, crate_types)?;
build_deps_args(&mut base, cx, unit)?;
Ok(base)
}
Expand Down Expand Up @@ -743,11 +742,11 @@ fn add_path_args(cx: &Context, unit: &Unit, cmd: &mut ProcessBuilder) {
fn build_base_args<'a, 'cfg>(cx: &mut Context<'a, 'cfg>,
cmd: &mut ProcessBuilder,
unit: &Unit<'a>,
crate_types: &[&str]) {
crate_types: &[&str]) -> CargoResult<()> {
let Profile {
ref opt_level, lto, codegen_units, ref rustc_args, debuginfo,
debug_assertions, overflow_checks, rpath, test, doc: _doc,
run_custom_build, ref panic, rustdoc_args: _, check,
run_custom_build, ref panic, rustdoc_args: _, check, incremental: _,
} = *unit.profile;
assert!(!run_custom_build);

Expand Down Expand Up @@ -888,6 +887,9 @@ fn build_base_args<'a, 'cfg>(cx: &mut Context<'a, 'cfg>,

opt(cmd, "-C", "ar=", cx.ar(unit.kind).map(|s| s.as_ref()));
opt(cmd, "-C", "linker=", cx.linker(unit.kind).map(|s| s.as_ref()));
cmd.args(&cx.incremental_args(unit)?);

Ok(())
}


Expand Down
4 changes: 3 additions & 1 deletion src/cargo/util/toml/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,7 @@ pub struct TomlProfile {
panic: Option<String>,
#[serde(rename = "overflow-checks")]
overflow_checks: Option<bool>,
incremental: Option<bool>,
}

#[derive(Clone, Debug, Serialize)]
Expand Down Expand Up @@ -1123,7 +1124,7 @@ fn build_profiles(profiles: &Option<TomlProfiles>) -> Profiles {
fn merge(profile: Profile, toml: Option<&TomlProfile>) -> Profile {
let &TomlProfile {
ref opt_level, lto, codegen_units, ref debug, debug_assertions, rpath,
ref panic, ref overflow_checks,
ref panic, ref overflow_checks, ref incremental,
} = match toml {
Some(toml) => toml,
None => return profile,
Expand All @@ -1149,6 +1150,7 @@ fn build_profiles(profiles: &Option<TomlProfiles>) -> Profiles {
run_custom_build: profile.run_custom_build,
check: profile.check,
panic: panic.clone().or(profile.panic),
incremental: incremental.unwrap_or(profile.incremental),
}
}
}
1 change: 1 addition & 0 deletions src/doc/config.md
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ rustdoc = "rustdoc" # the doc generator tool
target = "triple" # build for the target triple
target-dir = "target" # path of where to place all generated artifacts
rustflags = ["..", ".."] # custom flags to pass to all compiler invocations
incremental = true # whether or not to enable incremental compilation

[term]
verbose = false # whether cargo provides verbose output
Expand Down
4 changes: 4 additions & 0 deletions src/doc/environment-variables.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@ system:
* `RUSTFLAGS` - A space-separated list of custom flags to pass to all compiler
invocations that Cargo performs. In contrast with `cargo rustc`, this is
useful for passing a flag to *all* compiler instances.
* `CARGO_INCREMENTAL` - If this is set to 1 then Cargo will force incremental
compilation to be enabled for the current compilation, and when set to 0 it
will force disabling it. If this env var isn't present then Cargo's defaults
will otherwise be used.

Note that Cargo will also read environment variables for `.cargo/config`
configuration values, as described in [that documentation][config-env]
Expand Down
7 changes: 6 additions & 1 deletion src/doc/manifest.md
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ license-file = "..."

# Appveyor: `repository` is required. `branch` is optional; default is `master`
# `service` is optional; valid values are `github` (default), `bitbucket`, and
# `gitlab`; `id` is optional; you can specify the appveyor project id if you
# `gitlab`; `id` is optional; you can specify the appveyor project id if you
# want to use that instead. `project_name` is optional; use when the repository
# name differs from the appveyor project name.
appveyor = { repository = "...", branch = "master", service = "github" }
Expand Down Expand Up @@ -289,6 +289,7 @@ codegen-units = 1 # if > 1 enables parallel code generation which improves
# compile times, but prevents some optimizations.
# Passes `-C codegen-units`. Ignored when `lto = true`.
panic = 'unwind' # panic strategy (`-C panic=...`), can also be 'abort'
incremental = true # whether or not incremental compilation is enabled

# The release profile, used for `cargo build --release`.
[profile.release]
Expand All @@ -299,6 +300,7 @@ lto = false
debug-assertions = false
codegen-units = 1
panic = 'unwind'
incremental = false

# The testing profile, used for `cargo test`.
[profile.test]
Expand All @@ -309,6 +311,7 @@ lto = false
debug-assertions = true
codegen-units = 1
panic = 'unwind'
incremental = true

# The benchmarking profile, used for `cargo bench` and `cargo test --release`.
[profile.bench]
Expand All @@ -319,6 +322,7 @@ lto = false
debug-assertions = false
codegen-units = 1
panic = 'unwind'
incremental = false

# The documentation profile, used for `cargo doc`.
[profile.doc]
Expand All @@ -329,6 +333,7 @@ lto = false
debug-assertions = true
codegen-units = 1
panic = 'unwind'
incremental = true
```

# The `[features]` section
Expand Down
78 changes: 76 additions & 2 deletions tests/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,16 +58,90 @@ fn cargo_compile_incremental() {
assert_that(
p.cargo("build").arg("-v").env("CARGO_INCREMENTAL", "1"),
execs().with_stderr_contains(
"[RUNNING] `rustc [..] -Zincremental=[..][/]target[/]debug[/]incremental`\n")
"[RUNNING] `rustc [..] -C incremental=[..][/]target[/]debug[/]incremental[..]`\n")
.with_status(0));

assert_that(
p.cargo("test").arg("-v").env("CARGO_INCREMENTAL", "1"),
execs().with_stderr_contains(
"[RUNNING] `rustc [..] -Zincremental=[..][/]target[/]debug[/]incremental`\n")
"[RUNNING] `rustc [..] -C incremental=[..][/]target[/]debug[/]incremental[..]`\n")
.with_status(0));
}

#[test]
fn incremental_profile() {
if !is_nightly() {
return
}

let p = project("foo")
.file("Cargo.toml", r#"
[package]
name = "foo"
version = "0.1.0"
authors = []

[profile.dev]
incremental = false

[profile.release]
incremental = true
"#)
.file("src/main.rs", "fn main() {}")
.build();

assert_that(
p.cargo("build").arg("-v").env_remove("CARGO_INCREMENTAL"),
execs().with_stderr_does_not_contain("[..]C incremental=[..]")
.with_status(0));

assert_that(
p.cargo("build").arg("-v").env("CARGO_INCREMENTAL", "1"),
execs().with_stderr_contains("[..]C incremental=[..]")
.with_status(0));

assert_that(
p.cargo("build").arg("--release").arg("-v").env_remove("CARGO_INCREMENTAL"),
execs().with_stderr_contains("[..]C incremental=[..]")
.with_status(0));

assert_that(
p.cargo("build").arg("--release").arg("-v").env("CARGO_INCREMENTAL", "0"),
execs().with_stderr_does_not_contain("[..]C incremental=[..]")
.with_status(0));
}

#[test]
fn incremental_config() {
if !is_nightly() {
return
}

let p = project("foo")
.file("Cargo.toml", r#"
[package]
name = "foo"
version = "0.1.0"
authors = []
"#)
.file("src/main.rs", "fn main() {}")
.file(".cargo/config", r#"
[build]
incremental = false
"#)
.build();

assert_that(
p.cargo("build").arg("-v").env_remove("CARGO_INCREMENTAL"),
execs().with_stderr_does_not_contain("[..]C incremental=[..]")
.with_status(0));

assert_that(
p.cargo("build").arg("-v").env("CARGO_INCREMENTAL", "1"),
execs().with_stderr_contains("[..]C incremental=[..]")
.with_status(0));
}

#[test]
fn cargo_compile_manifest_path() {
let p = project("foo")
Expand Down
Loading