Skip to content

Commit c24a097

Browse files
committed
Auto merge of #5384 - ehuss:profile-override, r=matklad
Profile Overrides (RFC #2282 Part 1) Profile Overrides (RFC #2282 Part 1) WIP: Putting this up before I dig into writing tests, but should be mostly complete. I also have a variety of questions below. This implements the ability to override profiles for dependencies and build scripts. This includes a general rework of how profiles work internally. Closes #5298. Profile overrides are available with `profile-overrides` set in `cargo-features` in the manifest. Part 2 is to implement profiles in config files (to be in a separate PR). General overview of changes: - `Profiles` moved to `core/profiles.rs`. All profile selection is centralized there. - Removed Profile flags `test`, `doc`, `run_custom_build`, and `check`. - Removed `Profile` from `Unit` and replaced it with two enums: `CompileMode` and `ProfileFor`. This is the minimum information needed to compute profiles at a later stage. - Also removed `rustc_args`/`rustdoc_args` from `Profile` and place them in `Context`. This is currently not very elegant because it is a special case, but it works. An alternate solution I considered was to leave them in the `Profile` and add a special uber-override layer. Let me know if you think it should change. - Did some general cleanup in `generate_targets`. ## Misc Fixes - `cargo check` now honors the `--release` flag. Fixes #5218. - `cargo build --test` will set `panic` correctly for dependences. Fixes #5369. - `cargo check --tests` will no longer include bins twice (once as a normal check, once as a `--test` check). It only does `--test` check now. - Similarly, `cargo check --test name` no longer implicitly checks bins. - Examples are no longer considered a "test". (See #5397). Consequences: - `cargo test` will continue to build examples as a regular build (no change). - `cargo test --tests` will no longer build examples at all. - `cargo test --all-targets` will no longer build examples as tests, but instead build them as a regular build (now matches `cargo test` behavior). - `cargo check --all-targets` will no longer check examples twice (once as normal, once as `--test`). It now only checks it once as a normal target. ## Questions - Thumbs up/down on the general approach? - The method to detect if a package is a member of a workspace should probably be redone. I'm uncertain of the best approach. Maybe `Workspace.members` could be a set? - `Hash` and `PartialEq` are implemented manually for `Profile` only to avoid matching on the `name` field. The `name` field is only there for debug purposes. Is it worth it to keep `name`? Maybe useful for future use (like #4140)? - I'm unhappy with the `Finished` line summary that displays `[unoptimized + debuginfo]`. It doesn't actually show what was compiled. Currently it just picks the base "dev" or "release" profile. I'm not sure what a good solution is (to be accurate it would need to potentially display a list of different options). Is it ok? (See also #4140 for the wrong profile name being printed.) - Build-dependencies use different profiles based on whether or not `--release` flag is given. This means that if you want build-dependencies to always use a specific set of settings, you have to specify both `[profile.dev.build_override]` and `[profile.release.build_override]`. Is that reasonable (for now)? I've noticed some issues (like #1774, #2234, #2424) discussing having more control over how build-dependencies are handled. - `build --bench xxx` or `--benches` builds dependencies with dev profile, which may be surprising. `--release` does the correct thing. Perhaps print a warning when using `cargo build` that builds benchmark deps in dev mode? - Should it warn/error if you have an override for a package that does not exist? - Should it warn/error if you attempt to set `panic` on the `test` or `bench` profile? ## TODO - I have a long list of tests to add. - Address a few "TODO" comments left behind.
2 parents 3b14988 + e82e9c1 commit c24a097

29 files changed

+2703
-1028
lines changed

src/cargo/core/compiler/context/compilation_files.rs

+15-8
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,8 @@ use std::sync::Arc;
77

88
use lazycell::LazyCell;
99

10-
use core::{TargetKind, Workspace};
1110
use super::{Context, FileFlavor, Kind, Layout, Unit};
11+
use core::{TargetKind, Workspace};
1212
use util::{self, CargoResult};
1313

1414
#[derive(Clone, Hash, Eq, PartialEq, Ord, PartialOrd)]
@@ -97,7 +97,7 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> {
9797
/// Returns the appropriate output directory for the specified package and
9898
/// target.
9999
pub fn out_dir(&self, unit: &Unit<'a>) -> PathBuf {
100-
if unit.profile.doc {
100+
if unit.mode.is_doc() {
101101
self.layout(unit.kind).root().parent().unwrap().join("doc")
102102
} else if unit.target.is_custom_build() {
103103
self.build_script_dir(unit)
@@ -148,15 +148,15 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> {
148148
/// Returns the appropriate directory layout for either a plugin or not.
149149
pub fn build_script_dir(&self, unit: &Unit<'a>) -> PathBuf {
150150
assert!(unit.target.is_custom_build());
151-
assert!(!unit.profile.run_custom_build);
151+
assert!(!unit.mode.is_run_custom_build());
152152
let dir = self.pkg_dir(unit);
153153
self.layout(Kind::Host).build().join(dir)
154154
}
155155

156156
/// Returns the appropriate directory layout for either a plugin or not.
157157
pub fn build_script_out_dir(&self, unit: &Unit<'a>) -> PathBuf {
158158
assert!(unit.target.is_custom_build());
159-
assert!(unit.profile.run_custom_build);
159+
assert!(unit.mode.is_run_custom_build());
160160
let dir = self.pkg_dir(unit);
161161
self.layout(unit.kind).build().join(dir).join("out")
162162
}
@@ -211,7 +211,7 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> {
211211
} else {
212212
Some((
213213
out_dir.parent().unwrap().to_owned(),
214-
if unit.profile.test {
214+
if unit.mode.is_any_test() {
215215
file_stem
216216
} else {
217217
bin_stem
@@ -244,7 +244,10 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> {
244244
let mut ret = Vec::new();
245245
let mut unsupported = Vec::new();
246246
{
247-
if unit.profile.check {
247+
if unit.mode.is_check() {
248+
// This is not quite correct for non-lib targets. rustc
249+
// currently does not emit rmeta files, so there is nothing to
250+
// check for! See #3624.
248251
let path = out_dir.join(format!("lib{}.rmeta", file_stem));
249252
let hardlink = link_stem
250253
.clone()
@@ -296,7 +299,7 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> {
296299
| TargetKind::Test => {
297300
add("bin", FileFlavor::Normal)?;
298301
}
299-
TargetKind::Lib(..) | TargetKind::ExampleLib(..) if unit.profile.test => {
302+
TargetKind::Lib(..) | TargetKind::ExampleLib(..) if unit.mode.is_any_test() => {
300303
add("bin", FileFlavor::Normal)?;
301304
}
302305
TargetKind::ExampleLib(ref kinds) | TargetKind::Lib(ref kinds) => {
@@ -378,7 +381,7 @@ fn compute_metadata<'a, 'cfg>(
378381
// just here for rustbuild. We need a more principled method
379382
// doing this eventually.
380383
let __cargo_default_lib_metadata = env::var("__CARGO_DEFAULT_LIB_METADATA");
381-
if !(unit.profile.test || unit.profile.check)
384+
if !(unit.mode.is_any_test() || unit.mode.is_check())
382385
&& (unit.target.is_dylib() || unit.target.is_cdylib()
383386
|| (unit.target.is_bin() && cx.build_config.target_triple().starts_with("wasm32-")))
384387
&& unit.pkg.package_id().source_id().is_path()
@@ -423,6 +426,10 @@ fn compute_metadata<'a, 'cfg>(
423426
// panic=abort and panic=unwind artifacts, additionally with various
424427
// settings like debuginfo and whatnot.
425428
unit.profile.hash(&mut hasher);
429+
unit.mode.hash(&mut hasher);
430+
if let Some(ref args) = cx.extra_args_for(unit) {
431+
args.hash(&mut hasher);
432+
}
426433

427434
// Artifacts compiled for the host should have a different metadata
428435
// piece than those compiled for the target, so make sure we throw in

src/cargo/core/compiler/context/mod.rs

+40-44
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
#![allow(deprecated)]
2-
32
use std::collections::{HashMap, HashSet};
43
use std::env;
54
use std::path::{Path, PathBuf};
@@ -8,25 +7,27 @@ use std::sync::Arc;
87

98
use jobserver::Client;
109

11-
use core::{Package, PackageId, PackageSet, Profile, Resolve, Target};
12-
use core::{Dependency, Profiles, Workspace};
13-
use util::{internal, profile, Cfg, CfgExpr, Config};
10+
use core::profiles::{Profile, Profiles};
11+
use core::{Dependency, Workspace};
12+
use core::{Package, PackageId, PackageSet, Resolve, Target};
13+
use ops::CompileMode;
1414
use util::errors::{CargoResult, CargoResultExt};
15+
use util::{internal, profile, Cfg, CfgExpr, Config};
1516

16-
use super::TargetConfig;
1717
use super::custom_build::{self, BuildDeps, BuildScripts, BuildState};
1818
use super::fingerprint::Fingerprint;
1919
use super::job_queue::JobQueue;
2020
use super::layout::Layout;
2121
use super::links::Links;
22+
use super::TargetConfig;
2223
use super::{BuildConfig, Compilation, Executor, Kind};
2324

2425
mod unit_dependencies;
2526
use self::unit_dependencies::build_unit_dependencies;
2627

2728
mod compilation_files;
28-
use self::compilation_files::{CompilationFiles, OutputFile};
2929
pub use self::compilation_files::Metadata;
30+
use self::compilation_files::{CompilationFiles, OutputFile};
3031

3132
mod target_info;
3233
pub use self::target_info::{FileFlavor, TargetInfo};
@@ -45,7 +46,7 @@ pub use self::target_info::{FileFlavor, TargetInfo};
4546
/// example, it needs to know the target architecture (OS, chip arch etc.) and it needs to know
4647
/// whether you want a debug or release build. There is enough information in this struct to figure
4748
/// all that out.
48-
#[derive(Clone, Copy, Eq, PartialEq, Hash)]
49+
#[derive(Clone, Copy, Eq, PartialEq, Hash, Debug)]
4950
pub struct Unit<'a> {
5051
/// Information about available targets, which files to include/exclude, etc. Basically stuff in
5152
/// `Cargo.toml`.
@@ -55,15 +56,18 @@ pub struct Unit<'a> {
5556
/// build.
5657
pub target: &'a Target,
5758
/// The profile contains information about *how* the build should be run, including debug
58-
/// level, extra args to pass to rustc, etc.
59-
pub profile: &'a Profile,
59+
/// level, etc.
60+
pub profile: Profile,
6061
/// Whether this compilation unit is for the host or target architecture.
6162
///
6263
/// For example, when
6364
/// cross compiling and using a custom build script, the build script needs to be compiled for
6465
/// the host architecture so the host rustc can use it (when compiling to the target
6566
/// architecture).
6667
pub kind: Kind,
68+
/// The "mode" this unit is being compiled for. See `CompileMode` for
69+
/// more details.
70+
pub mode: CompileMode,
6771
}
6872

6973
/// The build context, containing all information about a build task
@@ -87,10 +91,16 @@ pub struct Context<'a, 'cfg: 'a> {
8791
pub links: Links<'a>,
8892
pub used_in_plugin: HashSet<Unit<'a>>,
8993
pub jobserver: Client,
94+
pub profiles: &'a Profiles,
95+
/// This is a workaround to carry the extra compiler args for either
96+
/// `rustc` or `rustdoc` given on the command-line for the commands `cargo
97+
/// rustc` and `cargo rustdoc`. These commands only support one target,
98+
/// but we don't want the args passed to any dependencies, so we include
99+
/// the `Unit` corresponding to the top-level target.
100+
extra_compiler_args: Option<(Unit<'a>, Vec<String>)>,
90101

91102
target_info: TargetInfo,
92103
host_info: TargetInfo,
93-
profiles: &'a Profiles,
94104
incremental_env: Option<bool>,
95105

96106
unit_dependencies: HashMap<Unit<'a>, Vec<Unit<'a>>>,
@@ -105,6 +115,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
105115
config: &'cfg Config,
106116
build_config: BuildConfig,
107117
profiles: &'a Profiles,
118+
extra_compiler_args: Option<(Unit<'a>, Vec<String>)>,
108119
) -> CargoResult<Context<'a, 'cfg>> {
109120
let incremental_env = match env::var("CARGO_INCREMENTAL") {
110121
Ok(v) => Some(v == "1"),
@@ -156,6 +167,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
156167

157168
unit_dependencies: HashMap::new(),
158169
files: None,
170+
extra_compiler_args,
159171
};
160172

161173
cx.compilation.host_dylib_path = cx.host_info.sysroot_libdir.clone();
@@ -174,8 +186,8 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
174186
let mut queue = JobQueue::new(&self);
175187
self.prepare_units(export_dir, units)?;
176188
self.prepare()?;
177-
self.build_used_in_plugin_map(&units)?;
178-
custom_build::build_map(&mut self, &units)?;
189+
self.build_used_in_plugin_map(units)?;
190+
custom_build::build_map(&mut self, units)?;
179191

180192
for unit in units.iter() {
181193
// Build up a list of pending jobs, each of which represent
@@ -200,7 +212,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
200212
None => &output.path,
201213
};
202214

203-
if unit.profile.test {
215+
if unit.mode.is_any_test() && !unit.mode.is_check() {
204216
self.compilation.tests.push((
205217
unit.pkg.clone(),
206218
unit.target.kind().clone(),
@@ -224,7 +236,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
224236
continue;
225237
}
226238

227-
if dep.profile.run_custom_build {
239+
if dep.mode.is_run_custom_build() {
228240
let out_dir = self.files().build_script_out_dir(dep).display().to_string();
229241
self.compilation
230242
.extra_env
@@ -236,7 +248,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
236248
if !dep.target.is_lib() {
237249
continue;
238250
}
239-
if dep.profile.doc {
251+
if dep.mode.is_doc() {
240252
continue;
241253
}
242254

@@ -313,16 +325,10 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
313325
None => None,
314326
};
315327

316-
let deps = build_unit_dependencies(units, &self)?;
328+
let deps = build_unit_dependencies(units, self)?;
317329
self.unit_dependencies = deps;
318-
let files = CompilationFiles::new(
319-
units,
320-
host_layout,
321-
target_layout,
322-
export_dir,
323-
self.ws,
324-
&self,
325-
);
330+
let files =
331+
CompilationFiles::new(units, host_layout, target_layout, export_dir, self.ws, self);
326332
self.files = Some(files);
327333
Ok(())
328334
}
@@ -414,7 +420,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
414420
// dependencies. However, that code itself calls this method and
415421
// gets a full pre-filtered set of dependencies. This is not super
416422
// obvious, and clear, but it does work at the moment.
417-
if unit.profile.run_custom_build {
423+
if unit.target.is_custom_build() {
418424
let key = (unit.pkg.package_id().clone(), unit.kind);
419425
if self.build_script_overridden.contains(&key) {
420426
return Vec::new();
@@ -476,25 +482,6 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
476482
self.build_config.jobs
477483
}
478484

479-
pub fn lib_profile(&self) -> &'a Profile {
480-
let (normal, test) = if self.build_config.release {
481-
(&self.profiles.release, &self.profiles.bench_deps)
482-
} else {
483-
(&self.profiles.dev, &self.profiles.test_deps)
484-
};
485-
if self.build_config.test {
486-
test
487-
} else {
488-
normal
489-
}
490-
}
491-
492-
pub fn build_script_profile(&self, _pkg: &PackageId) -> &'a Profile {
493-
// TODO: should build scripts always be built with the same library
494-
// profile? How is this controlled at the CLI layer?
495-
self.lib_profile()
496-
}
497-
498485
pub fn incremental_args(&self, unit: &Unit) -> CargoResult<Vec<String>> {
499486
// There's a number of ways to configure incremental compilation right
500487
// now. In order of descending priority (first is highest priority) we
@@ -572,6 +559,15 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
572559
Kind::Target => &self.target_info,
573560
}
574561
}
562+
563+
pub fn extra_args_for(&self, unit: &Unit<'a>) -> Option<&Vec<String>> {
564+
if let Some((ref args_unit, ref args)) = self.extra_compiler_args {
565+
if args_unit == unit {
566+
return Some(args);
567+
}
568+
}
569+
None
570+
}
575571
}
576572

577573
/// Acquire extra flags to pass to the compiler from various locations.

0 commit comments

Comments
 (0)