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

Just use only std atomic without portable-atomics? #346

Closed
repi opened this issue Mar 10, 2023 · 3 comments
Closed

Just use only std atomic without portable-atomics? #346

repi opened this issue Mar 10, 2023 · 3 comments
Labels
C-core Component: core functionality such as traits, etc. E-intermediate Effort: intermediate. T-ergonomics Type: ergonomics.

Comments

@repi
Copy link

repi commented Mar 10, 2023

Seems even when using the std-atomics feature the portable-atomics crate is still included and required as it is part of the public interface? And not re-exported in the crate either so users metrics with 64-bit counters have to manually import that crate with the matching version (0.3, latest is 1.0 which is incompatible).

This feels somewhat backwards, would it be possible that purely std atomics are used by default without bringing in portable-atomics (as in reverse the feature flag)?

cc @MarcusGrass

@tobz
Copy link
Member

tobz commented Mar 11, 2023

@repi We use portable-atomic to ensure we support both 32-bit and 64-bit platforms without issue, and we have to provide the implementations of CounterFn, etc, in metrics, due to the orphan rule... and so we need to provide portable-atomic implementations of the traits without having them behind a feature flag.

Thinking it through, maybe there's something we could do with build scripts, but the one thing I'm not clear on is whether or not we could also conditionally bring in portable-atomic is the build script determines it's needed.

Ultimately, while I'm sympathetic to reducing transitive dependencies, this was added to make metrics usable on more platforms, and the cost of compiling portable-atomic feels small enough that I'm not willing to make another feature flag-based change in this area. I'm going to leave it as-is unless some solution like the one I posit above can be made workable, or some other solution that allows providing portable-atomic usage transparently can be figured out.

@tobz tobz added C-core Component: core functionality such as traits, etc. E-intermediate Effort: intermediate. T-ergonomics Type: ergonomics. labels Mar 11, 2023
@repi
Copy link
Author

repi commented Mar 11, 2023

thanks. if it is just for 32-bit platforms portable-atomic is really needed then one should be able to use:

[target.'cfg(target_pointer_width = "32")'.dependencies]
portable-atomic = "0.3"

did that in PR #347 which think would work well and resolve this?

@tobz tobz added the S-awaiting-release Status: awaiting a release to be considered fixed/implemented. label Apr 15, 2023
@tobz
Copy link
Member

tobz commented Apr 16, 2023

Fixed via a3a706b / #347.

@tobz tobz closed this as completed Apr 16, 2023
@tobz tobz removed the S-awaiting-release Status: awaiting a release to be considered fixed/implemented. label Apr 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-core Component: core functionality such as traits, etc. E-intermediate Effort: intermediate. T-ergonomics Type: ergonomics.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants