During seeding properly uninstall present versions of the wheels #2186
Conversation
|
Yep, that's the next step. |
arcivanov
changed the title
|
@gaborbernat Please let me know if you'd like me to squash the changes into a single commit |
|
No thanks 👍 |
Codecov Report
@@ Coverage Diff @@
## main #2186 +/- ##
=======================================
Coverage 93.56% 93.57%
=======================================
Files 88 88
Lines 4380 4402 +22
=======================================
+ Hits 4098 4119 +21
- Misses 282 283 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
@gaborbernat anything else you require or is this ready for merge? |
|
I want to do some cleanup, so currently waiting on my availability. |
|
@gaborbernat do you want to cleanup this commit? or cleanup in general? Would it be possible to release VEnv with this fix and perform the cleanup later? |
|
I want to restructure a bit the code you've contributed 👍 will try to do this tomorrow morning. |
|
Thanks a lot! |
An existing dist-info may contain entrypoints that may interfere with normal functioning of the redeployed seeded wheel if there is a version mismatch fixes pypa#2185
Remove other recorded files from RECORD Remove dist-info itself
In the test ensure the directories are compared as sets and not lists Add setuptools downgrade to ensure proper cleanup of the existing version
gaborbernat
force-pushed
the
issue_2185
branch
from
f3cacb4 to
be05b86
Compare
Signed-off-by: Bernát Gábor <[email protected]>
gaborbernat
force-pushed
the
issue_2185
branch
from
be05b86 to
ddda7ce
Compare
| # sync image | ||
| for filename in self._image_dir.iterdir(): | ||
| into = self._creator.purelib / filename.name | ||
| if into.exists(): |
There was a problem hiding this comment.
@gaborbernat I'm sorry, this is NOT an equivalent change.
We went from "delete target prior to copying into" into "uninstall previous distro but do not delete target directory if it wasn't part of the distro".
I specifically preserved that functionality because if there are extraneous files that are NOT part of a valid distro, venv would override them on install and won't fail. Now VEnv will fail as soon as it encounters a directory/file that exists which can happen if overwriting a partially created VEnv directory.
There was a problem hiding this comment.
Hmm, how do you end up with a partially created venv? Do you have some real-world examples of this or are you talking HYPOTHETICALS?
There was a problem hiding this comment.
Ctrl-C during venv installation with partial install of the package. This will occur intermittently and how intermittent it'll be will depend on the PIP package installation logic which will also change from time to time (whether they write dist-info first or they write package data first etc).
My point is: there was specific logic here that I preserved (not introduced!) that resulted in a very specific behavior that made venv more robust. The cleanup resulted in that logic being removed which made venv less robust and more error prone while the problem I tried to fix was fixed.
Can we please make venv less brittle again and reintroduce the logic that was there before?
There was a problem hiding this comment.
This is virtualenv, not venv 😊 please.
There was a problem hiding this comment.
This change you introduced relies on everything else (PIP, wheel, bdist_wheel) working 100% correctly 100% of the time: fully installed packages with 100% correct RECORD file that recorded all the files that were actually installed and that package is installed successfully 100% of the time and never interrupted in the process of installation by user action or running out of disk space.
| @@ -150,6 +147,36 @@ def _create_console_entry_point(self, name, value, to_folder, version_info): | |||
| result.extend(Path(i) for i in new_files) | |||
| return result | |||
|
|
|||
| def _uninstall_previous_version(self): | |||
| dist_name = self._dist_info.stem.split("-")[0] | |||
| in_folders = chain.from_iterable([i.iterdir() for i in {self._creator.purelib, self._creator.platlib}]) | |||
There was a problem hiding this comment.
The installation happens only into the purelib: https://github.com/pypa/virtualenv/pull/2186/files#diff-05dbe4fcecab074226aa3cf0e1d6285e9a47e4a1c529a20ab49f2b572c3d49eaR38
Why bother with platlib when install will never actually touch it?
There was a problem hiding this comment.
If the package is either in the purelib or platlib is an implementation detail of the installer. Our installer uses purelib, but is completely legal for someone else to use platlib for it. We don't control the previous state.
There was a problem hiding this comment.
Then it's even more important to restore the logic above. If the package being removed is in the platlib but the extraneous files are in purelib then while the package is removed from platlib the purelib will stay polluted and installation will fail.
An existing dist-info may contain entrypoints that may interfere with
normal functioning of the redeployed seeded wheel if there is a version
mismatch
fixes #2185
Thanks for contributing, make sure you address all the checklists (for details on how see
development documentation)!
tox -e fix_lint)docs/changelogfolder