diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index 22f70bf67aa..32f79947843 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -23,7 +23,7 @@ use same_file::is_same_file; use serde::Serialize; use crate::core::manifest::TargetSourcePath; -use crate::core::profiles::{Lto, Profile}; +use crate::core::profiles::{Lto, PanicStrategy, Profile}; use crate::core::{PackageId, Target}; use crate::util::errors::{CargoResult, CargoResultExt, Internal, ProcessError}; use crate::util::paths; @@ -794,7 +794,7 @@ fn build_base_args<'a, 'cfg>( cmd.arg("-C").arg(&format!("opt-level={}", opt_level)); } - if let Some(panic) = panic.as_ref() { + if *panic != PanicStrategy::Unwind { cmd.arg("-C").arg(format!("panic={}", panic)); } diff --git a/src/cargo/core/profiles.rs b/src/cargo/core/profiles.rs index 18355b9b40e..be97a54ef15 100644 --- a/src/cargo/core/profiles.rs +++ b/src/cargo/core/profiles.rs @@ -111,8 +111,8 @@ impl Profiles { 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; + if !unit_for.is_panic_abort_ok() || mode.is_any_test() { + profile.panic = PanicStrategy::Unwind; } // Incremental can be globally overridden. @@ -390,8 +390,13 @@ fn merge_profile(profile: &mut Profile, toml: &TomlProfile) { if let Some(rpath) = toml.rpath { profile.rpath = rpath; } - if let Some(ref panic) = toml.panic { - profile.panic = Some(InternedString::new(panic)); + if let Some(panic) = &toml.panic { + profile.panic = match panic.as_str() { + "unwind" => PanicStrategy::Unwind, + "abort" => PanicStrategy::Abort, + // This should be validated in TomlProfile::validate + _ => panic!("Unexpected panic setting `{}`", panic), + }; } if let Some(overflow_checks) = toml.overflow_checks { profile.overflow_checks = overflow_checks; @@ -415,7 +420,7 @@ pub struct Profile { pub overflow_checks: bool, pub rpath: bool, pub incremental: bool, - pub panic: Option, + pub panic: PanicStrategy, } impl Default for Profile { @@ -430,7 +435,7 @@ impl Default for Profile { overflow_checks: false, rpath: false, incremental: false, - panic: None, + panic: PanicStrategy::Unwind, } } } @@ -530,26 +535,26 @@ impl Profile { fn comparable( &self, ) -> ( - &InternedString, - &Lto, - &Option, - &Option, - &bool, - &bool, - &bool, - &bool, - &Option, + InternedString, + Lto, + Option, + Option, + bool, + bool, + bool, + bool, + PanicStrategy, ) { ( - &self.opt_level, - &self.lto, - &self.codegen_units, - &self.debuginfo, - &self.debug_assertions, - &self.overflow_checks, - &self.rpath, - &self.incremental, - &self.panic, + self.opt_level, + self.lto, + self.codegen_units, + self.debuginfo, + self.debug_assertions, + self.overflow_checks, + self.rpath, + self.incremental, + self.panic, ) } } @@ -564,6 +569,23 @@ pub enum Lto { Named(InternedString), } +/// The `panic` setting. +#[derive(Clone, Copy, PartialEq, Eq, Debug, Hash, PartialOrd, Ord)] +pub enum PanicStrategy { + Unwind, + Abort, +} + +impl fmt::Display for PanicStrategy { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match *self { + PanicStrategy::Unwind => "unwind", + PanicStrategy::Abort => "abort", + } + .fmt(f) + } +} + /// Flags used in creating `Unit`s to indicate the purpose for the target, and /// to ensure the target's dependencies have the correct settings. #[derive(Copy, Clone, Debug, Eq, PartialEq, Hash)] @@ -571,11 +593,11 @@ pub struct UnitFor { /// A target for `build.rs` or any of its dependencies. This enables /// `build-override` profiles for these targets. custom_build: bool, - /// This is true if it is *allowed* to set the `panic` flag. Currently + /// This is true if it is *allowed* to set the `panic=abort` flag. Currently /// this is false for test/bench targets and all their dependencies, and /// "for_host" units such as proc macro and custom build scripts and their /// dependencies. - panic_ok: bool, + panic_abort_ok: bool, } impl UnitFor { @@ -584,7 +606,7 @@ impl UnitFor { pub fn new_normal() -> UnitFor { UnitFor { custom_build: false, - panic_ok: true, + panic_abort_ok: true, } } @@ -592,7 +614,7 @@ impl UnitFor { pub fn new_build() -> UnitFor { UnitFor { custom_build: true, - panic_ok: false, + panic_abort_ok: false, } } @@ -600,7 +622,7 @@ impl UnitFor { pub fn new_compiler() -> UnitFor { UnitFor { custom_build: false, - panic_ok: false, + panic_abort_ok: false, } } @@ -608,18 +630,18 @@ impl UnitFor { pub fn new_test() -> UnitFor { UnitFor { custom_build: false, - panic_ok: false, + panic_abort_ok: false, } } /// Creates a variant based on `for_host` setting. /// - /// When `for_host` is true, this clears `panic_ok` in a sticky fashion so - /// that all its dependencies also have `panic_ok=false`. + /// When `for_host` is true, this clears `panic_abort_ok` in a sticky fashion so + /// that all its dependencies also have `panic_abort_ok=false`. pub fn with_for_host(self, for_host: bool) -> UnitFor { UnitFor { custom_build: self.custom_build, - panic_ok: self.panic_ok && !for_host, + panic_abort_ok: self.panic_abort_ok && !for_host, } } @@ -630,8 +652,8 @@ impl UnitFor { } /// Returns `true` if this unit is allowed to set the `panic` compiler flag. - pub fn is_panic_ok(self) -> bool { - self.panic_ok + pub fn is_panic_abort_ok(self) -> bool { + self.panic_abort_ok } /// All possible values, used by `clean`. @@ -639,15 +661,15 @@ impl UnitFor { static ALL: [UnitFor; 3] = [ UnitFor { custom_build: false, - panic_ok: true, + panic_abort_ok: true, }, UnitFor { custom_build: true, - panic_ok: false, + panic_abort_ok: false, }, UnitFor { custom_build: false, - panic_ok: false, + panic_abort_ok: false, }, ]; &ALL diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 31099a78e81..a3d53d962d8 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -5,6 +5,7 @@ use std::path::{Path, PathBuf}; use std::rc::Rc; use std::str; +use failure::bail; use log::{debug, trace}; use semver::{self, VersionReq}; use serde::de; @@ -52,8 +53,9 @@ fn do_read_manifest( let package_root = manifest_file.parent().unwrap(); let toml = { - let pretty_filename = - manifest_file.strip_prefix(config.cwd()).unwrap_or(manifest_file); + let pretty_filename = manifest_file + .strip_prefix(config.cwd()) + .unwrap_or(manifest_file); parse(contents, pretty_filename, config)? }; @@ -78,7 +80,7 @@ fn do_read_manifest( TomlManifest::to_real_manifest(&manifest, source_id, package_root, config)?; add_unused(manifest.warnings_mut()); if !manifest.targets().iter().any(|t| !t.is_custom_build()) { - failure::bail!( + bail!( "no targets specified in the manifest\n \ either src/lib.rs, src/main.rs, a [lib] section, or \ [[bin]] section must be present" @@ -471,7 +473,7 @@ impl TomlProfile { "dev" | "release" => {} _ => { if self.overrides.is_some() || self.build_override.is_some() { - failure::bail!( + bail!( "Profile overrides may only be specified for \ `dev` or `release` profile, not `{}`.", name @@ -491,21 +493,31 @@ impl TomlProfile { } _ => {} } + + if let Some(panic) = &self.panic { + if panic != "unwind" && panic != "abort" { + bail!( + "`panic` setting of `{}` is not a valid setting,\ + must be `unwind` or `abort`", + panic + ); + } + } Ok(()) } fn validate_override(&self) -> CargoResult<()> { if self.overrides.is_some() || self.build_override.is_some() { - failure::bail!("Profile overrides cannot be nested."); + bail!("Profile overrides cannot be nested."); } if self.panic.is_some() { - failure::bail!("`panic` may not be specified in a profile override.") + bail!("`panic` may not be specified in a profile override.") } if self.lto.is_some() { - failure::bail!("`lto` may not be specified in a profile override.") + bail!("`lto` may not be specified in a profile override.") } if self.rpath.is_some() { - failure::bail!("`rpath` may not be specified in a profile override.") + bail!("`rpath` may not be specified in a profile override.") } Ok(()) } @@ -830,7 +842,7 @@ impl TomlManifest { let package_name = project.name.trim(); if package_name.is_empty() { - failure::bail!("package name cannot be an empty string") + bail!("package name cannot be an empty string") } validate_package_name(package_name, "package name", "")?; @@ -951,7 +963,7 @@ impl TomlManifest { let name = dep.name_in_toml(); let prev = names_sources.insert(name.to_string(), dep.source_id()); if prev.is_some() && prev != Some(dep.source_id()) { - failure::bail!( + bail!( "Dependency '{}' has different source paths depending on the build \ target. Each dependency must have a single canonical source path \ irrespective of build target.", @@ -1006,7 +1018,7 @@ impl TomlManifest { (None, root) => WorkspaceConfig::Member { root: root.cloned(), }, - (Some(..), Some(..)) => failure::bail!( + (Some(..), Some(..)) => bail!( "cannot configure both `package.workspace` and \ `[workspace]`, only one can be specified" ), @@ -1082,43 +1094,43 @@ impl TomlManifest { config: &Config, ) -> CargoResult<(VirtualManifest, Vec)> { if me.project.is_some() { - failure::bail!("virtual manifests do not define [project]"); + bail!("virtual manifests do not define [project]"); } if me.package.is_some() { - failure::bail!("virtual manifests do not define [package]"); + bail!("virtual manifests do not define [package]"); } if me.lib.is_some() { - failure::bail!("virtual manifests do not specify [lib]"); + bail!("virtual manifests do not specify [lib]"); } if me.bin.is_some() { - failure::bail!("virtual manifests do not specify [[bin]]"); + bail!("virtual manifests do not specify [[bin]]"); } if me.example.is_some() { - failure::bail!("virtual manifests do not specify [[example]]"); + bail!("virtual manifests do not specify [[example]]"); } if me.test.is_some() { - failure::bail!("virtual manifests do not specify [[test]]"); + bail!("virtual manifests do not specify [[test]]"); } if me.bench.is_some() { - failure::bail!("virtual manifests do not specify [[bench]]"); + bail!("virtual manifests do not specify [[bench]]"); } if me.dependencies.is_some() { - failure::bail!("virtual manifests do not specify [dependencies]"); + bail!("virtual manifests do not specify [dependencies]"); } if me.dev_dependencies.is_some() || me.dev_dependencies2.is_some() { - failure::bail!("virtual manifests do not specify [dev-dependencies]"); + bail!("virtual manifests do not specify [dev-dependencies]"); } if me.build_dependencies.is_some() || me.build_dependencies2.is_some() { - failure::bail!("virtual manifests do not specify [build-dependencies]"); + bail!("virtual manifests do not specify [build-dependencies]"); } if me.features.is_some() { - failure::bail!("virtual manifests do not specify [features]"); + bail!("virtual manifests do not specify [features]"); } if me.target.is_some() { - failure::bail!("virtual manifests do not specify [target]"); + bail!("virtual manifests do not specify [target]"); } if me.badges.is_some() { - failure::bail!("virtual manifests do not specify [badges]"); + bail!("virtual manifests do not specify [badges]"); } let mut nested_paths = Vec::new(); @@ -1151,7 +1163,7 @@ impl TomlManifest { &config.exclude, )), None => { - failure::bail!("virtual manifests must be configured with [workspace]"); + bail!("virtual manifests must be configured with [workspace]"); } }; Ok(( @@ -1162,7 +1174,7 @@ impl TomlManifest { fn replace(&self, cx: &mut Context<'_, '_>) -> CargoResult> { if self.patch.is_some() && self.replace.is_some() { - failure::bail!("cannot specify both [replace] and [patch]"); + bail!("cannot specify both [replace] and [patch]"); } let mut replace = Vec::new(); for (spec, replacement) in self.replace.iter().flat_map(|x| x) { @@ -1182,7 +1194,7 @@ impl TomlManifest { TomlDependency::Simple(..) => true, }; if version_specified { - failure::bail!( + bail!( "replacements cannot specify a version \ requirement, but found one for `{}`", spec @@ -1331,12 +1343,12 @@ impl DetailedTomlDependency { self.registry.as_ref(), self.registry_index.as_ref(), ) { - (Some(_), _, Some(_), _) | (Some(_), _, _, Some(_)) => failure::bail!( + (Some(_), _, Some(_), _) | (Some(_), _, _, Some(_)) => bail!( "dependency ({}) specification is ambiguous. \ Only one of `git` or `registry` is allowed.", name_in_toml ), - (_, _, Some(_), Some(_)) => failure::bail!( + (_, _, Some(_), Some(_)) => bail!( "dependency ({}) specification is ambiguous. \ Only one of `registry` or `registry-index` is allowed.", name_in_toml diff --git a/tests/testsuite/profiles.rs b/tests/testsuite/profiles.rs index 97815c395b6..21510f3833e 100644 --- a/tests/testsuite/profiles.rs +++ b/tests/testsuite/profiles.rs @@ -376,3 +376,40 @@ fn profile_doc_deprecated() { .with_stderr_contains("[WARNING] profile `doc` is deprecated and has no effect") .run(); } + +#[test] +fn panic_unwind_does_not_build_twice() { + // Check for a bug where `lib` was built twice, once with panic set and + // once without. Since "unwind" is the default, they are the same and + // should only be built once. + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + + [profile.dev] + panic = "unwind" + "#, + ) + .file("src/lib.rs", "") + .file("src/main.rs", "fn main() {}") + .file("tests/t1.rs", "") + .build(); + + p.cargo("test -v --tests --no-run") + .with_stderr_unordered( + "\ +[COMPILING] foo [..] +[RUNNING] `rustc --crate-name foo src/lib.rs [..]--crate-type lib [..] +[RUNNING] `rustc --crate-name foo src/lib.rs [..] --test [..] +[RUNNING] `rustc --crate-name foo src/main.rs [..]--crate-type bin [..] +[RUNNING] `rustc --crate-name foo src/main.rs [..] --test [..] +[RUNNING] `rustc --crate-name t1 tests/t1.rs [..] +[FINISHED] [..] +", + ) + .run(); +}