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

pakcage.resolver = "2" is implicitly inherited when cargo package, failing the crate build with MSRV 1.50 or older #14192

Closed
weihanglo opened this issue Jul 4, 2024 · 7 comments
Labels
A-workspace-inheritance Area: workspace inheritance RFC 2906 C-bug Category: bug Command-package S-triage Status: This issue is waiting on initial triage.

Comments

@weihanglo
Copy link
Member

weihanglo commented Jul 4, 2024

Problem

When one runs cargo package to produce a .crate tarball for a package with package.rust-version = "1.50" or older, it is expected to work with toolchain of the set MSRV. However, if the package is within a workspace and that workspace has set 2 or newer version to the workspace.resolver field, it'll automatically inherit from it, failing builds on the toolchain of the MSRV.

The resolver field is a feature stabilized since 1.51.

Steps

  1. Create a workspace with workspace.resolver = "2"
  2. Create a member crate with package.rust-version = "1.50"
  3. Run cargo +1.78.0 package
  4. check the normalized Cargo.toml in the .crate source tarball and found that package.resolver = "2", which is not compatible with 1.50 MSRV.

Possible Solution(s)

Don't inherit workspace.resolver when packaging/publishing crates. The resolver behavior is a workspace level setting. The value set in dependency Cargo.toml will be ignored. This might not be ideal because people might want resolver set with the same value from the workspace.

One alternative is don't inherit if package.rust-version is set with a value older than 1.51.0

Notes

I forgot if we've talked about this to be honest…

Version

cargo 1.81.0-nightly (4ed7bee47 2024-06-25)
release: 1.81.0-nightly
commit-hash: 4ed7bee47f7dd4416b36fada1909e9a62c546246
commit-date: 2024-06-25

The behavior appears to happen since 1.64.0 when workspace inheritance was introduced.

@weihanglo weihanglo added C-bug Category: bug Command-package A-workspace-inheritance Area: workspace inheritance RFC 2906 S-triage Status: This issue is waiting on initial triage. labels Jul 4, 2024
@epage
Copy link
Contributor

epage commented Jul 4, 2024

I remember dtolnay brought this up somewhere but I don't remember where.

Part of the problem is you are requesting a feature for a higher MSRV and it can be hard to tell if its relevant. In theory, we could say its only relevant if a bin or example is present but thats not always the case. Sometimes those are "private".

@weihanglo
Copy link
Member Author

Part of the problem is you are requesting a feature for a higher MSRV and it can be hard to tell if its relevant.

Cargo recommends people to set workspace.resolver = "2" when using a relatively new toolchain. This "bug" is a side effect that implicitly inherits resolver from the workspace. Users might not realize until someone tried building the crate on an older toolchain and failed.

I feel that the resolver V3 will have the same nasty papercut when stable. Haven't checked the logic though.

@epage
Copy link
Contributor

epage commented Jul 5, 2024

. Users might not realize until someone tried building the crate on an older toolchain and failed.

We've had some talk about providing warnings (using #12235) when using features that require a newer MSRV. This is one of the cases I've had in mind.

Cargo recommends people to set workspace.resolver = "2" when using a relatively new toolchain.
I feel that the resolver V3 will have the same nasty papercut when stable. Haven't checked the logic though.

in my mind, the general intention is we want to push people to set the resolver to their edition (unless they want something otherwise) which would have them match their MSRV already. In #12235, I've noted that we want to expand on the workspace.resolver warnings but need a warning system first.

@epage
Copy link
Contributor

epage commented Jul 6, 2024

#11047 reminded me of one of my concerns for auto-stripping of this:

In thinking about this, I guess you could say I'm a little less sympathetic to this use case because there isn't really a way to validate the MSRV but it has to be taken upon trust / care and planning (yes, you could do a cargo package and then build that .crate file but that is high friction as well). While there might be advanced users who can do a better job of tracking this (which serde-rs/serde#2603 shows it isn't perfect), I'm worried about the common users attempting this, not just with resolver = "2" but "3" and with other features.

@epage
Copy link
Contributor

epage commented Jul 6, 2024

@weihanglo should we close in favor of #11047 and instead decide if we want to re-open that to avoid splitting the conversation further?

@weihanglo
Copy link
Member Author

@weihanglo should we close in favor of #11047 and instead decide if we want to re-open that to avoid splitting the conversation further?

Sounds good to me.

@weihanglo weihanglo closed this as not planned Won't fix, can't repro, duplicate, stale Jul 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-workspace-inheritance Area: workspace inheritance RFC 2906 C-bug Category: bug Command-package S-triage Status: This issue is waiting on initial triage.
Projects
None yet
Development

No branches or pull requests

3 participants