top-level sync::Arc alias to support fork for targets with no atomic ptr
#2285
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2285 +/- ##
=======================================
Coverage 94.82% 94.82%
=======================================
Files 104 104
Lines 24100 24100
=======================================
Hits 22853 22853
Misses 1247 1247 ☔ View full report in Codecov by Sentry. |
|
@brodycj every time you push to a PR in this repo, you generate notifications for everyone watching the repo. If you make a fork in your personal account, you should be able to run CI workflows there too. For your DO NOT MERGE branches, I think that would be a better option. |
ee37cbe to
e0e091b
Compare
|
/cc @bjoernQ - should be ready for review now |
c0fdce4 to
fb43f2e
Compare
fb43f2e to
cd455fa
Compare
7364e8e to
a050bc6
Compare
|
Hey checking in on this, please let me know if I should rebase it again. |
d25bb5b to
efeecd5
Compare
efeecd5 to
d42a4dc
Compare
Benchmark resultsInstruction countsSignificant differencesThere are no significant instruction count differences Other differencesClick to expand
Wall-timeSignificant differencesThere are no significant wall-time differences Other differencesClick to expand
Additional informationCheckout details:
|
There was a problem hiding this comment.
I think these updates should be ready, would like to wait for PR #2310 to keep that update separate as I stated below.
|
I'm confused by the state of this PR. It seemed like we whittled it down to 1 commit and a general approach that was OK to myself and Ctz, but now the commit history is a bit of a mess and it's in draft status. What happened? Can you get this back to just the scope of the PR title and the minimum number of commits to implement exclusively what's needed for supporting a fork replacing the |
sync::Arc alias to support forking for targets with no atomic ptr
|
I will rebase this in a few minutes, my apologies. This kind of draft mess is what happens when I do too much mindless multi-tasking. P.S. @cpu please DO feel free to edit & update the title of both this PR and landed merge commit. Your rewording that I used in |
sync::Arc alias to support forking for targets with no atomic ptrsync::Arc alias to support a fork for targets with no atomic ptr
38a81c6 to
a067a3f
Compare
|
I have slightly updated the title, and I think this should be ready now. @rustls maintainers please DO feel free to update the title of this PR & the title of the commit message or any other possible nits you may see when merging, and give yourself co-authored-by credit in the commit message. I have rebased this now that CI update needed from #2310 is merged, and I made a few updates only to cleanup some comments in the CI changes (and remove a commented-out option that was removed in other PR #2310). Unfortunately some workarounds were needed in the CI due to the configuration I made in My intention has been to keep the proposed updates exclusively within the state of the PR title, and I have made a bit of effort to split some other updates out to separate PRs to keep these updates as focused & atomic as possible. Yes quite a bit of work over the past few months but I am really happy if we can improve support for the extra-tiny embedded devices as proposed here. I would also like to thank the @rustls maintainers for your efforts on reviewing my work and for supporting all of the @rustls requirements in general over the years. |
|
@cpu I just updated the description again: I fixed the module name, added a note in parens: "(A fork would also need to update import of I DO think that the updated description is what we should put in the commit message and would be happy to rebase again with this updated description if needed (your decision). I do also think that the info concerning where a fork would need to update the import in |
Those edits seem fine. Thanks.
Agreed, update the commit message to match please.
Sure, sounds reasonable.
We have a |
sync::Arc alias to support a fork for targets with no atomic ptrsync::Arc alias to support fork for targets with no atomic ptr
a067a3f to
f8de6aa
Compare
…c ptr Adds an internal `sync` module aliasing the `Arc` implementation to allow downstream forks of rustls targetting architectures without atomic pointers to replace the implementation with another implementation such as `portable_atomic_util::Arc` in one central location. This update should make it more straightforward for a fork of rustls to overwrite the top-level import of `alloc::sync::Arc` in `rustls/src/lib.rs` with another implementation such as `portable_atomic_util::Arc`. (A fork would also need to update single test import of `Arc` in `rustls/tests/common/mod.rs`.) To prevent accidental use of the non-aliased `Arc` the project `.clippy.toml` is upated to add `std::sync::Arc` to the `disallowed-types`. Since we only want this to apply to the main crate the CI clippy invocations targetting auxiliary crates are updated to ignore `disallowed-types` CEB7 findings. We can improve this in the future with more targetted lint configuration from `Cargo.toml` settings. --- Resolves rustls#2068 (ref: rustls#2068) --- Co-authored-by: Daniel McCarney <daniel@binaryparadox.net>
f8de6aa to
d8f6163
Compare
|
I have updated the title slightly yet again, made a couple more minor updates to the description & pushed an updated commit with the updated description (reformatted to fit decent git line width 75-77 characters). I did touch a couple CI comments again in the following force-push diff: I have also added I think this should be good now. @cpu & other @rustls maintainers please feel free to fix-up anything else you may see if ready to merge & of course keep yourself in the co-authored-by credit. Thanks again for all of your review efforts. P.S. I have closed my comment as resolved in regarding the new lint warning I added with ugly workaround needed in CI. I am deferring the improvement on my ugly workaround for whoever (anyone) may be able to improve this someday in the future. |
Adds an internal
syncmodule aliasing theArcimplementation to allow downstream forks of rustls targetting architectures without atomic pointers to replace the implementation with another implementation such asportable_atomic_util::Arcin one central location.This update should make it more straightforward for a fork of rustls to overwrite the top-level import of
alloc::sync::Arcinrustls/src/lib.rswith another implementation such asportable_atomic_util::Arc.(A fork would also need to single test import of
Arcinrustls/tests/common/mod.rs.)To prevent accidental use of the non-aliased
Arcthe project.clippy.tomlis upated to addstd::sync::Arcto thedisallowed-types. Since we only want this to apply to the main crate the CI clippy invocations targetting auxiliary crates are updated to ignoredisallowed-typesfindings. We can improve this in the future with more targetted lint configuration fromCargo.tomlsettings.Resolves #2068 (ref: #2068)
Replaces rejected PR: #2200