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

fix: MSRV fix by pinning idna_adapter version #722

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

zachschuermann
Copy link
Collaborator

@zachschuermann zachschuermann commented Feb 28, 2025

What changes are proposed in this pull request?

TLDR: pin idna_adapter to version 1.1.0.

problem: despite Url saying that MSRV is 1.63, it actually depends on the unicode backend you use. previously kernel took no opinion and was thus bumped to the newest releases of ICU4X which requires rust 1.81. for now, we will force the old unicode backend and then in 0.8.0 (or whenever we want to bump MSRV) upgrade/revert back to the ICU4X backend

from Url docs:

By default, idna uses ICU4X as its Unicode back end. If you wish to opt for different tradeoffs between correctness, run-time performance, binary size, compile time, and MSRV, please see the README of the latest version of the idna_adapter crate for how to opt into a different Unicode back end.

How was this change tested?

@github-actions github-actions bot added the breaking-change Change that will require a version bump label Feb 28, 2025
Copy link

codecov bot commented Feb 28, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.02%. Comparing base (4c00de4) to head (77adb15).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #722   +/-   ##
=======================================
  Coverage   84.02%   84.02%           
=======================================
  Files          77       77           
  Lines       18063    18063           
  Branches    18063    18063           
=======================================
  Hits        15178    15178           
  Misses       2167     2167           
  Partials      718      718           

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

# force URL to use unicode-rs instead of ICU4X otherwise we require MSRV 1.81
# from docs: compared to ICU4X, this makes build times faster, MSRV lower, binary size larger, and
# run-time performance slower.
idna_adapter = "=1.1.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't want to force this here, because then everyone depending on us is required to use this version. But, we can do it like I do with the home crate below. so, remove the url = "2" line and add:

[dependencies.url]
version = "2"
idna_adapter = "=1.1.0"

below

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hm it seems this doesn't force that version (just pulling in 1.2 still)

@zachschuermann zachschuermann requested a review from nicklan March 1, 2025 01:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Change that will require a version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants