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

Fix and consolidate MSRV handling #776

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

nyurik
Copy link
Contributor

@nyurik nyurik commented Feb 8, 2024

No description provided.

Copy link

codspeed-hq bot commented Feb 8, 2024

CodSpeed Performance Report

Merging #776 will not alter performance

Comparing nyurik:msrv (485f8fb) with main (089875a)

Summary

✅ 13 untouched benchmarks

.github/workflows/tests.yml Outdated Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
cargo_path = Path(__file__).parent.parent / "Cargo.toml"
cargo_toml = tomlkit.parse(cargo_path.read_text())
MSRV = cargo_toml["workspace"]["package"]["rust-version"]
print(f"MSRV from the root Cargo.toml: {MSRV}")
Copy link
Collaborator

@Tpt Tpt Feb 9, 2024

Choose a reason for hiding this comment

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

The current practice of Oxigraph is:

  • Have a conservative MSRV for source code allowing packaging in e.g. Debian Sid.
  • Have a newer MSRV for Cargo.lock and lints, allowing to use recent versions of e.g. clap in artifacts build from the main repository.

The test_msv tests ensure that the code still work with the "conversative MSRV" by downgrading dependencies to their lowest allowed version.

Does it sounds sensible to you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is slightly confusing... MSRV either "works" with a specific version, or it does not (compiles + runs). I am not sure there is any value in having multiple ones - e.g. if you compile for debian packaging, it has to pass the compiler, etc.

Now, lints and formatting - I would advise never to check those in the older Rust/Clippy version - these things change, and I may have even seen when different versions of Rust conflict in their suggestions (ether they had minor fmt differences, or clippy was advising for something one way, and then another way).

What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

To rephrase myself:

  • "conservative MSRV" is for me "you can do cargo install oxigraph-cli and add oxigraph = "..." in your Cargo.toml and it works (here from 1.70) This is what the rust-version in the packages mean
  • "newer MSRV" is "the version CI checks", it is the one the Cargo.lock relies on and the lints list too.

I agree this is a bit confusing but I would like to still support compilation on older rustc (eg. Debian Sid one) while still compiling my release artifacts with more recent dependencies and linting with a more recent clippy.

However, if you have a better idea of doing that, it's very welcome

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand about these two versions, but why do you call the second one MSRV? This might be a naming confusion, so let me clarify my understanding:

  • rust-version - minimal version of the rust compiler required to build/test/run everything in the project. This has nothing to do with the minimal dependency versions, and most projects call it MSRV.
  • Minimal versions of each dependency. This is listed in the root Cargo.toml (yei for consolidating it, thanks!). The project should be compilable/runable with the listed versions - e.g. if Cargo.toml has serde = "1.0.2", it must compile with that specific version, even though serde might be much newer and most people wouldn't want to use serde that old anyway.
  • Code as a library use case: when oxygraph is used as a library, there is no lock file - only the above two things have a role.
  • Code as an application (including test / bench / examples /...) - this is where we actually deal with the Cargo.lock file, and in case of an application, want to check that file in. This file IMO should be refreshed weekly - with a CI script that updates the file and runs tests and checks it into the repo. The lock file simply reflects the most up-to-date state of the app, but also it preserves in git history the state of the build environment if we ever need to go back in time and rebuild the app as if it was older.

So the clippy config would probably need just to use the latest in my opinion, and not even deal with MSRV/debian versions/etc. The MSV build/test must pass using the oldest allowed rust-version + versions listed in the Cargo.toml, and ignore the lock file. The "regular CI" should simply use latest available compiler + whatever is in the lock file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes! It's a naming confusion I agree with your understanding. It's way clearer than what I said. Thank you!

Your bullet points 1 and 2 are tested by the test_msv jobs in the CI

So the clippy config would probably need just to use the latest in my opinion, and not even deal with MSRV/debian versions/etc.

What I am doing is pinning a recent version of Rust (usually the current stable and previous one) to avoid breackages when rustc gets upgraded. The MSRV constant name in this file is an artifact of a time where I used the actual MSRV for linting. I should have renamed that. This constant is supposed to be the same as in the CI. But I am also fine with using just the latest Rust version (it might just require some quick fix when a new version of Rust get released and breaks the CI because of some new/updated lints)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you want to update this code to target 1.74 or 1.76 or just latest stable version?

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.

None yet

2 participants