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

Bump MSRV to 1.59 #347

Closed
wants to merge 1 commit into from
Closed

Bump MSRV to 1.59 #347

wants to merge 1 commit into from

Conversation

claui
Copy link
Contributor

@claui claui commented Aug 18, 2022

In #309, usage of the asm feature was promoted because it is now part of the stable Rust channel.

This, however, causes build failures when using any Rust version between 1.49 (the documented minimum version) up to 1.58, because the asm feature has only become stable in Rust 1.59.

This PR:

  • bumps the documented minimum supported Rust version (MSRV) to 1.59; and

  • adds a rust_version entry to Cargo.toml in order to codify the MSRV for cargo to pick it up.
    This leads to a more helpful and more actionable error message, for example:

    error: package parking_lot v0.12.1 ([…]) cannot be built because it requires rustc 1.59 or newer, while the currently active rustc version is 1.58.1

In PR #309 [1], usage of the `asm` feature was promoted because it is now
part of the stable Rust channel.

This, however, causes build failures when using any Rust version between
1.49 (the documented minimum version) up to 1.58, because the `asm`
feature has only become stable in Rust 1.59 [2].

Bump the documented minimum supported Rust version (MSRV) to 1.59.

Also, add a `rust_version` entry to `Cargo.toml` in order to codify
the MSRV for `cargo` to pick it up. This leads to a more helpful and
more actionable error message, for example:

> error: package `parking_lot v0.12.1 ([…])` cannot be built because
> it requires rustc 1.59 or newer, while the currently active rustc
> version is 1.58.1

[1]: #309

[2]: https://github.com/rust-lang/rust/releases/tag/1.59.0
@bjorn3
Copy link
Contributor

bjorn3 commented Aug 18, 2022

In #309, usage of the asm feature was promoted because it is now part of the stable Rust channel.

Even with that PR it is still behind the nightly feature, right?

@claui
Copy link
Contributor Author

claui commented Aug 18, 2022

In #309, usage of the asm feature was promoted because it is now part of the stable Rust channel.

Even with that PR it is still behind the nightly feature, right?

It looks like it’s behind the hardware-lock-elision feature.

This command line works with stable Rust 1.58.1:

cargo build --features arc_lock,owning_ref,nightly,serde,send_guard

This one works, too:

cargo build --features arc_lock,owning_ref,nightly,deadlock_detection,serde

But this one fails:

cargo build --features hardware-lock-elision
See error message
   Compiling parking_lot_core v0.9.3 (/home/claudia/Documents/dev/parking_lot/core)
   Compiling lock_api v0.4.7 (/home/claudia/Documents/dev/parking_lot/lock_api)
   Compiling parking_lot v0.12.1 (/home/claudia/Documents/dev/parking_lot)
error[E0658]: use of unstable library feature 'asm': inline assembly is not stable enough for use and is subject to change
  --> src/elision.rs:72:13
   |
72 |             asm!(
   |             ^^^
   |
   = note: see issue #72016 <https://github.com/rust-lang/rust/issues/72016> for more information

error[E0658]: use of unstable library feature 'asm': inline assembly is not stable enough for use and is subject to change
   --> src/elision.rs:102:13
    |
102 |             asm!(
    |             ^^^
    |
    = note: see issue #72016 <https://github.com/rust-lang/rust/issues/72016> for more information

warning: the item `asm` is imported redundantly
  --> src/elision.rs:60:17
   |
9  | use std::arch::asm;
   |     -------------- the item `asm` is already imported here
...
60 |             use core::arch::asm;
   |                 ^^^^^^^^^^^^^^^
   |
   = note: `#[warn(unused_imports)]` on by default

warning: the item `asm` is imported redundantly
  --> src/elision.rs:91:17
   |
9  | use std::arch::asm;
   |     -------------- the item `asm` is already imported here
...
91 |             use core::arch::asm;
   |                 ^^^^^^^^^^^^^^^

error[E0658]: use of unstable library feature 'asm': inline assembly is not stable enough for use and is subject to change
 --> src/elision.rs:9:5
  |
9 | use std::arch::asm;
  |     ^^^^^^^^^^^^^^
  |
  = note: see issue #72016 <https://github.com/rust-lang/rust/issues/72016> for more information

error[E0658]: use of unstable library feature 'asm': inline assembly is not stable enough for use and is subject to change
  --> src/elision.rs:60:17
   |
60 |             use core::arch::asm;
   |                 ^^^^^^^^^^^^^^^
   |
   = note: see issue #72016 <https://github.com/rust-lang/rust/issues/72016> for more information

error[E0658]: use of unstable library feature 'asm': inline assembly is not stable enough for use and is subject to change
  --> src/elision.rs:91:17
   |
91 |             use core::arch::asm;
   |                 ^^^^^^^^^^^^^^^
   |
   = note: see issue #72016 <https://github.com/rust-lang/rust/issues/72016> for more information

For more information about this error, try `rustc --explain E0658`.
warning: `parking_lot` (lib) generated 2 warnings
error: could not compile `parking_lot` due to 5 previous errors; 2 warnings emitted

@bjorn3
Copy link
Contributor

bjorn3 commented Aug 18, 2022

That changed since that PR then. In any case we could say that the MSRV for the hardware-lock-elision feature is 1.59, but keep 1.49 if it is disabled, right?

@claui claui mentioned this pull request Aug 18, 2022
@claui
Copy link
Contributor Author

claui commented Aug 18, 2022

In any case we could say that the MSRV for the hardware-lock-elision feature is 1.59, but keep 1.49 if it is disabled, right?

We could. However, I don’t know a way to encode that in Cargo.toml (in order to get a useful error message when trying to build with Rust < 1.59).

Opened #348 for the alternative.

@Amanieu
Copy link
Owner

Amanieu commented Aug 20, 2022

The hardware-lock-elision feature is disabled by default, so you'd have to actually opt-in to use it. I don't think this justifies bumping the MSRV of the entire crate though.

@claui
Copy link
Contributor Author

claui commented Aug 20, 2022

@Amanieu I totally agree. (I consider it a major UX issue in cargo that it – surprisingly – propagates the --all-features switch to the entire dependency tree, not just the features of the top-level crate that I meant to build. That’s how I ended up “opting in” to parking_lot’s hardware-lock-elision in the first place, without knowing that the crate even exists.)

Nevertheless, your point still stands. Closing.

@claui claui closed this Aug 20, 2022
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

3 participants