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

Use autocfg instead of unstable feature(cfg_target_has_atomic) #2294

Closed
wants to merge 3 commits into from

Conversation

taiki-e
Copy link
Member

@taiki-e taiki-e commented Dec 22, 2020

Instead of depending on unstable features of the compiler, use autocfg crate to detect if atomic CAS is available.

This completely removes the dependency on unstable features from crates other than futures and futures-util.
I'm planning to remove the dependency on unstable features from all crates, which is the first step in that plan.

feature(cfg_target_has_atomic) is propagated to the core/alloc imported in all dependencies if any of the dependencies graph crates are uses feature(cfg_target_has_atomic), so it works if the user just enables the feature(cfg_target_has_atomic). EDIT: See #2294 (comment)
(Previously you had to enable both feature(cfg_target_has_atomic) and futures crate's cfg-target-has-atomic feature.)

Unfortunately, this has some (potential) concerns about breakage:

I haven't implemented workarounds yet, but I think there are workarounds for possible issues (basically).

Also, the approach I adopt in this PR is basically the same as the one I'm planning to use in the changes proposed in #2207.

@taiki-e taiki-e force-pushed the cfg_target_has_atomic branch 2 times, most recently from 0a07109 to 627380a Compare December 22, 2020 14:31
@Nemo157
Copy link
Member

Nemo157 commented Dec 22, 2020

LGTM

feature(cfg_target_has_atomic) is propagated to the core/alloc imported in all dependencies if any of the dependencies graph crates are uses feature(cfg_target_has_atomic), so it works if the user just enables the feature(cfg_target_has_atomic).

The user doesn't even need to enable this, for anyone not supporting targets like thumbv6m they can just assume that the target supports atomics, or they could use a similar autocfg script checking the existence of the futures APIs they need.

futures-task/build.rs Outdated Show resolved Hide resolved
@taiki-e taiki-e force-pushed the cfg_target_has_atomic branch 4 times, most recently from c0e926c to 621ca85 Compare December 31, 2020 15:54
macro_rules! cfg_target_has_atomic {
($($item:item)*) => {$(
#[cfg_attr(feature = "cfg-target-has-atomic", cfg(target_has_atomic = "ptr"))]
#[cfg(not(no_atomic_cas))]
Copy link
Member Author

Choose a reason for hiding this comment

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

It's unfortunate that we need to use double negation, but AFAIK, this is the easiest way to implement a workaround for the first problem.

@taiki-e
Copy link
Member Author

taiki-e commented Dec 31, 2020

Another workaround for this is to pass a no-op expression (empty block or unit) to autocfg and treat cfg(target_has_atomic = "ptr") as true if it fails. This allows detecting cases that fail for unrelated reasons such as io.

After rust-lang#2299, this is no longer needed.
@cramertj
Copy link
Member

cramertj commented Jan 5, 2021

Just seeing this now. I'm very familiar with the hassle that custom build.rs scripts like this can cause other build systems, and it'd be really nice if we didn't introduce another point of friction for those users. That said, those users are already accustomed to some friction, so it seems right to look more closely at the costs to those users vs. the benefits to futures.

It's easy for custom build systems to enable a feature flag, but running arbitrary other rust code (autocfg) at build time may or may not be simple. Is it possible to use autocfg to detect whether the feature should be used, and then go through normal feature flags to enable the behavior?

This completely removes the dependency on unstable features from crates other than futures and futures-util.
I'm planning to remove the dependency on unstable features from all crates, which is the first step in that plan.

I'm not totally sure I understand the benefits of this change. These unstable feature usages are already gated on an unstable cargo feature flag right? That is, it isn't quite clear to me what the benefit is of removing unstable feature usages that aren't enabled without explicit opt-in.

@taiki-e
Copy link
Member Author

taiki-e commented Jan 8, 2021

Is it possible to use autocfg to detect whether the feature should be used, and then go through normal feature flags to enable the behavior?

Do you mean adding something like indexmap-rs/indexmap#145 and rust-num/num-traits#185? If so, it's definitely possible.

These unstable feature usages are already gated on an unstable cargo feature flag right? That is, it isn't quite clear to me what the benefit is of removing unstable feature usages that aren't enabled without explicit opt-in.

I think it's nice to remove dependencies on unstable features, whether explicitly gated or not.

That said, I agree that there isn't much benefit from this patch, as the user still need to enable unstable features on the user side to actually use cfg_target_has_atomic. So I'll close this for now. EDIT: This is not correct. see #2399 (comment)

@taiki-e taiki-e closed this Jan 8, 2021
@taiki-e taiki-e deleted the cfg_target_has_atomic branch April 25, 2021 06:32
taiki-e added a commit to crossbeam-rs/crossbeam that referenced this pull request May 27, 2021
Detect platforms that do not support AtomicU64 by using the same way.
AFAIK, this is more robust than the current way that uses autocfg.
See also rust-lang/futures-rs#2294.
taiki-e added a commit to crossbeam-rs/crossbeam that referenced this pull request May 27, 2021
Detect platforms that do not support AtomicU64 by using the same way.
AFAIK, this is more robust than the current way that uses autocfg.
See also rust-lang/futures-rs#2294.
taiki-e added a commit to crossbeam-rs/crossbeam that referenced this pull request May 27, 2021
Detect platforms that do not support AtomicU64 by using the same way.
AFAIK, this is more robust than the current way that uses autocfg.
See also rust-lang/futures-rs#2294.
taiki-e added a commit to crossbeam-rs/crossbeam that referenced this pull request May 27, 2021
Detect platforms that do not support AtomicU64 by using the same way.
AFAIK, this is more robust than the current way that uses autocfg.
See also rust-lang/futures-rs#2294.
taiki-e added a commit to crossbeam-rs/crossbeam that referenced this pull request May 27, 2021
Detect platforms that do not support AtomicU64 by using the same way.
AFAIK, this is more robust than the current way that uses autocfg.
See also rust-lang/futures-rs#2294.
taiki-e added a commit to crossbeam-rs/crossbeam that referenced this pull request May 27, 2021
Detect platforms that do not support AtomicU64 by using the same way.
AFAIK, this is more robust than the current way that uses autocfg.
See also rust-lang/futures-rs#2294.
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