someguynamedjosh/ouroboros

0.15.1 increase MSRV to 1.60

Closed this issue · 9 comments

alex commented 
     Compiling ouroboros v0.15.1
       Running `rustc --crate-name ouroboros --edition=2018 /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/ouroboros-0.15.1/src/lib.rs --error-format=json --json=diagnostic-rendered-ansi --crate-type lib --emit=dep-info,metadata,link -C opt-level=3 -C linker-plugin-lto -C overflow-checks=on -C metadata=991824e5d85b47ec -C extra-filename=-991824e5d85b47ec --out-dir /home/runner/work/cryptography/cryptography/src/rust/target/release/deps -L dependency=/home/runner/work/cryptography/cryptography/src/rust/target/release/deps --extern aliasable=/home/runner/work/cryptography/cryptography/src/rust/target/release/deps/libaliasable-a0690794418a9c48.rmeta --extern ouroboros_macro=/home/runner/work/cryptography/cryptography/src/rust/target/release/deps/libouroboros_macro-bd82fe48d1a0ccd3.so --cap-lints allow`
  error[E0658]: `cfg(target_has_atomic)` is experimental and subject to change
     --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/ouroboros-0.15.1/src/lib.rs:368:11
      |
  368 |     #[cfg(target_has_atomic = "ptr")] // alloc::sync is missing if this is false
      |           ^^^^^^^^^^^^^^^^^^^^^^^^^
      |
      = note: see issue #32976 <https://github.com/rust-lang/rust/issues/32976> for more information

This feature is not stable until 1.60. Previously ouroboros worked on 1.48 (probably lower as well, but 1.48 is what we test with)

alex commented

@kpreid looks like this came from #61 -- do you have any idea about how to make this work with an older MSRV?

kpreid commented

I hadn't thought of this issue when preparing the PR — sorry! I'm not very familiar with compatibility strategies for this sort of problem. Some ideas that come to mind:

  • Put the Arc support behind a default feature, to be disabled by no_std users.
  • Use a build script, https://github.com/dtolnay/rustversion, or some such thing to detect the rustc version and emit some configuration that makes the code unconditionally enabled (which will mean that the crate won't be compatible with no-atomic-ptr targets on older rustc).

I notice that ouroboros has no declared rust-version, nor does the CI .github/workflows/default.yml specify a toolchain to test against. If the MSRV policy is not "latest stable", it would be good if the CI tested against the specific version to prevent future incidents of this type. I'd be happy to help set that up, but it depends on what @joshua-maros wants to happen.

alex commented

Yes, I agree that the lack of a declared MSRV makes this complicated :-) As I said, we currently use it with an MSRV of 1.48 (and had previously used it with a 1.41) so this is a significant leap.

One question for you: Did you add this branch because you needed to support a platform without ptr width atomics, or just for generality?

rustversion seems like the most completely correct version, but a simple feature seemed easy enough.

kpreid commented

One question for you: Did you add this branch because you needed to support a platform without ptr width atomics, or just for generality?

I didn't have any specific target platforms in mind; I wanted to improve the Rust ecosystem by making more libraries no_std compatible where that was straightforward, and perhaps enable my own code that depends on ouroboros to run on a no_std target if one comes into the scope of my interest. Then, when I was testing the PR, I found that it didn't compile on thumbv6m-none-eabi (previously recommended as an example of a no_std target) due to the lack of atomic pointers, so I added the cfg.

So, for strictly my own purposes, I'd be happy to drop support for not(target_has_atomic = "ptr") platforms (my own code uses Arc unavoidably), but that would now technically be a breaking change since ouroboros 0.15.1 has been released.

alex commented

Got it. In the absence of any concrete users requesting this, perhaps the easiest thing is to just remove that line for now. There's plenty of good usecases for no_std that don't involve platforms without ptr-width atomics (e.g. Rust-for-Linux, one of my other projects :-)).

I guess we need some feedback from @joshua-maros on which direction he'd prefer.

urkle commented

@joshua-maros any idea on when a fix will be deployed? It broke the rust-imap crate.

I would also suggest you add a MSRV test into your github actions to test against your min-supported so you have automation to prevent this in the future. (I might suggest yanking the 0.15.1 release)

someguynamedjosh commented

Sorry for the delay, been under pressure for a tight deadline this month.

For now, I've elected to remove the problematic line as after some fiddling I wasn't able to get a feature guard working. I've also set the CI to use 1.48.0, but for some reason, it fails in Github Actions but not on my local machine. Some help in this area would be appreciated. SInce it has broken some things I'm going to go ahead and publish this as 0.15.2 regardless.

alex commented

Thanks, I appreciate it. I can take a look at getting CI working for 1.48 working tomorrow.

  • 👍1
someguynamedjosh commented

The CI is successfully building on 1.48 so I'll go ahead and close this issue.