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.

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

@@ -7,8 +7,8 @@ use std::sync::Arc;
 
 use lazycell::LazyCell;
 
-use core::{TargetKind, Workspace};
 use super::{Context, FileFlavor, Kind, Layout, Unit};
+use core::{TargetKind, Workspace};
 use util::{self, CargoResult};
 
 #[derive(Clone, Hash, Eq, PartialEq, Ord, PartialOrd)]
@@ -97,7 +97,7 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> {
     /// Returns the appropriate output directory for the specified package and
     /// target.
     pub fn out_dir(&self, unit: &Unit<'a>) -> PathBuf {
-        if unit.profile.doc {
+        if unit.mode.is_doc() {
             self.layout(unit.kind).root().parent().unwrap().join("doc")
         } else if unit.target.is_custom_build() {
             self.build_script_dir(unit)
@@ -148,15 +148,15 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> {
     /// Returns the appropriate directory layout for either a plugin or not.
     pub fn build_script_dir(&self, unit: &Unit<'a>) -> PathBuf {
         assert!(unit.target.is_custom_build());
-        assert!(!unit.profile.run_custom_build);
+        assert!(!unit.mode.is_run_custom_build());
         let dir = self.pkg_dir(unit);
         self.layout(Kind::Host).build().join(dir)
     }
 
     /// Returns the appropriate directory layout for either a plugin or not.
     pub fn build_script_out_dir(&self, unit: &Unit<'a>) -> PathBuf {
         assert!(unit.target.is_custom_build());
-        assert!(unit.profile.run_custom_build);
+        assert!(unit.mode.is_run_custom_build());
         let dir = self.pkg_dir(unit);
         self.layout(unit.kind).build().join(dir).join("out")
     }
@@ -211,7 +211,7 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> {
             } else {
                 Some((
                     out_dir.parent().unwrap().to_owned(),
-                    if unit.profile.test {
+                    if unit.mode.is_any_test() {
                         file_stem
                     } else {
                         bin_stem
@@ -244,7 +244,10 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> {
         let mut ret = Vec::new();
         let mut unsupported = Vec::new();
         {
-            if unit.profile.check {
+            if unit.mode.is_check() {
+                // This is not quite correct for non-lib targets.  rustc
+                // currently does not emit rmeta files, so there is nothing to
+                // check for!  See #3624.
                 let path = out_dir.join(format!("lib{}.rmeta", file_stem));
                 let hardlink = link_stem
                     .clone()
@@ -296,7 +299,7 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> {
                     | TargetKind::Test => {
                         add("bin", FileFlavor::Normal)?;
                     }
-                    TargetKind::Lib(..) | TargetKind::ExampleLib(..) if unit.profile.test => {
+                    TargetKind::Lib(..) | TargetKind::ExampleLib(..) if unit.mode.is_any_test() => {
                         add("bin", FileFlavor::Normal)?;
                     }
                     TargetKind::ExampleLib(ref kinds) | TargetKind::Lib(ref kinds) => {
@@ -378,7 +381,7 @@ fn compute_metadata<'a, 'cfg>(
     // just here for rustbuild. We need a more principled method
     // doing this eventually.
     let __cargo_default_lib_metadata = env::var("__CARGO_DEFAULT_LIB_METADATA");
-    if !(unit.profile.test || unit.profile.check)
+    if !(unit.mode.is_any_test() || unit.mode.is_check())
         && (unit.target.is_dylib() || unit.target.is_cdylib()
             || (unit.target.is_bin() && cx.build_config.target_triple().starts_with("wasm32-")))
         && unit.pkg.package_id().source_id().is_path()
@@ -423,6 +426,10 @@ fn compute_metadata<'a, 'cfg>(
     // panic=abort and panic=unwind artifacts, additionally with various
     // settings like debuginfo and whatnot.
     unit.profile.hash(&mut hasher);
+    unit.mode.hash(&mut hasher);
+    if let Some(ref args) = cx.extra_args_for(unit) {
+        args.hash(&mut hasher);
+    }
 
     // Artifacts compiled for the host should have a different metadata
     // piece than those compiled for the target, so make sure we throw in

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

@@ -1,5 +1,4 @@
 #![allow(deprecated)]
-
 use std::collections::{HashMap, HashSet};
 use std::env;
 use std::path::{Path, PathBuf};
@@ -8,25 +7,27 @@ use std::sync::Arc;
 
 use jobserver::Client;
 
-use core::{Package, PackageId, PackageSet, Profile, Resolve, Target};
-use core::{Dependency, Profiles, Workspace};
-use util::{internal, profile, Cfg, CfgExpr, Config};
+use core::profiles::{Profile, Profiles};
+use core::{Dependency, Workspace};
+use core::{Package, PackageId, PackageSet, Resolve, Target};
+use ops::CompileMode;
 use util::errors::{CargoResult, CargoResultExt};
+use util::{internal, profile, Cfg, CfgExpr, Config};
 
-use super::TargetConfig;
 use super::custom_build::{self, BuildDeps, BuildScripts, BuildState};
 use super::fingerprint::Fingerprint;
 use super::job_queue::JobQueue;
 use super::layout::Layout;
 use super::links::Links;
+use super::TargetConfig;
 use super::{BuildConfig, Compilation, Executor, Kind};
 
 mod unit_dependencies;
 use self::unit_dependencies::build_unit_dependencies;
 
 mod compilation_files;
-use self::compilation_files::{CompilationFiles, OutputFile};
 pub use self::compilation_files::Metadata;
+use self::compilation_files::{CompilationFiles, OutputFile};
 
 mod target_info;
 pub use self::target_info::{FileFlavor, TargetInfo};
@@ -45,7 +46,7 @@ pub use self::target_info::{FileFlavor, TargetInfo};
 /// example, it needs to know the target architecture (OS, chip arch etc.) and it needs to know
 /// whether you want a debug or release build. There is enough information in this struct to figure
 /// all that out.
-#[derive(Clone, Copy, Eq, PartialEq, Hash)]
+#[derive(Clone, Copy, Eq, PartialEq, Hash, Debug)]
 pub struct Unit<'a> {
     /// Information about available targets, which files to include/exclude, etc. Basically stuff in
     /// `Cargo.toml`.
@@ -55,15 +56,18 @@ pub struct Unit<'a> {
     /// build.
     pub target: &'a Target,
     /// The profile contains information about *how* the build should be run, including debug
-    /// level, extra args to pass to rustc, etc.
-    pub profile: &'a Profile,
+    /// level, etc.
+    pub profile: Profile,
     /// Whether this compilation unit is for the host or target architecture.
     ///
     /// For example, when
     /// cross compiling and using a custom build script, the build script needs to be compiled for
     /// the host architecture so the host rustc can use it (when compiling to the target
     /// architecture).
     pub kind: Kind,
+    /// The "mode" this unit is being compiled for.  See `CompileMode` for
+    /// more details.
+    pub mode: CompileMode,
 }
 
 /// The build context, containing all information about a build task
@@ -87,10 +91,16 @@ pub struct Context<'a, 'cfg: 'a> {
     pub links: Links<'a>,
     pub used_in_plugin: HashSet<Unit<'a>>,
     pub jobserver: Client,
+    pub profiles: &'a Profiles,
+    /// This is a workaround to carry the extra compiler args for either
+    /// `rustc` or `rustdoc` given on the command-line for the commands `cargo
+    /// rustc` and `cargo rustdoc`.  These commands only support one target,
+    /// but we don't want the args passed to any dependencies, so we include
+    /// the `Unit` corresponding to the top-level target.
+    extra_compiler_args: Option<(Unit<'a>, Vec<String>)>,
 
     target_info: TargetInfo,
     host_info: TargetInfo,
-    profiles: &'a Profiles,
     incremental_env: Option<bool>,
 
     unit_dependencies: HashMap<Unit<'a>, Vec<Unit<'a>>>,
@@ -105,6 +115,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
         config: &'cfg Config,
         build_config: BuildConfig,
         profiles: &'a Profiles,
+        extra_compiler_args: Option<(Unit<'a>, Vec<String>)>,
     ) -> CargoResult<Context<'a, 'cfg>> {
         let incremental_env = match env::var("CARGO_INCREMENTAL") {
             Ok(v) => Some(v == "1"),
@@ -156,6 +167,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
 
             unit_dependencies: HashMap::new(),
             files: None,
+            extra_compiler_args,
         };
 
         cx.compilation.host_dylib_path = cx.host_info.sysroot_libdir.clone();
@@ -174,8 +186,8 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
         let mut queue = JobQueue::new(&self);
         self.prepare_units(export_dir, units)?;
         self.prepare()?;
-        self.build_used_in_plugin_map(&units)?;
-        custom_build::build_map(&mut self, &units)?;
+        self.build_used_in_plugin_map(units)?;
+        custom_build::build_map(&mut self, units)?;
 
         for unit in units.iter() {
             // Build up a list of pending jobs, each of which represent
@@ -200,7 +212,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
                     None => &output.path,
                 };
 
-                if unit.profile.test {
+                if unit.mode.is_any_test() && !unit.mode.is_check() {
                     self.compilation.tests.push((
                         unit.pkg.clone(),
                         unit.target.kind().clone(),
@@ -224,7 +236,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
                     continue;
                 }
 
-                if dep.profile.run_custom_build {
+                if dep.mode.is_run_custom_build() {
                     let out_dir = self.files().build_script_out_dir(dep).display().to_string();
                     self.compilation
                         .extra_env
@@ -236,7 +248,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
                 if !dep.target.is_lib() {
                     continue;
                 }
-                if dep.profile.doc {
+                if dep.mode.is_doc() {
                     continue;
                 }
 
@@ -313,16 +325,10 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
             None => None,
         };
 
-        let deps = build_unit_dependencies(units, &self)?;
+        let deps = build_unit_dependencies(units, self)?;
         self.unit_dependencies = deps;
-        let files = CompilationFiles::new(
-            units,
-            host_layout,
-            target_layout,
-            export_dir,
-            self.ws,
-            &self,
-        );
+        let files =
+            CompilationFiles::new(units, host_layout, target_layout, export_dir, self.ws, self);
         self.files = Some(files);
         Ok(())
     }
@@ -414,7 +420,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
         // dependencies. However, that code itself calls this method and
         // gets a full pre-filtered set of dependencies. This is not super
         // obvious, and clear, but it does work at the moment.
-        if unit.profile.run_custom_build {
+        if unit.target.is_custom_build() {
             let key = (unit.pkg.package_id().clone(), unit.kind);
             if self.build_script_overridden.contains(&key) {
                 return Vec::new();
@@ -476,25 +482,6 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
         self.build_config.jobs
     }
 
-    pub fn lib_profile(&self) -> &'a Profile {
-        let (normal, test) = if self.build_config.release {
-            (&self.profiles.release, &self.profiles.bench_deps)
-        } else {
-            (&self.profiles.dev, &self.profiles.test_deps)
-        };
-        if self.build_config.test {
-            test
-        } else {
-            normal
-        }
-    }
-
-    pub fn build_script_profile(&self, _pkg: &PackageId) -> &'a Profile {
-        // TODO: should build scripts always be built with the same library
-        //       profile? How is this controlled at the CLI layer?
-        self.lib_profile()
-    }
-
     pub fn incremental_args(&self, unit: &Unit) -> CargoResult<Vec<String>> {
         // There's a number of ways to configure incremental compilation right
         // now. In order of descending priority (first is highest priority) we
@@ -572,6 +559,15 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
             Kind::Target => &self.target_info,
         }
     }
+
+    pub fn extra_args_for(&self, unit: &Unit<'a>) -> Option<&Vec<String>> {
+        if let Some((ref args_unit, ref args)) = self.extra_compiler_args {
+            if args_unit == unit {
+                return Some(args);
+            }
+        }
+        None
+    }
 }
 
 /// Acquire extra flags to pass to the compiler from various locations.