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

Remove or relax Rust toolchain constraints #32

Closed
braised-babbage opened this issue Jan 16, 2024 · 4 comments · Fixed by #33
Closed

Remove or relax Rust toolchain constraints #32

braised-babbage opened this issue Jan 16, 2024 · 4 comments · Fixed by #33

Comments

@braised-babbage
Copy link

From this discussion

Although honestly we can just remove the rust-toolchain.toml file from this repo instead. That's the crux of the issue here as it's defaulting for rustup to install/use 1.70 but that's not needed in most cases.

@jakelishman
Copy link
Member

jakelishman commented Jan 16, 2024

As an immediate work-around: the rust-toolchain.toml file is pretty close to the bottom of the pecking order of version considerations, so you can get a working version even on AArch64 with cargo +1.72.0 build or the like, or make it persistent by running rustup override set 1.72.0 in the repo root, etc.

But yeah, it's not an ideal situation with aHash being in the situation it is right now.

@jlapeyre
Copy link
Collaborator

It makes sense to me that we should have the MSRV verified programmatically, right? And also human readable. Is there a way to do this other than rust-toolchain ?

Also, I'll repeat what I wrote here #26 (comment) . It may be that dropping the required version of hashbrown in openqasm3_parser is enough to obviate this problem. I didn't try it because I don't have the aarch64 platform.

@jakelishman
Copy link
Member

On Qiskit we hard-pin aHash to 0.8.6 (in the lockfile) to prevent the problematic 0.8.7 version from coming in, but on Qiskit we're not building a reusable Rust library, so it's not really a problem that we have exactly pinned dependencies. It's not so great here, because we'd have to specify it as a core pin in the Cargo.toml rather than in the lockfile, but it's potentially passable.

I agree it's a good idea to have the MSRV verified in CI, but that doesn't necessarily need to be done with a rust-toolchain.toml file—we can use one of the other override methods in the CI test job—and we don't necessarily need to have the same MSRV for all different platforms, though it's pretty annoyingly that aHash bumped up so high even though it's only for AArch64.

We have a rust-toolchain.toml file on Qiskit mostly as a convenience to non-Rust developers; it forces their rustup to push them up to the MSRV, so as long as they've got that installed, they don't need to know anything about Rust at all. The same argument applies a little less here, where it's a pure Rust library, perhaps.

@jlapeyre
Copy link
Collaborator

I just read this about rust-toolchain. I had misunderstood it as a sort of specification of a MSRV. But no, it's meant to pin the version of rust.

I agree that pinning the compiler version does not make sense in a library like this. Someone building the library should be able to use a higher version of rustc to work around a problem with dependencies. This kind of problem is very common in compiled libraries.

I support the idea of removing rust-toolchain.

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 a pull request may close this issue.

3 participants