-
Notifications
You must be signed in to change notification settings - Fork 3k
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: Remove deprecation warning from vendored pkg_resources #12660
Conversation
Well, this turned into a hairier issue than I realized. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey thanks for your interest in improving pip!
Making changes exclusively to the vendored code is not permitted. The change needs to be carried as a patch as per pip's vendoring policy.
Also, CI is broken project-wide due to changes to GHA macOS runners. We're working on addressing it, don't worry about all of the red1 right now :)
Footnotes
-
Well, actually, the vendoring job is failing due to the lack of a patch. ↩
9842a28
to
aa37b29
Compare
@ichard26 Thanks for the pointer! Switched to a patch. With this diff in a patch, the comment seems less necessary. I can pare it down or even remove it. LMK what makes sense to you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(deleted commentary on PYTHONWARNINGS=error
)
Since the warning is now patched out, we could also remove the filter that's currently suppressing this warning:
pip/src/pip/_internal/cli/main.py
Lines 51 to 57 in 3f3bc60
# Suppress the pkg_resources deprecation warning | |
# Note - we use a module of .*pkg_resources to cover | |
# the normal case (pip._vendor.pkg_resources) and the | |
# devendored case (a bare pkg_resources) | |
warnings.filterwarnings( | |
action="ignore", category=DeprecationWarning, module=".*pkg_resources" | |
) |
From this thread #11997 (comment), one goal of this filter was to play nicely if someone devendors:
I will defer to you if you still want it removed, but I had left it in intentionally, after reading that. |
I would be happy to remove the filter, on the basis that anyone devendoring should be reviewing our patches and making arrangements to cover whatever fixes they make. But I'll be honest, I'm guessing what's reasonable here - ideally someone who actually devendors would be the best person to comment, but I've no idea how we could get such input. |
I don't want to claim any authority as I'm still new here, but my 2c is that if we don't have any expert opinions available, we may as well stick with the least disruptive change. The warning filter is not a maintenance burden and we don't need to potentially impact downstream unnecessarily. AFAIU, |
Alright. It sounds like there's nothing for me to do, but please let me know if I'm misunderstanding and there's something left. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I realise this has multiple approvals but I'm gonna say this isn't going to be added to 24.1 in the last minute, sorry. |
Thanks @effigies! ^.^ |
Leaves a comment to flag the change for the next time setuptools is vendored.
Fixes gh-12243.