From 5415a341a50aabafaf227c96e65f10b83aff641a Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sat, 5 May 2018 16:13:09 -0700 Subject: [PATCH] Add package spec support to profile overrides. Note: It errors out if multiple overrides match the same package. This could potentially merge the overrides together with a hierarchy similar to CSS specificity. However, that seems overkill for now. --- .../compiler/context/unit_dependencies.rs | 2 +- src/cargo/core/package_id_spec.rs | 22 ++- src/cargo/core/profiles.rs | 139 ++++++++++++++---- src/cargo/ops/cargo_clean.rs | 4 +- src/cargo/ops/cargo_compile.rs | 8 +- src/cargo/util/toml/mod.rs | 36 ++++- tests/testsuite/profile_overrides.rs | 134 ++++++++++++++++- 7 files changed, 304 insertions(+), 41 deletions(-) diff --git a/src/cargo/core/compiler/context/unit_dependencies.rs b/src/cargo/core/compiler/context/unit_dependencies.rs index ba742456a43..7f44ff85aab 100644 --- a/src/cargo/core/compiler/context/unit_dependencies.rs +++ b/src/cargo/core/compiler/context/unit_dependencies.rs @@ -329,7 +329,7 @@ fn new_unit<'a>( mode: CompileMode, ) -> Unit<'a> { let profile = bcx.profiles.get_profile( - &pkg.name(), + &pkg.package_id(), bcx.ws.is_member(pkg), profile_for, mode, diff --git a/src/cargo/core/package_id_spec.rs b/src/cargo/core/package_id_spec.rs index 798c746cc5b..43dd9683d6b 100644 --- a/src/cargo/core/package_id_spec.rs +++ b/src/cargo/core/package_id_spec.rs @@ -2,6 +2,7 @@ use std::collections::HashMap; use std::fmt; use semver::Version; +use serde::{de, ser}; use url::Url; use core::PackageId; @@ -17,7 +18,7 @@ use util::errors::{CargoResult, CargoResultExt}; /// If any of the optional fields are omitted, then the package id may be ambiguous, there may be /// more than one package/version/url combo that will match. However, often just the name is /// sufficient to uniquely define a package id. -#[derive(Clone, PartialEq, Eq, Debug)] +#[derive(Clone, PartialEq, Eq, Debug, Hash, Ord, PartialOrd)] pub struct PackageIdSpec { name: String, version: Option, @@ -253,6 +254,25 @@ impl fmt::Display for PackageIdSpec { } } +impl ser::Serialize for PackageIdSpec { + fn serialize(&self, s: S) -> Result + where + S: ser::Serializer, + { + self.to_string().serialize(s) + } +} + +impl<'de> de::Deserialize<'de> for PackageIdSpec { + fn deserialize(d: D) -> Result + where + D: de::Deserializer<'de>, + { + let string = String::deserialize(d)?; + PackageIdSpec::parse(&string).map_err(de::Error::custom) + } +} + #[cfg(test)] mod tests { use core::{PackageId, SourceId}; diff --git a/src/cargo/core/profiles.rs b/src/cargo/core/profiles.rs index 8ad40abd8f6..a83ed755c99 100644 --- a/src/cargo/core/profiles.rs +++ b/src/cargo/core/profiles.rs @@ -3,9 +3,9 @@ use std::{cmp, fmt, hash}; use core::compiler::CompileMode; use core::interning::InternedString; -use core::Shell; +use core::{PackageId, PackageIdSpec, PackageSet, Shell}; use util::lev_distance::lev_distance; -use util::toml::{StringOrBool, TomlProfile, U32OrBool}; +use util::toml::{ProfilePackageSpec, StringOrBool, TomlProfile, U32OrBool}; use util::CargoResult; /// Collection of all user profiles. @@ -55,7 +55,7 @@ impl Profiles { /// workspace. pub fn get_profile( &self, - pkg_name: &str, + pkg_id: &PackageId, is_member: bool, profile_for: ProfileFor, mode: CompileMode, @@ -86,7 +86,7 @@ impl Profiles { CompileMode::Bench => &self.bench, CompileMode::Doc { .. } => &self.doc, }; - let mut profile = maker.profile_for(pkg_name, is_member, profile_for); + let mut profile = maker.profile_for(Some(pkg_id), is_member, profile_for); // `panic` should not be set for tests/benches, or any of their // dependencies. if profile_for == ProfileFor::TestDependency || mode.is_any_test() { @@ -112,18 +112,14 @@ impl Profiles { /// select for the package that was actually built. pub fn base_profile(&self, release: bool) -> Profile { if release { - self.release.profile_for("", true, ProfileFor::Any) + self.release.profile_for(None, true, ProfileFor::Any) } else { - self.dev.profile_for("", true, ProfileFor::Any) + self.dev.profile_for(None, true, ProfileFor::Any) } } /// Used to check for overrides for non-existing packages. - pub fn validate_packages( - &self, - shell: &mut Shell, - packages: &HashSet<&str>, - ) -> CargoResult<()> { + pub fn validate_packages(&self, shell: &mut Shell, packages: &PackageSet) -> CargoResult<()> { self.dev.validate_packages(shell, packages)?; self.release.validate_packages(shell, packages)?; self.test.validate_packages(shell, packages)?; @@ -149,7 +145,12 @@ struct ProfileMaker { } impl ProfileMaker { - fn profile_for(&self, pkg_name: &str, is_member: bool, profile_for: ProfileFor) -> Profile { + fn profile_for( + &self, + pkg_id: Option<&PackageId>, + is_member: bool, + profile_for: ProfileFor, + ) -> Profile { let mut profile = self.default; if let Some(ref toml) = self.toml { merge_profile(&mut profile, toml); @@ -160,19 +161,38 @@ impl ProfileMaker { } if let Some(ref overrides) = toml.overrides { if !is_member { - if let Some(star) = overrides.get("*") { - merge_profile(&mut profile, star); + if let Some(all) = overrides.get(&ProfilePackageSpec::All) { + merge_profile(&mut profile, all); } } - if let Some(byname) = overrides.get(pkg_name) { - merge_profile(&mut profile, byname); + if let Some(pkg_id) = pkg_id { + let mut matches = overrides.iter().filter_map( + |(key, spec_profile)| match key { + &ProfilePackageSpec::All => None, + &ProfilePackageSpec::Spec(ref s) => if s.matches(pkg_id) { + Some(spec_profile) + } else { + None + }, + }, + ); + if let Some(spec_profile) = matches.next() { + merge_profile(&mut profile, spec_profile); + // `validate_packages` should ensure that there are + // no additional matches. + assert!( + matches.next().is_none(), + "package `{}` matched multiple profile overrides", + pkg_id + ); + } } } } profile } - fn validate_packages(&self, shell: &mut Shell, packages: &HashSet<&str>) -> CargoResult<()> { + fn validate_packages(&self, shell: &mut Shell, packages: &PackageSet) -> CargoResult<()> { let toml = match self.toml { Some(ref toml) => toml, None => return Ok(()), @@ -181,23 +201,88 @@ impl ProfileMaker { Some(ref overrides) => overrides, None => return Ok(()), }; - for key in overrides.keys().filter(|k| k.as_str() != "*") { - if !packages.contains(key.as_str()) { + // Verify that a package doesn't match multiple spec overrides. + let mut found = HashSet::new(); + for pkg_id in packages.package_ids() { + let matches: Vec<&PackageIdSpec> = overrides + .keys() + .filter_map(|key| match key { + &ProfilePackageSpec::All => None, + &ProfilePackageSpec::Spec(ref spec) => if spec.matches(pkg_id) { + Some(spec) + } else { + None + }, + }) + .collect(); + match matches.len() { + 0 => {} + 1 => { + found.insert(matches[0].clone()); + } + _ => { + let specs = matches + .iter() + .map(|spec| spec.to_string()) + .collect::>() + .join(", "); + bail!( + "multiple profile overrides in profile `{}` match package `{}`\n\ + found profile override specs: {}", + self.default.name, + pkg_id, + specs + ); + } + } + } + + // Verify every override matches at least one package. + let missing_specs = overrides.keys().filter_map(|key| { + if let &ProfilePackageSpec::Spec(ref spec) = key { + if !found.contains(spec) { + return Some(spec); + } + } + None + }); + for spec in missing_specs { + // See if there is an exact name match. + let name_matches: Vec = packages + .package_ids() + .filter_map(|pkg_id| { + if pkg_id.name().as_str() == spec.name() { + Some(pkg_id.to_string()) + } else { + None + } + }) + .collect(); + if name_matches.len() == 0 { let suggestion = packages - .iter() - .map(|p| (lev_distance(key, p), p)) + .package_ids() + .map(|p| (lev_distance(spec.name(), &p.name()), p.name())) .filter(|&(d, _)| d < 4) .min_by_key(|p| p.0) .map(|p| p.1); match suggestion { Some(p) => shell.warn(format!( - "package `{}` for profile override not found\n\nDid you mean `{}`?", - key, p + "profile override spec `{}` did not match any packages\n\n\ + Did you mean `{}`?", + spec, p ))?, - None => { - shell.warn(format!("package `{}` for profile override not found", key))? - } - }; + None => shell.warn(format!( + "profile override spec `{}` did not match any packages", + spec + ))?, + } + } else { + shell.warn(format!( + "version or URL in profile override spec `{}` does not \ + match any of the packages: {}", + spec, + name_matches.join(", ") + ))?; } } Ok(()) diff --git a/src/cargo/ops/cargo_clean.rs b/src/cargo/ops/cargo_clean.rs index 65e47fb9aeb..6e830970c15 100644 --- a/src/cargo/ops/cargo_clean.rs +++ b/src/cargo/ops/cargo_clean.rs @@ -60,7 +60,7 @@ pub fn clean(ws: &Workspace, opts: &CleanOptions) -> CargoResult<()> { for profile_for in ProfileFor::all_values() { let profile = if mode.is_run_custom_build() { profiles.get_profile_run_custom_build(&profiles.get_profile( - &pkg.name(), + pkg.package_id(), ws.is_member(pkg), *profile_for, CompileMode::Build, @@ -68,7 +68,7 @@ pub fn clean(ws: &Workspace, opts: &CleanOptions) -> CargoResult<()> { )) } else { profiles.get_profile( - &pkg.name(), + pkg.package_id(), ws.is_member(pkg), *profile_for, *mode, diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs index 1d93885958f..0703b0873a5 100644 --- a/src/cargo/ops/cargo_compile.rs +++ b/src/cargo/ops/cargo_compile.rs @@ -254,11 +254,7 @@ pub fn compile_ws<'a>( } let profiles = ws.profiles(); - let package_names = packages - .package_ids() - .map(|pid| pid.name().as_str()) - .collect::>(); - profiles.validate_packages(&mut config.shell(), &package_names)?; + profiles.validate_packages(&mut config.shell(), &packages)?; let mut extra_compiler_args = None; @@ -494,7 +490,7 @@ fn generate_targets<'a>( default_arch_kind }; let profile = profiles.get_profile( - &pkg.name(), + pkg.package_id(), ws.is_member(pkg), profile_for, target_mode, diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 667b2ea0902..fd664572ecf 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -380,10 +380,44 @@ pub struct TomlProfile { pub panic: Option, pub overflow_checks: Option, pub incremental: Option, - pub overrides: Option>, + pub overrides: Option>, pub build_override: Option>, } +#[derive(Clone, Debug, PartialEq, Eq, Ord, PartialOrd, Hash)] +pub enum ProfilePackageSpec { + Spec(PackageIdSpec), + All, +} + +impl ser::Serialize for ProfilePackageSpec { + fn serialize(&self, s: S) -> Result + where + S: ser::Serializer, + { + match *self { + ProfilePackageSpec::Spec(ref spec) => spec.serialize(s), + ProfilePackageSpec::All => "*".serialize(s), + } + } +} + +impl<'de> de::Deserialize<'de> for ProfilePackageSpec { + fn deserialize(d: D) -> Result + where + D: de::Deserializer<'de>, + { + let string = String::deserialize(d)?; + if string == "*" { + Ok(ProfilePackageSpec::All) + } else { + PackageIdSpec::parse(&string) + .map_err(de::Error::custom) + .map(|s| ProfilePackageSpec::Spec(s)) + } + } +} + impl TomlProfile { fn validate( &self, diff --git a/tests/testsuite/profile_overrides.rs b/tests/testsuite/profile_overrides.rs index 59a61616dd4..eae75847cde 100644 --- a/tests/testsuite/profile_overrides.rs +++ b/tests/testsuite/profile_overrides.rs @@ -106,7 +106,7 @@ fn profile_override_basic() { } #[test] -fn profile_override_bad_name() { +fn profile_override_warnings() { let p = project("foo") .file( "Cargo.toml", @@ -125,6 +125,9 @@ fn profile_override_bad_name() { [profile.dev.overrides.no-suggestion] opt-level = 3 + + [profile.dev.overrides."bar:1.2.3"] + opt-level = 3 "#, ) .file("src/lib.rs", "") @@ -136,10 +139,11 @@ fn profile_override_bad_name() { p.cargo("build").masquerade_as_nightly_cargo(), execs().with_status(0).with_stderr_contains( "\ -[WARNING] package `bart` for profile override not found +[WARNING] version or URL in profile override spec `bar:1.2.3` does not match any of the packages: bar v0.5.0 ([..]) +[WARNING] profile override spec `bart` did not match any packages Did you mean `bar`? -[WARNING] package `no-suggestion` for profile override not found +[WARNING] profile override spec `no-suggestion` did not match any packages [COMPILING] [..] ", ), @@ -343,3 +347,127 @@ fn profile_override_hierarchy() { ), ); } + +#[test] +fn profile_override_spec_multiple() { + let p = project("foo") + .file( + "Cargo.toml", + r#" + cargo-features = ["profile-overrides"] + + [package] + name = "foo" + version = "0.0.1" + + [dependencies] + bar = { path = "bar" } + + [profile.dev.overrides.bar] + opt-level = 3 + + [profile.dev.overrides."bar:0.5.0"] + opt-level = 3 + "#, + ) + .file("src/lib.rs", "") + .file("bar/Cargo.toml", &basic_lib_manifest("bar")) + .file("bar/src/lib.rs", "") + .build(); + + assert_that( + p.cargo("build -v").masquerade_as_nightly_cargo(), + execs().with_status(101).with_stderr_contains( + "\ +[ERROR] multiple profile overrides in profile `dev` match package `bar v0.5.0 ([..])` +found profile override specs: bar, bar:0.5.0", + ), + ); +} + +#[test] +fn profile_override_spec() { + let p = project("foo") + .file( + "Cargo.toml", + r#" + cargo-features = ["profile-overrides"] + + [workspace] + members = ["m1", "m2"] + + [profile.dev.overrides."dep:1.0.0"] + codegen-units = 1 + + [profile.dev.overrides."dep:2.0.0"] + codegen-units = 2 + "#) + + // m1 + .file("m1/Cargo.toml", + r#" + [package] + name = "m1" + version = "0.0.1" + + [dependencies] + dep = { path = "../../dep1" } + "#) + .file("m1/src/lib.rs", + r#" + extern crate dep; + "#) + + // m2 + .file("m2/Cargo.toml", + r#" + [package] + name = "m2" + version = "0.0.1" + + [dependencies] + dep = {path = "../../dep2" } + "#) + .file("m2/src/lib.rs", + r#" + extern crate dep; + "#) + + .build(); + + project("dep1") + .file( + "Cargo.toml", + r#" + [package] + name = "dep" + version = "1.0.0" + "#, + ) + .file("src/lib.rs", "") + .build(); + + project("dep2") + .file( + "Cargo.toml", + r#" + [package] + name = "dep" + version = "2.0.0" + "#, + ) + .file("src/lib.rs", "") + .build(); + + assert_that( + p.cargo("build -v").masquerade_as_nightly_cargo(), + execs() + .with_status(0) + .with_stderr_contains( + "[RUNNING] `rustc [..]dep1[/]src[/]lib.rs [..] -C codegen-units=1 [..]", + ) + .with_stderr_contains( + "[RUNNING] `rustc [..]dep2[/]src[/]lib.rs [..] -C codegen-units=2 [..]", + ), + ); +}