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 pin_project_lite. remove futures-util #453

Merged
merged 7 commits into from
Dec 31, 2020
Merged

Conversation

fakeshadow
Copy link
Contributor

@fakeshadow fakeshadow commented Dec 31, 2020

PR Type

Refactor

PR Checklist

Check your PR fulfills the following:

  • Tests for the changes have been added / updated.
  • A changelog entry has been made for the appropriate packages.
  • Format code with the latest stable rustfmt

Overview

Replace futures_channel with tokio::sync
Replace pin_project with pin_project_lite
Replace futures_util with futures_core/sink/task

Sadly this does not cut down the amount of deps with default featurs.(5 less dep with no-default-features)

CHANGES.md Outdated Show resolved Hide resolved
Co-authored-by: Daniel Egger <daniel@eggers-club.de>
@@ -57,7 +58,7 @@ trust-dns-resolver = { version = "0.20.0" , optional = true, default-features =

[dev-dependencies]
doc-comment = "0.3"
futures-channel = { version = "0.3.7", default-features = false, features = ["sink"] }
futures-util = { version = "0.3.7", default-features = false }

Choose a reason for hiding this comment

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

Suggested change
futures-util = { version = "0.3.7", default-features = false }
futures-util = { version = "0.3.8", default-features = false }

I'm aware it doesn't really matter but since 0.3.8 is the current version, we might as well use that. ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for point out but we are using 0.3.7 across all the actix-* crates.
There is no lock on the crates so users can choose to use anything above 0.3.7

Copy link
Member

@robjtede robjtede Dec 31, 2020

Choose a reason for hiding this comment

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

Yep, no need to needlessly bound the version in a library when we don't need to. It's ^0.3.7 to avoid a security warning. Eg: https://deps.rs/crate/actix-web/3.3.2.

Choose a reason for hiding this comment

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

I'm not saying we should fix the version, rather the opposite. I actually would even prefer going 0.3 instead of specifying the minor. But by specifying 0.3.8 we're forcing all dependencies to go at least to 0.3.8, without cargo update which is a good thing in my book.

Copy link
Member

@robjtede robjtede Dec 31, 2020

Choose a reason for hiding this comment

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

Allowing versions below 0.3.7 is a security risk and shows up on our deps.rs page as such.

FYI @therealprof: https://github.com/RustSec/advisory-db/blob/master/crates/futures-util/RUSTSEC-2020-0059.md

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was a UB for waker fixed in 0.3.6. So it's suggested to use at least that. It's always good to do our part on this matter.

Choose a reason for hiding this comment

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

Allowing versions below 0.3.7 is a security risk and shows up on our deps.rs page as such.

FYI @therealprof: https://github.com/RustSec/advisory-db/blob/master/crates/futures-util/RUSTSEC-2020-0059.md

I don't understand what you're trying to tell me, sorry. I would like to go (higher, i.e. 0.3.8), not lower.

Copy link
Member

Choose a reason for hiding this comment

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

In terms of maintaining a library (rather than an app), allowing the most versions of crates in a dep tree makes it most compatible and gives developers the option to upgrade as they decide or, more importantly, pin a lower compatible version because of a bug/yanked crate/security issue or whatever else.

Copy link

@therealprof therealprof left a comment

Choose a reason for hiding this comment

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

I'm not a huge fan of switching between using pin_project_lite::pin_project! and using pin_project_lite::pin_project; pin_project! but other than that looks great to me!

src/fut/either.rs Outdated Show resolved Hide resolved
src/fut/either.rs Outdated Show resolved Hide resolved
src/fut/stream_finish.rs Show resolved Hide resolved
fakeshadow and others added 4 commits December 31, 2020 19:26
Co-authored-by: Rob Ede <robjtede@icloud.com>
Co-authored-by: Rob Ede <robjtede@icloud.com>
Co-authored-by: Rob Ede <robjtede@icloud.com>
@robjtede robjtede merged commit d40b610 into master Dec 31, 2020
@robjtede robjtede deleted the pin_project_lite branch December 31, 2020 12:25
robjtede added a commit to Michael-F-Bryan/actix that referenced this pull request Jul 24, 2023
Co-authored-by: Rob Ede <robjtede@icloud.com>
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.

3 participants