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.

Sign up for GitHub

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

Jump to bottom

Add build profile. #6577

Closed
wants to merge 1 commit into from
Closed

Add build profile. #6577

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 3 additions & 0 deletions src/cargo/core/features.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,9 @@ features! {
// Overriding profiles for dependencies.
[unstable] profile_overrides: bool,

// Build profile.
[unstable] build_profile: bool,

// Separating the namespaces for features and dependencies
[unstable] namespaced_features: bool,

Expand Down
132 changes: 96 additions & 36 deletions src/cargo/core/profiles.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
use std::collections::HashSet;
use std::path::Path;
use std::{cmp, env, fmt, hash};

use serde::Deserialize;

use crate::core::compiler::CompileMode;
use crate::core::interning::InternedString;
use crate::core::{Features, PackageId, PackageIdSpec, PackageSet, Shell};
use crate::core::{Feature, Features, PackageId, PackageIdSpec, PackageSet, Shell};
use crate::util::errors::CargoResultExt;
use crate::util::lev_distance::lev_distance;
use crate::util::toml::{ProfilePackageSpec, StringOrBool, TomlProfile, TomlProfiles, U32OrBool};
Expand All @@ -19,10 +20,13 @@ pub struct Profiles {
test: ProfileMaker,
bench: ProfileMaker,
doc: ProfileMaker,
build: ProfileMaker,
/// Incremental compilation can be overridden globally via:
/// - `CARGO_INCREMENTAL` environment variable.
/// - `build.incremental` config value.
incremental: Option<bool>,
// Temporary flag to detect if unstable feature is enabled.
build_enabled: bool,
}

impl Profiles {
Expand All @@ -31,9 +35,13 @@ impl Profiles {
config: &Config,
features: &Features,
warnings: &mut Vec<String>,
manifest_path: &Path,
) -> CargoResult<Profiles> {
if let Some(profiles) = profiles {
profiles.validate(features, warnings)?;
if profiles.build.is_some() {
features.require(Feature::build_profile())?;
}
}

let config_profiles = config.profiles()?;
Expand All @@ -43,6 +51,18 @@ impl Profiles {
None => config.get::<Option<bool>>("build.incremental")?,
};

let config_build_profile =
if !features.is_enabled(Feature::build_profile()) && config_profiles.build.is_some() {
warnings.push(format!(
"profile `build` in config file will be ignored for \
manifest `{}` because \"build-profile\" feature is not enabled",
manifest_path.display()
));
None
} else {
config_profiles.build.clone()
};

Ok(Profiles {
dev: ProfileMaker {
default: Profile::default_dev(),
Expand All @@ -69,7 +89,13 @@ impl Profiles {
toml: profiles.and_then(|p| p.doc.clone()),
config: None,
},
build: ProfileMaker {
default: Profile::default_build(),
toml: profiles.and_then(|p| p.build.clone()),
config: config_build_profile,
},
incremental,
build_enabled: features.is_enabled(Feature::build_profile()),
})
}

Expand Down Expand Up @@ -100,10 +126,14 @@ impl Profiles {
// `build_unit_profiles` normally ensures that it selects the
// ancestor's profile. However, `cargo clean -p` can hit this
// path.
if release {
&self.release
let maker = if release { &self.release } else { &self.dev };
if (unit_for.build)
&& self.build_enabled
&& !maker.has_build_override()
{
&self.build
} else {
&self.dev
maker
}
}
CompileMode::Doc { .. } => &self.doc,
Expand Down Expand Up @@ -148,9 +178,11 @@ impl Profiles {
/// select for the package that was actually built.
pub fn base_profile(&self, release: bool) -> Profile {
if release {
self.release.get_profile(None, true, UnitFor::new_normal())
self.release
.get_profile(None, true, UnitFor::new_normal())
} else {
self.dev.get_profile(None, true, UnitFor::new_normal())
self.dev
.get_profile(None, true, UnitFor::new_normal())
}
}

Expand All @@ -165,6 +197,7 @@ impl Profiles {
self.test.validate_packages(shell, packages)?;
self.bench.validate_packages(shell, packages)?;
self.doc.validate_packages(shell, packages)?;
self.build.validate_packages(shell, packages)?;
Ok(())
}
}
Expand Down Expand Up @@ -198,10 +231,22 @@ impl ProfileMaker {
) -> Profile {
let mut profile = self.default;
if let Some(ref toml) = self.toml {
merge_toml(pkg_id, is_member, unit_for, &mut profile, toml);
merge_toml(
pkg_id,
is_member,
unit_for,
&mut profile,
toml,
);
}
if let Some(ref toml) = self.config {
merge_toml(pkg_id, is_member, unit_for, &mut profile, toml);
merge_toml(
pkg_id,
is_member,
unit_for,
&mut profile,
toml,
);
}
profile
}
Expand Down Expand Up @@ -318,6 +363,13 @@ impl ProfileMaker {
}
Ok(())
}

fn has_build_override(&self) -> bool {
self.toml
.as_ref()
.map(|tp| tp.build_override.is_some())
.unwrap_or(false)
}
}

fn merge_toml(
Expand Down Expand Up @@ -449,6 +501,7 @@ compact_debug! {
"test" => (Profile::default_test(), "default_test()"),
"bench" => (Profile::default_bench(), "default_bench()"),
"doc" => (Profile::default_doc(), "default_doc()"),
"build" => (Profile::default_build(), "default_build()"),
_ => (Profile::default(), "default()"),
};
[debug_the_fields(
Expand Down Expand Up @@ -529,6 +582,13 @@ impl Profile {
}
}

fn default_build() -> Profile {
Profile {
name: "build",
..Profile::default()
}
}

/// Compares all fields except `name`, which doesn't affect compilation.
/// This is necessary for `Unit` deduplication for things like "test" and
/// "dev" which are essentially the same.
Expand Down Expand Up @@ -604,31 +664,33 @@ pub struct UnitFor {
impl UnitFor {
/// A unit for a normal target/dependency (i.e., not custom build,
/// proc macro/plugin, or test/bench).
pub fn new_normal() -> UnitFor {
pub const fn new_normal() -> UnitFor {
UnitFor {
build: false,
panic_abort_ok: true,
}
}

/// A unit for a custom build script or its dependencies.
pub fn new_build() -> UnitFor {
pub const fn new_build() -> UnitFor {
UnitFor {
build: true,
panic_abort_ok: false,
}
}

/// A unit for a proc macro or compiler plugin or their dependencies.
pub fn new_compiler() -> UnitFor {
pub const fn new_compiler() -> UnitFor {
// Note: This is currently the same as `new_build`, but keeping it
// separate for now in case it is useful in the future.
UnitFor {
build: false,
build: true,
panic_abort_ok: false,
}
}

/// A unit for a test/bench target or their dependencies.
pub fn new_test() -> UnitFor {
pub const fn new_test() -> UnitFor {
UnitFor {
build: false,
panic_abort_ok: false,
Expand Down Expand Up @@ -660,18 +722,9 @@ impl UnitFor {
/// All possible values, used by `clean`.
pub fn all_values() -> &'static [UnitFor] {
static ALL: [UnitFor; 3] = [
UnitFor {
build: false,
panic_abort_ok: true,
},
UnitFor {
build: true,
panic_abort_ok: false,
},
UnitFor {
build: false,
panic_abort_ok: false,
},
UnitFor::new_normal(),
UnitFor::new_build(),
UnitFor::new_test(),
];
&ALL
}
Expand All @@ -682,22 +735,29 @@ impl UnitFor {
pub struct ConfigProfiles {
dev: Option<TomlProfile>,
release: Option<TomlProfile>,
build: Option<TomlProfile>,
}

impl ConfigProfiles {
pub fn validate(&self, features: &Features, warnings: &mut Vec<String>) -> CargoResult<()> {
if let Some(ref profile) = self.dev {
profile
.validate("dev", features, warnings)
.chain_err(|| failure::format_err!("config profile `profile.dev` is not valid"))?;
}
if let Some(ref profile) = self.release {
profile
.validate("release", features, warnings)
.chain_err(|| {
failure::format_err!("config profile `profile.release` is not valid")
})?;
macro_rules! check_profile {
($name:ident) => {
if let Some(ref profile) = self.$name {
profile
.validate(stringify!($name), features, warnings)
.chain_err(|| {
failure::format_err!(
"config profile `profile.{}` is not valid",
stringify!($name)
)
})?;
}
};
}

check_profile!(dev);
check_profile!(release);
check_profile!(build);
Ok(())
}
}
27 changes: 24 additions & 3 deletions src/cargo/util/toml/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,7 @@ pub struct TomlProfiles {
pub bench: Option<TomlProfile>,
pub dev: Option<TomlProfile>,
pub release: Option<TomlProfile>,
pub build: Option<TomlProfile>,
}

impl TomlProfiles {
Expand All @@ -294,6 +295,9 @@ impl TomlProfiles {
if let Some(ref release) = self.release {
release.validate("release", features, warnings)?;
}
if let Some(ref build) = self.build {
build.validate("build", features, warnings)?;
}
Ok(())
}
}
Expand Down Expand Up @@ -470,7 +474,7 @@ impl TomlProfile {
}

match name {
"dev" | "release" => {}
"dev" | "release" | "build" => {}
_ => {
if self.overrides.is_some() || self.build_override.is_some() {
bail!(
Expand All @@ -491,6 +495,11 @@ impl TomlProfile {
warnings.push(format!("`panic` setting is ignored for `{}` profile", name))
}
}
"build" => {
if self.build_override.is_some() {
failure::bail!("`build` profile cannot specify build overrides.")
}
}
_ => {}
}

Expand Down Expand Up @@ -1023,7 +1032,13 @@ impl TomlManifest {
`[workspace]`, only one can be specified"
),
};
let profiles = Profiles::new(me.profile.as_ref(), config, &features, &mut warnings)?;
let profiles = Profiles::new(
me.profile.as_ref(),
config,
&features,
&mut warnings,
&package_root.join("Cargo.toml"),
)?;
let publish = match project.publish {
Some(VecStringOrBool::VecString(ref vecstring)) => Some(vecstring.clone()),
Some(VecStringOrBool::Bool(false)) => Some(vec![]),
Expand Down Expand Up @@ -1154,7 +1169,13 @@ impl TomlManifest {
};
(me.replace(&mut cx)?, me.patch(&mut cx)?)
};
let profiles = Profiles::new(me.profile.as_ref(), config, &features, &mut warnings)?;
let profiles = Profiles::new(
me.profile.as_ref(),
config,
&features,
&mut warnings,
&root.join("Cargo.toml"),
)?;
let workspace_config = match me.workspace {
Some(ref config) => WorkspaceConfig::Root(WorkspaceRootConfig::new(
root,
Expand Down
30 changes: 30 additions & 0 deletions src/doc/src/reference/unstable.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,36 @@ cargo +nightly build -Z config-profile
```


### Build Profile
* Tracking Issue: [rust-lang/rust#48683](https://github.com/rust-lang/rust/issues/48683)

The `build` profile controls the build settings for build scripts,
proc-macros, compiler plugins, and all of their dependencies. It requires the
`build-profile` feature to be enabled.

```toml
cargo-features = ["build-profile"]

[profile.build]
# The following illustrates the defaults.
opt-level = 0
debug = false
rpath = false
lto = false
debug-assertions = false
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little wary about this being false, it seems like this may want to be true by default to help weed out mistakes more quickly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, I wouldn't mind making it true. Maybe the same for overflow-checks? I don't have a sense of how much slower that typically makes things, but I suspect it would not be perceivable by most scripts/macros. I think debug is the bigger question of how it should be defaulted.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think this and overflow-checks should probably default to on, build scripts are typically rarely a bottleneck and if they both of these options can be disabled pretty easily (both from crates.io via changing apis or locally by changing profiles).

For debug I wonder if we could perhaps try setting a default of 1? That means we only generate line tables for backtraces, but no local variable info as no one's really using gdb on these

codegen-units = 16
panic = 'unwind'
incremental = false
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be true? (was this copy/pasted from somewhere else that needs an update?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now it is false. The default build profile is defined here based on the default here. This is similar to release mode.

I don't have a strong opinion about any of the defaults. I think the theory on this one is that build scripts are rarely modified. But I can see how it would be annoying when you are actively working on it.

Maybe you are thinking of #6564 which hasn't merged, yet? That would change the default.

Copy link
Member

Choose a reason for hiding this comment

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

Oh nah I just wanted to confirm. I think that we should have incremental turned on by default as the overhead only applies for path dependencies anyway, in this case typically just the build script itself.

I could see this going either way though. Build scripts are typically quite small and fast to compile, which means that incremental isn't a hit for them nor does it really matter too much. I'd personally err on the side of enabling incremental though

overflow-checks = false
```

It is compatible with [Profile Overrides](#profile-overrides) and [Config
Profiles](#config-profiles). If `build-override` is specified in a dev or
release profile, that takes precedence over the `build` profile. Enabling
`build-profile` will cause `build-override` to also affect proc-macros and
plugins (normally it only affects build scripts).


### Namespaced features
* Original issue: [#1286](https://github.com/rust-lang/cargo/issues/1286)
* Tracking Issue: [#5565](https://github.com/rust-lang/cargo/issues/5565)
Expand Down
Loading