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

Upgrade socket2 dependency #1803

Merged
merged 1 commit into from
Nov 30, 2020
Merged

Upgrade socket2 dependency #1803

merged 1 commit into from
Nov 30, 2020

Conversation

faern
Copy link
Contributor

@faern faern commented Nov 30, 2020

PR Type

Bug Fix / Other: Upgrades a dependency to a version not making invalid assumptions about the memory layout of standard library types (std::net::SocketAddr).

Helps unblock rust-lang/rust#78802. See that PR for more details. This PR simply helps adoption of the fixed version(s) of socket2.

Upgrades to a version not making invalid assumptions about
the memory layout of std::net::SocketAddr
@codecov
Copy link

codecov bot commented Nov 30, 2020

Codecov Report

Merging #1803 (1dfeac8) into master (ea8bf36) will increase coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1803      +/-   ##
==========================================
+ Coverage   53.76%   53.80%   +0.04%     
==========================================
  Files         132      129       -3     
  Lines       12278    12266      -12     
==========================================
- Hits         6601     6600       -1     
+ Misses       5677     5666      -11     
Impacted Files Coverage Δ
actix-http/src/ws/mask.rs 0.00% <0.00%> (-2.64%) ⬇️
...ctix-http/src/header/common/if_unmodified_since.rs
actix-http/src/header/common/accept_charset.rs
actix-http/src/header/common/allow.rs
actix-http/src/header/common/mod.rs 10.00% <0.00%> (+2.30%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea8bf36...1dfeac8. Read the comment docs.

@JohnTitor
Copy link
Member

Since we don't lock socket2 patch version and cargo tries to pull the newest 0.3.x version, this doesn't make much sense for me.

@faern
Copy link
Contributor Author

faern commented Nov 30, 2020

Cargo only tries to pull the latest version when you have no lockfile. Anyone with a lockfile will continue to pull the version they already have until they do cargo update or cargo update -p socket2. This change would force the upgrade. It does not hurt actix-web in any way. All it does is help the adoption of the patch relesea of socket2 that stops making invalid assumptions about the standard library.

The faster we can make the community adopt these fixed versions the faster the linked to PR on the standard library can advance.

Other crates that accepted the same type of bump in order to help out with adoption rate: alexcrichton/curl-rust#365, hyperium/hyper#2349 and rust-lang/cargo#8909

@JohnTitor
Copy link
Member

Cargo only tries to pull the latest version when you have no lockfile. Anyone with a lockfile will continue to pull the version they already have until they do cargo update or cargo update -p socket2. This change would force the upgrade.

But then you should do cargo update -p actix-web. Hm but yeah, we usually don't accept this type of PR, in this case, "invalid assumptions" seems a reasonable reason to accept.

@JohnTitor JohnTitor merged commit 32d59ca into actix:master Nov 30, 2020
@faern
Copy link
Contributor Author

faern commented Nov 30, 2020

Thank you!

@robjtede
Copy link
Member

robjtede commented Nov 30, 2020

Yep I agree with this merge, in this case.

@robjtede robjtede added A-web project: actix-web B-semver-norelease change that does not require a release labels Dec 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-web project: actix-web B-semver-norelease change that does not require a release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants