During seeding properly uninstall present versions of the wheels by arcivanov · Pull Request #2186 · pypa/virtualenv

arcivanov · 2021-09-17T00:11:38Z

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)!

ran the linter to address style issues (tox -e fix_lint)
wrote descriptive pull request text
ensured there are test(s) validating the fix
added news fragment in docs/changelog folder
updated/extended the documentation

arcivanov · 2021-09-17T00:12:05Z

@gaborbernat

gaborbernat

You'll need to add a test to validate the change and defend against regressions.

gaborbernat

Please add test for this and ensure that the diff coverage is 💯🙏 thanks

arcivanov · 2021-09-18T23:33:48Z

Yep, that's the next step.

arcivanov · 2021-09-20T20:23:06Z

@gaborbernat Please let me know if you'd like me to squash the changes into a single commit

gaborbernat · 2021-09-20T22:03:28Z

No thanks 👍

codecov · 2021-09-20T22:11:44Z

Codecov Report

Merging #2186 (ddda7ce) into main (40d2f18) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2186   +/-   ##
=======================================
  Coverage   93.56%   93.57%           
=======================================
  Files          88       88           
  Lines        4380     4402   +22     
=======================================
+ Hits         4098     4119   +21     
- Misses        282      283    +1

Flag	Coverage Δ
tests	`93.57% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ualenv/seed/embed/via_app_data/pip_install/base.py	`96.05% <100.00%> (+0.66%)`	⬆️
src/virtualenv/seed/embed/base_embed.py	`96.22% <0.00%> (-1.89%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 40d2f18...ddda7ce. Read the comment docs.

arcivanov · 2021-09-20T23:27:01Z

@gaborbernat anything else you require or is this ready for merge?

gaborbernat · 2021-09-21T10:15:36Z

I want to do some cleanup, so currently waiting on my availability.

arcivanov · 2021-09-23T15:54:30Z

@gaborbernat do you want to cleanup this commit? or cleanup in general?
Thing is, lack of this fix is blocking pybuilder/pybuilder release as we bundle VEnv and it's critical to our function.

Would it be possible to release VEnv with this fix and perform the cleanup later?

gaborbernat · 2021-09-23T15:55:35Z

I want to restructure a bit the code you've contributed 👍 will try to do this tomorrow morning.

arcivanov · 2021-09-23T17:04:20Z

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

Signed-off-by: Bernát Gábor <gaborjbernat@gmail.com>

arcivanov · 2021-09-24T15:59:25Z

        # sync image
        for filename in self._image_dir.iterdir():
            into = self._creator.purelib / filename.name
-            if into.exists():


@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.

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?

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?

This is virtualenv, not venv 😊 please.

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.

arcivanov · 2021-09-24T16:01:15Z


+    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}])


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?

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.

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.

gaborbernat requested changes Sep 17, 2021

View reviewed changes

Comment thread src/virtualenv/seed/embed/via_app_data/pip_install/base.py Outdated

gaborbernat requested changes Sep 18, 2021

View reviewed changes

Comment thread src/virtualenv/seed/embed/via_app_data/pip_install/base.py Outdated

Comment thread src/virtualenv/seed/embed/via_app_data/pip_install/base.py Outdated

gaborbernat reviewed Sep 18, 2021

View reviewed changes

arcivanov changed the title ~~During seeding remove dist-info for present versions of the wheels~~ During seeding properly uninstall present versions of the wheels Sep 19, 2021

arcivanov added 3 commits September 24, 2021 08:31

During seeding remove dist-info for present versions of the wheels

f54bfd5

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 package directories from dist-info top_level packages

65de5e8

Remove other recorded files from RECORD Remove dist-info itself

Do not resolve paths prior to removal for symlink mode

140f84d

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 September 24, 2021 08:25

gaborbernat approved these changes Sep 24, 2021

View reviewed changes

PR Feedback

ddda7ce

Signed-off-by: Bernát Gábor <gaborjbernat@gmail.com>

gaborbernat force-pushed the issue_2185 branch from be05b86 to ddda7ce Compare September 24, 2021 08:40

gaborbernat merged commit e13ea2a into pypa:main Sep 24, 2021

arcivanov commented Sep 24, 2021

View reviewed changes

arcivanov mentioned this pull request Sep 25, 2021

Support overriding corrupt virtual environments #2198

Closed

gaborbernat mentioned this pull request Oct 17, 2026

🐛 fix(seed): contain metadata cleanup #3428

Merged

Conversation

arcivanov commented Sep 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Thanks for contributing, make sure you address all the checklists (for details on how see

Uh oh!

arcivanov commented Sep 17, 2021

Uh oh!

Uh oh!

gaborbernat left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gaborbernat left a comment

Choose a reason for hiding this comment

Uh oh!

arcivanov commented Sep 18, 2021

Uh oh!

arcivanov commented Sep 20, 2021

Uh oh!

gaborbernat commented Sep 20, 2021

Uh oh!

codecov Bot commented Sep 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

arcivanov commented Sep 20, 2021

Uh oh!

gaborbernat commented Sep 21, 2021

Uh oh!

arcivanov commented Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaborbernat commented Sep 23, 2021

Uh oh!

arcivanov commented Sep 23, 2021

Uh oh!

arcivanov Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

gaborbernat Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

arcivanov Sep 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaborbernat Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

arcivanov Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

gaborbernat Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

arcivanov Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

gaborbernat Sep 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arcivanov Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

arcivanov commented Sep 17, 2021 •

edited

Loading

codecov Bot commented Sep 20, 2021 •

edited

Loading

arcivanov commented Sep 23, 2021 •

edited

Loading

arcivanov Sep 24, 2021 •

edited

Loading

gaborbernat Sep 24, 2021 •

edited

Loading