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

Remove uses of unstable feature(cfg_target_has_atomic) #2400

Merged
merged 1 commit into from May 6, 2021

Conversation

taiki-e
Copy link
Member

@taiki-e taiki-e commented Apr 20, 2021

Instead of depending on unstable features of the compiler, use the TARGET environment variables provided by cargo for the build script and a list of targets that do not have atomic CAS.

This completely removes the dependency on unstable features from crates other than futures, futures-util, futures-io.

This way is 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.)

Closes #2399
Replaces #2294

r? @Nemo157 @cramertj
cc @jamesmunns @Dirbaio

@Dirbaio
Copy link

Dirbaio commented Apr 20, 2021

This is great! Thanks for working on it :)

Removing cfg-target-has-atomic will break users building for custom targets that have no atomics, though.

@taiki-e
Copy link
Member Author

taiki-e commented Apr 20, 2021

Removing cfg-target-has-atomic will break users building for custom targets that have no atomics, though.

Hmm, you are right.

That said, we want to reduce dependency on unstable features as much as possible... So perhaps the compromise is to make cfg(no_atomic_cas) an unstable public API so that atomics can be disabled via RUSTFLAGS:

RUSTFLAGS='--cfg futures_no_atomic_cas' cargo build

@Dirbaio
Copy link

Dirbaio commented Apr 20, 2021

Why not simply keep cfg-target-has-atomic as-is? I'm not a fan of having to use RUSTFLAGS. The standard way to customize lib behavior is cargo features.

Aditionally, removing it (or keeping it but making it do nothing) is semver-breaking: it makes code that compiled before stop compiling.

@taiki-e
Copy link
Member Author

taiki-e commented Apr 20, 2021

Why not simply keep cfg-target-has-atomic as-is? I'm not a fan of having to use RUSTFLAGS. The standard way to customize lib behavior is cargo features.

semver-breaking

No, cfg-target-has-atomic feature is unstable and stability is not guaranteed.

# Unstable features
# These features are outside of the normal semver guarantees and require the
# `unstable` feature as an explicit opt-in to unstable API.

@Dirbaio
Copy link

Dirbaio commented Apr 20, 2021

I wasn't aware of this issue with unstable features and the shift to use RUSTFLAGS. Looks good to me then! 👍

@jamesmunns
Copy link
Member

jamesmunns commented Apr 23, 2021

Hey @taiki-e, thank you for implementing this!

I have tested this change in my cassette repo (which prompted this), and I was able to replace my copy/paste exports with just exporting the futures dependency.

I can confirm this change works as expected for my project, at least :)

@taiki-e taiki-e force-pushed the cfg_target_has_atomic-2 branch from 0457c51 to 3f73ddd Compare Apr 23, 2021
@taiki-e
Copy link
Member Author

taiki-e commented Apr 23, 2021

@jamesmunns Thanks for testing this!

// The rustc-cfg listed below are considered public API, but it is *unstable*
// and outside of the normal semver guarantees:
//
// - `futures_no_atomic_cas`
// Assume the target does not have atomic CAS (compare-and-swap).
// This is usually detected automatically by the build script, but you may
// need to enable it manually when building for custom targets or using
// non-cargo build systems that don't run the build script.
//
// With the exceptions mentioned above, the rustc-cfg strings below are
// *not* public API. Please let us know by opening a GitHub issue if your build
// environment requires some way to enable these cfgs other than by executing
// our build script.
Copy link
Member Author

@taiki-e taiki-e Apr 23, 2021

Choose a reason for hiding this comment

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

As said in #2400 (comment), exposes cfg(futures_no_atomic_cas) as unstable public API (outside of the normal semver guarantees).

Copy link
Member Author

@taiki-e taiki-e Apr 23, 2021

Choose a reason for hiding this comment

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

Also, we probably need to set #![cfg_attr(docsrs, doc(cfg_hide(futures_no_atomic_cas)))] when rust-lang/rust#79341 merged...

Copy link
Member

@jyn514 jyn514 Apr 23, 2021

Choose a reason for hiding this comment

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

That might be a while 😅

@taiki-e taiki-e added the 0.3-backport: pending label Apr 23, 2021
@taiki-e taiki-e force-pushed the cfg_target_has_atomic-2 branch 2 times, most recently from 7a092cb to 75c184b Compare May 5, 2021
@taiki-e taiki-e force-pushed the cfg_target_has_atomic-2 branch from 75c184b to d0ca511 Compare May 5, 2021
@taiki-e taiki-e merged commit c180a3a into rust-lang:master May 6, 2021
19 checks passed
@taiki-e taiki-e deleted the cfg_target_has_atomic-2 branch May 6, 2021
@taiki-e taiki-e mentioned this pull request May 7, 2021
@taiki-e taiki-e added 0.3-backport: completed and removed 0.3-backport: pending labels May 7, 2021
taiki-e added a commit to tokio-rs/valuable that referenced this issue May 11, 2021
Some no-std targets do not have atomic CAS and cannot use Arc, etc. 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 have atomic CAS.

This is a port of rust-lang/futures-rs#2400.
See that PR for more.
bors bot added a commit to crossbeam-rs/crossbeam that referenced this issue May 29, 2021
698: Remove uses of unstable feature(cfg_target_has_atomic) r=taiki-e a=taiki-e

Some no-std targets (e.g., ARMv6-M) do not support atomic CAS operations and cannot use Arc, etc.

Currently, we are using an unstable feature to detect them, but it has caused breakage in the past (#435).
Also, users of stable Rust are not able to compile crossbeam on those targets.

Instead of depending on unstable features of the compiler, 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](rust-lang/futures-rs#2400) and [valuable](tokio-rs/valuable#12), and was originally inspired by the way [heapless](japaric/heapless@44c66a7) and [defmt](https://github.com/knurling-rs/defmt/blob/963152f0fc530fca64ba4ff1492d9c4b7bf76062/build.rs#L42-L51) do, but this doesn't maintain the target list manually. (It's not really fully automated, but [it's very easy to update](https://github.com/crossbeam-rs/crossbeam/blob/a42dbed87a5739228b576f526b1e2fd80260a29b/.github/workflows/ci.yml#L89).)

Also, this completely removes the dependency on unstable features from crates other than crossbeam-epoch.

refs: rust-lang/rust#51953, rust-lang/futures-rs#2400, tokio-rs/valuable#12

704: Add AtomicCell::fetch_update r=taiki-e a=taiki-e

Equivalent of [`std::sync::atomic::AtomicN::fetch_update`](https://doc.rust-lang.org/nightly/core/sync/atomic/struct.AtomicUsize.html#method.fetch_update) that stabilized in Rust 1.45.



Co-authored-by: Taiki Endo <te316e89@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants