Implemet dependency-specific profiles

[matklad]

RFC: https://github.com/rust-lang/rfcs/pull/2282 Tracking issue: https://github.com/rust-lang/rust/issues/48683

cc @ehuss you wanted to work on this, so let's make this issue a point of coordination.

I'll try to write some mentoring docs later today! Cargo's current profile implementation has a certain historical linage, so the first step here might be to just clean up an existing implementation :)

[ehuss]

Thanks! I've been reading as much as I can find on what has been expressed around profiles. I have a few initial questions:

1. I assume you still want this behind two separate feature flags?
2. Should rpath and lto be top-level only (like panic)? (Are there even legitimate use cases for rpath?)
3. Should this support all 5 profiles? The rfc only really mentioned dev. I know you've mentioned a desire to simplify to just dev/release. What exactly are Cargo's backward-compatibility requirements?
4. AFAIK, the only reason the existing Profile struct is serializable is to dump the JSON structure as part of the Artifact which is displayed for each artifact. The reason I ask is I was wondering if that can change at all? In particular, it exposes the "test" flag, which isn't a user-visible profile option.
5. Documentation: I prefer to write some documentation sooner rather than later. However, there doesn't seem to be any precedent for documenting unstable cargo features. I was wondering if it would be OK to add a new chapter to the Cargo book for unstable features? (Similar to the new chapter for rustdoc).

I have some thoughts on cleaning up the current implementation, but would love to hear if you have some ideas in mind already!

[alexcrichton]

Sure yeah I think two feature flags should work just fine! I wouldn't worry too much about rpath and lto, for now I think it's probably best to only accept that at the top-level. (like panic). I also think it's fine to start out with just dev and release. We can expand to future profiles later.

For the Profile struct itself yeah we're free to change it so long as we preserve the existing cargo metadata output. It doesn't have to literally retain the same structure though!

For docs I think you're right in that a new chapter is best, we haven't documented too many unstable cargo features yet!

[matklad]

What exactly are Cargo's backward-compatibility requirements?

So, the backwards compatibility story is interesting here! Profiles do not affect downstream crates at all, so I think we can actually break stuff here, if we really want to :-)

Should this support all 5 profiles?

I'd conservatively restricted new syntax to dev and release, just in case. Though, I think it makes sense to do what is easier now, and revisit this question closer to stabilization.

AFAIK, the only reason the existing Profile struct is serializable is to dump the JSON structure as part of the Artifact which is displayed for each artifact. The reason I ask is I was wondering if that can change at all? In particular, it exposes the "test" flag, which isn't a user-visible profile option.

I think it's better to preserve metadata output. The test flag is sort-of important, because you get two artifacts for lib.rs, one with and one without test. We actually look at at in intellij. However, I also think it does not necessary belong to the profiles per se, and probably should be a top-level field of an artifact instead? So, I think the path forward is to remove derive(Serialize) from Profile, and instead, when constructing an Artifact, use a specialized SerializedArtifact struct, sort-of how we wrap resolve for metadata. This alone should allow us to tweak profiles however we like, while preserving output. If we decide that test should not be a part of profile at all in JSON, we can move it to top-level, and pass test: false in profile to avoid breaking existing tools.

It also seems like we have 11 profiles currently: https://github.com/rust-lang/cargo/blob/681756252b718c4e638b3b8004bdc068b5529ae3/src/cargo/core/manifest.rs#L192-L204. I think this is a combinatorial explosion due to the fact that Profile includes test, doc, run_custom_build and check fields, which are more like "what to do" and not "what optimization flags to pass". I think a good first step here might be to try to extract these flag into a separate struct, add it as a field to Unit and try to replace branching on profile (like here) with branching on the value of this field.

EDIT: to expand a bit on the last point, I think one the problems of current profile implementation is that we make decisions in cargo, based on the current profile. I'd love to get rid of this profile-based decision making, and the above seems like a first step in that direction :)

[ehuss]

I think a good first step here might be to try to extract these flag into a separate struct, add it as a field to Unit and try to replace branching on profile (like here) with branching on the value of this field.

Excellent, that's exactly what I was thinking. It should hopefully simplify things quite a bit. There might be some tricky parts (like generate_targets which I think will need to return a triple for each target instead of just (target, profile) pair). I'll start down that path and see what the fallout is.

[matklad]

which I think will need to return a triple for each target instead of just

I think that this probably could generate Units directly? I am not sure, but it looks like historically compilation process operated on targets, and Units were a somewhat later addition, so some things still mention targets, although units make more sense.

[Boscop]

Btw, is there already an issue for "separate debug/release profiles for .cargo/config [build] settings"? E.g. I want "-Ctarget-feature=+crt-static" only in --release.

[matklad]

@Boscop : https://github.com/rust-lang/cargo/issues/2535, but we don’t even have a design for this feature, so it is unclear when/if this would be implemented.

[ehuss]

@matklad I have a few questions:

I'm finding the result of splitting up the existing Profile struct a little awkward, and not really gaining the benefits I hoped for. I have a new approach that I have been working on that I'm happier with, and just wanted to run it by you:

• Rename the existing Profile struct to TargetOptions to better reflect what it really is.
• Profiles will be responsible for creating TargetOptions, giving a nice, centralized location where all decisions are made. The current API for fetching them looks something like this (for now):

  pub fn target_opts(
  &mut self,
  pkg: &Package,
  target_kind: &TargetKind,
  mode: CompileMode,
  release: bool,
  ) -> &TargetOptions {...}

Some other questions:

1. At the end of the compilation, it prints this:

   Finished dev [unoptimized + debuginfo] target(s) in 0.44 secs

   I'm not sure what to print in the [unoptimized + debuginfo] part. In a workspace, different packages may have different settings (since it can be overridden). Any thoughts? Print something different? Don't print it if there are multiple packages?

2. The term "target" is overloaded (target architecture and cargo target), and can be a little confusing (both in the code and as a user). Any thoughts about using different terminology? I know it's asking a big thing, I'm just asking because I'm touching a lot of parts related to target selection. Maybe something to think about separately.

[matklad]

I'm finding the result of splitting up the existing Profile struct a little awkward

Hm, interesting. Another option is, instead of splitting profile, is to try to lift fields like run_custom_build to Unit directly.

Profiles will be responsible for creating TargetOptions, giving a nice, centralized location where all decisions are made.

Seems nifty!

I'm not sure what to print in the [unoptimized + debuginfo] part.

A nice start would be to use the settings from the root profile.

Any thoughts about using different terminology?

One option is to use triple for target architecture. But, no good solutions here. Cargo target is sort of crate as well...

[ehuss]

Question: Do overrides merge settings with higher-level definitions? Or does each override start over with all settings at default?

[profile.dev]
opt-level = 3

[profile.dev.overrides.image]
debug = false

[profile.dev.overrides."*"]
debug-assertions = false

Does image get opt-level=3 or 0? Does image get debug-assertions=false or true?

I would assume they merge (3 and false in this example), but I wanted to verify.

[matklad]

Yeah, merging is the most sane behavior I think.

[ehuss]

I was wondering if you could give any suggestions on how to handle the ownership of the struct with all of the target settings (what Profile is today). I'd like to avoid pre-computing them (like Profiles is today) because that ends up being very complex. I've tried quite a few different approaches, but I keep running into problems.

I would prefer to place the struct directly in the Unit, but that runs into many issues because Unit is copied in a large number of places (as keys for maps/sets). I tried brute-force cloning Units everywhere, but job_queue::Key doesn't like that (there are many issues with making Key non-Copy, and I don't think it can take a reference to the unit).

I tried leaving the settings as a reference in the Unit and keeping a cache in the Context. However, this cache needs to be mutable, but there are a few places that only have immutable references to the Context (unit_dependencies is the main place), and I was unable to get a mutable Context reference threaded through all the code (trouble with it getting captured in closures).

I feel like this shouldn't be so difficult, but Unit is intertwined all over. Any help would be appreciated.

[matklad]

It's hard to give any advice in the abstract, but I personally would have tried to keep unit as a simple key to what is compiled (package/target, architecture, special flags like test or custom build scripts), and to compute compiler flags on demand.

[ehuss]

compute compiler flags on demand

Ah, that's very helpful! I've gone down this road and it's working much better. Thanks!

[ehuss]

@matklad I'd like to write some tests that are more precise about exactly which commands get run (and verify no extra commands are run). However, with_stderr with -v will have problems when the order changes with dependencies. I'd also like to verify which options are used and more importantly which ones are not used for each command. I don't see anything that will meet those requirements. Do you know of a way to verify the output like that?

If not, do you have any ideas on a reliable way to implement that? I was thinking of something simple, along the lines of listing the commands and options (and options that should not be there), and it would just check the output from -v, ignoring the order (assuming out-of-order bugs are unlikely).

[matklad]

@ehuss I think multiple with_stderr_contains and with_stderr_does_not_contain usually do the trick. See https://github.com/matklad/cargo/blob/76b0a8a02ee07e9e80df3b4ac1fb5192f4ef2044/tests/testsuite/rustc_info_cache.rs for an example.

It's also to submit a PR first, and add tests later ;)

[Boscop]

If I want to build all external crates in release mode (when building my crate in debug mode), would this require writing a profile for every external crate? Or is there a catch-all way to do this? E.g. to apply a profile to "all crates that aren't this crate", "all crates outside of the workspace of this crate", or "all crates except the current crate and these: [..]"?

[matklad]

@Boscop the current syntax for catch all is [profile.dev.overrides."*"]

[Boscop]

So this applies to "all crates that aren't this crate", right? I would really like to have an option for "all crates outside of the workspace of this crate", too, because I'm often editing all of the crates in a workspace and want recompilation to stay as fast as possible, but build 3rdparty deps with --release..

[ehuss]

@Boscop "*" only applies to non-workspace packages. For reference, the (limited) docs are at https://github.com/rust-lang/cargo/blob/master/src/doc/src/reference/unstable.md#profile-overrides

[Boscop]

Thanks, but it doesn't seem to be working with nightly 03-06.. Any idea which nightly is the earliest to support this? (I only have a 16kb/s connection for now, that's why I'm still on the old nightly..)

When I add it to Cargo.toml, I get warning: unused manifest key: profile.dev.overrides.

[ehuss]

@Boscop it won't show up in nightly until someone (manually) updates Cargo in the Rust repository. I don't know when that will happen.

[Boscop]

@ehuss Thanks, I updated to nightly-05-04, and I have this in my Cargo.toml:

cargo-features = ["profile-overrides"]

[profile.dev.overrides."*"]
opt-level = 3

But it still complains:

error: failed to parse manifest

Caused by:
  feature `profile-overrides` is required

consider adding `cargo-features = ["profile-overrides"]` to the manifest

But I have that key, why am I still getting the error?

[matklad]

@Boscop cargo-features should be the top-level key in Cargo toml (i.e, the first line, not nested under [project]). We need to give a better error message here

[Boscop]

Ah, thanks, I put it before [package] now and it works :)

[Boscop]

But my application has the exact same CPU usage in debug build (opt-level=1) with that setting and without. Does the setting apply to both dev and release profiles?