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

Use a single profile set per workspace #3249

Merged
merged 1 commit into from
Nov 4, 2016

Conversation

matklad
Copy link
Member

@matklad matklad commented Nov 3, 2016

This aims to close #3206.

I have not figured out how to do this 100% backward compatibly, that's why I want to discuss this separately, although a related PR (#3221) is already in flight.

The problem is this: suppose that you have a workspace with two members, A and B and that A includes a profiles section and B does not. Now, mentally cd inside B and run cargo build. Today, Cargo will use a default profile. We want it to use a profile from A. So here the silent behavior switch will inevitably occur :( Looks like we can't detect this situation.

So this PR just switches the behavior to always use root profiles, and to print a warning if a non-root package specifies profiles. Feel free to reuse it in any form for #3221 if that's convenient!

@rust-highfive
Copy link

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member

@bors: r+

Thanks!

Seems reasonable to me in terms of backcompat story

@bors
Copy link
Contributor

bors commented Nov 4, 2016

📌 Commit 151f12f has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Nov 4, 2016

⌛ Testing commit 151f12f with merge f9ddf56...

bors added a commit that referenced this pull request Nov 4, 2016
Use a single profile set per workspace

This aims to close #3206.

I have not figured out how to do this 100% backward compatibly, that's why I want to discuss this separately, although a related PR (#3221) is already in flight.

The problem is this: suppose that you have a workspace with two members, A and B and that A includes a profiles section and B does not. Now, mentally `cd` inside B and run `cargo build`. Today, Cargo will use a default profile. We want it to use a profile from A. So here the silent behavior switch will inevitably occur :( Looks like we can't detect this situation.

So this PR just switches the behavior to always use root profiles, and to print a warning if a non-root package specifies profiles. Feel free to reuse it in any form for #3221 if that's convenient!
@matklad
Copy link
Member Author

matklad commented Nov 4, 2016

Should this be tagged with relnotes?

@alexcrichton alexcrichton added the relnotes Release-note worthy label Nov 4, 2016
@bors
Copy link
Contributor

bors commented Nov 4, 2016

☀️ Test successful - cargo-cross-linux, cargo-linux-32, cargo-linux-64, cargo-mac-32, cargo-mac-64, cargo-win-gnu-32, cargo-win-gnu-64, cargo-win-msvc-32, cargo-win-msvc-64
Approved by: alexcrichton
Pushing f9ddf56 to master...

@bors bors merged commit 151f12f into rust-lang:master Nov 4, 2016
@alexbool
Copy link

alexbool commented Nov 7, 2016

Why can't workspace members override workspace profiles? (My project reiled on different profiles in different crates and now it's broken)
That would make this PR backwards-compatible.

@alexcrichton
Copy link
Member

@alexbool that's a possible extension we could implement, yes, but wasn't the intention of workspaces. It leads to surprising behavior currently when you switch between members and everything is recompiled due to profile changes.

@alexbool
Copy link

alexbool commented Nov 8, 2016

@alexcrichton I tend to agree with you, but what makes me sad is that it could be done with no breakage...

@JeanMertz
Copy link

It might be best to open up a new issue for this, but I've been working in a workspace that includes both a server-side crate, and a client-side crate using wasm. I'd like to have the wasm-based crate to produce the smallest possible binary using opt-level = 5, but the server-side crate should be as fast as possible, irregardless of the binary size.

This seems like a valid use-case to allow per-workspace-member profile overrides, which I believe isn't possible yet:

$ cargo build
profiles for the non root package will be ignored, specify profiles at the workspace root

@ehuss
Copy link
Contributor

ehuss commented Jun 7, 2019

@JeanMertz You can use Profile Overrides to set the profile per-package. That will only apply to a single package (not dependencies). If you want to toggle between settings that apply to the entire crate graph, you can do tricks with Config Profiles and environment variables (or swapping .cargo/config files). Soon there will be named profiles to make that easier.

@LaylBongers
Copy link

LTO is one of the recommended options for WASM and doesn't work in overrides right now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Release-note worthy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Workspaces: Cargo profiles should be shared among all workspace members.
8 participants