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

ci: add test for msrv #20

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

jasonish
Copy link

The current MSRV is too new for our project, but thought I'd add a test for the
current MSRV.

In Cargo.toml, set rust-version to the current MSRV of 1.70.

Add a ci test to make sure the build and tests pass with the MSRV.

In Cargo.toml, set rust-version to the current MSRV of 1.70.

Add a ci test to make sure the build and tests pass with the MSRV.
@Ethiraric
Copy link
Owner

I do plan on lowering MSRV. I do keep in mind this PR for the CI file. Wouldn't it be easier to use cargo-msrv?

@davvid
Copy link
Collaborator

davvid commented Mar 30, 2024

As a point of reference, Alma/Rocky/RHEL 9.2 currently ship with rust/cargo 1.66.1. I'm always a fan of casting a wide compatibility net if it doesn't impede development too much.

@jasonish
Copy link
Author

I do plan on lowering MSRV. I do keep in mind this PR for the CI file. Wouldn't it be easier to use cargo-msrv?

IMO, not really. This is a single awk expression to get the MSRV version of Rust right from the start. To use cargo-msrv you first have to install a Rust that meets its requirements, install cargo-msrv which takes about 10x longer to build than yaml-rust2, it will then go pull down the MSRV of Rust, and rebuild. Seems like a lot of overhead vs a single awk expression :)

Glad to hear you are lowering the MSRV. Our project tries to keep the MSRV inline with the major Linux distributions, right now Debian 12 is the hold-back with Rust 1.63.

@Ethiraric
Copy link
Owner

It will go down from 1.70 to 1.65 though. Going further down is possible but would be painful. 1.65 does only incur minimal changes to the code.

@jasonish
Copy link
Author

jasonish commented Mar 30, 2024

It will go down from 1.70 to 1.65 though. Going further down is possible but would be painful. 1.65 does only incur minimal changes to the code.

[Edit: read your comment wrong :)]

Anyways, this is more just a comment on how we determine our MSRV. My usage of this crate will factor into our next major release which I hope to have a newer MSRV for anyways.

@Ethiraric
Copy link
Owner

While searching whether it was possible to have features change the MSRV, I found this: https://doc.rust-lang.org/cargo/guide/continuous-integration.html#verifying-rust-version

Do you have any idea how it fares in terms of CI times?

@jasonish
Copy link
Author

jasonish commented Mar 30, 2024

While searching whether it was possible to have features change the MSRV, I found this: https://doc.rust-lang.org/cargo/guide/continuous-integration.html#verifying-rust-version

Do you have any idea how it fares in terms of CI times?

I have not tried cargo-hack, but with pre-compiled binaries, it should be faster than cargo-msrv. But IMO, why introduce more into your "supply chain" when a single awk expression will do :) Just my opinion though, my main concern is MSRV validation, not how it's done.

Comment on lines +48 to +51
- run: rustup toolchain install $(awk '/^rust-version/ { gsub(/"/, ""); print $3 }' Cargo.toml) --profile minimal --no-self-update
- run: cargo build --all-targets
- run: git submodule update --init
- run: cargo test --all-targets
Copy link
Owner

Choose a reason for hiding this comment

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

Can this PR be rebased over master and the following changes be made:

  • I'd like to ensure MSRV can go down to 1.65.0 with all features disabled. Would it be possible to have an extra key (even if it's commented) in Cargo.toml, like rust-version-no-features that would contain the 1.65.0? This is a temporary ugly hack.
  • Can you run cargo check instead of compiling and running all tests? This also removes the git submodule update.

The defaults for yaml-rust2 are wrong on many levels and fixing them is a task for future me.

Copy link
Author

Choose a reason for hiding this comment

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

Will look in the next few days.

@Ethiraric
Copy link
Owner

But IMO, why introduce more into your "supply chain" when a single awk expression will do :)

I do get your point. Reading back, I may have sounded like I disregarded your implementation, and I apologize. I mostly wanted to survey options and make the sanest choice.

@jasonish
Copy link
Author

No worries. Just glad to see interest in commitment to a MSRV.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants