fix(maitake-sync): don't enable portable-atomic/critical-section#561
Conversation
The `critical-section` feature in both `maitake-sync` and `maitake` was enabling `portable-atomic/critical-section`, which conflicts with `portable-atomic/unsafe-assume-single-core` (used by e.g. esp-hal). Per portable-atomic's docs, libraries should leave the backend choice to the end user. Remove the forwarded feature activation so the two can coexist. Fixes hawkw#559
hawkw
left a comment
There was a problem hiding this comment.
Overall, I think I'm okay with this change, as I discussed here:
However, there are a few additional notes before we can merge this. In particular, our README currently states that the critical-section feature flag also enables portable-atomic/critical-section, which it now no longer does:
mycelium/maitake-sync/README.md
Line 217 in d13c2e5
We should remove that --- and maybe add an explicit note that we don't enable it in the documentation for this feature.
We may also want to add some more discussion in the documentation here:
mycelium/maitake-sync/README.md
Lines 118 to 206 in d13c2e5
I just want to make sure that users understand what they have to do to get everything using either critical-section or whatever else is required on their platform consistently.
.github/workflows/ci.yml
Outdated
| # targets without native CAS atomics, portable-atomic needs a backend. | ||
| # The maitake-sync crate intentionally does NOT enable this feature | ||
| # itself (see hawkw/mycelium#559), leaving the choice to the end user. | ||
| run: cargo check -p maitake-sync --target thumbv6m-none-eabi --no-default-features --features=critical-section,portable-atomic/critical-section |
There was a problem hiding this comment.
nitpick, sorry: this is a super long line, could we maybe wrap it to make it less hard to read?
There was a problem hiding this comment.
Done, wrapped the line.
Update documentation in both maitake-sync and maitake READMEs to reflect that the critical-section feature no longer enables portable-atomic/critical-section. Users on no-CAS targets must now configure a portable-atomic backend themselves.
|
@hawkw could we please have a new release of |
|
Sure, I'll publish a release. Unfortunately this will be a semver-breaking change, so please keep that in mind when updating. |
|
I think the CI needs an approval to run |
|
@hawkw do I need to do anything else here or can it be merged? |
hawkw
left a comment
There was a problem hiding this comment.
Thanks for the documentation changes, I think this looks good. I'll publish a release once this makes it through CI.
Summary
critical-sectionfeature in bothmaitake-syncandmaitakewas enablingportable-atomic/critical-section, which conflicts withportable-atomic/unsafe-assume-single-core(used by e.g. esp-hal for ESP32-C3)portable-atomic/critical-section, simulating what an end user on a no-CAS target would doFixes #559