ci: check MSRV correctly

[xxchan]

Our current MSRV check in CI doesn't work at all. We only build with nightly, but didn't check it compiles with stable toolchain. (Actually it does not compile.)

This PR added the MSRV check in CI and fixed some compilation issues.

• We used some unstable APIs.
• Update Cargo.lock to use dependencies with proper MSRV. Use generate-lockfile -Z minimal-versions for MSRV

---

More background: #445 (comment)

• In chore: Use nightly toolchain for check #445: We added both rust-toolchain.toml (nightly), and install toolchain with rust_msrv in CI using rustup default ${{ inputs.rust-version }}
• However, rust-toolchain.toml takes precedence over rustup default. Therefore, we always use nightly, instead of rust_msrv

[xxchan]

We can verify that currently CI uses nightly, instead of rust_msrv

[Xuanwo]

We can verify that currently CI uses nightly, instead of rust_msrv

Hi, it is intentional that we are using the nightly version for CI, as specified in

iceberg-rust/rust-toolchain.toml

Lines 19 to 20 in 21d9568

	channel = "nightly-2024-06-10"
	components = ["rustfmt", "clippy"]

.

We use the nightly toolchain for formatting and linting with Clippy.

For more context: #440

[xxchan]

I know we want to use nighty for fmt and check. But we should use msrv for build.

And we don't need to install both nighly and stable for them.

[xxchan]

Well, we can see msrv isn't working

[xxchan]

@Xuanwo How should we proceed? Increase MSRV and fix build, or use nightly for build, which one do you prefer?

[Xuanwo]

I know we want to use nighty for fmt and check. But we should use msrv for build.

We currently only have msrv for iceberg. I'm not sure if our goal is to maintain msrv for all our integration crates, such as datafusion.

[Xuanwo]

use nightly for build

This is fine since it's the version we use to develop.

[xxchan]

I know we want to use nighty for fmt and check. But we should use msrv for build.

We currently only have msrv for iceberg. I'm not sure if our goal is to maintain msrv for all our integration crates, such as datafusion.

use nightly for build

This is fine since it's the version we use to develop.

As shown above, currently msrv for iceberg isn't working too. I'm not sure whether you mean it's also a non-goal?

[xxchan]

Oh, just noticed the compilation error is indeed in datafusion.

[xxchan]

Then I think we need to separate the integration's build/test from core library in CI

[Xuanwo]

Hi, do we truly need an additional tool for this? I assumed it would be as straightforward as cargo +1.70 build. Also, we can use RUSTUP_TOOLCHAIN env to make it easier to play without makefile.

[xxchan]

MSRV check failed again for iceberg. I think check-in Cargo.lock (#844) is a must for MSRV check. Otherwise we can't do things like cargo update home --precise 0.5.9 to lock the dependency version

❯ cargo msrv verify
  [Meta]   cargo-msrv 0.17.0                                                                                                                                 

Compatibility Check #1: Rust 1.77.1
  [FAIL]   Is incompatible 
                                                                                                                                               
  ╭───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮
  │ error: package `home v0.5.11` cannot be built because it requires rustc 1.81 or newer, while the currently active rustc version is 1.77.1 │
  │ Either upgrade to rustc 1.81 or newer, or use                                                                                             │
  │ cargo update [email protected] --precise ver                                                                                                    │
  │ where `ver` is the latest version of `home` supporting rustc 1.77.1                                                                       │
  │                                                                                                                                           │
  ╰───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯
                                                                                                                                               


Crate source was found to be incompatible with Rust version '1.77.1' specified as MSRV in the Cargo manifest located at '/Users/xxchan/Projects/iceberg-rust/crates/iceberg/Cargo.toml'%  

[Xuanwo]

I think check-in Cargo.lock (#844) is a must for MSRV check. Otherwise we can't do things like cargo update home --precise 0.5.9 to lock the dependency version

I think we need -Z minimal-versions. We only need to protect that the version we specify in Cargo.toml works with msrv.

It's the user's own responsibility if they have higher versions in Cargo.lock; we just need to ensure we didn't impose this requirement on them. Checking Cargo.lock is not a direct solution to fix it.

(I wasn't opposed to checking Cargo.lock in, though.)

[xxchan]

I think we need -Z minimal-versions. We only need to protect that the version we specify in Cargo.toml works with msrv.

It doesn't work 🤪

❯ cargo  -Z minimal-versions build   
error: the `-Z` flag is only accepted on the nightly channel of Cargo, but this is the `stable` channel

[xxchan]

I tried to re-generate Cargo.lock using latest MSRV-aware resolver rust-lang/cargo#14639 .

CARGO_RESOLVER_INCOMPATIBLE_RUST_VERSIONS=fallback cargo +stable generate-lockfile

But it seems that it doesn't automatically find the best version for me. So I manually fixed some (mainly AWS crates) with cargo update --precise

BTW FYI that AWS SDK's MSRV is 1.81 https://github.com/awslabs/aws-sdk-rust?tab=readme-ov-file#supported-rust-versions-msrv

[xxchan]

S3 tables is a quite new package, so we can't maintain a low MSRV.

[xxchan]

cc @Xuanwo

[xxchan]

@Xuanwo I explored several ways to pin dependencies:

1. Modify Cargo.lock to get it work.
   • Drawback: Need quite some manual work. And perhaps we need to disable dependabot
   • Benefit: guaranteed to work afterwards
2. Use cargo generate-lockfile -Z minimal-versions, but it doesn't work as expected: e.g., aws-sdk-glue is specified as 1.39.0, but it generated v1.71.0 (latest: v1.76.0), which is quite strange. Dependencies resolution with --minimal-versions rust-lang/cargo#5657 (comment)
3. Use -Z direct-minimal-versions as mentioned in the issue. It's indeed works better,
   • We need to manually specify the actual min versions in Cargo.toml. But I guess it will also work afterwards.
   • Another problem is some indirect dependencies' versions not constrained and break MSRV. I found specifying -Z direct-minimal-versions -Z minimal-versions can work..

[Xuanwo]

Really appreciate your work here!

[xxchan]

Oops, it still doesn't compile. I guess it's because some dependencies' failed to specify correct "min version" for their dependencies.

@Xuanwo So which approach above do you prefer? If the current way looks good, I can try to fix compilation later.

[Xuanwo]

If the current way looks good, I can try to fix compilation later.

Current way looks really great to me.

[xxchan]

@Xuanwo CI is green now!

[Xuanwo]

Perfect, thank you @xxchan for working this. Really nice!