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

Document interaction between custom profiles and overrides #13590

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

vlovich
Copy link

@vlovich vlovich commented Mar 15, 2024

Not sure if I actually captured the behavior correctly.

What does this PR try to resolve?

Do custom profiles keep package overrides requiring the exact same set to be duplicated if a setting needs to be changed, or are the overrides discarded for a custom profile & the overrides need to be repeated if any settings need to be persisted?

How should we test and review this PR?

N/A - just need to document current behavior.

Additional information

N/A

Not sure if I actually captured the behavior correctly.

Do custom profiles keep package overrides requiring the exact
same set to be duplicated if a setting needs to be changed, or are the overrides discarded for a custom profile & the overrides need to be repeated if any settings need to be persisted?
@rustbot
Copy link
Collaborator

rustbot commented Mar 15, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @epage (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added A-documenting-cargo-itself Area: Cargo's documentation S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 15, 2024
@vlovich vlovich marked this pull request as ready for review March 15, 2024 16:35
@vlovich
Copy link
Author

vlovich commented Mar 15, 2024

@rustbot review

@ehuss
Copy link
Contributor

ehuss commented Mar 15, 2024

Overrides should be inherited. If you are having problems with the inheritance, I'd recommend opening an issue with a reproduction showing what doesn't seem to be working.

@vlovich
Copy link
Author

vlovich commented Mar 15, 2024

@ehuss fixed the wording to reflect the correct state.

Comment on lines +372 to +373
NOTE: Custom profiles inherit package overrides as well and those take precedence
over the default custom profile settings. See the [overrides](#overrides) section for more.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about adding a sentence at line 250, instead of this?

- setting is not specified.
+ setting is not specified. Note that a custom profile inherits everything from
+ the other profile, including package and build [overrides](#overrides).

@@ -457,6 +460,26 @@ match wins):

Overrides cannot specify the `panic`, `lto`, or `rpath` settings.

Overrides inherit to custom profiles and take precedence unless explicitly overriden.
For example, in the below example package `foo` will still be built with `opt-level = 1`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to match the current behavior. Check this test as a reference:

fn overrides_with_custom() {
let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.0.1"
edition = "2015"
authors = []
[dependencies]
xxx = {path = "xxx"}
yyy = {path = "yyy"}
[profile.dev]
codegen-units = 7
[profile.dev.package.xxx]
codegen-units = 5
[profile.dev.package.yyy]
codegen-units = 3
[profile.other]
inherits = "dev"
codegen-units = 2
[profile.other.package.yyy]
codegen-units = 6
"#,
)
.file("src/lib.rs", "")
.file("xxx/Cargo.toml", &basic_lib_manifest("xxx"))
.file("xxx/src/lib.rs", "")
.file("yyy/Cargo.toml", &basic_lib_manifest("yyy"))
.file("yyy/src/lib.rs", "")
.build();
// profile overrides are inherited between profiles using inherits and have a
// higher priority than profile options provided by custom profiles
p.cargo("build -v")
.with_stderr_unordered(
"\
[LOCKING] 3 packages
[COMPILING] xxx [..]
[COMPILING] yyy [..]
[COMPILING] foo [..]
[RUNNING] `rustc --crate-name xxx [..] -C codegen-units=5 [..]`
[RUNNING] `rustc --crate-name yyy [..] -C codegen-units=3 [..]`
[RUNNING] `rustc --crate-name foo [..] -C codegen-units=7 [..]`
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [..]
",
)
.run();
// This also verifies that the custom profile names appears in the finished line.
p.cargo("build --profile=other -v")
.with_stderr_unordered(
"\
[COMPILING] xxx [..]
[COMPILING] yyy [..]
[COMPILING] foo [..]
[RUNNING] `rustc --crate-name xxx [..] -C codegen-units=5 [..]`
[RUNNING] `rustc --crate-name yyy [..] -C codegen-units=6 [..]`
[RUNNING] `rustc --crate-name foo [..] -C codegen-units=2 [..]`
[FINISHED] `other` profile [unoptimized + debuginfo] target(s) in [..]
",
)
.run();
}

@weihanglo
Copy link
Member

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-documenting-cargo-itself Area: Cargo's documentation S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants