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

srt-tokio: remove trust-dns-resolver dependency #207

Merged
merged 1 commit into from
Jul 28, 2023
Merged

srt-tokio: remove trust-dns-resolver dependency #207

merged 1 commit into from
Jul 28, 2023

Conversation

Kijewski
Copy link
Contributor

Without trust-dns-resolver a boatload of dependencies are removed from the project:

  • data-encoding, enum-as-inner, heck, hostname, ipconfig, ipnet, linked-hash-map, lock_api, lru-cache, match_cfg, matches, parking_lot, resolv-conf, scopeguard, trust-dns-proto, trust-dns-resolver, widestring, winreg

If the user wants to use DNS-over-TLS, they can either lookup the name themself and supply a SocketAddr instead of a domain name, or they can set up their system resolver to use DNS-over-TLS.

This PR effectly reverts #202, but fixes the bug in #201 instead. The problem was actually that Tokio's lookup_host() resolves to a SocketAddr instead of an IpAddr, so its argument needs a domain name and a port, not only a domain name.

Without `trust-dns-resolver` a boatload of dependencies are removed from
the project:

* `data-encoding`, `enum-as-inner`, `heck`, `hostname`, `ipconfig`,
  `ipnet`, `linked-hash-map`, `lock_api`, `lru-cache`, `match_cfg`,
  `matches`, `parking_lot`, `resolv-conf`, `scopeguard`,
  `trust-dns-proto`, `trust-dns-resolver`, `widestring`, `winreg`

If the user wants to use DNS-over-TLS, they can either lookup the name
themself and supply a `SocketAddr` instead of a domain name, or they can
set up their system resolver to use DNS-over-TLS.

This PR effectly reverts <#202>,
but fixes the bug in <#201>
instead. The problem was actually that Tokio's `lookup_host()` resolves
to a `SocketAddr` instead of an `IpAddr`, so its argument needs a domain
name and a port, not only a domain name.
@Kijewski
Copy link
Contributor Author

@nipierre, could you please verify if this change is fine or if it reintroduces the bug in #201 for you?

@codecov
Copy link

codecov bot commented Jul 27, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.08% ⚠️

Comparison is base (17021aa) 75.73% compared to head (242d982) 75.66%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #207      +/-   ##
==========================================
- Coverage   75.73%   75.66%   -0.08%     
==========================================
  Files          88       85       -3     
  Lines       14339    14326      -13     
==========================================
- Hits        10860    10840      -20     
- Misses       3479     3486       +7     
Files Changed Coverage Δ
srt-tokio/src/net.rs 90.62% <100.00%> (+3.95%) ⬆️

... and 17 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@russelltg russelltg merged commit e45f837 into russelltg:main Jul 28, 2023
10 checks passed
@russelltg
Copy link
Owner

Thanks for the contribution!

@Kijewski Kijewski deleted the pr-no-trust_dns_resolver branch July 28, 2023 07:56
@nipierre
Copy link
Contributor

@nipierre, could you please verify if this change is fine or if it reintroduces the bug in #201 for you?

Fine by me, good job ! :)

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.

4 participants