Fix site.getsitepackages() broken on python2 on debian #2108

freundTech · 2021-05-04T16:23:19Z

Fixes #2105.
This PR also adds a regression test.
There might be a merge conflict between this PR and #2107, because they both add a test to the end of tests/unit/create/test_creator.py

gaborbernat · 2021-05-04T16:37:06Z

Can you rebase on the master and fix the merge conflict? Thanks!

tests/unit/create/test_creator.py

gaborbernat · 2021-05-04T16:38:27Z

src/virtualenv/create/via_global_ref/builtin/python2/site.py

+        sitepackages = orig_getsitepackages()
+        for prefix in (sys.prefix, sys.exec_prefix):
+            # os is not defined here, but will be in site.py, where we monkey-patch this function.
+            path = os.path.join(prefix, "lib", "python" + sys.version[:3], "site-packages")  # noqa: F821


You can't write code like this, that's platform dependent. You must detect if you're on debian and execute this only then.

Oh. Right. I forgot the check. I think checking for linux should be enough though, as non debian linux will already contain the path, so if path not in sitepackages will be False.

The code now does the same check the debians patched site.py does. This prevents the code from running on systems where it would break.

It will still run on non-debian systems, but shouldn't have any effect there. I don't think specifically detecting debian would be practical.

Oh. Right. I forgot the check. I think checking for linux should be enough though, as non debian linux will already contain the path, so if path not in sitepackages will be False.

That's some very presumptions statement there. What stops macOS from patching similarly as does Debian? Or any other distribution? We really must restrict this to Debian only.

I think that this should also handle all cases where an OS patches python in a way similar to debian.
The added path isn't debian specific. It is virtualenv specific and should always be in getsitepackages. The problem is just that because of debians patches it isn't.

The only thing I would have to fix is use ___EXPECTED_SITE_PACKAGES___ instead of hardcoding "site-packages".

I can however try restricting it to debian to be save.
Detecting debian and it derivatives isn't easy though. Just checking if platform.dist is debian isn't enough as we also need to detect Ubuntu and other derivatives.

After looking through all the debian patches I think the best way would be to detect the "deb_system" key in distutils.command.install.INSTALL_SCHEMES.
Alternatively it would be possible to search for occurrences of "dist-packages" in the source of getsitepackages.

The only thing I would have to fix is use ___EXPECTED_SITE_PACKAGES___ instead of hardcoding "site-packages".

This might suffice.

If you can do this then 👍

I just pushed a new version. I pulled some code from load_host_site into a new function to reuse it in rewrite_getsitepackages.

Because the code now makes no assumptions about the operating system other than what is already assumed by load_host_site I removed the os check.

codecov · 2021-05-04T16:58:21Z

Codecov Report

Merging #2108 (dcc2a84) into main (cc91e3f) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2108   +/-   ##
=======================================
  Coverage   93.70%   93.70%           
=======================================
  Files          88       88           
  Lines        4385     4385           
=======================================
  Hits         4109     4109           
  Misses        276      276

Flag	Coverage Δ
tests	`93.70% <ø> (ø)`

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

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 cc91e3f...dcc2a84. Read the comment docs.

gaborbernat · 2021-05-04T17:37:26Z

src/virtualenv/create/via_global_ref/builtin/python2/site.py

+        sitepackages = orig_getsitepackages()
+        for prefix in (sys.prefix, sys.exec_prefix):
+            # os is not defined here, but will be in site.py, where we monkey-patch this function.
+            path = os.path.join(prefix, "lib", "python" + sys.version[:3], "site-packages")  # noqa: F821


Oh. Right. I forgot the check. I think checking for linux should be enough though, as non debian linux will already contain the path, so if path not in sitepackages will be False.

That's some very presumptions statement there. What stops macOS from patching similarly as does Debian? Or any other distribution? We really must restrict this to Debian only.

gaborbernat · 2021-05-05T07:51:27Z

src/virtualenv/create/via_global_ref/builtin/python2/site.py

+        sitepackages = orig_getsitepackages()
+        for prefix in (sys.prefix, sys.exec_prefix):
+            # os is not defined here, but will be in site.py, where we monkey-patch this function.
+            path = os.path.join(prefix, "lib", "python" + sys.version[:3], "site-packages")  # noqa: F821


If you can do this then 👍

gaborbernat

I'll approve if you manage to get the CI working.

freundTech · 2021-05-05T10:55:31Z

I'm currently looking into it. It seems to be a kind of bootstrap problem, where currently it only fails if the tests are run inside a virtualenv that already contains this PR, due to the way I get the system site-packages to test against.

freundTech · 2021-05-05T11:57:42Z

CI should pass now.

Looks like one of the runs is locked up though.

freundTech · 2021-05-05T17:19:55Z

I retriggered the CI with an empty commit and everything works now.

gaborbernat

gaborbernat · 2021-05-05T23:05:31Z

Released via https://pypi.org/project/virtualenv/20.4.6/

freundTech · 2021-05-06T08:59:12Z

Thanks!

freundTech mentioned this pull request May 4, 2021

Fix site.getsitepackages() ignoring --system-site-packages on python2 #2107

Merged

gaborbernat reviewed May 4, 2021

View reviewed changes

tests/unit/create/test_creator.py Outdated Show resolved Hide resolved

gaborbernat requested changes May 4, 2021

View reviewed changes

freundTech added 4 commits May 4, 2021 18:51

Fix site.getsitepackages() broken on python2 on debian

7692c9f

Fix linter

574d9ee

Prevent rewriting getsitepackages on non-unix systems

d42da04

Add underscores to test name

40c2751

freundTech force-pushed the fix-getsitepackages-debian branch from 6f50ebf to 40c2751 Compare May 4, 2021 16:52

Make test case-insesitive for insensitive filesystems

87ba615

freundTech force-pushed the fix-getsitepackages-debian branch from 59e206c to 87ba615 Compare May 4, 2021 17:23

gaborbernat requested changes May 4, 2021

View reviewed changes

gaborbernat requested changes May 5, 2021

View reviewed changes

Refactor getsitepackages rewriting

0852b9e

gaborbernat reviewed May 5, 2021

View reviewed changes

Don't patch getsitepackages if prefix is not in PREFIXES

b5a3948

freundTech force-pushed the fix-getsitepackages-debian branch from 38e2050 to b5a3948 Compare May 5, 2021 15:57

Trigger CI

dcc2a84

gaborbernat approved these changes May 5, 2021

View reviewed changes

gaborbernat merged commit 7bfd444 into pypa:main May 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix site.getsitepackages() broken on python2 on debian #2108

Fix site.getsitepackages() broken on python2 on debian #2108

freundTech commented May 4, 2021

gaborbernat commented May 4, 2021

gaborbernat May 4, 2021

freundTech May 4, 2021

freundTech May 4, 2021

gaborbernat May 4, 2021

freundTech May 4, 2021

gaborbernat May 4, 2021

gaborbernat May 5, 2021

freundTech May 5, 2021

codecov bot commented May 4, 2021 •

edited

Loading

gaborbernat May 4, 2021

gaborbernat May 5, 2021

gaborbernat left a comment

freundTech commented May 5, 2021

freundTech commented May 5, 2021

freundTech commented May 5, 2021

gaborbernat left a comment

gaborbernat commented May 5, 2021

freundTech commented May 6, 2021

Fix site.getsitepackages() broken on python2 on debian #2108

Fix site.getsitepackages() broken on python2 on debian #2108

Conversation

freundTech commented May 4, 2021

gaborbernat commented May 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 4, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaborbernat left a comment

Choose a reason for hiding this comment

freundTech commented May 5, 2021

freundTech commented May 5, 2021

freundTech commented May 5, 2021

gaborbernat left a comment

Choose a reason for hiding this comment

gaborbernat commented May 5, 2021

freundTech commented May 6, 2021

codecov bot commented May 4, 2021 •

edited

Loading