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

Make "atomic-polyfill" an optional dep for thumbv6m-none-eabi only #220

Merged
merged 1 commit into from Jul 30, 2021

Conversation

therealprof
Copy link
Contributor

... otherwise any crate using heapless will automatically add all the
funny ARMv6-M only dependencies regardless.

Signed-off-by: Daniel Egger daniel@eggers-club.de

... otherwise any crate using heapless will automatically add all the
funny ARMv6-M only dependencies regardless.

Signed-off-by: Daniel Egger <daniel@eggers-club.de>
@therealprof
Copy link
Contributor Author

Noticed while trying to update the heapless dependency in nushell that atomic-polyfill, bare-metal and more started showing up in the Cargo.lock.

@Dirbaio
Copy link
Member

Dirbaio commented Jul 15, 2021

What's the reason for making atomic-polyfill thumbv6m-only in the first place?

atomic-polyfill already does the detection of whether polyfilling is required itself. On platforms with native atomics it'll just reexport core::sync::atomic. It can be used unconditionally, it does the right thing.

If it was used unconditionally, heapless would gain support for more polyfilled archs for "free" when they're added to atomic-polyfill.

@therealprof
Copy link
Contributor Author

What's the reason for making atomic-polyfill thumbv6m-only in the first place?

No idea.

If it was used unconditionally, heapless would gain support for more polyfilled archs for "free" when they're added to atomic-polyfill.

Fair enough, but there're crates using heapless on a regular CPU and depending on atomic-polyfill seems to trigger cargo to include bare-metal, embedded-hal, nb and whatnot in the dependency chain which is quite undesirable. I was trying to bump heapless from 0.6 to 0.7 but I'm pretty sure the maintainers will not appreciate the additional baggage coming with that change...

@Dirbaio
Copy link
Member

Dirbaio commented Jul 15, 2021

atomic-polyfill seems to trigger cargo to include bare-metal, embedded-hal, nb and whatnot in the dependency chain which is quite undesirable

indeed... Maybe it should be fixed in atomic-polyfill instead, I'll take a look

@Dirbaio
Copy link
Member

Dirbaio commented Jul 15, 2021

Okay: atomic-polyfill is pulling in cortex-m on thumbv6m only already.

Dependencies specified with [target.thumbv6m-none-eabi.dependencies] still appear on Cargo.lock even for non-v6m targets. I think it's because Cargo does version resolution for all targets together. For non-v6m targets, v6m-only deps don't appear in cargo tree and don't get built.

That might be causing the confusion? something appearing in Cargo.lock doesn't mean it's in the dependency tree for the current target.

@therealprof
Copy link
Contributor Author

Not sure what exactly is happening, it's very opaque...

However this PR still adds the ability to opt out of the atomic-polyfill dependency if the cas feature is not enabled.

@korken89 korken89 merged commit 186b2b5 into rust-embedded:master Jul 30, 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 this pull request may close these issues.

None yet

3 participants