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

Tracking issue for RFC 2282 - Cargo profile dependencies #48683

Closed
Manishearth opened this issue Mar 2, 2018 · 72 comments
Closed

Tracking issue for RFC 2282 - Cargo profile dependencies #48683

Manishearth opened this issue Mar 2, 2018 · 72 comments

Comments

@Manishearth
Copy link
Member

@Manishearth Manishearth commented Mar 2, 2018

This is a tracking issue for RFC 2282 (rust-lang/rfcs#2282).

Cargo issue: rust-lang/cargo#5298

Steps:

(cc @rust-lang/cargo for mentorship instructions)

Unresolved questions:

  • We can bikeshed the names of the new keys that were introduces
@Boscop
Copy link

@Boscop Boscop commented Apr 4, 2018

Any update on this? :)

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented Apr 4, 2018

Someone needs to implement it.

@ehuss
Copy link
Contributor

@ehuss ehuss commented Apr 5, 2018

If you're looking for someone to work on this I'd be happy to help!

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented Apr 5, 2018

Sure! I don't have the bandwidth to mentor this but feel free to try it out. Might want to file an issue on the cargo side to coordinate the implementation work.

@ehuss
Copy link
Contributor

@ehuss ehuss commented Apr 28, 2018

Some issues came up during implementation.

  • @alexcrichton mentioned that the package name could support full-spec syntax (crates.io/foo, etc.). It looks like it should be easy to support, just let me know if you'd like me to add it! (Currently it just matches the name.)
  • I briefly discussed with @matklad the possibility of having a dedicated profile used for scripts. It would be easy to add, and would remove "build-override" which is essentially a pseudo-profile (and seems a little confusing to me). It would also decouple building build scripts from the --release flag. Let me know if you want to consider this route!
@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented Apr 28, 2018

Full spec sounds great.

The problem with having a dedicated profile for scripts is that you may wish them to be compiled differently in debug vs release. build-override lets you do both.

The original proposal (not the one we RFCd) allowed nesting of profiles which would have made this work well (you declare a new profile, and link it in as the build profile for an existing one)

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented Apr 28, 2018

(Also to me it seems like having a build profile just makes the whole profile vs workflow deal even messier -- we already are in a situation where the test/bench profiles are a bit weird)

@ehuss
Copy link
Contributor

@ehuss ehuss commented Apr 28, 2018

Yep! I was just meaning that it can probably be improved on without too much effort. I think there was some concern during the RFC process that shared dependencies would be a problem, but they shouldn't be. I think @matklad had more thoughts on the profile/workflow stuff, so I'll let him say more.

@matklad
Copy link
Member

@matklad matklad commented May 2, 2018

may wish them [build scripts] to be compiled differently in debug vs release.

@Manishearth could you give an example for this? An important practical reason for compiling build scripts exactly the same in dev and release is to avoid compiling syn and friends twice, when you do , say, cargo test followed by cargo bench. You can achieve this with current design of build_overrides, but you'll have to explicitly override most of options for build profiles, because, by default, dev and release are quite different.

More broadly, I think the following properties make a dedicated build profile an interesting option to consider:

  • It makes sense to use a single profile for build scripts, so as to compile them once for all cargo invocations.

  • Semantically, it makes little sense to use the same profile for build script and library code. This is easily understood in the context of cross compilation: you compile library for the target arch, and the build script for host arch, and they are executed on completely different machines. The motivations for picking particular compiler flags must be different in these two separate worlds.

  • Similarly to the previous bullet, the build scripts profile settings do not affect the generated artifacts in any way.

  • The best defaults for the build scripts are likely to be different. It seems to me that we want something like the following:

    [profile.build]
    # low hanging optimization fruits which shouldn't affect build time much
    opt-level = 1 
    # save build time and disk space by not emitting huge amount of debug-info
    debug = false 
    # speed up both compile time and runtime
    debug-assertions = false
    overflow-checks = false
    # build scripts are small and rarely modified
    incremental = false

    this differs significantly from both dev and release profiles.

The interaction with workflows is also interesting here: in a nutshell, a workflow would allow one to compile and run some custom rust code on the host to do execute custom tasks, like cargo subcommands, but locally to the project. And this code needs to be compiled with some optimization settings and such :)

I envision the build profile as the profile for all of build scripts, proc macros and workflow tasks. All three are compiled on the host, could be shared across cargo build and cargo build --release and have similar runtime/compile-time trade offs.

It is true that test, bench and especially doc profile (which exists and does nothing) are messy today. But it seems that, unlike those profiles, the suggested build one actually has compelling reasons to exist, as discussed above. Also, if we compare build_overrides with the build profile, I would argue that the latter has less overall complexity, because it reuses the existing concept, while the former is one is a special case with a special syntax and such.

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented May 3, 2018

could you give an example for this?

My dev workflow may be one where I'm recompiling often (but I don't edit the build scripts) so I need the build scripts to be fast, but my release workflow may be one where I only run it occasionally (incremental builds are less common), so build scripts should be compiled in dev.

This isn't everyone, but this could be someone.

An important practical reason for compiling build scripts exactly the same in dev and release is to avoid compiling

This is an argument for why the defaults should be such that build profiles are the same , not an argument for why we shouldn't expose this functionality.

Firefox, for example, would probably want an optimized build script with an unoptimized libsyntax/syn/whatever.

you compile library for the target arch, and the build script for host arch, and they are executed on completely different machines. The motivations for picking particular compiler flags must be different in these two separate worlds.

IMO it's not relevant to bring cross compilation into the picture here because cross compilation works differently -- dependencies are not shared. If cargo was rearchitected so that shared deps between build scripts and the main build may be compiled into separate artifacts, this makes more sense, but as it stands, these are shared, and IMO it's easier to reason about these shared dependencies when you have a single profile instead of two.

@ehuss
Copy link
Contributor

@ehuss ehuss commented May 3, 2018

these are shared

Not anymore. Shared deps are now compiled separately. (The graph for features/versions is still unified, though.)

@ehuss
Copy link
Contributor

@ehuss ehuss commented May 6, 2018

I had some questions on how to feature-gate profiles in config files. I can imagine a variety of ways to implement it:

  • Command-line option (-Z profile-config)?
  • Feature flag in Cargo.toml (cargo-features = ["profile-config"])?
  • Some kind of flag in the config file itself?
  • Should it ignore entries in config if you don't specify the flag? Or warn? Or error?

I can imagine if you put a profile in a global config it could be really fussy. Let me know if you have any ideas.

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented May 6, 2018

I'd have some kind of flag in the config itself.

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented May 6, 2018

But that's not a strong opinion

@ehuss
Copy link
Contributor

@ehuss ehuss commented May 7, 2018

I have some more questions (cc @alexcrichton @matklad).

  • I had some questions/doubts about warnings for config files. In general, it looks like config files never warn about misspelled/unused keys. Is that intentional for some sort of forward-compatibility?
    • My implementation currently does not warn about unused override package names in config files ([profile.dev.overrides.foo]). I was thinking someone might place an entry in a global config, which would cause warnings in all projects not using that package. Sound reasonable?
    • Let me know if I should add warnings or errors for the following:
      • Unknown profile names ([profile.misspelled]).
      • Unsupported profile names (test, bench, doc).
      • Unknown keys in a profile.
  • Config files do not support merging values with mixed data types. For example, if you have lto = true in one config and lto = 'thin' in another, it fails. How much of a problem is this? (This affects lto, opt-level, and debug.)
  • If we go the route of adding unstable feature flags to config files, there are some design questions I have. To start with:
    • Should it be an error to specify a feature flag when using a stable release? Or should it warn and ignore the new settings? Or maybe just silently ignore? My concern is that it would be very cumbersome to use unstable features in a global config file (since running on stable would always fail or print annoying warnings).
    • Should unknown feature names be ignored, warn, or error?
  • (EDIT) Another idea for handling warnings would be to silently ignore unless you run with verbose.
@alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented May 8, 2018

Aha some excellent questions @ehuss! Lemme see if I can dig in...

  • It's true yeah that we're quite lenient with config files. I don't think we can warn about unused (aka misspelled) keys here though in all situations. Not all builds may use all configuration, so we'd probably be going a bit too far out of our way to get this working. Now that being said I think profiles specifically could probably helpfully reject any configuration other than release/dev or any other obvious missteps, I think it's only possible to reject configuration that couldn't possibly be used though rather than "this isn't used for this particular build"

  • Hm... We may just want to tweak how configs are merged and say that a string can override a string or something like that. I don't think there's necessarily a downside to that approach I think, but do you forsee any problems with that?

  • I think it's probably safe to just require features to be enabled in the local cargo build (via the CLI maybe?) rather than the configuration. If the local features aren't enabled but configuration is detected then we could print a warning or something like that saying that the features aren't adequately enabled.

@ehuss
Copy link
Contributor

@ehuss ehuss commented May 8, 2018

profiles specifically could probably helpfully reject any configuration other than release/dev or any other obvious missteps,

My concern with rejecting with an error is that if in the future cargo supports additional profile names, it would make it impossible to use the new names or keys in a global config and continue using older versions of cargo. I would feel more comfortable with just warnings (profile `foo` is not supported or profile key `bar` is not supported).

string can override a string

Do you mean string can override an int? If so, that sounds fine to me.

via the CLI maybe?

That seems fine with me, too. Would this be a command-line flag that is only necessary during the unstable period?

My concern with unconditionally warning if you have a profile in a global config and you don't specify the command-line option is that you will end up with seeing that warning everywhere you don't specify the flag, which would be rather noisy. I kinda like the idea of only warning with -v since that would avoid the noise. That may not be very discoverable, though. ☹️ How common do we think profiles in global configs will be? If it's extremely rare, maybe the noisiness doesn't matter.

I had another question: What should the precedence be? This comment suggests it should be manifest over config. However, incremental uses config over manifest. The RFC doesn't specify. I was actually thinking it would be config over manifest to give the user control over what happens. With manifest over config their only alternative would be to edit the manifest. If config takes precedence, I wouldn't worry too much about a user globally specifying an important flag like panic (that just seems weird to me).

@alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented May 8, 2018

I would feel more comfortable with just warnings

An excellent point and sounds good to me!

Do you mean string can override an int? If so, that sounds fine to me.

Oops yes, indeed!

Would this be a command-line flag that is only necessary during the unstable period?

Correct!

I'd also be ok not warning for now. It may lead to some confusion but you're right in that it's likely less than otherwise

What should the precedence be?

I definitely agree it's config over manifest because you modify your local config as opposed to the manifest which is shared amongst all users.

@ehuss
Copy link
Contributor

@ehuss ehuss commented Jun 9, 2018

Part 2 that includes config profiles has landed on nightly. The (minimal) documentation is at: https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#config-profiles

@ehuss
Copy link
Contributor

@ehuss ehuss commented Sep 9, 2019

cc #63484, a surprising result of using profile overrides.

@est31
Copy link
Contributor

@est31 est31 commented Sep 10, 2019

if we want to put this feature through its paces we would ideally all but delete src/bootstrap/bin/rustc.rs

For reference, this is happening in #64316 .

also cc #64324

@est31
Copy link
Contributor

@est31 est31 commented Sep 26, 2019

Update: #64316 has been merged.

I wonder whether this feature can be stabilized. Is it ready for an FCP yet?

@ehuss
Copy link
Contributor

@ehuss ehuss commented Sep 26, 2019

To be clear, there are two or three features here which may be possible to stabilize independently.

Some blockers:

profile-overrides:

  • I'd like to wait for named profiles to land, to make sure there aren't any unforeseen interactions. It looks like it should be OK, but that PR needs to land first at a minimum. It looks like it is getting close to finished. That PR also changes some behavior here.
  • I plan to rename override to package once 6989 is done.
  • I'm not sure whether or not this should wait for build-profile. Probably not, but should think some more about it.

config-profile:

build-profile

  • The PR is blocked needing more design work on directory restructuring. Some of that is done here, but there needs to be mitigations and migrations for existing tools' assumptions about the layout.
bors added a commit to rust-lang/cargo that referenced this issue Oct 15, 2019
Rename `overrides` to `package` in profiles.

Renaming per the discussion at rust-lang/rust#48683 (comment).

I left backwards-compatibility in case anyone is using it, since it required very little effort.

cc @da-x, FYI.
@est31
Copy link
Contributor

@est31 est31 commented Oct 24, 2019

@ehuss now that rust-lang/cargo#6989 and rust-lang/cargo#7504 have been merged, can we maybe think about stabilizing the core profile-overrides feature? I'm mainly interested in the "only optimize dependencies" use case and it seems that feature alone would unlock it.

@elichai
Copy link
Contributor

@elichai elichai commented Oct 24, 2019

Almost opened a new issue and then saw this.
I want to change globally the default that all release builds will also be built with -Clto.
this isn't easy to do in a shell function without having a complicated one (i.e. requires checking on cargo install if --debug is set and on cargo build if --release is set)

@ehuss
Copy link
Contributor

@ehuss ehuss commented Nov 4, 2019

The cargo team discussed stabilizing the profile-overrides side, and are generally positive. However, I remembered an issue that I forgot to bring up, and wanted to see what people think.

When defining an override for a package, it can be awkward when the thing you want to override is split across multiple crates. For example, if I wanted to change a couple settings for rand, it may need to list out profiles for 3 or 4 different crates. Another example is specifying the override for the standard library. The user may not even know which crates comprise the standard library, so they won't even know which ones to override.

What do people think about that? Should there be some kind of capability to apply an override to dependencies of a crate? Or maybe a way to specify multiple crates for a single override? Or is this a niche concern, and almost everyone will just use "*"?

I think it might be good to avoid situations like this:

[profile.dev.package.rand]
opt-level = 2
debug-assertions = false
overflow-checks = false

[profile.dev.package.rand_core]
opt-level = 2
debug-assertions = false
overflow-checks = false

[profile.dev.package.rand_chacha]
opt-level = 2
debug-assertions = false
overflow-checks = false
@kornelski
Copy link
Contributor

@kornelski kornelski commented Nov 4, 2019

Could a crate like rand say that its dependencies should inherit its own settings?

Otherwise Cargo and crates-io may need to develop some concept of a published group of crates, and that's probably whole another can of worms.

@est31
Copy link
Contributor

@est31 est31 commented Nov 4, 2019

Thanks for pushing this forward, @ehuss !

@kornelski right now, non-root crates can't even override their own settings. The current profile overrides feature only applies to the top level crate's Cargo.toml, to be specific to the root Cargo.toml of the top level crate's workspace. Cargo displays a warning if it finds non-root profile overrides in the same workspace, but it silently ignores profile overrides of dependencies.

I think it's a good idea to provide such a feature (I personally would enable it in lewton to always optimize it, because lewton is very slow in debug mode) and @matklad has also expressed interest in it. Should the current behavior be changed? What if a crate only wants to override something for local development?

@ehuss

What do people think about that? Should there be some kind of capability to apply an override to dependencies of a crate? Or maybe a way to specify multiple crates for a single override? Or is this a niche concern, and almost everyone will just use "*"?

You have a point that you quickly get into the situation of repetitive sections. In the only place where I use profile-overrides, I have copy-pasted the same section for * as for one crate in my workspace.

The same time though, the more features one adds, the harder a Cargo.toml becomes to parse for humans. A Cargo.toml with tons of repetition isn't easy to parse either though. IDK. These features seem they can be added in a backwards compatible fashion as well.

@kornelski
Copy link
Contributor

@kornelski kornelski commented Nov 4, 2019

@est31 I'm not sure if dependencies should be able to set any specific settings for themselves. I was thinking about a flag like:

[package]
name = "rand"
if-my-settings-are-overriden-then-also-override-my-deps = true

or maybe:

[package]
name = "rand"

[dependencies.rand_chacha]
version = "0"
profile = "inherit" # inherit being the only value accepted on crates-io

but this raises a question — what happens if two different crates do this to the same dependency. Whose settings "win"?

@est31
Copy link
Contributor

@est31 est31 commented Nov 4, 2019

I'm not sure if dependencies should be able to set any specific settings for themselves.

Only talking about the defaults here. Of course, the leaf crate should always have final say here. @matklad spoke in even weaker terms of suggestions.

I was thinking about a flag like

Oh I see what you want. It isn't even strictly a feature to group crates together like you would group all of the rand crates together. You could also say that e.g. an inflate implementation maintained by a different organization should get the same options as you because your speed depends on inflate.

raises a question — what happens if two different crates do this to the same dependency. Whose settings "win"?

I'd propose that if crate A (transitively) depends on crate B and both depend on C, then A's settings override those of B. If there is no hierarchy between A and B, cargo could create an error message or at least a warning.


Anyways, I believe that these discussions shouldn't be blocking stabilization.

@ehuss
Copy link
Contributor

@ehuss ehuss commented Nov 15, 2019

A proposal to stabilize the profile-override part has been posted at rust-lang/cargo#7591.

bors added a commit to rust-lang/cargo that referenced this issue Dec 2, 2019
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.
@ehuss
Copy link
Contributor

@ehuss ehuss commented Jan 23, 2020

The proposal to stabilize the config-profile part has been posted at rust-lang/cargo#7823.

bors added a commit to rust-lang/cargo that referenced this issue Jan 31, 2020
Stabilize config-profile.

This is a proposal to stabilize config-profiles. This feature was proposed in [RFC 2282](rust-lang/rfcs#2282) and implemented in #5506. Tracking issue is rust-lang/rust#48683.

This is intended to land in 1.43 which will reach the stable channel on April 23rd.

This is a fairly straightforward extension of profiles where the exact same syntax from `Cargo.toml` can be specified in a config file. Environment variables are supported for everything except the `package` override table, where we do not support the ability to read arbitrary keys in the environment name.
@jonas-schievink
Copy link
Member

@jonas-schievink jonas-schievink commented Jan 31, 2020

Is there anything left to do here or can this be closed?

@lnicola
Copy link
Contributor

@lnicola lnicola commented Jan 31, 2020

I want to mention an issue I've just ran into: I tried to use build-overrides to speed up release builds, but some crates ended up being built twice because they were both build- and run-time dependencies. That's not ideal, but it's understandable, I suppose.

@ehuss
Copy link
Contributor

@ehuss ehuss commented Jan 31, 2020

Yes, this can be closed now.

@lnicola That is the intended behavior.

@elichai
Copy link
Contributor

@elichai elichai commented May 9, 2020

Hi,
Now that we can override profiles in the config file, anyway we can get profile.*.rustflags too?
I'd like to pass specific compiler flags only when compiled with release profile (like target-cpu=native)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet