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

Take advantage of weak features in Rust 1.60 for TLS enablement #454

Merged
merged 1 commit into from Oct 21, 2022

Conversation

MarijnS95
Copy link
Contributor

Previously it was impossible to have a unified feature named rustls or native-tls that would turn on the respective TLS backend in the chosen transport (reqwest or ureq) as a feature of <crate>/tls would implicitly turn on <crate>. Since Rust 1.60 it is now possible to specify this crate-feature enablement through the use of the question mark in <crate>?/tls, which will only enable the tls feature if <crate> was enabled through other means (another feature).

Secondly we can now also let optional crates have the same name as a feature, and use dep:<crate> to refer to the crate instead of the dependency, making for a more pleasant experience without renames to underscore suffixes.


Filing this as draft just to get the word out and discuss this for a bit, as it will only be mergeable in 6 months in accordance with the MSRV policy. It goes without saying that this is a breaking change.

Also note that I haven't renamed other optional crates like anyhow_, log_ etc yet, until this is accepted as a (future) solution.

@MarijnS95 MarijnS95 force-pushed the weak-features branch 2 times, most recently from a061b23 to a3f872c Compare April 8, 2022 09:04
Copy link
Contributor

@flub flub left a comment

Choose a reason for hiding this comment

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

Agree that this would be a nice improvement. Though you correctly note that we can't just change this right now and need to wait a while.

sentry/Cargo.toml Outdated Show resolved Hide resolved
sentry/src/transports/curl.rs Outdated Show resolved Hide resolved
@MarijnS95 MarijnS95 force-pushed the weak-features branch 2 times, most recently from d2d3be0 to b7b62af Compare April 8, 2022 09:14
@Swatinem
Copy link
Member

@MarijnS95 Its been a while ;-)
We have now bumped MSRV to 1.60 in master, so the PR is unblocked now.
Are you up to update it? Otherwise I might take over the PR in the coming days.

@MarijnS95 MarijnS95 force-pushed the weak-features branch 2 times, most recently from acc56b3 to 10b737e Compare October 21, 2022 14:04
@MarijnS95 MarijnS95 marked this pull request as ready for review October 21, 2022 14:04
@MarijnS95
Copy link
Contributor Author

@Swatinem cool, sure! That was a bit more work than expected, with both conflicts and more crates in other Cargo.toml that were imported with _ suffixes (and I either missed them when initially creating this PR, or they were added recently).

Let's see how the CI likes this :)

Copy link
Member

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

nice!

there is a few places that still has pkg = { package = "pkg", …

It might be worth removing that, but its also fine to merge like this.

Previously it was impossible to have a unified feature named `rustls` or
`native-tls` that would turn on the respective TLS backend in the chosen
transport (`reqwest` or `ureq`) as a feature of `<crate>/tls` would
implicitly turn on `<crate>`.  Since Rust 1.60 it is now possible to
specify this crate-feature enablement through the use of the question
mark in `<crate>?/tls`, which will only enable the `tls` feature if
`<crate>` was enabled through other means (another feature).

Secondly we can now also let optional crates have the same name as a
feature, and use `dep:<crate>` to refer to the crate instead of the
dependency, making for a more pleasant experience without renames to
underscore suffixes (that subsequently have to be dealt with in Rust
code, e.g. with `use log_ as log;` renames).
@MarijnS95
Copy link
Contributor Author

@Swatinem Good catch, looks like I missed those while resolving conflicts!

There was also a stray anyhow_ dev-dependencies import remaining, but it's not optional so shouldn't conflict with the anyhow feature.

@codecov-commenter
Copy link

Codecov Report

Merging #454 (cd2ca25) into master (89b31f8) will increase coverage by 0.05%.
The diff coverage is 100.00%.

❗ Current head cd2ca25 differs from pull request most recent head 7b1f3cd. Consider uploading reports for the commit 7b1f3cd to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #454      +/-   ##
==========================================
+ Coverage   78.60%   78.65%   +0.05%     
==========================================
  Files          76       76              
  Lines        9206     9192      -14     
==========================================
- Hits         7236     7230       -6     
+ Misses       1970     1962       -8     

@Swatinem Swatinem merged commit 8e8f7c3 into getsentry:master Oct 21, 2022
@MarijnS95 MarijnS95 deleted the weak-features branch October 21, 2022 15:11
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.

None yet

4 participants