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: Remove deprecation warning from vendored pkg_resources #12660

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

Conversation

effigies
Copy link

Leaves a comment to flag the change for the next time setuptools is vendored.

Fixes gh-12243.

@effigies
Copy link
Author

Well, this turned into a hairier issue than I realized.

Copy link
Member

@ichard26 ichard26 left a 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

  1. Well, actually, the vendoring job is failing due to the lack of a patch.

@effigies
Copy link
Author

@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.

Copy link
Member

@ichard26 ichard26 left a 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:

# 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"
)

@effigies
Copy link
Author

effigies commented May 2, 2024

From this thread #11997 (comment), one goal of this filter was to play nicely if someone devendors:

Ideally, I'd have made the filter just on pip._vendor.pkg_resources, but that wouldn't have handled the devendoring case. I could have had 2 separate filters, or even left it for whoever does the devendoring to sort out, but that seems unnecessarily unfriendly.

I will defer to you if you still want it removed, but I had left it in intentionally, after reading that.

@pfmoore
Copy link
Member

pfmoore commented May 2, 2024

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.

@ichard26
Copy link
Member

ichard26 commented May 2, 2024

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, pkg_resources and associated code (like this) can be removed from the codebase entirely once Python 3.10 and eggs are unsupported.

@effigies
Copy link
Author

effigies commented May 3, 2024

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.

Copy link
Member

@ichard26 ichard26 left a comment

Choose a reason for hiding this comment

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

Thanks!

@ichard26 ichard26 added this to the 24.1 milestone May 5, 2024
@pradyunsg pradyunsg removed this from the 24.1 milestone May 6, 2024
@pradyunsg
Copy link
Member

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.

@pradyunsg pradyunsg added this to the 24.2 milestone May 6, 2024
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.

Suppress deprecation warnings in vendored projects during normal operation
5 participants