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

Change precedence of build-override #10787

Closed

Conversation

Imberflur
Copy link

@Imberflur Imberflur commented Jun 24, 2022

See the original PR here #9382

If you specify both build-override and package."*" then the glob profile has higher precedence than the build-override one which is much more specific. In practice this means that very often the build-override is just completely ignored and all the proc macro crates get compiled without the build-override actually doing anything. This usually results in debug builds taking much longer than they should, often even longer than release builds. This Pull Request fixes this precedence issue.

This includes those changes along with the necessary documentation changes. Additionally, the build-override defaults (and not just overrides supplied by the user) are now applied before package."*" overrides.

Fixes #9351

@rust-highfive
Copy link

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

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 24, 2022
Copy link
Member

@Rustin170506 Rustin170506 left a comment

Choose a reason for hiding this comment

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

Looks nice to me. It would be nice if the commits could be squashed. Thanks!

If you specify both `build-override` and `package."*"` then the glob
profile has higher precedence than that `build-override` one which is
much more specific. In practice this means that very often the
`build-override` is just completely ignored and all the proc macro
crates get compiled without the `build-override` actually doing
anything. This usually results in `debug` builds taking much longer than
they should, often even longer than `release` builds. This Pull Request
fixes this precedence issue.

Update docs to reflect change in `build-override` precedence

Also, apply `package."*"` overrides before `build-override` defaults. This ensures the defaults for `build-override` take precedence as well (and not just the overrides provided by the user).
@Imberflur
Copy link
Author

Squashed 🙂

Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Looks nice. Thanks for the PR!

Also thanks @hi-rustin for the review

@weihanglo
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 29, 2022

📌 Commit ede065f has been approved by weihanglo

@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 Jun 29, 2022
@bors
Copy link
Collaborator

bors commented Jun 29, 2022

⌛ Testing commit ede065f with merge d012044...

bors added a commit that referenced this pull request Jun 29, 2022
Change precedence of `build-override`

See the original PR here <#9382>

> If you specify both `build-override` and `package."*"` then the glob profile has higher precedence than the `build-override` one which is much more specific. In practice this means that very often the `build-override` is just completely ignored and all the proc macro crates get compiled without the `build-override` actually doing anything. This usually results in `debug` builds taking much longer than they should, often even longer than `release` builds. This Pull Request fixes this precedence issue.

This includes those changes along with the necessary documentation changes. Additionally, the `build-override` defaults (and not just overrides supplied by the user) are now applied before `package."*"` overrides.

Fixes #9351
@weihanglo
Copy link
Member

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 29, 2022
@weihanglo weihanglo added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-cargo Team: Cargo and removed S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Jun 29, 2022
@bors
Copy link
Collaborator

bors commented Jun 29, 2022

💔 Test failed - checks-actions

@weihanglo
Copy link
Member

@rfcbot merge

It makes sense to me as a more specific overrides taking higher precedence than glob one. Though this changes the old behaviour, need the team to take a look.

@rfcbot
Copy link
Collaborator

rfcbot commented Jun 29, 2022

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

Concerns:

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 labels Jun 29, 2022
@epage
Copy link
Contributor

epage commented Jun 30, 2022

@rfcbot concern future-globs

This changes the precedence from

  1. Profiles in .cargo/config files (using same order as below).
  2. profile.dev.package.<name>
  3. profile.dev.package.*
  4. profile.dev.build-override
  5. profile.dev

to

  1. Profiles in .cargo/config files (using same order as below).
  2. profile.dev.package.<name>
  3. profile.dev.build-override
  4. profile.dev.package.*
  5. profile.dev

With the original, it can really be simplified to

  1. Profiles in .cargo/config files (using same order as below).
  2. profile.dev.package.<glob>
  3. profile.dev.build-override
  4. profile.dev

Where our glob syntax only allows * without anything before or after.

What is the likelihood we'd expand our glob syntax? What would this change look like after expanding our glob syntax? Would we want literals to come first, then build-override then any glob? Or would we want all non naked * globs to come before `build-override? Or would we not want this change after expanding the glob syntax?

@epage
Copy link
Contributor

epage commented Jun 30, 2022

@rfcbot concern intuition

When writing up the previous concern, I realized that there is also an intuition problem. While the original issue said that * is expected to be lower than build-override within a specific profile configuration, I suspect users would not expect <name> to be at a different precedence level than * and that this would lead to confusion. While we are documenting the behavior, that is likely to be later in the development cycle and more frustrating.

@Imberflur
Copy link
Author

Imberflur commented Jun 30, 2022

This might be a bit of a sidetrack, but IMO it is already a bit unintuitive that workspace members are excluded from *.

What is the likelihood we'd expand our glob syntax?

With crates having arbitrary names I find it difficult to imagine a more complex glob being reliable. Maybe if it was limited to workspace members? Some way to configure multiple crates at once seems like it would be useful (unless profile.dev.package.<name> already has a way to list crates that I'm not aware of).

I suspect users would not expect <name> to be at a different precedence level than * and that this would lead to confusion. While we are documenting the behavior, that is likely to be later in the development cycle and more frustrating.

This makes me realize the current changes would still leave no way to specifically configure <name> as a regular dependency without also affecting its configuration when used as a build dependency. But if build-override precedence is simply pushed up further then there is no way to specifically configure particular build dependencies. 🤔

I wasn't aware that .cargo/config can provide a higher precedence. That seems like a decent (albeit hacky) way to work around the problem these changes are trying to address.

@epage
Copy link
Contributor

epage commented Jul 2, 2022

With crates having arbitrary names I find it difficult to imagine a more complex glob being reliable.

Brainstorming hat: Besides any *derive*, rust-lang/rfcs#3243 might provide opportunities for more structured naming for globbing.

@Imberflur
Copy link
Author

This makes me realize the current changes would still leave no way to specifically configure <name> as a regular dependency without also affecting its configuration when used as a build dependency. But if build-override precedence is simply pushed up further then there is no way to specifically configure particular build dependencies.

It seems like we would need a whole separate .build-override.package.

@bors
Copy link
Collaborator

bors commented Jul 8, 2022

☀️ Try build successful - checks-actions
Build commit: d012044 (d012044dadf58209d47b1aa93581ed0dadb9dc91)

@Imberflur
Copy link
Author

With the concerns pointed out by @epage, I'm not really happy with the current solution.

For my own purposes, it seems like in the short term it would be better to just target specific build-time crates with configuration and/or see if I can use .cargo/config profiles to get a higher precedence.

What would the steps be to move forward with designing an alternative? (happy to discuss further on zulip if that would be ideal)

The initial idea that comes to mind is being able to specify build-override."*" and build-override.<name>. With higher precedence than any of the package. sections. But that might be a bit too ad hoc and would yield two options with the same effect: build-override."*" and just build-override.

@weihanglo
Copy link
Member

@rfcbot close

Thanks @Imberflur for your efforts. We're still seeking a solution to #9351.

@weihanglo
Copy link
Member

@rfcbot cancel

@rfcbot
Copy link
Collaborator

rfcbot commented Mar 7, 2023

@weihanglo proposal cancelled.

@weihanglo weihanglo closed this Mar 7, 2023
@weihanglo
Copy link
Member

The Cargo team doesn't have a bandwidth to lead this change by ourselves. That is to say, if @Imberflur or someone wants things to move forward, you must be the driver for the design! We're still open to discuss on either Zulip or GitHub issue (#9351).

Closing for now. Thank you!

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 proposed-final-comment-period An FCP proposal has started, but not yet signed off. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-cargo Team: Cargo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Precedence issue with build-override and package."*"
10 participants