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

Bump workspace.rust-version to 1.70.0 and use for all members #4653

Closed
wants to merge 2 commits into from

Conversation

mbrobbel
Copy link
Contributor

@mbrobbel mbrobbel commented Aug 5, 2023

Which issue does this PR close?

None.

Rationale for this change

Some crates use the workspace.rust-version which is set to 1.62. However, for example arrow-buffer won't compile with 1.62, because it depends on half, which requires 1.70.

What changes are included in this PR?

This bumps the workspace.rust-version to 1.70.0, and sets all members (which didn't already do this) to use the workspace.rust-version.

Are there any user-facing changes?

Yes, the rust-version field for all crates in the workspace is now correct and the same.

@github-actions github-actions bot added parquet Changes to the parquet crate arrow Changes to the arrow crate labels Aug 5, 2023
@mbrobbel mbrobbel changed the title Bump workspace.rust-version to 1.70.0 and use for all members Bump workspace.rust-version to 1.70.1 and use for all members Aug 5, 2023
@mbrobbel mbrobbel changed the title Bump workspace.rust-version to 1.70.1 and use for all members Bump workspace.rust-version to 1.70.0 and use for all members Aug 5, 2023
@github-actions github-actions bot added the arrow-flight Changes to the arrow-flight crate label Aug 5, 2023
Copy link
Member

@viirya viirya left a comment

Choose a reason for hiding this comment

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

Looks okay. Just one question. I think half only requires 1.70 since v2.3.0. But arrow-buffer uses half 2.1, cannot it be compiled using 1.62?

@tustvold
Copy link
Contributor

tustvold commented Aug 5, 2023

The MSRV CI job appears to be failing as a result of this change?

@mbrobbel
Copy link
Contributor Author

mbrobbel commented Aug 7, 2023

Looks okay. Just one question. I think half only requires 1.70 since v2.3.0. But arrow-buffer uses half 2.1, cannot it be compiled using 1.62?

arrow-buffer requires half 2.1 so that is >=2.1.0, <3.0.0, and the REAMDE of half-rs states: Requires Rust 1.70 or greater. If you need support for older versions of Rust, use 1.x versions of this crate.. We would have to use tilde requirements to pin to a minor version.

@mbrobbel
Copy link
Contributor Author

mbrobbel commented Aug 7, 2023

The MSRV CI job appears to be failing as a result of this change?

Looks like workspace.rust-version is unsupported until the next release of cargo msrv: foresterre/cargo-msrv#590 (comment). That probably explains why these were not using the workspace rust.version. Maybe this PR should just bump the workspace.rust-version then?

@tustvold
Copy link
Contributor

tustvold commented Aug 7, 2023

We would have to use tilde requirements to pin to a minor version.

Unless I am mistaken, cargo will not select a crate version that is not compatible with the selected version of Rust. Therefore if someone is running 1.62 it will select an older version of half to satisfy this. Provided this compiles, which the CI job would indicate it does, we are perfectly correct to state the MSRV as 1.62. This would only change if we were to specify a half version constraint that could not be satisfied by any version compatible with 1.62

@mbrobbel
Copy link
Contributor Author

mbrobbel commented Aug 7, 2023

We would have to use tilde requirements to pin to a minor version.

Unless I am mistaken, cargo will not select a crate version that is not compatible with the selected version of Rust. Therefore if someone is running 1.62 it will select an older version of half to satisfy this. Provided this compiles, which the CI job would indicate it does, we are perfectly correct to state the MSRV as 1.62. This would only change if we were to specify a half version constraint that could not be satisfied by a version compatible with 1.62

Hm I ran into this (after adding arrow-buffer as dependency) here:

error: package `half v2.3.1` cannot be built because it requires rustc 1.70 or newer, while the currently active rustc version is 1.65.0
Either upgrade to rustc 1.70 or newer, or use
cargo update -p half@2.3.1 --precise ver
where `ver` is the latest version of `half` supporting rustc 1.65.0
Error: Process completed with exit code 101.

@tustvold
Copy link
Contributor

tustvold commented Aug 7, 2023

Hm I ran into this (after adding arrow-buffer as dependency) here:

Is it possible your workspace contains a version constraint or lockfile that is forcing it to use v2.3.1 of half?

@mbrobbel
Copy link
Contributor Author

mbrobbel commented Aug 7, 2023

Hm I ran into this (after adding arrow-buffer as dependency) here:

Is it possible your workspace contains a version constraint or lockfile that is forcing it to use v2.3.1 of half?

I have no Cargo.lock file (library crate), and no other dependencies that depend on half.

@tustvold
Copy link
Contributor

tustvold commented Aug 7, 2023

It would appear I am incorrect and cargo does not perform MSRV-aware resolution. There is a ticket rust-lang/cargo#9930 to track supporting this.

I confirmed this with

$ cargo new msrv_test
$ cd msrv_test
$ rustup override set 1.62
$ cargo --version
cargo 1.62.1 (a748cf5a3 2022-06-08)
$ echo 'arrow-buffer = "45.0"' >> Cargo.toml
$ cargo build
error: package `half v2.3.1` cannot be built because it requires rustc 1.70 or newer, while the currently active rustc version is 1.62.1
$ cargo update -p half --precise 2.1.0
$ cargo build
   Compiling autocfg v1.1.0
   Compiling half v2.1.0
   Compiling num-traits v0.2.16
   Compiling num-integer v0.1.45
   Compiling num-bigint v0.4.3
   Compiling num-iter v0.1.43
   Compiling num-rational v0.4.1
   Compiling num-complex v0.4.3
   Compiling num v0.4.1
   Compiling arrow-buffer v45.0.0
   Compiling msrv_test v0.1.0 (/home/raphael/repos/external/msrv_test)
    Finished dev [unoptimized + debuginfo] target(s) in 5.11s

That being said I don't actually know what the correct policy here is, I would expect the MSRV to track the minimum MSRV where a compatible set of crate versions exists, as opposed to the MSRV of the latest version of all crates... I'll see if I can't ping some other crate maintainers who may be able to weigh in with greater authority

@tustvold
Copy link
Contributor

tustvold commented Aug 8, 2023

Having thought about this I think I would rather keep the MSRV set as currently, this is based off the following rationale:

  • The crate can be compiled with 1.62, although this does require manual intervention
  • If someone is on a version < 1.70 they will get effectively same message as they would if we updated the MSRV to 1.70, just reported by half instead of this crate

As such bumping the MSRV does not improve the UX for users on old versions, they get the same error regardless, but does reduce their options for how to deal with it, by forcing them to update the Rust toolchain.

@mbrobbel mbrobbel closed this Aug 9, 2023
@tustvold tustvold mentioned this pull request Aug 9, 2023
@mbrobbel mbrobbel deleted the workspace-msrv branch August 9, 2023 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate parquet Changes to the parquet crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants