-
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
More pkg_resources refactoring #10511
Conversation
95c8722
to
0c1dec0
Compare
05eb831
to
d90011e
Compare
PEP 660 editables are regular wheels so uninstallation should not have to care that they are editables. |
The "problem" is old editables do need special treatments, so uninstallation code needs to distinguish whether an editable is legacy or not so PEP 660 editables don't incorrectly fall into the legacy code path :p (More specifically, |
Sure. But do you mean we should worry about uninstallation bugs now that PEP 660 is merged, or is your concern specific to this PR ? |
Specific to this PR. |
d90011e
to
0034cb1
Compare
78e6a76
to
9d42d7b
Compare
This is rebased against main so I think PEP 660 should be covered correctly. |
9d42d7b
to
35ec437
Compare
Anyone interested in taking a look at this? |
Ping me again at the weekend, and I'll try to take a look then. |
I'll take a look tomorrow morning. |
paths_to_remove = [exe_name] | ||
if WINDOWS: | ||
paths_to_remove.append(exe_name + ".exe") | ||
paths_to_remove.append(exe_name + ".exe.manifest") | ||
if is_gui: | ||
paths_to_remove.append(exe_name + "-script.pyw") | ||
else: | ||
paths_to_remove.append(exe_name + "-script.py") | ||
return paths_to_remove | ||
yield exe_name | ||
if not WINDOWS: | ||
return | ||
yield f"{exe_name}.exe" | ||
yield f"{exe_name}.exe.manifest" | ||
if is_gui: | ||
yield f"{exe_name}-script.pyw" | ||
else: | ||
yield f"{exe_name}-script.py" |
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.
Hurray!
if "/$platlibdir/" in platlib and hasattr(sys, "platlibdir"): | ||
platlib = platlib.replace("/$platlibdir/", f"/{sys.platlibdir}/") | ||
if "/$platlibdir/" in platlib: | ||
platlib = platlib.replace("/$platlibdir/", f"/{_PLATLIBDIR}/") |
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.
This seems unrelated to the rest of the changes.
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.
Yup, but it seems like the right thing to do; and I imagine it's here because TP noticed this bug while working on this. :)
Relevant definition has a fallback to lib
if sys.platlibdir isn't defined. (see line 46)
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.
Sorry, I wasn't suggesting it shouldn't be done. And I have no objection to it being in this PR. I was mostly flagging it in case there was a reason it needed to be here that I'd missed 😉
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.
If I remember the details correctly (it’s been a while), this was fixing a Mypy error that somehow only shows up when I was working on this. I don’t know why main isn’t complaining, maybe it’s due to me running Mypy on Windows? Not sure, and this new code is margianlly better anyway.
Note that this does not actually change any behaviour because before 3.9, the /$platlibdir/
was just /lib/
, and after 3.9 _PLATLIBDIR
is always sys.platlibdir
. It’s just Mypy is not smart enough to figure that out.
Any more comments on this one? |
TODO: This will need review to make sure uninstallation works for PEP 660 editables.