Skip to content

Commit

Permalink
Auto merge of #7750 - ehuss:named-config-profiles, r=alexcrichton
Browse files Browse the repository at this point in the history
Add named config profiles.

This adds support for named config profiles. Previously, only `dev` and `release` were allowed in config files, it now supports all profile names. I think it would be strange to have arbitrarily named profiles in `Cargo.toml`, but not allow them in config. This is a deviation from the RFC, but RFC 2282 was written before named profiles which I think changes the landscape.

This diff is a little large due to some refactoring to make it work well. Overview of the changes:
- Removed `ProfileKind` and only use an `InternedString` to track the name of the profile. I didn't feel like the enum carried its cognitive weight, and it seems to simplify some things.
- `Profiles` is no longer stored in the manifest. There was no need to do a bunch of processing for each manifest. `Manifest` now only retains the low-level `TomlProfiles`. A single `Profiles` now lives in `BuildContext`.
- The profile name requested by the user is no longer passed around. It is given to `Profiles::new` and retained inside `Profiles`.
- `Profiles::get_profile` no longer follows the priority stack and inheritance each time a profile is requested. Instead, the profile is computed once (in `Profile::new`) and merged into a single profile. This simplifies getting a profile, and makes it easier to deal with getting the config values in one place.
- I switched profile names to be `InternedString` instead of `String`. There's not a strong reason to do this, other than it seemed a little strange to be creating lots of `String`s.
    - I also added `PartialEq<str>` for `InternedString`. It has come up a few times in the past, and it seems useful. I'm not sure if it was excluded intentionally?
- The validation that the profile exists is now done in one place (`Profiles::new`).
- I removed the back-compatibility for the `overrides` key (which was renamed to `package` back in October).

Notes:
- Some of the error messages aren't as good as before, because they don't tell you where the error is located (`Cargo.toml` or `.cargo/config`). This is because the location data is lost by the time validation is done. Hopefully it will be obvious from the profile name and error message. I tried to improve error messages wherever I could.
- There are more calls to `clone()` than I would like, but they are kinda hard to avoid. Should be fewer than before.
- I noticed a bug with `-Zpanic-abort-tests` not supporting named profiles. I'll fix that separately.
- I think this fixes some bugs where package overrides in config weren't merging properly with package overrides defined in `Cargo.toml`.
  • Loading branch information
bors committed Jan 13, 2020
2 parents 56a5503 + dafacbb commit ad3dbe1
Show file tree
Hide file tree
Showing 27 changed files with 749 additions and 624 deletions.
8 changes: 2 additions & 6 deletions src/bin/cargo/commands/bench.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
use crate::command_prelude::*;

use cargo::ops::{self, TestOptions};

pub fn cli() -> App {
Expand Down Expand Up @@ -80,11 +79,8 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
ProfileChecking::Checked,
)?;

compile_opts.build_config.profile_kind = args.get_profile_kind(
config,
ProfileKind::Custom("bench".to_owned()),
ProfileChecking::Checked,
)?;
compile_opts.build_config.requested_profile =
args.get_profile_name(config, "bench", ProfileChecking::Checked)?;

let ops = TestOptions {
no_run: args.is_present("no-run"),
Expand Down
2 changes: 1 addition & 1 deletion src/bin/cargo/commands/clean.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
config,
spec: values(args, "package"),
target: args.target(),
profile_kind: args.get_profile_kind(config, ProfileKind::Dev, ProfileChecking::Checked)?,
requested_profile: args.get_profile_name(config, "dev", ProfileChecking::Checked)?,
profile_specified: args.is_present("profile") || args.is_present("release"),
doc: args.is_present("doc"),
};
Expand Down
4 changes: 2 additions & 2 deletions src/bin/cargo/commands/install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,8 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
ProfileChecking::Checked,
)?;

compile_opts.build_config.profile_kind =
args.get_profile_kind(config, ProfileKind::Release, ProfileChecking::Checked)?;
compile_opts.build_config.requested_profile =
args.get_profile_name(config, "release", ProfileChecking::Checked)?;

let krates = args
.values_of("crate")
Expand Down
7 changes: 2 additions & 5 deletions src/bin/cargo/commands/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -108,11 +108,8 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
ProfileChecking::Checked,
)?;

compile_opts.build_config.profile_kind = args.get_profile_kind(
config,
ProfileKind::Custom("test".to_owned()),
ProfileChecking::Checked,
)?;
compile_opts.build_config.requested_profile =
args.get_profile_name(config, "test", ProfileChecking::Checked)?;

// `TESTNAME` is actually an argument of the test binary, but it's
// important, so we explicitly mention it and reconfigure.
Expand Down
32 changes: 5 additions & 27 deletions src/cargo/core/compiler/build_config.rs
Original file line number Diff line number Diff line change
@@ -1,27 +1,9 @@
use std::cell::RefCell;

use serde::ser;

use crate::core::compiler::{CompileKind, CompileTarget};
use crate::core::interning::InternedString;
use crate::util::ProcessBuilder;
use crate::util::{CargoResult, Config, RustfixDiagnosticServer};

#[derive(Debug, Clone)]
pub enum ProfileKind {
Dev,
Release,
Custom(String),
}

impl ProfileKind {
pub fn name(&self) -> &str {
match self {
ProfileKind::Dev => "dev",
ProfileKind::Release => "release",
ProfileKind::Custom(name) => name,
}
}
}
use serde::ser;
use std::cell::RefCell;

/// Configuration information for a rustc build.
#[derive(Debug)]
Expand All @@ -31,7 +13,7 @@ pub struct BuildConfig {
/// Number of rustc jobs to run in parallel.
pub jobs: u32,
/// Build profile
pub profile_kind: ProfileKind,
pub requested_profile: InternedString,
/// The mode we are compiling in.
pub mode: CompileMode,
/// `true` to print stdout in JSON format (for machine reading).
Expand Down Expand Up @@ -92,7 +74,7 @@ impl BuildConfig {
Ok(BuildConfig {
requested_kind,
jobs,
profile_kind: ProfileKind::Dev,
requested_profile: InternedString::new("dev"),
mode,
message_format: MessageFormat::Human,
force_rebuild: false,
Expand All @@ -111,10 +93,6 @@ impl BuildConfig {
}
}

pub fn profile_name(&self) -> &str {
self.profile_kind.name()
}

pub fn test(&self) -> bool {
self.mode == CompileMode::Test || self.mode == CompileMode::Bench
}
Expand Down
4 changes: 2 additions & 2 deletions src/cargo/core/compiler/build_context/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ pub struct BuildContext<'a, 'cfg> {
pub ws: &'a Workspace<'cfg>,
/// The cargo configuration.
pub config: &'cfg Config,
pub profiles: &'a Profiles,
pub profiles: Profiles,
pub build_config: &'a BuildConfig,
/// Extra compiler args for either `rustc` or `rustdoc`.
pub extra_compiler_args: HashMap<Unit<'a>, Vec<String>>,
Expand Down Expand Up @@ -58,7 +58,7 @@ impl<'a, 'cfg> BuildContext<'a, 'cfg> {
packages: &'a PackageSet<'cfg>,
config: &'cfg Config,
build_config: &'a BuildConfig,
profiles: &'a Profiles,
profiles: Profiles,
units: &'a UnitInterner<'a>,
extra_compiler_args: HashMap<Unit<'a>, Vec<String>>,
) -> CargoResult<BuildContext<'a, 'cfg>> {
Expand Down
3 changes: 1 addition & 2 deletions src/cargo/core/compiler/context/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -281,8 +281,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
export_dir: Option<PathBuf>,
units: &[Unit<'a>],
) -> CargoResult<()> {
let profile_kind = &self.bcx.build_config.profile_kind;
let dest = self.bcx.profiles.get_dir_name(profile_kind);
let dest = self.bcx.profiles.get_dir_name();
let host_layout = Layout::new(self.bcx.ws, None, &dest)?;
let mut targets = HashMap::new();
if let CompileKind::Target(target) = self.bcx.build_config.requested_kind {
Expand Down
9 changes: 3 additions & 6 deletions src/cargo/core/compiler/job_queue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ use super::job::{
};
use super::timings::Timings;
use super::{BuildContext, BuildPlan, CompileMode, Context, Unit};
use crate::core::compiler::ProfileKind;
use crate::core::{PackageId, TargetKind};
use crate::handle_error;
use crate::util;
Expand All @@ -44,7 +43,6 @@ pub struct JobQueue<'a, 'cfg> {
progress: Progress<'cfg>,
next_id: u32,
timings: Timings<'a, 'cfg>,
profile_kind: ProfileKind,
}

pub struct JobState<'a> {
Expand Down Expand Up @@ -148,7 +146,6 @@ impl<'a, 'cfg> JobQueue<'a, 'cfg> {
progress,
next_id: 0,
timings,
profile_kind: bcx.build_config.profile_kind.clone(),
}
}

Expand Down Expand Up @@ -415,15 +412,15 @@ impl<'a, 'cfg> JobQueue<'a, 'cfg> {
}
self.progress.clear();

let build_type = self.profile_kind.name();
let profile_name = cx.bcx.build_config.requested_profile;
// NOTE: this may be a bit inaccurate, since this may not display the
// profile for what was actually built. Profile overrides can change
// these settings, and in some cases different targets are built with
// different profiles. To be accurate, it would need to collect a
// list of Units built, and maybe display a list of the different
// profiles used. However, to keep it simple and compatible with old
// behavior, we just display what the base profile is.
let profile = cx.bcx.profiles.base_profile(&self.profile_kind)?;
let profile = cx.bcx.profiles.base_profile();
let mut opt_type = String::from(if profile.opt_level.as_str() == "0" {
"unoptimized"
} else {
Expand All @@ -440,7 +437,7 @@ impl<'a, 'cfg> JobQueue<'a, 'cfg> {
} else if self.queue.is_empty() && queue.is_empty() {
let message = format!(
"{} [{}] target(s) in {}",
build_type, opt_type, time_elapsed
profile_name, opt_type, time_elapsed
);
if !cx.bcx.build_config.build_plan {
cx.bcx.config.shell().status("Finished", message)?;
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/core/compiler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ use anyhow::Error;
use lazycell::LazyCell;
use log::debug;

pub use self::build_config::{BuildConfig, CompileMode, MessageFormat, ProfileKind};
pub use self::build_config::{BuildConfig, CompileMode, MessageFormat};
pub use self::build_context::{BuildContext, FileFlavor, TargetInfo};
use self::build_plan::BuildPlan;
pub use self::compilation::{Compilation, Doctest};
Expand Down
4 changes: 1 addition & 3 deletions src/cargo/core/compiler/standard_lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,7 @@ pub fn resolve_std<'cfg>(
/*replace*/ Vec::new(),
patch,
ws_config,
// Profiles are not used here, but we need something to pass in.
ws.profiles().clone(),
/*profiles*/ None,
crate::core::Features::default(),
);

Expand Down Expand Up @@ -139,7 +138,6 @@ pub fn generate_std_roots<'a>(
/*is_member*/ false,
unit_for,
mode,
bcx.build_config.profile_kind.clone(),
);
let features = std_resolve.features_sorted(pkg.package_id());
Ok(bcx.units.intern(
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/core/compiler/timings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ impl<'a, 'cfg> Timings<'a, 'cfg> {
})
.collect();
let start_str = humantime::format_rfc3339_seconds(SystemTime::now()).to_string();
let profile = bcx.build_config.profile_kind.name().to_owned();
let profile = bcx.build_config.requested_profile.to_string();

Timings {
config: bcx.config,
Expand Down
1 change: 0 additions & 1 deletion src/cargo/core/compiler/unit_dependencies.rs
Original file line number Diff line number Diff line change
Expand Up @@ -546,7 +546,6 @@ fn new_unit_dep<'a>(
state.bcx.ws.is_member(pkg),
unit_for,
mode,
state.bcx.build_config.profile_kind.clone(),
);
new_unit_dep_with_profile(state, parent, pkg, target, unit_for, kind, mode, profile)
}
Expand Down
12 changes: 12 additions & 0 deletions src/cargo/core/interning.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,18 @@ impl PartialEq for InternedString {
}
}

impl PartialEq<str> for InternedString {
fn eq(&self, other: &str) -> bool {
*self == other
}
}

impl<'a> PartialEq<&'a str> for InternedString {
fn eq(&self, other: &&str) -> bool {
**self == **other
}
}

impl Eq for InternedString {}

impl InternedString {
Expand Down
19 changes: 9 additions & 10 deletions src/cargo/core/manifest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,10 @@ use serde::Serialize;
use url::Url;

use crate::core::interning::InternedString;
use crate::core::profiles::Profiles;
use crate::core::{Dependency, PackageId, PackageIdSpec, SourceId, Summary};
use crate::core::{Edition, Feature, Features, WorkspaceConfig};
use crate::util::errors::*;
use crate::util::toml::TomlManifest;
use crate::util::toml::{TomlManifest, TomlProfiles};
use crate::util::{short_hash, Config, Filesystem};

pub enum EitherManifest {
Expand All @@ -33,7 +32,7 @@ pub struct Manifest {
include: Vec<String>,
metadata: ManifestMetadata,
custom_metadata: Option<toml::Value>,
profiles: Profiles,
profiles: Option<TomlProfiles>,
publish: Option<Vec<String>>,
publish_lockfile: bool,
replace: Vec<(PackageIdSpec, Dependency)>,
Expand Down Expand Up @@ -64,7 +63,7 @@ pub struct VirtualManifest {
replace: Vec<(PackageIdSpec, Dependency)>,
patch: HashMap<Url, Vec<Dependency>>,
workspace: WorkspaceConfig,
profiles: Profiles,
profiles: Option<TomlProfiles>,
warnings: Warnings,
features: Features,
}
Expand Down Expand Up @@ -399,7 +398,7 @@ impl Manifest {
links: Option<String>,
metadata: ManifestMetadata,
custom_metadata: Option<toml::Value>,
profiles: Profiles,
profiles: Option<TomlProfiles>,
publish: Option<Vec<String>>,
publish_lockfile: bool,
replace: Vec<(PackageIdSpec, Dependency)>,
Expand Down Expand Up @@ -475,8 +474,8 @@ impl Manifest {
pub fn warnings(&self) -> &Warnings {
&self.warnings
}
pub fn profiles(&self) -> &Profiles {
&self.profiles
pub fn profiles(&self) -> Option<&TomlProfiles> {
self.profiles.as_ref()
}
pub fn publish(&self) -> &Option<Vec<String>> {
&self.publish
Expand Down Expand Up @@ -563,7 +562,7 @@ impl VirtualManifest {
replace: Vec<(PackageIdSpec, Dependency)>,
patch: HashMap<Url, Vec<Dependency>>,
workspace: WorkspaceConfig,
profiles: Profiles,
profiles: Option<TomlProfiles>,
features: Features,
) -> VirtualManifest {
VirtualManifest {
Expand All @@ -588,8 +587,8 @@ impl VirtualManifest {
&self.workspace
}

pub fn profiles(&self) -> &Profiles {
&self.profiles
pub fn profiles(&self) -> Option<&TomlProfiles> {
self.profiles.as_ref()
}

pub fn warnings_mut(&mut self) -> &mut Warnings {
Expand Down
Loading

0 comments on commit ad3dbe1

Please sign in to comment.