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

race module cannot be used on targets without atomics #136

Closed
cr1901 opened this issue Feb 23, 2021 · 6 comments · Fixed by #137
Closed

race module cannot be used on targets without atomics #136

cr1901 opened this issue Feb 23, 2021 · 6 comments · Fixed by #137

Comments

@cr1901
Copy link
Contributor

cr1901 commented Feb 23, 2021

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
Copy link
Owner

matklad commented Feb 23, 2021

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
Copy link
Owner

matklad commented Feb 23, 2021

Oh, how unfortunate: the cfg attribute I am thinking about is unstable: rust-lang/rust#32976 (comment)

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
Copy link
Contributor Author

cr1901 commented Feb 24, 2021

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
Copy link
Collaborator

vorner commented Feb 24, 2021

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
Copy link
Owner

matklad commented Feb 24, 2021

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
Copy link
Contributor Author

cr1901 commented Feb 24, 2021

@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.

@bors bors bot closed this as completed in da01f25 Feb 24, 2021
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