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

Remove vendored charset_normalizer #12703

Merged
merged 3 commits into from
May 24, 2024

Conversation

nateprewitt
Copy link
Sponsor Member

@nateprewitt nateprewitt commented May 15, 2024

This PR proposes removing charset_normalizer from pip's vendored dependencies following a discussion in #12638 (comment). Requests provides support for charset detection on bytes payloads for some APIs, previously through chardet, and now through charset_normalizer. pip however doesn't utilize any of this functionality.

We've merged psf/requests#6702 which makes both of these dependencies optional, allowing them to be removed when Requests is repackaged or vendored. We likely won't release Requests 2.32.0 until some time in June, but the diff here is relatively small that overlaps with existing patches so I'm raising this now. If we'd like to wait till the official release, I'll update this after that's completed.

Requests 2.32.0 was released today so this patch now includes the Requests 2.32.0 upgrade as well to facilitate this change.

@nateprewitt nateprewitt marked this pull request as draft May 15, 2024 02:58
@nateprewitt nateprewitt marked this pull request as ready for review May 15, 2024 03:25
@pradyunsg
Copy link
Member

@nateprewitt Would it be possible for requests to have a patch release with just this change, forked off of the most recent requests release tag?

@nateprewitt
Copy link
Sponsor Member Author

@pradyunsg I can take a look at backporting it if it'll be helpful. The diff here will only change 6 lines once Requests releases (lines 197-202 are the only non-pip specific changes).

@pradyunsg
Copy link
Member

My main concern is the first line from our vendoring policy:

Vendored libraries MUST not be modified except as required to successfully vendor them.

This doesn't really violate that but it's more in a gray area -- if it's not too much effort to cut a requests release, that would be preferable since holding this patch on our end feels a bit icky to me. 😅

That said, if it's a bunch of busywork for you, I'd be happy with just holding this patch on our end for this. :)

@nateprewitt
Copy link
Sponsor Member Author

Totally, I completely understand that. Let me take a look once PyCon ends and I can probably get something out early next week if that's a workable timeline.

@pradyunsg
Copy link
Member

You're at PyCon US? We should meet!

Ping me over urllib3 discord? We can figure out the logistics there, so it we don't notify various people who aren't around. :)

@nateprewitt
Copy link
Sponsor Member Author

Yep! Should be there this evening. I'm in the discord so happy to chat there, same handle. Looking forward to it!

@nateprewitt nateprewitt force-pushed the remove_char_detection branch 2 times, most recently from 229a7f1 to 9f46361 Compare May 20, 2024 21:42
@nateprewitt
Copy link
Sponsor Member Author

Alright, we released Requests 2.32.0 today which contains the change for removing chardet/charset_normalizer. I tried to keep this PR partitioned between upgrading Requests and then removing the charset_normalizer package.

I think this should be good to go but it looks like testing CI is having issues on multiple PRs. I'll check back later to see if that's resolved, otherwise feel free to ping me if you have concerns in the meantime.

@nateprewitt
Copy link
Sponsor Member Author

Looks like checks are passing with the latest commits on main.

@pradyunsg pradyunsg merged commit fb71f4c into pypa:main May 24, 2024
28 checks passed
@nateprewitt nateprewitt deleted the remove_char_detection branch May 24, 2024 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants