rust-lang / cargo

The Rust package manager
https://doc.rust-lang.org/cargo
Apache License 2.0
12.75k stars 2.42k forks source link

Internally storing profiles as special-cased booleans #4140

Closed kornelski closed 2 years ago

kornelski commented 7 years ago

I wanted to fix a pet peeve of mine (cargo bench reports it finished building a "release" profile when it actually used a "bench" profile), and found that it's not straightforward to do:

  1. Cargo doesn't use a reference to the Profile object to track which profile was used,
  2. and profiles.bench is not returned by lib_profile() used in the end condition of drain_the_queue() (that seems like a bug stemming from lack of (1).)

The profile setting appears to be spread over booleans in Context, BuildConfig and global-ish Profile objects. In some places Cargo uses a set of flags to "guess" what profile was used (Context::lib_profile()) instead of keeping a reference to an actual Profile object used. In other places all profile information is reduced to a single boolean flag if self.is_release { "release" } else { "dev" }.

I've got a hunch that these multiple boolean flags spread throughout the code are a source of needless complexity and quirks (like the "test" and "bench" profiles vaguely overlapping with identity of "dev" and "release" profiles).

I would like to refactor it, but I'd like your guidance whether it makes sense and how it should be implemented.

Existence of lib_profile()/lib_or_check_profile()/build_script_profile() makes me think that there are actually two distinct concepts of a "profile" in Cargo:

Currently the first kind is represented by booleans in Context, BuildConfig (release,is_release,check) and Profile (test/check/doc), and the second kind is the remainder of the Profile struct (opt_level/rustc_args, etc.).

I'd like to replace most code in form let foo = if profile.foo { this } else { that } with let foo = profile.foo().

I'm thinking about refactoring it this way:

Does that make sense? Will that work?

I haven't looked closely at Workspaces yet. Are there any complications there?

Anything else that I'm missing?

alexcrichton commented 7 years ago

Yeah the implementation of profiles in cargo I feel like definitely haven't stood well to the test of time, and refactoring would be most welcome! There's lots of trickery to how the profiles are handled but I think it's accurate to say that we attempted to interpret requests on the command line to translate to intermediate structures. These structures though are then reinterpreted to extract the original command line request in a bunch of places which creates this sort of ad-hoc behavior.

I think that the direction you're going in sounds great. Basically bundle up the actual request (what's going on) and then just interpret that at all the relevant locations (with methods ideally, not duplicated logic).

I believe that @matklad was thinking about the design of profiles lately as well, so he may wish to chime in as well!

matklad commented 7 years ago

Yeah, I've been thinking about profiles quite a bit, but without a definitive conclusion. I'll dump my current thoughts here then, don't expect to find a lot of structure :)

The biggest problem with profiles imo is not the implantation, but the fact that almost nobody understands which profile is activated when. For example, cargo test will build the [lib] crate of the package with dev profile, the [test] crates with test profile (and link it with the dev [lib]), and then it will build [lib] again in test profile for unit tests! Logically, cargo test --release builds everything in bench profile.

This in turn makes all features which should interact with profiles difficult to implement. Examples of such features are "custom profiles", "always build deps with optimizations", "select feature flags based on profiles".

I would say that we basically need profiles 2.0 in Cargo :), but I have only vague ideas how they should look like :(

Fundamentally, profile is a function which maps each crate in the DAG of dependencies to "auxilary" compiler command line flags.

The great things about current systems are:

The bad things about current system:

So, for profiles 2.0 I think we first of all need some sort of toml DSL to specify functions from crates to command line flags. Then, we need some kind of mechanism to select which function is used for the current compilation. Certainly, we need to preserve --release flag as way to do this. We should drop test, doc, bench and other obscure profiles altogether. So that leaves us with just two default profiles: dev and release. For anything else, you probably can create a custom profile.

There's also a desire to be able to influence downstream profiles from a library. For example, if I write a library which is very slow without optimizations, I would like to be able to say "compile this library with optimizations even in dev profile". Not sure how this should interact with custom profiles though.

cc @rust-lang/cargo

ehuss commented 2 years ago

I'm going to close as profiles were rewritten in 2018. The final "Finished" line still may not be completely accurate due to overrides, but that is unavoidable and would require rethinking what to display there.