boinkor-net / governor

A rate-limiting library for Rust (f.k.a. ratelimit_meter)
https://github.com/boinkor-net/governor
MIT License
550 stars 43 forks source link

no_std: spinning_top, portable-atomic #222

Closed mammothbane closed 6 months ago

mammothbane commented 7 months ago

Fix #![no_std] support.

I'd also recommend adding a cargo check --target thumbv7em-none-eabi (or similar no_std target) to the CI pipeline in the future, as the tests don't cover no_std for dependencies (see below).

notes

29 (which introduced this change) looks like it was generated by shotgunning dependency analysis tools without reading or understanding the implications (notably, it was during GitHub's Hacktoberfest event, which is notorious for this).

parking_lot does not work, and to my knowledge never has worked, in no_std environments:

Consider the following alternatives (all of which support no_std): ...

...

ci / testing

I notice that someone reported this issue in #96 and it was closed because CI is passing. However, rust's libtest transitively includes libstd, i.e. cargo test can't be run without including std. governor may be built for test without std, but its dependencies are not.

governor/ $ rustup target add thumbv7em-none-eabi
governor/ $ cargo check --target thumbv7em-none-eabi --no-default-features --features no_std
error[E0463]: can't find crate for `std`
  |
  = note: the `thumbv7em-none-eabi` target may not support the standard library
  = note: `std` is required by `parking_lot_core` because it does not declare `#![no_std]`
  = help: consider building the standard library from source with `cargo build -Zbuild-std`
codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (f0cb09c) 98.25% compared to head (c2cfc0b) 98.22%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #222 +/- ## ========================================== - Coverage 98.25% 98.22% -0.04% ========================================== Files 31 31 Lines 2182 2140 -42 ========================================== - Hits 2144 2102 -42 Misses 38 38 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

antifuchs commented 6 months ago

Aaah, thanks for reasoning through this. Now I finally get what the concern is, and how I missed all that. The change looks great, and I'll get it merged when CI passes.

There's one open question for me (and consider this optional for this PR, please!) - is there a way to verify that a repo can be considered no_std compatible? If so, we should probably add a CI job for that.

antifuchs commented 6 months ago

Another exciting development, it sounds like portable-atomic has a critical-section feature which might allow enabling AtomicU64 on platforms that don't support it, using a suitable critical-section implementation. This might be useful for #89!

mammothbane commented 6 months ago

Another exciting development, it sounds like portable-atomic has a critical-section feature which might allow enabling AtomicU64 on platforms that don't support it, using a suitable critical-section implementation. This might be useful for #89!

Yes -- it works! Using it in my projects