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

Implement cfg portable_atomic_critical_section #167

Closed
wants to merge 1 commit into from

Conversation

BjornTheProgrammer
Copy link

It is really hard to keep track if critical-section should be enabled with a complex project of many different dependencies with feature flags, which all require critical-section to be added.

As such, the critical-section feature should get an alias called portable_atomic_critical_section cfg, just like how unsafe-assume-single-core feature has the alias portable_atomic_unsafe_assume_single_core cfg, to maintain feature parity.

@taiki-e
Copy link
Owner

taiki-e commented Aug 4, 2024

Unfortunately, unlike Cargo features, it is not possible to enable optional dependencies using cfg.

Currently, to use a critical-section implementation, we must depend on critical-section crate; the logic for sharing a critical-section implementation in the dependency graph is not considered a stable API and is incompatible depending on which Cargo features of critical-section crate are enabled.

@BjornTheProgrammer
Copy link
Author

@taiki-e I think I've found a solution to that issue, by using

[target.'cfg(portable_atomic_critical_section)'.dependencies]

It's possible to have the dependency load upon setting a cfg flag. The seem to mostly be passing also (except for some fetch request fails, and some strange error about "${cmd}" "$@")

@BjornTheProgrammer
Copy link
Author

I've verified that this actually works (at least on newer versions of rust), is there any plan to implement this? Otherwise I'll close this pull request.

Copy link
Owner

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

The problem with the current approach is that the way cargo and crates.io handle cfg-ed dependencies is not very good, confuses users, and some people complain when new dependency appears in Cargo.lock (even if it never actually builds).
(e.g., crossbeam-rs/crossbeam#487 (comment), crossbeam-rs/crossbeam#665, tokio-rs/bytes#411, tokio-rs/bytes#400, etc.)

And the known workaround for that problem is to make the dependency optional (crossbeam-rs/crossbeam#666), which conflicts with your request here.

@@ -1,6 +1,6 @@
[package]
name = "portable-atomic"
version = "1.7.0" #publish:version
version = "1.7.1" #publish:version
Copy link
Owner

Choose a reason for hiding this comment

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

Please revert any version change. It is auto-handled at the time of release.

Also, since this is a new interface addition, the minor version should be increased, not the patch version (1.8.0 instead of 1.7.1).

@@ -165,6 +165,9 @@ RUSTFLAGS="--cfg portable_atomic_no_outline_atomics" cargo ...

Originally, we were providing these as cfgs instead of features, but based on a strong request from the embedded ecosystem, we have agreed to provide them as features as well. See [#94](https://github.com/taiki-e/portable-atomic/pull/94) for more.

- <a name="optional-cfg-portable-atomic"></a>**`--cfg portable_atomic_critical_section`**<br>
Since 1.7.1, this cfg is an alias of [`critical-section` feature](#optional-features-critical-section).
Copy link
Owner

@taiki-e taiki-e Aug 11, 2024

Choose a reason for hiding this comment

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

I feel it is a bit misleading to use the same term to describe unsafe-assume-single-core, which cfg existed before 1.0 and then the Cargo feature was added, and critical-section, which is the opposite situation.

@taiki-e
Copy link
Owner

taiki-e commented Aug 11, 2024

If my memory of the current Cargo limitations is correct, I think that it is better to have the critical-section crate side expose an API that can be used appropriately regardless of which Cargo features of critical-section crate are enabled.

I don't think it would be technically difficult to support it on critical-section, but unfortunately critical-section v1 supports 64-bit state even on 32-bit targets, so I don't think the critical-section maintainer would want to do that on critical-section v1 for performance reasons.

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.

2 participants