cargo icon indicating copy to clipboard operation
cargo copied to clipboard

Change precedence of `build-override`

Open Imberflur opened this issue 3 years ago • 16 comments

See the original PR here https://github.com/rust-lang/cargo/pull/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

Imberflur avatar Jun 24 '22 03:06 Imberflur

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 avatar Jun 24 '22 03:06 rust-highfive

Squashed :slightly_smiling_face:

Imberflur avatar Jun 27 '22 16:06 Imberflur

@bors r+

weihanglo avatar Jun 29 '22 05:06 weihanglo

:pushpin: Commit ede065f5bbbf6dac4a520469949ec0e97a7d2d92 has been approved by weihanglo

bors avatar Jun 29 '22 05:06 bors

:hourglass: Testing commit ede065f5bbbf6dac4a520469949ec0e97a7d2d92 with merge d012044dadf58209d47b1aa93581ed0dadb9dc91...

bors avatar Jun 29 '22 05:06 bors

@bors r-

weihanglo avatar Jun 29 '22 05:06 weihanglo

:broken_heart: Test failed - checks-actions

bors avatar Jun 29 '22 05:06 bors

@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.

weihanglo avatar Jun 29 '22 05:06 weihanglo

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

  • [x] @Eh2406
  • [ ] @ehuss
  • [ ] @epage
  • [ ] @joshtriplett
  • [x] @weihanglo

Concerns:

  • future-globs (https://github.com/rust-lang/cargo/pull/10787#issuecomment-1170646225)
  • intuition (https://github.com/rust-lang/cargo/pull/10787#issuecomment-1170647445)

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 avatar Jun 29 '22 05:06 rfcbot

@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 avatar Jun 30 '22 01:06 epage

@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.

epage avatar Jun 30 '22 01:06 epage

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. :thinking:

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.

Imberflur avatar Jun 30 '22 18:06 Imberflur

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

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

epage avatar Jul 02 '22 02:07 epage

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.

Imberflur avatar Jul 07 '22 18:07 Imberflur

:sunny: Try build successful - checks-actions Build commit: d012044dadf58209d47b1aa93581ed0dadb9dc91 (d012044dadf58209d47b1aa93581ed0dadb9dc91)

bors avatar Jul 08 '22 01:07 bors

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.

Imberflur avatar Aug 23 '22 06:08 Imberflur

@rfcbot close

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

weihanglo avatar Mar 07 '23 17:03 weihanglo

@rfcbot cancel

weihanglo avatar Mar 07 '23 17:03 weihanglo

@weihanglo proposal cancelled.

rfcbot avatar Mar 07 '23 17:03 rfcbot

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!

weihanglo avatar Mar 07 '23 17:03 weihanglo