Handle legacy loaders for pip patch #1691
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Bernat Gabor <bgabor8@bloomberg.net>
|
Any update? |
|
@ofek are you affected, can you check that the pr fixes your issue? |
|
Not affected, just curious 🙂 |
|
it appears the functions take different arguments: this gets ~closer: $ diff -u /tmp/pymonkey/.tox/py36/lib/python3.6/site-packages/_virtualenv.py{.old,}
--- /tmp/pymonkey/.tox/py36/lib/python3.6/site-packages/_virtualenv.py.old 2020-03-05 19:10:41.798224966 -0800
+++ /tmp/pymonkey/.tox/py36/lib/python3.6/site-packages/_virtualenv.py 2020-03-05 19:11:56.546224966 -0800
@@ -62,12 +62,17 @@
old(module)
patch_dist(module)
+ def load_module(*args, **kwargs):
+ module = old(*args, **kwargs)
+ patch_dist(module)
+ return module
+
if hasattr(loader, "exec_module"):
old = loader.exec_module
loader.exec_module = exec_module
else:
old = loader.load_module
- loader.load_module = exec_module
+ loader.load_module = load_module
return spec
finally:
self.fullname = Nonehowever that patch never gets undone and mutates a singleton causing other modules to fail: |
|
@ofek also lol it's been less than a day chill my dude <3 |
|
@asottile wait, the patch should only activate for setuptoools/distutils dist, so it does not need to be undone 🤔 do you have a docker build that reproduces your test? |
|
if you clone github.com/asottile/pymonkey and |
|
Ok now tested with that test suite and passes 👍 |
Signed-off-by: Bernat Gabor <bgabor8@bloomberg.net>
asottile
reviewed
Mar 6, 2020
| @@ -55,13 +56,24 @@ def find_spec(self, fullname, path, target=None): | |||
| try: | |||
| spec = find_spec(fullname, path) | |||
| if spec is not None: | |||
| old = spec.loader.exec_module | |||
| # https://www.python.org/dev/peps/pep-0451/#how-loading-will-work | |||
| spec.loader = deepcopy(spec.loader) # loaders may be shared, create new that also patches | |||
There was a problem hiding this comment.
this just moves the bug slightly:
- not all loaders are copyable
- some loaders depend on being singletons and copying them breaks that invariant
|
Shouldn't those raise some exception on copy?
…On Fri, 6 Mar 2020, 19:14 Anthony Sottile, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/virtualenv/create/via_global_ref/_virtualenv.py
<#1691 (comment)>:
> @@ -55,13 +56,24 @@ def find_spec(self, fullname, path, target=None):
try:
spec = find_spec(fullname, path)
if spec is not None:
- old = spec.loader.exec_module
+ # https://www.python.org/dev/peps/pep-0451/#how-loading-will-work
+ spec.loader = deepcopy(spec.loader) # loaders may be shared, create new that also patches
this just moves the bug slightly:
- not all loaders are copyable
- some loaders depend on being singletons and copying them breaks that
invariant
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#1691?email_source=notifications&email_token=AAFIQPUUKDZSMTHDNF6D4FLRGFDTFA5CNFSM4LCEE5TKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCYLFTFI#pullrequestreview-370563477>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFIQPU4NHMOYLCBLI5X3GTRGFDTFANCNFSM4LCEE5TA>
.
|
|
not necessarily, it's unlikely someone would go out of their way to block copying if there's no expected interface to reach their loader mine in particular needs to be a singleton to function but ~kinda hobbles along without it so that's why the tests probably passed but either way, if it did raise on copying this patch is still broken for it ;) |
|
@asottile will fill a follow-up PR to patch only once the loader, not copy, and instead defend the patch itself 🤷♂ |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #1690.
@asottile can you check that this solves your problem?