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

Pre-load script wrapper code to avoid errors when pip is being upgraded #12669

Merged
merged 3 commits into from
May 5, 2024

Conversation

pfmoore
Copy link
Member

@pfmoore pfmoore commented May 2, 2024

Fixes #12666

@pfmoore pfmoore added skip news Does not need a NEWS file entry (eg: trivial changes) and removed skip news Does not need a NEWS file entry (eg: trivial changes) labels May 2, 2024
@pfmoore pfmoore added this to the 24.1 milestone May 2, 2024
@pradyunsg
Copy link
Member

@pfmoore Have you considered filing a PR against distlib for this?

@pfmoore
Copy link
Member Author

pfmoore commented May 2, 2024

I assumed that it wasn't worth it - it uses extra memory and adds an import-time performance hit, simply to protect against a situation that should never come up in normal code (modifying site-packages while using packages imported from it is really dodgy, unless you're an installer like pip, and even then it's perilous). But maybe distlib would be OK with it anyway.

I've created a PR for this - pypa/distlib#215 - we can see what happens.

Is it worth merging this anyway, on the assumption that we won't get a new version of distlib before 24.1? It's not a major problem, but it would be nice to get a fix in now we know what's been causing it all this time 🙂

@pfmoore
Copy link
Member Author

pfmoore commented May 3, 2024

Fix has been merged into distlib and will be released in their next scheduled release (mid-year). I declined their offer of an interim release as I don't think this is urgent enough to warrant it.

Should we merge this as a temporary fix, or drop it and leave the issue until the distlib release? I'm inclined to merge - we have a fix and removing the patch when distlib gets released isn't hard - but @pradyunsg if you prefer not to, that's fine.

@pradyunsg
Copy link
Member

Let's go ahead and merge this -- I think it's suboptimal that we have to take in an upstream patch for this but this is a piece of technical debt that we'd pay down on the next upgrade of distlib and it shouldn't really affect non-Windows users (who I expect won't be debundling pip for redistribution).

@pradyunsg pradyunsg merged commit 5545a15 into pypa:main May 5, 2024
28 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can not install editable pip into venv on Windows
2 participants