matklad / once_cell

Rust library for single assignment cells and lazy statics without macros
Apache License 2.0
1.84k stars 110 forks source link

`race` module cannot be used on targets without atomics #136

Closed cr1901 closed 3 years ago

cr1901 commented 3 years ago

As of v1.6.0, the race module became stable. I've been using once_cell in my msp430 code since v1.2.0 without issue. Unfortunately, since the release of v1.6.0, I've been unable to compile the module:

William@DESKTOP-H0PMN4M MINGW64 ~/Projects/rust-embedded/once_cell
$ cargo build --release -Zbuild-std=core --target=msp430-none-elf --no-default-features
   Compiling once_cell v1.6.0 (C:\msys64\home\William\Projects\rust-embedded\once_cell)
error[E0432]: unresolved import `core::sync::atomic::AtomicUsize`
  --> src\race.rs:11:20
   |
11 |     sync::atomic::{AtomicUsize, Ordering},
   |                    ^^^^^^^^^^^ no `AtomicUsize` in `sync::atomic`

error[E0283]: type annotations needed
  --> src\race.rs:17:5
   |
17 |     inner: AtomicUsize,
   |     ^^^^^^^^^^^^^^^^^^ cannot infer type
   |
   = note: cannot satisfy `_: Default`
   = note: required by `core::default::Default::default`
   = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to 2 previous errors

Some errors have detailed explanations: E0283, E0432.
For more information about an error, try `rustc --explain E0283`.
error: could not compile `once_cell`

To learn more, run the command again with --verbose.

Is it possible to gate the race module behind a feature so than I can continue to use once_cell on a target without atomics, or would this be considered a semver breaking change?

matklad commented 3 years ago

Good catch! I think the right solution here would be to add cfg(target_has_atomic_usize) or some other built-in cfg like that. Would you be up to sending a PR? If not, I’ll try to get to this tomorrow.

matklad commented 3 years ago

Oh, how unfortunate: the cfg attribute I am thinking about is unstable: https://github.com/rust-lang/rust/issues/32976#issuecomment-716787769

I guess, we need to add a race feature than, but keep it default.

Also, this is not the first time I am bitten in the back by “compiles on my machine”. I wish the language had better sorry here.

cr1901 commented 3 years ago

I guess, we need to add a race feature than, but keep it default.

Is this considered a backwards-compat change according to semver? Also, FWIW, msp430 is stuck on nightly for various reasons1 and will continue to be so for a while. Though I agree a general solution would be better.

Anyways, I could try to do the PR myself.

  1. Yes, stable code will work on msp430, but no interrupts or asm, which kinda defeats the purpose.
vorner commented 3 years ago

compiles on my machine

I'm abusing the fact that cargo check --target needs very minimal setup. So I have a CI which checks that things at least compile on many „weird“ platforms:

https://github.com/vorner/signal-hook/blob/master/.github/workflows/test.yaml#L183

Not sure if it would help here, but feel free to borrow any part of that.

matklad commented 3 years ago

Is this considered a backwards-compat change according to semver?

I think its' backward-compatible enough :) This is a new API addition, so I wouldn't mind breaking it in edge cases.

cr1901 commented 3 years ago

@vorner I like your idea, and tried incorporating it into GHA myself.

Unfortunately, I literally just started learning GHA yesterday, and I ran into two (related) snags, that make me unsure how to add msp430-none-elf or other weird platforms as a test:

  1. msp430-none-elf is not an upstream target that can be added via rustup component add. It probably could be at this point, but there are other things that upstream needs to figure out before msp430 can really be stabilized, so it's not been on my radar.

    Therefore, instead of the following:

    - name: Install Rust
         uses: actions-rs/toolchain@v1
       with:
           toolchain: ${{ matrix.toolchain }}
           profile: minimal
           default: true
             target: ${{ matrix.target }}

    for MSP430 and other bare-metal targets, you'd use something like (along with passing -Zbuild-std=core to cargo:

    - uses: actions-rs/toolchain@v1
        with:
          profile: minimal
          toolchain: ${{ matrix.toolchain }}
          override: true
          components: rust-src

    I'm not sure how to switch how the toolchain is downloaded based on the capabilities of the target.

  2. MSP430 doesn't (and never will :)) have libstd, so passing --tests to cargo check will fail, even with cross. cargo check catches the error though, so I think that may be enough for now?

Anyways, I added a minimal fix for now.