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

Named profile updates #9685

Merged
merged 3 commits into from
Jul 20, 2021
Merged

Named profile updates #9685

merged 3 commits into from
Jul 20, 2021

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Jul 13, 2021

A few updates for named profiles to push them closer to stabilization:

  • Disable the dir-name profile setting. dir-name primarily exists for translating the built-in profiles or sharing artifacts between profiles. In order to simplify the UI, we would like to not expose it to the user for the initial stabilization. The code to support it is kept in case we want to add it in the future.
  • Reserve some profile names. Just to give a little flexibility in the future in case we want to use these, or that they could cause confusion. Also updated the error text a little.
  • Add support for custom profiles to legacy commands. Their old behavior is still retained for backwards compatibility. That is:
    • cargo check
      • --profile=test: This forces the test mode. For example, cargo check --lib --profile=test will check the library as a unit test (with --test).
    • cargo fix
      • --profile=test: Forces test mode, same as above.
    • cargo rustc
      • --profile=test: Forces test mode, same as above.
      • --profile=bench: Forces bench mode.
      • --profile=check: Forces check mode.
    • These commands also allow mixing the above options with --release, which is normally not allowed.
  • Fix cargo bench to support the --profile option. I think it was just forgotten.

ehuss added 3 commits July 13, 2021 11:38
dir-name primarily exists for translating the built-in profiles. In
order to simplify the UI, we would like to not expose it to the user for
the initial stabilization. The code to support it is kept in case we
want to add it in the future.
Just to give a little flexibility in the future in case we want to use
these, or that they could cause confusion. Also updated the error text a
little.
This makes the following changes:

- Allows `cargo check`, `cargo fix`, and `cargo rustc` to support custom
  named profiles. This retains the legacy behavior of those commands.
- Fixes `cargo bench` so that it supports custom named profiles.
@rust-highfive
Copy link

r? @alexcrichton

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 13, 2021
@ehuss
Copy link
Contributor Author

ehuss commented Jul 14, 2021

During our previous meeting, we discussed the inconsistency of the legacy commands compared to other commands. Specifically, cargo rustc --profile=test will build with rustc --test flag, but cargo build --profile=test will not. We considered making it so that --profile=test or --profile=bench or any profile that inherits from those would imply the --test flag should be used. But after considering it more, I am having more doubts about that. Looking at using --profile=test with several commands:

  • test and bench: These already use --test
  • clean: Doesn't really matter
  • install: --test doesn't make sense
  • doc: --test doesn't make sense
  • run: --test doesn't make sense (use cargo test instead)
  • rustdoc: --test could potentially be used, but I think we should keep that as cargo test --doc

So the only command that is really questionable is cargo build. My feeling is that if we really want to support this, that there should be something like a --mode flag to explicitly switch the mode.

I'm not completely set on this decision, but changing the behavior will require much deeper changes (essentially adding a test field to the Profile, which would have a big ripple effect).

I acknowledge that this is a bit of a wart in regards to retaining backwards compatibility, but I hope that it won't be too confusing, and can be addressed in the documentation to mention this special behavior.

@alexcrichton alexcrichton added T-cargo Team: Cargo S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 14, 2021
@alexcrichton
Copy link
Member

@rfcbot fcp merge

These changes all look good to me! I think the various warts/etc here are minor enough personally (and I can't think of better alternatives)

@rfcbot
Copy link
Collaborator

rfcbot commented Jul 14, 2021

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.

@rfcbot rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge final-comment-period FCP — a period for last comments before action is taken labels Jul 14, 2021
@rfcbot
Copy link
Collaborator

rfcbot commented Jul 14, 2021

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

@rfcbot rfcbot removed the proposed-final-comment-period An FCP proposal has started, but not yet signed off. label Jul 14, 2021
@joshtriplett
Copy link
Member

As discussed in the @rust-lang/cargo meeting:

It sounds like cargo fix defaults to fixing all targets, including test code. So I don't think we need to treat that as a special-case, nor do we need to document it as one.

The cargo rustc behavior having several special cases for backwards compatibility doesn't seem like a concern to me; cargo rustc has always been an unusual command that requires care to use. So I don't have an issue just documenting those and letting them stand.

That just leaves cargo check. I believe cargo check should behave like cargo fix by default in this regard, and check test code in addition to other code. I don't know how hard this will be to change, but it seems like the right behavior, and it would eliminate this special case.

@ehuss
Copy link
Contributor Author

ehuss commented Jul 20, 2021

Yea, the behavior of cargo check is a bit uncertain since I imagine most cases want the "check test" behavior. I'm just uncertain at this time what consequences that would have. I seem to recall some edge cases where that could cause problems (maybe that was just cfg(not(test)) stuff?). I think that's a big enough change that it would be worth considering that as a followup.

I'm going to go ahead and approve, and think about some of these edge cases more.

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Jul 20, 2021

📌 Commit 73dba76 has been approved by alexcrichton

@bors 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 Jul 20, 2021
@bors
Copy link
Contributor

bors commented Jul 20, 2021

⌛ Testing commit 73dba76 with merge 4e143fd...

@bors
Copy link
Contributor

bors commented Jul 20, 2021

☀️ Test successful - checks-actions
Approved by: alexcrichton
Pushing 4e143fd to master...

@bors bors merged commit 4e143fd into rust-lang:master Jul 20, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jul 21, 2021
Update cargo

6 commits in 27277d966b3cfa454d6dea7f724cb961c036251c..4e143fd131e0c16cefd008456e974236ca54e62e
2021-07-16 00:50:39 +0000 to 2021-07-20 21:55:45 +0000
- Named profile updates (rust-lang/cargo#9685)
- Inform build scripts of rustc compiler context (rust-lang/cargo#9601)
- Factor version preferences into a struct (rust-lang/cargo#9703)
- docs: Fix sentence & update link for GitLab CI docs (rust-lang/cargo#9704)
- Deduplicate compiler diagnostics. (rust-lang/cargo#9675)
- Re-enable future-incompatible tests. (rust-lang/cargo#9698)
ehuss added a commit to ehuss/rust that referenced this pull request Jul 21, 2021
Update cargo

6 commits in 27277d966b3cfa454d6dea7f724cb961c036251c..4e143fd131e0c16cefd008456e974236ca54e62e
2021-07-16 00:50:39 +0000 to 2021-07-20 21:55:45 +0000
- Named profile updates (rust-lang/cargo#9685)
- Inform build scripts of rustc compiler context (rust-lang/cargo#9601)
- Factor version preferences into a struct (rust-lang/cargo#9703)
- docs: Fix sentence & update link for GitLab CI docs (rust-lang/cargo#9704)
- Deduplicate compiler diagnostics. (rust-lang/cargo#9675)
- Re-enable future-incompatible tests. (rust-lang/cargo#9698)
@rfcbot rfcbot added to-announce and removed final-comment-period FCP — a period for last comments before action is taken labels Jul 24, 2021
bors added a commit that referenced this pull request Sep 10, 2021
Fix rustc --profile=dev unstable check.

This fixes an oversight from #9685 where `cargo rustc --profile=dev` was accidentally changed to require `-Zunstable-options`.  1.54 and earlier supported that [here](https://github.com/rust-lang/cargo/blob/rust-1.54.0/src/bin/cargo/commands/rustc.rs#L490.

Fixes #9897
@ehuss ehuss added this to the 1.55.0 milestone Feb 6, 2022
ehuss added a commit to ehuss/cargo that referenced this pull request Mar 2, 2023
Allowing the `dir-name` key for profiles was removed in rust-lang#9685.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge FCP with intent to merge finished-final-comment-period FCP complete S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-cargo Team: Cargo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants