Skip to content

Use portable-atomic to allow use on targets without atomic instructions#30

Merged
orhun merged 6 commits intoratatui:mainfrom
9names:portable_atomic
Sep 3, 2025
Merged

Use portable-atomic to allow use on targets without atomic instructions#30
orhun merged 6 commits intoratatui:mainfrom
9names:portable_atomic

Conversation

@9names
Copy link
Copy Markdown
Contributor

@9names 9names commented Aug 31, 2025

Some targets do not natively support atomics (eg rp2040, esp32-c3).
By adding portable-atomic, these targets can select a method to emulate atomics.

It might be nice to make this a feature instead to avoid forcing it on for everyone.
Happy to get some feedback on this.

Copy link
Copy Markdown
Member

@j-g00da j-g00da left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This is great, but it should definitely be behind a feature flag. critical-section and unsafe-assume-single-core features should also be exposed.

@9names
Copy link
Copy Markdown
Contributor Author

9names commented Sep 1, 2025

critical-section and unsafe-assume-single-core features should also be exposed.

This would not be idiomatic. For portable-atomic it's usually the binary crate that specifies these features.
See https://github.com/taiki-e/portable-atomic#usage

But re-reading the usage example above has reminded me that for the features this needs I think I should have written it like this:

portable-atomic = { version = "1.3", default-features = false, features = ["require-cas"] }

Copy link
Copy Markdown
Member

@orhun orhun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to have this under a feature flag and then it should be good to go :)

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Sep 1, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.24%. Comparing base (c557892) to head (0db79a3).

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #30   +/-   ##
=======================================
  Coverage   76.24%   76.24%           
=======================================
  Files           8        8           
  Lines        1229     1229           
=======================================
  Hits          937      937           
  Misses        292      292           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Member

@j-g00da j-g00da left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Run fmt and I think this is good to go, thanks!

Suggested commit message: feat: Add portable-atomic feature for targets without atomic instructions.

On a side note, it would be nice to make the ci run with std / no std / no std with portable-atomic separately. (In a separate PR.)

After releasing new version we can integrate it into Ratatui.

cc: @joshka

Copy link
Copy Markdown
Member

@j-g00da j-g00da left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@orhun orhun merged commit 125b0ee into ratatui:main Sep 3, 2025
4 checks passed
@github-actions github-actions bot mentioned this pull request Aug 17, 2025
orhun pushed a commit that referenced this pull request Sep 4, 2025
Add builds for some common no_std targets to ensure that we don't
accidentally break them.

See comment requesting this feature here:
#30 (review)

Will not succeed until #30 is merged because the portable-atomic feature
is not present until then.
orhun pushed a commit that referenced this pull request Sep 4, 2025
## 🤖 New release

* `kasuari`: 0.4.7 -> 0.4.8 (✓ API compatible changes)

<details><summary><i><b>Changelog</b></i></summary><p>

<blockquote>

## [0.4.8](v0.4.7...v0.4.8) -
2025-09-04

### Other

- *(deps)* bump the rust-dependencies group across 1 directory with 2
updates ([#32](#32))
- add smoke tests for no_std targets
([#31](#31))
- use portable-atomic to allow use on targets without atomic
instructions ([#30](#30))
- *(deps)* bump actions/checkout from 4 to 5 in the github-actions group
([#27](#27))
- *(deps)* bump the rust-dependencies group with 2 updates
([#28](#28))
- *(deps)* bump rstest from 0.25.0 to 0.26.1 in the rust-dependencies
group ([#25](#25))
</blockquote>


</p></details>

---
This PR was generated with
[release-plz](https://github.com/release-plz/release-plz/).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@joshka
Copy link
Copy Markdown
Member

joshka commented Sep 6, 2025

I think it might be useful to add some more docs to this (just the same things that you've mentioned in the justification of the PR). Basically something that will help answer "Why should I enable this feature and what does it change?".

Right now the doc that will show up on the features lists is "use portable-atomic to polyfill CAS atomics on targets that do not have them", which for someone unfamiliar with those things might be a little too bare. This also applies to the same idea on the Ratatui side. Think about the docs.rs output needs of this.

@9names 9names deleted the portable_atomic branch October 6, 2025 10:02
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.

5 participants