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

Basic skeleton to start pkg_resources migration #8114

Merged
merged 4 commits into from
Feb 3, 2021

Conversation

uranusjr
Copy link
Member

This implements the basic shim structure I mentioned in #7413 (comment). I moved the lru_cache() shim to utils.compat since it’s useful for get_environment().

One very trivial change is made in self_outed_check to demostrate how the end result would look like in essence.

I think (hope?) this would be enough for someone to run with the idea and start refactoring. I think the logical next refactor would be to change AbstractDistribution to return BaseDistribution instead, and modify the prepare code. After that, usages in the new resolver can switch.

@uranusjr uranusjr added the skip news Does not need a NEWS file entry (eg: trivial changes) label Apr 23, 2020
@uranusjr uranusjr requested a review from pradyunsg April 23, 2020 04:47
@deveshks
Copy link
Contributor

Thank you for this. I think this will help one of my PRs #8054 as well (also pointed out in #8054 (comment))

Should I should hold on to making changes to that PR until this gets matured and merged?

@uranusjr
Copy link
Member Author

I think it works either way. pkg_resources usages in that part of the code is local, and can be rewritten relatively easily no matter the order. I’ll leave the decision to @pradyunsg.

Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

LGTM!

@pypa-bot
Copy link

pypa-bot commented Jul 6, 2020

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

@pypa-bot pypa-bot added the needs rebase or merge PR has conflicts with current master label Jul 6, 2020
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Jul 6, 2020
@uranusjr
Copy link
Member Author

uranusjr commented Jul 6, 2020

Rebased to include changes from #8054. I chose to keep get_distribution() and get_installed_distributions() in pip._internal.utils.misc as wrappers to pip._internal.metadata for now to avoid cluttering the patch. We should remove usages of them gradually and eventually always call pip._internal.metadata directly instead.

@uranusjr uranusjr force-pushed the importlib-metadata branch 8 times, most recently from a6c172e to 6072de9 Compare July 7, 2020 01:50
@uranusjr
Copy link
Member Author

uranusjr commented Jul 7, 2020

@deveshks You’ll need to take a look to make sure I got everything right from #8054.

@uranusjr uranusjr force-pushed the importlib-metadata branch 2 times, most recently from d41b59f to 3f1cdd3 Compare July 7, 2020 02:08
@pfmoore
Copy link
Member

pfmoore commented Jul 7, 2020

We should remove usages of them gradually and eventually always call pip._internal.metadata directly instead.

Maybe we should add this as a comment in the code, to guide future maintainers in refactoring? A note in an issue comment is likely to get forgotten, whereas a note in the code won't.

@uranusjr uranusjr force-pushed the importlib-metadata branch 2 times, most recently from fef276c to 1fa6ebd Compare July 7, 2020 08:25
@uranusjr
Copy link
Member Author

uranusjr commented Jul 7, 2020

Good idea, I added the following comment to both functions:

# TODO: Refactor the call site to make it call get_environment() directly,
# and operate on the wrapper PipMetadataDistribution instead.

@uranusjr
Copy link
Member Author

This seems to rebase cleanly to master, hopefully the test suite agrees. I want to get this merged soon and start working on the transition. The plan is to push this out for Python 3 in the 20.3 release (but keeping the old implementation in --use-deprecated so things won’t break as bad), and remove the old implementation when we drop Python 2 support.

@uranusjr
Copy link
Member Author

uranusjr commented Jan 7, 2021

So obviously that didn’t happen. Since we’ll want 21.0 to be a minimal change release, I’m adding this to 21.1, which should be plenty of time to get this worked into a mergable state.

@uranusjr uranusjr added this to the 21.1 milestone Jan 7, 2021
@uranusjr uranusjr force-pushed the importlib-metadata branch 2 times, most recently from 1fd8d71 to 0491b79 Compare January 7, 2021 18:41
@uranusjr
Copy link
Member Author

uranusjr commented Jan 10, 2021

Note for future self. With python/importlib_metadata#111 resolved, pip should have all the needed parts to remove pkg_resources entirely. This finds locations of all distributions matching name in a list of prefixes:

for path in MetadataPathFinder._search_paths(name, search_paths):
    distribution = PathDistribution(path)  # Build a distribution class at `path` for inspection.

The _search_paths() function is private, but importlib-metadata promises to keep it steady for pip. pip needs its own Distribution wrapper class to bind a distribution to the right path, which should not be too difficult.

@uranusjr uranusjr force-pushed the importlib-metadata branch 2 times, most recently from ad7f93a to 8d40580 Compare January 18, 2021 20:47
@uranusjr uranusjr force-pushed the importlib-metadata branch 3 times, most recently from 6f7ee06 to e93d83e Compare January 19, 2021 02:52
pkg_resources performs annoying caching that needs to be worked around
in some parts of pip. This makes it easier to represent the difference
between environments backend by WorkingSet() and working_set.
@pfmoore
Copy link
Member

pfmoore commented Jan 31, 2021

From #9540:

Things are blocked by #8114, I’m waiting for a good timing to merge it.

I'm not sure what "good timing" you want here, this has no conflicts and 2 approvals so IMO, it's good to go. I'm grateful you didn't land it just before 21.0 (and even more so that you didn't land it between 21.0 and 21.0.1!) but I think the 21.0 cycle is more or less done now. Let's wait a week in case anyone screams about 21.0.1, but unless they do, I'd say you can merge this any time from then.

@uranusjr
Copy link
Member Author

I'm not sure what "good timing" you want here

At this point, a couple of more days to make sure 21.0.1 does not contain catastrophic issues that require an emergency release.

@uranusjr
Copy link
Member Author

uranusjr commented Feb 3, 2021

A few weekdays have passed and I think it’s safe to assume the 21.0 release if free of major issues. I’ll merge this later today and start working on migration.

@uranusjr uranusjr merged commit 7afe0a7 into pypa:master Feb 3, 2021
@uranusjr uranusjr deleted the importlib-metadata branch February 3, 2021 13:15
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
skip news Does not need a NEWS file entry (eg: trivial changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants