Skip to content

Fix CI: half requiring rust 1.81 #1505

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

Merged
merged 2 commits into from
May 5, 2025

Conversation

Vrixyz
Copy link
Contributor

@Vrixyz Vrixyz commented Apr 7, 2025

Criterion brings half crate which Minimum Supported Rust Version is 1.81, so CI is failing, let's discuss how to fix:

I removed the warning to errors, let's see how it goes.

Copy link
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

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

We probably don't need to cover whatever targets pull in criterion in the MSRV test? 1.81 seems fine to me though.

@Vrixyz
Copy link
Contributor Author

Vrixyz commented Apr 7, 2025

We probably don't need to cover whatever targets pull in criterion in the MSRV test? 1.81 seems fine to me though.

Not sure I understand, are you suggesting to keep a test on MSRV ?

@Ralith
Copy link
Collaborator

Ralith commented Apr 7, 2025

Not sure I understand, are you suggesting to keep a test on MSRV ?

No, I was referring to the CI job that checks MSRV.

it's challenging because criterion is pulled from our dev-dependencies, which we can't make optional (

We don't need to make it optional, we just need to not compile any targets that involve dev-dependencies for purposes of validating the MSRV, because they are not relevant to it.

@Ralith
Copy link
Collaborator

Ralith commented Apr 19, 2025

@Vrixyz do you intend to fix the MSRV test to only compile library targets?

@Vrixyz
Copy link
Contributor Author

Vrixyz commented Apr 23, 2025

@Vrixyz do you intend to fix the MSRV test to only compile library targets?

I can make fixes, but I'm not 100% sure what you mean:

I believe Cargo check doesn't test for dev-dependencies?

Do you mean passing cargo check with specific packages ? Or do you mean changing the features set ?

Thanks for your patience (I have limited computer access for ~1 week)

@Ralith
Copy link
Collaborator

Ralith commented Apr 24, 2025

The purpose of the MSRV test is to ensure that downstream users on older rustc versions can still compile nalgebra. Criterion, being a dev dependency, is not relevant. We should configure the MSRV validation CI job to not attempt to build Criterion (or any other dev-dependency), and instead to only build library targets.

@Vrixyz
Copy link
Contributor Author

Vrixyz commented May 5, 2025

🤔 That's kind of what this PR is doing then:

  • There was not a dedicated "MSRV test", it was running cargo test, which inherently requires dev-dependencies.
  • So I created a new cargo check, dedicated to MSRV test.
    • (cargo check doesn't bring in dev-dependencies if we do not provide --tests or --benches)

I could add a --lib to that new msrv check, but as we don't have a binary, this wouldn't change anything.

I changed the existing cargo test to use the lowest supported rust version for tests, but that can be changed to latest; I think it's interesting to know which MSRV we have for "dev-dependencies".

Sorry if I'm misunderstanding something, thx for the patience :)

@Ralith Ralith merged commit 1c84b0c into dimforge:main May 5, 2025
12 checks passed
@Ralith
Copy link
Collaborator

Ralith commented May 5, 2025

I think it's interesting to know which MSRV we have for "dev-dependencies".

I don't think this is useful, but since CI does check exact error messages it seems like we need to pin this version to something anyway.

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.

2 participants