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

Determine minimum required rust version #5

Closed
jlapeyre opened this issue Jan 9, 2024 · 5 comments
Closed

Determine minimum required rust version #5

jlapeyre opened this issue Jan 9, 2024 · 5 comments

Comments

@jlapeyre
Copy link
Collaborator

jlapeyre commented Jan 9, 2024

[Manually migrated from original repo]

I added rust-toolchain.toml. How to organize this? For use in qiskit we want to check the min required rust version (MRRV) and adjust it down if possible. But we don't want to limit the use as a library elsewhere. Do we include rust-toolchain.toml in the repo? Will this interfere with distributing crates?

@mtreinish commented:

I took a quick look and because of the rust-analyzer upstream crates ra_ap_stdx and ra_ap_limit (as an aside is there a reason they're both not at version 0.0.188?) the MSRV for this library will be 1.70.0. I don't think there is any getting around that because those crates set their rust version in the cargo.toml and cargo will error if you try to use an older version of rust.

As for the mechanics, the best way to set the MSRV is via the rust-version field in the cargo.toml (which you already do correctly). I don't believe the rust-toolchain.toml file will have any effect for a published crate so if qiskit depends on a crate from here it will use whatever version of rust is being used to compile qiskit and cargo will check the rust-version metadata to ensure the msrv is being fulfilled.

@jlapeyre
Copy link
Collaborator Author

jlapeyre commented Jan 9, 2024

I think there is very little being used from ra_ap_stdx. We could probably copy the code in question, or use a different construction.

@jlapeyre
Copy link
Collaborator Author

as an aside is there a reason they're both not at version 0.0.188 ?

I don't know. I copied those lines. My guess is no.

In fact if I edit the Cargo.toml files, both ra_ap_stdx and ra_ap_limit will compile with 1.64 and the tests in this repo pass. (Well they will pass on a dev branch)

I started to copy the code. It is straightforward. Although it would be really nice to avoid vendoring it.

I'll open issues upstream for these incorrect MSRVs.

@mtreinish
Copy link
Member

I expect there isn't a remedy available upstream. The msrv for everything rust-analyzer was 1.70 at the time, they likely weren't trying to set an msrv per crate and only doing it in the workspace as trying to track n minimum versions for a constellation of libraries gets really tricky. Also they've moved on as the newer releases have a higher msrv now and their versioning and release schedule don't look like stable releases are a thing for rust-analyzer.

@mtreinish
Copy link
Member

Fwiw we raised the msrv in qiskit to 1.70 in anticipation of using this library with an msrv of 1.70 here Qiskit/qiskit#11493

@jlapeyre
Copy link
Collaborator Author

Fwiw we raised the msrv in qiskit to 1.70

I knew it was under consideration, but I didn't search because I didn't think it was done yet. Last night, I went ahead with trying 1.64 compatibility. In all the workspace crates there were a very small number of ">1.64"isms that could be easily changed. I thought it is gratuitous to leave them. But maintaining 1.64 compatibility will get harder, not least because generic associated types will be more common.

In any case this workspace has an MSRV of 1.70 already, so there is nothing to do. I'll close this.

@jlapeyre jlapeyre mentioned this issue Jan 11, 2024
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

No branches or pull requests

2 participants