bytes

bytes copied to clipboard

Some no-std targets (e.g., ARMv6-M) do not support atomic CAS operations and cannot use Arc, Atomic*::compare_exchange, etc. And on those targets, bytes currently fails to build (#461).

EDIT: This PR has been updated to adopt the third idea of https://github.com/tokio-rs/bytes/pull/573#issuecomment-1271567631.

Alternatively, let the processing of those targets to an external crate such as portable-atomic, avoiding the implementation of our own logic

Perhaps it would be preferable to start with the third and then select the first or second in the future if necessary?

The latest version of this PR uses portable-atomic to support platforms without atomic CAS. This currently uses the feature name ("extra-platforms") mentioned by carllerche in https://github.com/tokio-rs/bytes/issues/461#issuecomment-1215798767.

old version

This patch detects those targets using the TARGET environment variables provided by cargo for the build script, and a list of targets that do not support atomic CAS operations.

This way is the same as the way we recently adopted in futures and valuable, and was originally inspired by the way heapless and defmt do, but this doesn't maintain the target list manually. (It's not really fully automated, but it's very easy to update.)

Refs:

• rust-lang/rust#51953
• rust-lang/futures-rs#2400
• tokio-rs/valuable#12

Closes #461 Closes #573

https://github.com/rust-lang/futures-rs/pull/2400 is a more good way to do this. I'll update to use it.

I'll wait for https://github.com/tokio-rs/valuable/pull/12 merged and follow the decision in that PR.

Updated to use the same way as valuable (https://github.com/tokio-rs/valuable/pull/12).

(I should port https://github.com/tokio-rs/valuable/pull/90 to support auto-creating the PR )

Any reason this is still just a draft?

@Darksonn you seem to be the most active reviewer on this crate based on the latest commits.

Could you provide some light regarding the status of this PR? What is blocking it from being merged? What comes to mind would be:

• Unclear whether there is a user need, but the PR has 7 thumbs up so far.
• Unclear whether it is technically correct/sustainable, but there hasn't be any request for change.
• Unclear who should review it and has time to do so. If this is true, what would you suggest? Could the community help?

Taking new dependencies is always complicated. It raises various questions about to what extent we lock ourselves in for the future, what effect it has on MSRV and so on. I also see that there's another PR that uses a different crate for atomics, and I'll need to understand what happened there. I see that there's a bunch of discussion on the other PR.

Other than that, sometimes what a PR needs is just to ping someone and spur them to action. I'm at a conference right now, but I can try to take a look when I fly back to Europe (if I find the energy to look at my laptop during the flight).

Taking new dependencies is always complicated. It raises various questions about to what extent we lock ourselves in for the future, what effect it has on MSRV and so on.

Makes sense, let me try to give you some pointers in that regard, so you don't spend too much time doing archaeology.

I also see that there's another PR that uses a different crate for atomics, and I'll need to understand what happened there. I see that there's a bunch of discussion on the other PR.

Even though #573 is more recent and has more discussions, those discussions are reflected in this PR. The main point of contention as I see it (anyone correct me if I'm wrong) is how portable-atomic was designed back then, essentially preferring backend selection with --cfg rather than --feature (to avoid accidental unsoundess to the price of user convenience). But this point of contention has been resolved since then:

• Crates like heapless and usb-device started using portable-atomic instead of atomic-polyfill.
• portable-atomic ended up providing a feature as collaborative work between the persons involved in the #573 discussion.

So my understanding would be that portable-atomic is now the defacto crate for portable atomic operations (that's even advertised on https://docs.rs/atomic-polyfill).

I'm at a conference right now, but I can try to take a look when I fly back to Europe (if I find the energy to look at my laptop during the flight).

No worries, I was just checking. Good luck at the conference!

Should a new PR be opened? Possibly with a smaller diff such that it's easier to review?

Sorry, I never got around to it.

I'm happy to accept this feature, with the following changes:

• We should document that this feature exists in the readme or lib.rs.
• We should clarify that the list of platforms supported by extra-platforms is subject to change, and that we may drop platforms without a breaking change.

Also, the merge conflicts will need to be fixed.

@taiki-e