Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Internally storing profiles as special-cased booleans #4140

Closed
kornelski opened this issue Jun 7, 2017 · 3 comments
Closed

Internally storing profiles as special-cased booleans #4140

kornelski opened this issue Jun 7, 2017 · 3 comments
Labels
A-profiles Area: profiles C-cleanup Category: cleanup within the codebase

Comments

@kornelski
Copy link
Contributor

kornelski commented Jun 7, 2017

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:

  • One that describes a high-level user choice (a combination of --release flag and TargetKind) — the "what are we building" part

  • and the other kind that just describes low-level compiler settings (for compiling lib, deps, build.rs) — the "how" part.

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:

  • Make a ProfileSelection object (it's not a great name, bikeshedding welcome) that represents the high-level profile chosen by the user (one that corresponds to profile names from Cargo.toml: "dev","release","bench", etc.). It would be a single object for the whole build that encapsulates all the booleans based on the --release flag, subcommand, target, etc.

  • Remove the test, doc and check fields of Profile and turn it into a "dumb" anonymous collection of settings for rustc. Probably rename it to ProfileSettings.

  • Make ProfileSelection know how to create appropriately-configured instance of ProfileSettings for each part of the build (lib_profile()/lib_or_check_profile()/build_script_profile()). It'll probably replace or encapsulate the Profiles (plural) type as well.

  • Replace all uses BuildConfig.release/.test, fn generate_targets(release: bool) with an instance of ProfileSelection or ProfileSettings as appropriate.

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
Copy link
Member

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
Copy link
Member

matklad commented Jun 7, 2017

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:

  • not exposing the raw compiler flags to the user

  • --release switch

  • reasonable defaults (though we maybe want to compile depedencies with some optimizations even in dev mode)

The bad things about current system:

  • there's no single profile for the compilation. During test especially, different parts of the code compile with different profiles.

  • profiles expose to much. There shouldn't be a test profile ideally, you should be able to run tests or application itself both in dev and in release mode, at least by default.

  • you can't specify arbitrary function (for example, for per-package profiles inside a worksapce, or for always building a specific dependency with all optimizations).

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

@carols10cents carols10cents added A-profiles Area: profiles C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` C-cleanup Category: cleanup within the codebase and removed C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` labels Aug 30, 2017
bors added a commit that referenced this issue Apr 27, 2018
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.
dwijnand added a commit to dwijnand/cargo that referenced this issue May 6, 2018
See the NOTE block: this is still "a bit inaccurate", but perhaps it's
more accurate than just "release"/"dev".

Refs rust-lang#4140
@ehuss
Copy link
Contributor

ehuss commented Jul 3, 2022

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.

@ehuss ehuss closed this as not planned Won't fix, can't repro, duplicate, stale Jul 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-profiles Area: profiles C-cleanup Category: cleanup within the codebase
Projects
None yet
Development

No branches or pull requests

5 participants