fix: verify msrv with rust-version fields (with cargo-msrv 0.16) #4941
Conversation
|
It seems that ubuntu-latest is bumped from Ubuntu 22.04 to 24.04 today. Edit: Fixed by #4942 |
eitsupi
changed the title
eitsupi
changed the title
|
Sorry, but this was a misunderstanding, see foresterre/cargo-msrv#1023. |
|
@max-sixty Do you remember why the MSRV checks were only available for nightly builds? To me this seems rather odd. |
| # Required for `cargo-msrv`, which doesn't yet support workspaces | ||
| metadata.msrv = "1.73.0" | ||
|
|
||
| rust-version = "1.73.0" |
There was a problem hiding this comment.
I am not sure if this is the ideal situation as I believe the MSRV is modified by the cli feature.
(All downstream crates that don't use the cli feature would also be affected by this)
There was a problem hiding this comment.
In my opinion, it is not a good idea to manage the compiler itself and the CLI in one crate (much less make it a default feature).
There was a problem hiding this comment.
On the narrow question of whether the rust-version here is correct — it's fine if the version constraint could be a bit lower. We currently test that 1.73.0 covers all features. So I think we're good?
There was a problem hiding this comment.
My point is that it is the cli feature that pushes the MSRV to 1.73, if excluding the feature, MSRV is 1.70.
However, there is no way to express that in the current Cargo.toml, so the rust-version here must be 1.73 and downstream crates cannot use Rust 1.70.
This is just the current situation, but my point is that it may be difficult to combine those that should keep MSRV as low as possible for downstream considerations and those that should not into a single crate and manage them with feature flags.
I'm fine with it as it is for now, but ideally the CLI should be separated into a separate crate like prqlc-cli.
There was a problem hiding this comment.
Yes, I agree that's a downside with having them integrated. I do think there are benefits to having them in a single crate — in particular cargo install prqlc, but we have gone back & forth, there are tradeoffs...
There was a problem hiding this comment.
I do think there are benefits to having them in a single crate — in particular
cargo install prqlc, but we have gone back & forth, there are tradeoffs...
I do not see how there is any problem with cargo install prqlc changing to cargo install prqlc-cli.
Or rename other than CLI to something like prqlc-core.
In any case, this is a separate issue that needs to be taken up.
| # This isn't tested since `cargo msrv verify` doesn't support workspaces | ||
| # https://github.com/foresterre/cargo-msrv/issues/590 | ||
| # But we can find the MSRV by `cargo msrv find` | ||
| rust-version = "1.77.2" |
There was a problem hiding this comment.
If no crates have rust-version.workspace = true, does this have any effect?
There was a problem hiding this comment.
What do you mean?
lutra has rust-version.workspace = true
There was a problem hiding this comment.
OK I see. Possible we push that down to lutra, and leave this empty.
Hopefully we can have an MSRV for most of the workspace set here, and then lutra can have its own higher one. But possibly that won't work yet.
There was a problem hiding this comment.
Sorry, my understanding of MSRV was incorrect (foresterre/cargo-msrv#1023).
I will not work on it today so I will draft it and come back to it in a few days.
There was a problem hiding this comment.
Ah great, thanks a lot for following up with that. I added a comment to the issue.
If that issue is resolved, I would vote to have an MSRV of something like 1.73.0 here, have most crates inherit it, and then Lutra has a different higher one.
"nightly" is a bit of a misnomer — it's run on any big change, such as a dependency change. It's not run on every tiny change though, the change of something breaking is quite low, and would get picked up in the scheduled run. |
Given that cargo's MSRV-aware resolver will soon be enabled by default, MSRV validation will become more and more important.