Profile Overrides (RFC #2282 Part 1) #5384

ehuss · 2018-04-18T02:08:13Z

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. Added CompileMode to Unit to replace them.
Profile is no longer a reference in Unit.
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 Cargo check does not respect the --release flag #5218.
cargo build --test will set panic correctly for dependences. Fixes cargo build for tests builds dependencies with panic #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 cargo check --tests fails to compile macros in examples #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 Internally storing profiles as special-cased booleans #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 Internally storing profiles as special-cased booleans #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 Single profile for build dependencies #1774, cargo bench builds build dependencies in release mode #2234, Feature request: compile build.rs scripts in release mode even when doing a debug build #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.

matklad

Holy cow, this looks awesome @ehuss !

matklad · 2018-04-18T07:42:56Z

src/doc/src/reference/unstable.md

+opt-level = 3
+
+# All dependencies (but not this crate itself) will be compiled
+# with -Copt-level=2 . This includes build dependencies.


This probably should mention workspaces?

matklad · 2018-04-18T07:48:16Z

tests/testsuite/profiles.rs

+[RUNNING] `rustc --crate-name foo [..]`
+[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]",
+        )
+        // TODO: does_not_contain does not support patterns!


I think it does support [..]? Looks like [COMPILING] is missing?

I don't think it does, see:

cargo/tests/testsuite/cargotest/support/mod.rs

Lines 689 to 700 in 5ac4ab1

MatchKind::NotPresent => {

if !actual.contains(out) {

Ok(())

} else {

Err(format!(

"expected not to find:\n\

{}\n\n\

but found in output:\n\

{}",

out, actual

))

}

I mentioned it on IRC, I'll take a look at fixing it.

matklad · 2018-04-18T07:49:11Z

tests/testsuite/test.rs

@@ -1649,6 +1649,7 @@ fn test_run_implicit_test_target() {
        )
        .build();

+    // TODO FIXME: This needs to better verify that examples are not built.


This can be done by putting a compile-time error in the example.

matklad · 2018-04-18T07:49:25Z

tests/testsuite/test.rs

@@ -1742,13 +1742,13 @@ fn test_run_implicit_example_target() {
        )
        .build();

+    // TODO FIXME - verify example does NOT get run.


Example could panic with runtime error to detect this.

matklad · 2018-04-18T07:51:08Z

src/cargo/util/toml/mod.rs

@@ -347,20 +369,65 @@ impl<'de> de::Deserialize<'de> for U32OrBool {
 }

 #[derive(Deserialize, Serialize, Clone, Debug, Default)]
+#[serde(rename_all = "kebab-case")]


matklad · 2018-04-18T07:56:12Z

src/cargo/util/toml/mod.rs

+            bail!("Profile overrides cannot be nested.");
+        }
+        if self.panic.is_some() {
+            bail!("`panic` may not be specified in a build override.")


s/build/profile/ ?

matklad · 2018-04-18T08:07:30Z

src/cargo/ops/cargo_compile.rs

    Check { test: bool },
+    /// A target being built for a benchmark.
    Bench,


I wonder if we can merge Test and Bench, or perhaps move Bench to ProfileFor? From compiler pov, I think test & bench are indistinguishable?

An interesting idea. I'll think about it some more, but it may complicate cargo_compile since it needs to know the overall mode we're in (like selecting default targets, etc.).

matklad · 2018-04-18T08:10:26Z

src/cargo/core/profiles.rs

+                // ancestor's profile.  However `cargo clean -p` can hit this
+                // path.
+                // TODO: I think `cargo clean -p xxx` is not cleaning out
+                // the "OUT_DIR" directory.  This is not a new bug.


OUT_DIR here is the --out-dir? It can't be cleaned by clean at all I think.

I don't think so. I was thinking of the build_script_out_dir which is something like target/debug/build/pkgname-hash/out. But I was mistaken, I was actually just seeing the output, stderr, and root-output directories left behind. Probably not too important, and unrelated to these changes.

matklad · 2018-04-18T08:15:52Z

src/cargo/core/profiles.rs

+        cx: &Context<'a, 'cfg>,
+        map: &mut HashMap<Unit<'a>, Profile>,
+    ) {
+        if !map.contains_key(unit) {


It's slightly better to use early returns here, to save indentation and keep mental stack shallow:

if map.contains_key(unit) { return; }

Ideally, I think we should just top-sort the units once and avoid writing recursive functions all over the place, but this definitely shouldn't be done in this PR.

matklad · 2018-04-18T09:04:28Z

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

+        let unit_deps = compute_deps(unit, cx, deps, profile_for)?;
+        let to_insert: Vec<_> = unit_deps.iter().map(|&(unit, _)| unit).collect();
+        deps.insert(*unit, to_insert);
+        for (unit, profile_for) in unit_deps {


If I add assert_eq!(unit.profile_for, profile_for); it is not triggered, so, perhaps, we don't need to return pairs after all?

There was a bug. Since units no longer include ProfileFor, this is no longer the case.

matklad · 2018-04-18T09:26:07Z

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 think current implementation is OK: linear scan is perfectly fine until you have hundreds of members.

Is it worth it to keep name?

I have no opinion here :)

I'm unhappy with the Finished line summary

It still works correctly when no overrides are present, and otherwise it is a minor detail, so I suggest to punt on this for know: current solution seems at least OK to me!

Build-dependencies use different profiles based on whether or not --release flag is given.

Yeah, that seems generally wrong, but OK for now, because it more or less matches the current behavior. I would say that this is probably an instance where we do need a separate, not dev/not release profile, but let's deal with that later.

build --bench xxx or --benches builds dependencies with dev profile,

Hm, is this the current behavior? It definitely is a bug!

Should it warn/error if you have an override for a package that does not exist?

Let's give at least a warning if it's not too hard: typoes are likely in the names of packages, especially with - vs _ distinction.

Should it warn/error if you attempt to set panic on the test or bench profile?

It is allowed now, right? Then I think error is probably not feasible, but the warning seems useful!

matklad · 2018-04-18T11:05:32Z

src/cargo/core/profiles.rs

+}
+
+impl ProfileFor {
+    pub fn all_values() -> Vec<ProfileFor> {


Mincooptimization, but here and in CompileMode::all_modes, we could return &'static [Stuff], to save some allocations.

matklad · 2018-04-18T11:09:24Z

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

+    /// command-line for `cargo rustc` and `cargo rustdoc`.  These commands
+    /// only support one target, but we don't want the args passed to any
+    /// dependencies.
+    pub extra_compiler_args: HashMap<Unit<'a>, Vec<String>>,


Perhaps we can use Option<(Unit, Vec<String>)> instead? Let's also document that this is overloaded in the sense that this can be arguments for different tools, rustc or rustdoc, depending on the command.

matklad · 2018-04-18T11:14:44Z

src/cargo/ops/cargo_compile.rs

+    // Helper for creating a Unit struct.
+    let new_unit =
+        |pkg: &'a Package, target: &'a Target, mode: CompileMode, profile_for: ProfileFor| {
+            let actual_profile_for = if profile_for != ProfileFor::Any {


Just a personal taste, but I would name this just profile_for to shadow the input parameter, so that, down the line, we can't use the wrong profile_for.

ehuss · 2018-04-18T14:28:08Z

Thanks @matklad for doing such a detailed review so quickly!

Unfortunately, I've discovered a pretty fundamental flaw in how it works. Units are not being properly de-duplicated. Example: test→lib and bin→lib, the lib gets compiled twice (once as a regular dependency, and again as a test dependency). But if you don't have panic set they get compiled with the exact same settings!

I think Profile really must be a part of Unit. I had a previous attempt that had it part of the Unit, but I was concerned about the huge amount of copying. However, I was using Strings before (instead of InternedString), and it seemed much worse at the time (require lots of clone() calls). I'm going to rewrite it to use this approach. Profile is only 24 bytes on my system, so it shouldn't be too bad. On the plus side, it should simplify some things.

Sorry for the churn, I'll try to fix this today and address all your comments.

matklad · 2018-04-18T15:18:04Z

But if you don't have panic set they get compiled with the exact same settings!

Wow, that's interesting! I quite like the current approach actually, but yeah, looks like we'd better to store raw flags to avoid duplication 😿

ehuss · 2018-04-19T05:42:24Z

I've pushed the update that now embeds the profile inside Unit. It works quite a bit better now. I fear there are still some hidden gotchas, so I'll need to spend a fair amount of time making tests. I left a long comment in there about a subtle issue with cargo test, that I think I'm going to revisit because it's not quite right. Getting proper panic support is turning out to be very tricky. Doc tests are also turning out to be very tricky, because it greedily links against any libs it finds in Context.libraries.

bors · 2018-04-19T14:31:17Z

☔ The latest upstream changes (presumably #5392) made this pull request unmergeable. Please resolve the merge conflicts.

matklad · 2018-04-21T11:07:58Z

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

+        // cleared, and avoid building the lib thrice (once with `panic`, once
+        // without, once for --test).  In particular, the lib included for
+        // doctests and examples are `Build` mode here.
+        let profile_for = if unit.mode.is_any_test() || cx.build_config.test {


Hm, I am genuinely not sure what the correct behavior would be here? Suppose we are compiling the integration test, which only executes the binary in the same package. It is reasonable to assume that the binary might be compiled with panic=abort, but the test itself should be compiled with panic=unwind, so common dependencies should be compiled twice.

Perhaps the best path forward is to preserve the current, buggy, behavior, land this PR, and then fix the behavior in a follow-up PR?

matklad · 2018-04-21T11:10:33Z

src/cargo/ops/cargo_compile.rs

+                // `panic` set.
+                //
+                // As a consequence, Examples and Binaries get compiled
+                // without `panic` set.  This probably isn't a bad deal.


Ah yeah, this is exactly what I was thinking about... Hm, compiling binaries with different panic setting actually seems like a big deal to me...

Yea, it's a tricky issue, I was going to ask about it. The immediate problem is that doc tests attempt to link with everything in Context.libraries. I'm uncertain what the easiest way to fix that would be. One thought I had would be to change run_doc_tests to only grab the dependencies for the CompileMode::Doctest unit and remove Context.libraries. There would be some other minor things to accommodate that, but I think it should work. I can do it now or later, let me know. It would remove this weird special-case.

Would it be difficult to preserve exactly the behavior (and bugs :) ) we have today? I think this would be the best way forward for now, because changes which both add unstable features and fix/modify stable behavior are tricky...

Yea, it should be preserving the old behavior. I have a few changes I haven't pushed, yet, that ensure that. I'm keeping a list of behavior changes, and I'll post it soon. A few are clearly bug fixes, but a couple are questionable. We can review them when I post it.

ehuss · 2018-04-22T01:41:21Z

The CI failures are due to the introduction of rust-lang/rust#49289 (starting with nightly 2018-04-15). cargo check will now emit rmeta files for all targets. This will actually address #3624. This means that running cargo check twice will no longer re-check binaries. However, I'm uncertain how the test should be structured now. It's trying to verify the "freshness", but the behavior has changed between rust versions. How do you typically accommodate different rustc versions?

ehuss · 2018-04-22T01:44:33Z

(I've updated the first comment with some other Misc Fixes that probably aren't too controversial, but please review them. Sorry for the long comment, I just wanted to be thorough.)

The latest push includes some tests (profile_targets.rs) that illustrate what I was asking about before in regards to a way to verify exactly which commands get run, but ignoring the order. I don't think it would be possible to achieve the same level of verification with the existing functions (in particular, verifying what doesn't get run). Let me know if you would prefer it to be done in a different way, though. One alternative thought I had would be to have an option for with_json to ignore the order of messages, and add some more information to ArtifactProfile to be able to distinguish which profile is used.

Questions

The doc profile does not seem to be used. Internally it is assigned to running rustdoc, but rustdoc does not use any flags from the profile. Is that intended? Perhaps we could deprecate it (or maybe just remove it from the docs)? There are about 24 crates on crates.io that set it for whatever reason.
I just want to double-check... I've been using the term "profile override" in a few places, but I'm starting to feel that might not be clear (compared to [profile.dev] is overriding the default dev profile). "package profile override" would be more accurate, but is maybe a little wordy?

Behavior Changes to Discuss

The following are new changes in behavior that I'm uncertain if we should keep or change.

cargo build --release --all-targets now compiles libs and bins twice (once for benchmarks, once for unit-tests). Previously the de-dupe logic would only compile them once (since they are essentially identical). This is because with CompileMode in Unit it distinguishes between a Bench and Test, so it doesn't get de-duped. I could maybe add a special case for this?
If a build.rs is needed for different profiles, it will now be run separately for each one. For example, build --all-targets will run build.rs once for the dev targets, and once for the bench targets (because the opt-level is different between them). I think this is more-correct behavior, but may be a surprising change to some. Thoughts?

Panic Changes and Issues

Getting panic correct has been quite tricky. This is the list of changes and issues I'm currently aware of. These assume you have panic set in some profile.

cargo build with tests or benches was previously failing (error: the crate `foo` is compiled with the panic strategy `abort` which is incompatible with this crate's strategy of `unwind` ). This is now fixed.
cargo test builds binaries without panic set. This is a tricky issue. If this is fixed, then the lib would get built three times (once without panic for tests, once with panic for bins, once with --test as a unit-test). The bigger problem is that rustdoc for doc-tests would fail to run because it would pick up both copies of the lib. See comment previously about needing to change Context.libraries to fix this. Leave as-is? Fix now/later?
build.rs scripts and their dependencies have panic cleared because the panic-clearing logic thinks it is a compiler plugin (see build_base_args). However, I don't think build.rs scripts should be treated the same as compiler plugins/proc-macros. Would it be worth it to fix this? The change is fairly easy -- I would probably add for_plugin (for_compiler?) to Target and adjust walk_used_in_plugin_map to use that instead of for_host. This is a minor issue, but I was a little surprised to see it in one of my test cases.
The consequences are:
- build.rs scripts are always built without panic.
- If you have a dependency that is shared as a regular dependency and a build dependency, and you run cargo test or cargo bench, the dependency will be built twice, but with the same settings. Internally it thinks one is "dev without panic" (for tests) and the other is regular "dev" (for build.rs). It works fine, but slows things down since the shared dependency is built twice.

Known Bugs

DEBUG env var in build.rs is incorrect if profile debug=0. DEBUG env var in build.rs is incorrect if profile debug=0 #5370. Will fix in a separate PR.
Having a global build_config.release flag causes some issues. I'm uncertain how difficult this is to fix. Generally getting rid of the global build_config.release would be the correct approach. Thoughts? Address now or later? I think this only affects cargo build, and the workaround is to pass --release to it. Consequences:
- PROFILE env var in build.rs is incorrect for cargo build --bench name, it says it is "debug" when in reality it is "release".
- cargo build --bench xxx will build dependencies in dev profile, which may be surprising (cargo build --all-targets generates two copies of crate test binary #4929).
cargo check only ever uses dev or release profile. For example, cargo check --test foo uses dev instead of test. I'm uncertain how any of the profile flags might affect rustc when running check.

matklad · 2018-04-22T14:04:01Z

The latest push includes some tests (profile_targets.rs) that illustrate what I was asking about before in regards to a way to verify exactly which commands get run, but ignoring the order. I don't think it would be possible to achieve the same level of verification with the existing functions (in particular, verifying what doesn't get run)

Yeah, that seems reasonable. Let's add with_stderr_unordered perhaps which compares sets of lines? (EDIT: and now matklad actually reads the diff and sees with_stderr_unordered :) )

matklad · 2018-04-22T15:33:30Z

However, I'm uncertain how the test should be structured now. It's trying to verify the "freshness", but the behavior has changed between rust versions. How do you typically accommodate different rustc versions?

I think we can add

// If you are reading this, and 1.27.0 is already stable, feel free to just remove this if!
if !is_nightly() { return; }

matklad · 2018-04-22T17:06:35Z

The doc profile does not seem to be used. Internally it is assigned to running rustdoc, but rustdoc does not use any flags from the profile. Is that intended? Perhaps we could deprecate it (or maybe just remove it from the docs)? There are about 24 crates on crates.io that set it for whatever reason.

Yep, I believe we can safely deprecate this profile. That is, leave the syntax in Cargo.toml, give a warning about it, and remove traces of doc profile from the code. Eventually I think we'll do the same for test and bench profiles, but they are used, so we'll deal with that separately.

"package profile override" would be more accurate, but is maybe a little wordy?

I personally think about it as just "package profile". We need override in the surface syntax just to namespace this feature. I wonder if a sweeter syntax is possible? [profiles.dev.for.image] perhaps?

cargo build --release --all-targets now compiles libs and bins twice

I think it is important to remove this duplication. Could we get rid of the Bench compile mode?

If a build.rs is needed for different profiles, it will now be run separately for each one. For example, build --all-targets will run build.rs once for the dev targets, and once for the bench targets (because the opt-level is different between them). I think this is more-correct behavior, but may be a surprising change to some. Thoughts?

Hm, so currently build_scripts are compiled only with dev or release profiles. I suggest we keep this behavior, for three reasons:

No change means that we won't break someone's code,
If we remove all profiles except dev and test, we'll effectively have to change it back,
Test fiddle with panic setting, and this I think should not affect build scripts.

Long term, I believe the current behavior of using both dev and release for build scripts is also wrong. I think we need a separate use-visible profile for build scripts and their dependencies, so that cargo build && cargo build --release does not compile build scripts twice.

I don't think there should be any sharing between the build script and the main code. Especially for cross-compilation, it seems unreasonable to use the same profile for both.

matklad · 2018-04-22T17:21:13Z

cargo test builds binaries without panic set. This is a tricky issue. If this is fixed, then the lib would get built three times (once without panic for tests, once with panic for bins, once with --test as a unit-test)

This is the current behavior, right? I believe this is very wrong, but fixing it indeed adds even more compilations. So let's leave it as is for know.

However, I don't think build.rs scripts should be treated the same as compiler plugins/proc-macros.

Could you elaborate why do you think so? At least to me, they seem very similar indeed! And my hypothecical build profile would apply both to proc macros, build scripts and tasks (from aaturon's blogpost). It seems they all are things which a build only on host, and don't actually care about --release flag.

If you have a dependency that is shared as a regular dependency and a build dependency, and you run cargo test or cargo bench, the dependency will be built twice, but with the same settings. Internally it thinks one is "dev without panic" (for tests) and the other is regular "dev" (for build.rs).

I think it's fine to leave this duplication as is, but, if we are to fix it, then perhaps " Internally it thinks one is "dev without panic" (for tests) and the other is regular "dev" (for build.rs)." is the bit that needs adjustments?

ehuss · 2018-04-22T18:43:13Z

Yep, I believe we can safely deprecate this profile.

Would you like that now, or in a separate PR (since this one is getting a little long)?

[profiles.dev.for.image] perhaps?

Hm, that is a little clearer. Not sure if anyone else wants to weigh in?

I think it is important to remove this duplication. Could we get rid of the Bench compile mode?

It would be nice, because once the Unit is constructed, it is not needed. However, there would need to be a way for generate_targets and generate_default_targets to know that we are cargo bench to select the correct targets and profiles. I could pass a flag in through CompileOptions, what do you think? An alternate solution is to just swap the mode in generate_targets when the Unit is made which would be a one-line change to fix this.

Hm, so currently build_scripts are compiled only with dev or release profiles. I suggest we keep this behavior, for three reasons:

This is how it works (before and after this change) for compiling the build.rs script. I was talking about executing it. In the new code, it is run twice because the OPT_LEVEL is different for different targets' dependencies. This goes back to, should cargo build --bench xxx build the dependencies of a benchmark in dev mode? I guess I left it sort this in a halfway state. I see two options. One is to restore the old behavior (benches get dev for custom-built dependencies), and I can fix that by just checking the global release flag for running build.rs, which I think is an easy change. The other is to actually fix that problem, which might be more difficult (involves removing the use of the global release flag). I'm fine with restoring the old behavior, this is an edge case (building benchmarks with cargo build without --release flag). I'm also fine with trying to fix it properly. Or other options (like not even trying to build benches with cargo build if you don't set --release?).

I think we need a separate use-visible profile for build scripts and their dependencies

Isn't that what build-override is essentially? I think I mentioned it earlier, that this pseudo-profile already exists, but you have to define it twice (profile.dev.build-override and profile.release.build-override), and it has a weird relationship with other overrides.

This brings up a issue that was nagging at me. I didn't fully understand the reasoning for the override precedence in the RFC. Since other rules take precedence over build-override, it seems like that can yank away the ability to independently control how a build script is compiled vs how the dependency itself is compiled. There is a comment in the RFC thread that you and Manishearth had separately about this, but it didn't make sense to me. The build-dependencies are always kept independent of the final binary, even if we removed the precedence. Changing the precedence could give the user more control. (Or even adding something like [profile.scripts.overrides.somedep] which would be very easy to do.) I guess my rambling question is, why not give independent control?

Could you elaborate why do you think so?

The only reason this exception is made is to ensure the panic flag is cleared (because plugins are linked with things that can't have it set). build.rs scripts don't have that limitation since they aren't linked with the compiler. I'm fine with clearing it for build.rs. I'm uncertain how likely it is to be a problem for someone. Considering it has always been this way, and things built with panic=abort is very rare to start with, it might not bother anyone. However, if panic is set, it could cause shared dependencies to be needlessly built twice.

I think it's fine to leave this duplication as is, but, if we are to fix it, then perhaps " Internally it thinks one is "dev without panic" (for tests) and the other is regular "dev" (for build.rs)." is the bit that needs adjustments?

Correct, it's a small, trivial change to say "panic = None" for build scripts in the profile builder.

matklad · 2018-04-22T19:40:59Z

Would you like that now, or in a separate PR (since this one is getting a little long)?

A separate one. I'd prefer to merge this sooner, and not to fix all profile bugs at once :)

Hm, that is a little clearer. Not sure if anyone else wants to weigh in?

I personally don't care about syntax at this stage. There's already override in there, so let's not change it for now.

I could pass a flag in through CompileOptions, what do you think? An alternate solution is to just swap the mode in generate_targets when the Unit is made which would be a one-line change to fix this.

Another option is to split CompileMode into two enums: one to be used with CompileOptions, which corresponds to which cli options were passed, and one to be used with Units. But "swapping the mode" solution also sounds fine with me!

I was talking about executing it

Oh, right! I've completely misunderstood your original comment then! The new behavior indeed seems more correct to me as well!

Isn't that what build-override is essentially? I think I mentioned it earlier, that this pseudo-profile already exists, but you have to define it twice (profile.dev.build-override and profile.release.build-override), and it has a weird relationship with other overrides.

Yep, it's almost like build-override, with two important differences: you don't need to override it twice, and, by default, build profile is the same for cargo build and cargo build --release.

The build-dependencies are always kept independent of the final binary, even if we removed the precedence.

Hm, I think we though that dependencies are shared between the library and the build script. Because they are independent, priority for build_overrides indeed does not make sense.

ehuss · 2018-04-22T21:00:18Z

Hm, I think we though that dependencies are shared between the library and the build script. Because they are independent, priority for build_overrides indeed does not make sense.

OK, let me know if you want to iterate on this now or later. It's pretty easy to change.

Let me know if there's anything else. I think I've gone through the issues I can think of.

matklad · 2018-04-24T15:45:48Z

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

@@ -38,27 +50,33 @@ fn deps_of<'a, 'b, 'cfg>(
    unit: &Unit<'a>,
    cx: &Context<'a, 'cfg>,
    deps: &'b mut HashMap<Unit<'a>, Vec<Unit<'a>>>,
+    profile_for: ProfileFor,


I feel uneasy about this profile_for argument. This function is memoized, so the sequence of calls deps_of(u, cx, deps, ProfileFor::Any); deps_of(u, cx, deps, ProfileFor::TestDependency) will return the same result each time (and the result might be different if we swap the order of calls).

I understand what you mean. In practice, that's never a problem for TestDependency since it only exists for clearing panic, and you'll never ask for a unit with panic set as a TestDependency.

I'm trying to think of a scenario where it would matter for CustomBuild, but I can't think of any. It might matter if we later add a "build-script"-specific profile, and you have an override deep in a dependency chain.

Adding it to the map would require a separate de-duplication phase, which I think would add ~15 lines of code.

Let me know if you think it's important. I'd lean towards not changing it unless there's a testable case. Or to make it easier to add a build-script profile later if there's a strong chance of that.

I think I'm fine with it: I would love to refactor code in such a way that this problem just not arises, or at least to put some sort of run-time assertion, but I have no ideas of how to actually do this. Storing pairs in the HashMap would be worse than the status quo.

Let's perhaps just add a small comment here though?

matklad · 2018-04-24T15:59:48Z

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

@@ -273,28 +283,65 @@ fn maybe_lib<'a, 'cfg>(unit: &Unit<'a>, cx: &Context<'a, 'cfg>) -> Option<Unit<'
 /// script itself doesn't have any dependencies, so even in that case a unit
 /// of work is still returned. `None` is only returned if the package has no
 /// build script.
-fn dep_build_script<'a, 'cfg>(unit: &Unit<'a>, cx: &Context<'a, 'cfg>) -> Option<Unit<'a>> {
+fn dep_build_script<'a>(cx: &Context, unit: &Unit<'a>) -> Option<(Unit<'a>, ProfileFor)> {


Some function in this module now use cx, unit arg order, and some use (unit, cx). Let's unify them to one order? I don't have any preference for a specific order, as long as it is an order :-)

Yea, I derped. I had an earlier version that I had removed Context, but when I added it back I should have kept it the same.

matklad · 2018-04-24T18:56:26Z

tests/testsuite/profile_targets.rs

+foo custom build PROFILE=release DEBUG=false OPT_LEVEL=3
+[RUNNING] `rustc --crate-name foo src[/]lib.rs --crate-type lib --emit=dep-info,link -C opt-level=3 -C panic=abort -C codegen-units=2 [..]
+[RUNNING] `rustc --crate-name foo src[/]main.rs --crate-type bin --emit=dep-info,link -C opt-level=3 -C panic=abort -C codegen-units=2 [..]
+[FINISHED] release [optimized] [..]


I guess I don't understand something about panic handling... How this even works? We link bar, which is compiled with panic=unwind into foo, which is compiled with panic=abort and rustc does not complain. Is it the expected behavior? Testing locally, I see that unwind -> abort direction works, and abort -> unwind fails. Does this mean that it is always safe to compile dependencies with panic = unwind?

This probably has nothing to do with the PR though, because the behavior has not changed here.

Hm, yea, I don't know much how panic handling is implemented. I did a test where I attempt to panic in a dependency that was compiled with unwind, and it still aborts. It looks like it just affects whether libpanic_abort or libpanic_unwind is linked. I'm guessing since the API is the same, the dependencies don't know the difference.

It looks like it just affects whether libpanic_abort or libpanic_unwind is linked. I

I would guess that it also affects generation of landing pads? So we might be getting a bunch of dead code here?

@alexcrichton could you explain how mixing panic modes is supposed to work?

Ah yes the observed behavior here is expected, we , for example, ship panic=unwind libstd to everyone but still allow compiling with panic=abort

matklad · 2018-04-24T18:59:43Z

I've reviewed everything except the tests (which are super awesome!), and I don't have any more comments here! I would probably want to inrdocude a separate feature flag to build override, and maybe switch to a separate build profile, but this don't need to happen in this PR.

I'd like to hear what @alexcrichton thinks as well! :)

alexcrichton

Everything is looking amazing here. I'm blown away at the thoroughness and cleanliness of the patch, thanks so much for taking this on @ehuss!

I've got basically a few tiny nits below, but overall the strategy here sounds great. I don't think I've fully grok'd everything yet, especially the ProfileFor threw me particularly for a spin around the unit_dependencies.rs files. Nothing here looks wrong though, tests are all passing, and the caveats you've mentioned along the way all sound great to me.

tl;dr; I'd be happy to land this at any time!

alexcrichton · 2018-04-27T16:37:43Z

src/cargo/core/profiles.rs

+    }
+
+    fn validate_packages(&self, shell: &mut Shell, packages: &HashSet<&str>) -> CargoResult<()> {
+        if let Some(ref toml) = self.toml {


Stylistically I'm always personally a fan of reducing indent here:

let toml = match self.toml { Some(ref toml) => toml, None => return Ok(()), }; // ...

alexcrichton · 2018-04-27T16:39:01Z

src/cargo/core/profiles.rs

+        if let Some(ref toml) = self.toml {
+            if let Some(ref overrides) = toml.overrides {
+                for key in overrides.keys().filter(|k| k.as_str() != "*") {
+                    if !packages.contains(key.as_str()) {


I think this is something we'll want to note on the tracking issue, I'd expect the packages set here to be more like a HashSet<PackageId> (or a Resolve) and the keys of overrides here would be a PackageIdSpec in theory

I'm not sure I follow. Are you saying that overrides could support full spec strings? That looks like it'd be pretty easy to support.

Basically yeah. We've got the idea of a "package id specification" which is intended to describe a particular crate in a crate graph unbambiguously, and this would allow you to, for example, customize the compilation of foo from crates.io but not customize foo from github.com/example/foo (or something like that)

…lease`. This changes it so that `Build` mode is not set in the Unit, since there is no difference between Bench and Test once the profile has been selected.

…map.

matklad · 2018-04-27T23:21:22Z

@bors r+

Thank you so much for this, @ehuss! It was an amazing experience reviewing all this awesome work!

bors · 2018-04-27T23:21:23Z

📋 Looks like this PR is still in progress, ignoring approval

matklad · 2018-04-27T23:22:48Z

@bors r+

My undestanding is that this is not really a WIP, but, anyway, we could land additional fixes in separate PR's just fine!

bors · 2018-04-27T23:22:48Z

📌 Commit e82e9c1 has been approved by matklad

bors · 2018-04-27T23:22:56Z

⌛ Testing commit e82e9c1 with merge c24a097...

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.

bors · 2018-04-27T23:48:33Z

☀️ Test successful - status-appveyor, status-travis
Approved by: matklad
Pushing c24a097 to master...

Stabilize profile-overrides. This stabilizes the profile-overrides feature. This was proposed in [RFC 2282](rust-lang/rfcs#2282) and implemented in #5384. Tracking issue is rust-lang/rust#48683. This is intended to land in 1.41 which will reach the stable channel on Jan 30th. This includes a new documentation chapter on profiles. See the ["Overrides" section](https://github.com/rust-lang/cargo/blob/9c993a92ce33f219aaaed963bef51fc0f6a7677a/src/doc/src/reference/profiles.md#overrides) in `profiles.md` file for details on what is being stabilized. Note: The `config-profile` and `named-profiles` features are still unstable. Closes #6214 **Concerns** - There is some risk that `build-override` may be confusing with the [proposed future dedicated `build` profile](#6577). There is some more discussion about the differences at rust-lang/rust#48683 (comment). I don't expect it to be a significant drawback. If we proceed later with a dedicated `build` profile, I think we can handle explaining the differences in the documentation. (The linked PR is designed to work with profile-overrides.) - I don't anticipate any unexpected interactions with `config-profiles` or `named-profiles`. - Some of the syntax like `"*"` or `build-override` may not be immediately obvious what it means without reading the documentation. Nobody suggested any alternatives, though. - Configuring overrides for multiple packages is currently a pain, as you have to repeat the settings separately for each package. I propose that we can extend the syntax in the future to allow a comma-separated list of package names to alleviate this concern if it is deemed worthwhile. - The user may not know which packages to override, particularly for some dependencies that are split across multiple crates. I think, since this is an advanced feature, the user will likely be comfortable with using things like `cargo tree` to understand what needs to be overridden. There is [some discussion](rust-lang/rust#48683 (comment)) in the tracking issue about automatically including a package's dependencies, but this is somewhat complex. - There is some possibly confusing interaction with the test/bench profile. Because dependencies are always built with the dev/release profiles, overridding test/bench usually does not have an effect (unless specifying a workspace member that is being tested/benched). Overriding test/bench was previously prohibited, but was relaxed when named profiles were added. - We may want to allow overriding the `panic`, `lto`, and `rpath` settings in the future. I can imagine a case where someone has a large workspace, and wants to override those settings for just one package in the workspace. They are currently not allowed because it doesn't make sense to change those settings for rlibs, and `panic` usually needs to be in sync for the whole profile. - There are some strange interactions with `dylib` crates detailed in rust-lang/rust#64319. A fix was attempted, but later reverted. Since `dylib` crates are rare (this mostly applied to `libstd`), and a workaround was implemented for `build-std` (it no longer builds a dylib), I'm not too worried about this. - The interaction with `share-generics` can be quite confusing (see rust-lang/rust#63484). I added a section in the docs that tries to address this concern. It's also possible future versions of `rustc` may handle this better. - The new documentation duplicates some of the information in the rustc book. I think it's fine, as there are subtle differences, and it avoids needing to flip back and forth between the two books to learn what the settings do.

matklad reviewed Apr 18, 2018

View reviewed changes

matklad mentioned this pull request Apr 18, 2018

Let CompileOptions create a BuildConfig directly #5358

Merged

ehuss force-pushed the profile-override branch from 83d742a to e09a839 Compare April 19, 2018 16:44

matklad mentioned this pull request Apr 21, 2018

cargo check --tests fails to compile macros in examples #5397

Closed

matklad self-assigned this Apr 21, 2018

matklad reviewed Apr 21, 2018

View reviewed changes

ehuss force-pushed the profile-override branch from e09a839 to 4396fe2 Compare April 22, 2018 00:23

matklad reviewed Apr 24, 2018

View reviewed changes

alexcrichton reviewed Apr 27, 2018

View reviewed changes

ehuss added 6 commits April 27, 2018 13:42

Add some more tests.

b9181ef

Update tests now that cargo check does not re-check bins.

025e23b

Avoid building libs and bins twice in `cargo build --all-targets --re…

195520b

…lease`. This changes it so that `Build` mode is not set in the Unit, since there is no difference between Bench and Test once the profile has been selected.

Fix Unit/Context parameter order.

902cb98

Comment why deps_of doesn't need to keep track of profile_for in …

040f984

…map.

Minor style update.

e82e9c1

ehuss force-pushed the profile-override branch from a9823b3 to e82e9c1 Compare April 27, 2018 20:52

matklad changed the title ~~[WIP] Profile Overrides (RFC #2282 Part 1)~~ Profile Overrides (RFC #2282 Part 1) Apr 27, 2018

bors merged commit e82e9c1 into rust-lang:master Apr 27, 2018

This was referenced Apr 27, 2018

Introduction of namespaced features (see #1286) #5300

Merged

Add --build-plan for 'cargo build' #5301

Merged

Allow cfg(...)ing targets in Cargo.toml #5367

Closed

This was referenced Apr 28, 2018

Should examples be unit-testable? #5437

Closed

Race condition when trying to compile a lib multiple times for different purposes. #5444

Closed

matklad mentioned this pull request May 27, 2018

cargo build --all-targets executes build.rs twice in beta and nightly #5575

Closed

matklad mentioned this pull request Jun 27, 2018

Add @ehuss to cargo reviewers rust-lang/rust-central-station#57

Merged

dwijnand unassigned matklad Feb 17, 2019

Manishearth mentioned this pull request Mar 6, 2019

Tracking issue for RFC 2282 - Cargo profile dependencies rust-lang/rust#48683

Closed

5 tasks

ehuss mentioned this pull request Nov 15, 2019

Stabilize profile-overrides. #7591

Merged

ehuss added this to the 1.27.0 milestone Feb 6, 2022

weihanglo mentioned this pull request Sep 8, 2023

build-override results in the same dependency being compiled twice #12645

Closed

link2xt mentioned this pull request Sep 8, 2023

Allow to set panic='abort' in build-override #12647

Closed

	MatchKind::NotPresent => {
	if !actual.contains(out) {
	Ok(())
	} else {
	Err(format!(
	"expected not to find:\n\
	{}\n\n\
	but found in output:\n\
	{}",
	out, actual
	))
	}

Profile Overrides (RFC #2282 Part 1) #5384

Profile Overrides (RFC #2282 Part 1) #5384

Conversation

ehuss commented Apr 18, 2018 • edited by dwijnand Loading

Misc Fixes

Questions

TODO

matklad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matklad commented Apr 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehuss commented Apr 18, 2018

matklad commented Apr 18, 2018

ehuss commented Apr 19, 2018

bors commented Apr 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehuss commented Apr 22, 2018 • edited Loading

ehuss commented Apr 22, 2018

Questions

Behavior Changes to Discuss

Panic Changes and Issues

Known Bugs

matklad commented Apr 22, 2018 • edited Loading

matklad commented Apr 22, 2018

matklad commented Apr 22, 2018

matklad commented Apr 22, 2018

ehuss commented Apr 22, 2018

matklad commented Apr 22, 2018

ehuss commented Apr 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matklad commented Apr 24, 2018

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matklad commented Apr 27, 2018

bors commented Apr 27, 2018

matklad commented Apr 27, 2018

bors commented Apr 27, 2018

bors commented Apr 27, 2018

bors commented Apr 27, 2018

ehuss commented Apr 18, 2018 •

edited by dwijnand

Loading

ehuss commented Apr 22, 2018 •

edited

Loading

matklad commented Apr 22, 2018 •

edited

Loading