feat: support rustflags per profile by zhamlin · Pull Request #10217 · rust-lang/cargo

zhamlin · 2021-12-19T04:15:29Z

Fix for issue #7878

rust-highfive · 2021-12-19T04:15:32Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

Please see the contribution instructions for more information.

alexcrichton

Thanks for the PR for this! I think there's also some documentation in src/doc which may need updating as well.

Given that this is somewhat of a major feature as well I think I would personally prefer to err on the side of placing this behind a feature gate to start with. Others on the team may feel differently though!

src/cargo/core/profiles.rs

ehuss · 2021-12-20T15:32:39Z

I think it would be good to also talk about how this works with rustdocflags. If there are flags that are required for correctness, then cargo test --doc would fail. I'm not suggesting that this should add rustdocflags, though. Just to consider what the impact will be.

I'm a little nervous adding this, as it is a very low-level thing which we haven't exposed in Cargo.toml before. One of my biggest concerns is that it can cause backward-compatibility hazards. For example, when Cargo adds new features or changes the way it invokes rustc, that can interfere with pre-existing flags. In a sense, this seems to be an end-run around Cargo not exposing the workflows that users want.

I'm not concerned enough to block it, as I can see its utility. However, I think we should tread very carefully.

alexcrichton · 2021-12-21T15:25:43Z

I share many of @ehuss's concerns as well, which is primarily why I'd like to see this feature-gated to start off with.

joshtriplett · 2021-12-24T08:36:09Z

I agree that this needs a feature-gate initially.

alexcrichton

Can you add a test that this is unstable?

Additionally I think that support also needs to be added for CARGO_PROFILE_RELEASE_RUSTFLAGS=... by configuring profiles via env vars.

src/doc/src/reference/unstable.md

tests/testsuite/profile_config.rs

bors · 2022-01-04T18:03:44Z

☔ The latest upstream changes (presumably #10088) made this pull request unmergeable. Please resolve the merge conflicts.

zhamlin · 2022-01-05T14:13:59Z

Thanks for the review.

Fixed the issues you mentioned and added a test to verify passing rustflags via CARGO_PROFILE_RELEASE_RUSTFLAGS, it appears to be working without any additional code changes.

alexcrichton · 2022-01-05T15:26:39Z

Looks good to me, thanks! I'd like to double-check though that others are ok merging this as well (note that this implementation is gated behind a cargo feature)

@rfcbot fcp merge

rfcbot · 2022-01-05T15:26:40Z

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

src/cargo/core/profiles.rs

src/doc/src/reference/unstable.md

ehuss · 2022-01-07T19:28:32Z

I was a little concerned about the large number of new clone() calls, but in some rough benchmarking it doesn't seem to have much of an impact.

rfcbot · 2022-01-08T21:03:45Z

🔔 This is now entering its final comment period, as per the review above. 🔔

ehuss · 2022-01-10T16:56:12Z

Thanks!

@bors r+

bors · 2022-01-10T16:56:13Z

📌 Commit 68fd673 has been approved by ehuss

bors · 2022-01-10T16:56:19Z

⌛ Testing commit 68fd673 with merge 0655dd2...

bors · 2022-01-10T17:31:13Z

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 0655dd2 to master...

Update cargo 6 commits in 358e79fe56fe374649275ca7aebaafd57ade0e8d..06b9d31743210b788b130c8a484c2838afa6fc27 2022-01-04 18:39:45 +0000 to 2022-01-11 23:47:29 +0000 - Port cargo to clap3 (rust-lang/cargo#10265) - feat: support rustflags per profile (rust-lang/cargo#10217) - Make bors ignore the PR template so it doesn't end up in merge messages (rust-lang/cargo#10267) - Be resilient to most IO error and filesystem loop while walking dirs (rust-lang/cargo#10214) - Remove the option to disable pipelining (rust-lang/cargo#10258) - Always ask rustc for messages about artifacts, and always process them (rust-lang/cargo#10255)

Update cargo 16 commits in 358e79fe56fe374649275ca7aebaafd57ade0e8d..95bb3c92bf516017e812e7f1c14c2dea3845b30e 2022-01-04 18:39:45 +0000 to 2022-01-18 17:39:35 +0000 - Error when setting crate type of both dylib and cdylib in library (rust-lang/cargo#10243) - Include `help` in `--list` (rust-lang/cargo#10300) - Add report subcommand to bash completion. (rust-lang/cargo#10295) - Downgrade some log messages. (rust-lang/cargo#10296) - Enable shortcut for triage bot (rust-lang/cargo#10298) - Bump to 0.61.0, update changelog (rust-lang/cargo#10294) - use new cargo fmt option (rust-lang/cargo#10291) - Add `run-fail` to semver-check for docs (rust-lang/cargo#10287) - Use `is_symlink()` method (rust-lang/cargo#10290) - Stabilize namespaced and weak dependency features. (rust-lang/cargo#10269) - Port cargo to clap3 (rust-lang/cargo#10265) - feat: support rustflags per profile (rust-lang/cargo#10217) - Make bors ignore the PR template so it doesn't end up in merge messages (rust-lang/cargo#10267) - Be resilient to most IO error and filesystem loop while walking dirs (rust-lang/cargo#10214) - Remove the option to disable pipelining (rust-lang/cargo#10258) - Always ask rustc for messages about artifacts, and always process them (rust-lang/cargo#10255)

rust-highfive assigned alexcrichton Dec 19, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 19, 2021

zhamlin marked this pull request as ready for review December 19, 2021 05:02

alexcrichton reviewed Dec 20, 2021

View reviewed changes

src/cargo/core/profiles.rs Outdated Show resolved Hide resolved

zhamlin force-pushed the feat/profile-rustflags branch 5 times, most recently from 5f4febf to da525b4 Compare December 29, 2021 04:26

zhamlin requested a review from alexcrichton December 29, 2021 05:04

alexcrichton reviewed Jan 4, 2022

View reviewed changes

src/doc/src/reference/unstable.md Outdated Show resolved Hide resolved

tests/testsuite/profile_config.rs Outdated Show resolved Hide resolved

zhamlin force-pushed the feat/profile-rustflags branch 2 times, most recently from 7708da6 to 501f710 Compare January 5, 2022 02:46

zhamlin requested a review from alexcrichton January 5, 2022 14:15

alexcrichton added the T-cargo Team: Cargo label Jan 5, 2022

rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Jan 5, 2022

ehuss mentioned this pull request Jan 7, 2022

Tracking Issue for profile-rustflags #10271

Open

5 tasks

ehuss reviewed Jan 7, 2022

View reviewed changes

src/cargo/core/profiles.rs Outdated Show resolved Hide resolved

src/doc/src/reference/unstable.md Outdated Show resolved Hide resolved

danielhjacobs mentioned this pull request Jan 7, 2022

web: Build two WASM modules, with/without extensions, load the appropriate one ruffle-rs/ruffle#5834

Merged

rfcbot added the final-comment-period FCP — a period for last comments before action is taken label Jan 8, 2022

rfcbot removed the proposed-final-comment-period An FCP proposal has started, but not yet signed off. label Jan 8, 2022

zhamlin force-pushed the feat/profile-rustflags branch from 501f710 to 056e273 Compare January 9, 2022 17:16

feat: support rustflags per profile

68fd673

zhamlin force-pushed the feat/profile-rustflags branch from 056e273 to 68fd673 Compare January 9, 2022 17:35

zhamlin requested a review from ehuss January 10, 2022 00:50

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 10, 2022

bors merged commit 0655dd2 into rust-lang:master Jan 10, 2022

bors mentioned this pull request Jan 10, 2022

Add support for specifying Natvis flags for Cargo to pass through to rustc #10192

Closed

This was referenced Jan 10, 2022

Allow specifying rustflags per profile #7878

Closed

Update cargo rust-lang/rust#92807

Merged

rfcbot added finished-final-comment-period FCP complete to-announce and removed final-comment-period FCP — a period for last comments before action is taken labels Jan 18, 2022

ehuss mentioned this pull request Jan 19, 2022

Update cargo rust-lang/rust#93063

Merged

ehuss added this to the 1.60.0 milestone Feb 14, 2022

MiSawa mentioned this pull request Mar 8, 2022

Add --strip flag fornwall/rust-script#36

Closed

cuviper mentioned this pull request Mar 11, 2022

Release notes for 1.60.0 rust-lang/rust#94817

Merged

Conversation

zhamlin commented Dec 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Dec 19, 2021

Uh oh!

alexcrichton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ehuss commented Dec 20, 2021

Uh oh!

alexcrichton commented Dec 21, 2021

Uh oh!

joshtriplett commented Dec 24, 2021

Uh oh!

alexcrichton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bors commented Jan 4, 2022

Uh oh!

zhamlin commented Jan 5, 2022

Uh oh!

alexcrichton commented Jan 5, 2022

Uh oh!

rfcbot commented Jan 5, 2022 • edited by ehuss Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ehuss commented Jan 7, 2022

Uh oh!

rfcbot commented Jan 8, 2022

Uh oh!

ehuss commented Jan 10, 2022

Uh oh!

bors commented Jan 10, 2022

Uh oh!

bors commented Jan 10, 2022

Uh oh!

bors commented Jan 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

zhamlin commented Dec 19, 2021 •

edited

Loading

rfcbot commented Jan 5, 2022 •

edited by ehuss

Loading