Skip to content

Conversation

@ElliottjPierce
Copy link
Contributor

@ElliottjPierce ElliottjPierce commented Mar 27, 2025

This lets users use this crate on targets that don't support atomics.

cargo check --no-default-features --target thumbv6m-none-eabi Fails on main.

But cargo check --no-default-features --target thumbv6m-none-eabi --features portable-atomic,portable-atomic/fallback,portable-atomic/critical-section passes on this PR.

This is blocking work in dependent PRs, for example, see here.

Co-authored-by: Zachary Harrold <[email protected]>
This lets us remove the old hack!
@taiki-e
Copy link
Collaborator

taiki-e commented Mar 28, 2025

Thanks for the PR! The implementation looks good.

Could you update CI config to check the case where this feature enabled?

It would be sufficient to replace the following two with the commands used in async-task (this and this),

run: cargo hack build --all --no-dev-deps
- run: cargo hack build --all --target thumbv7m-none-eabi --no-default-features --no-dev-deps

- run: cargo hack build --rust-version
- run: cargo hack build --no-default-features --rust-version

and add two additional cases (cargo test --feature portable-atomic and cargo test --no-default-features --feature portable-atomic) after the following.

- run: cargo test
- run: cargo test --no-default-features

@ElliottjPierce
Copy link
Contributor Author

@taiki-e Thanks! I'm no expert with CI, so I'm not certain this does what you're asking, but I think it does. Does it look right to you now?

Comment on lines 66 to 67
- name: Run cargo check for non atomic platforms
run: cargo hack build --feature-powerset --no-dev-deps --target thumbv7m-none-eabi --skip std,default
Copy link
Collaborator

Choose a reason for hiding this comment

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

non atomic platforms

thumbv7m has atomics...
You have to use thumbv6m if you want to check target without atomics.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll be honest, I don't know that much about these platforms. I copied this check from bevy ci. If you wan't to change, the 7 to a 6, go for it. (Or I can if you prefer.) But I don't have the context to know which is what y'all want to use here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. I actually prefer to check both, so I added the change for that.

@taiki-e taiki-e merged commit 9a96317 into smol-rs:master Apr 28, 2025
7 checks passed
@ElliottjPierce
Copy link
Contributor Author

Sounds good. Thanks!

@taiki-e
Copy link
Collaborator

taiki-e commented Jul 3, 2025

Published in 2.4.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants