GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||
GeezerVison is showing github.com — GH-66409: check if exec_prefix is the same as prefix before searching executable_dir by FFY00 · Pull Request #127974 · python/cpython Conversation
|
|
Looks like this has actually resolved the issue in #106045. I'm going to write a test for it in any case - watch this space. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Actually, this might have also introduced a bug in that it now over-resolves the executable. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Please open an issue with reproduction instructions and I'll have a look 👍 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
The following test fails before your change: diff --git a/Lib/test/test_getpath.py b/Lib/test/test_getpath.py index f86df9d0d03..1c2e2a0b3fc 100644 --- a/Lib/test/test_getpath.py +++ b/Lib/test/test_getpath.py @@ -864,6 +864,55 @@ def test_PYTHONHOME_in_venv(self): actual = getpath(ns, expected) self.assertEqual(expected, actual) + def test_venv_w_symlinked_base_executable(self): + """ + If we symlink the base executable, and point to it via home in pyvenv.cfg, + we should have it as sys.executable (and sys.prefix should be the resolved location) + """ + ns = MockPosixNamespace( + argv0="/venv/bin/python3", + PREFIX="/some/_internal/prefix", + ) + # Setup venv + ns.add_known_xfile("/venv/bin/python3") + ns.add_known_xfile("/usr/local/bin/python3") + ns.add_known_xfile("/some/_internal/prefix/bin/python3") + + ns.add_known_file("/venv/pyvenv.cfg", [ + # The published based executable location is /usr/local/bin - we don't want to + # expose /some/internal/directory (this location can change under our feet) + r"home = /usr/local/bin" + ]) + ns.add_known_link("/venv/bin/python3", "/usr/local/bin/python3") + ns.add_known_link("/usr/local/bin/python3", "/some/_internal/prefix/bin/python3") + + ns.add_known_file("/some/_internal/prefix/lib/python9.8/os.py") + ns.add_known_dir("/some/_internal/prefix/lib/python9.8/lib-dynload") + + # Put a file completely outside of /usr/local to validate that the issue + # in https://github.com/python/cpython/issues/106045 is resolved. + ns.add_known_dir("/usr/lib/python9.8/lib-dynload") + + expected = dict( + executable="/venv/bin/python3", + prefix="/venv", + exec_prefix="/venv", + base_prefix="/some/_internal/prefix", + base_exec_prefix="/some/_internal/prefix", + # It is important to maintain the link to the original executable, as this + # is used when creating a new virtual environment (which should also have home + # set to /usr/local/bin to avoid bleeding the internal path to the venv) + base_executable="/usr/bin/python3", + module_search_paths_set=1, + module_search_paths=[ + "/some/_internal/prefix/lib/python98.zip", + "/some/_internal/prefix/lib/python9.8", + "/some/_internal/prefix/lib/python9.8/lib-dynload", + ], + ) + actual = getpath(ns, expected) + self.assertEqual(expected, actual) + # ****************************************************************************** DEFAULT_NAMESPACE = dict( With: After your change, it fails with: In conclusion: I think this change has improved the situation. The issue that remains can be considered a new one, and is the one I mention in #106045 (comment), namely:
I'll report it now. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Also, to get it off my chest: I worry about the tests in test_getpath since they are only mocking the real behaviour. To give an example of a problem that might get missed if we use Imagine that we add a test in test_getpath which validates that a prefix My conclusion is that test_getpath is only good for quick developer validation, and that we should duplicate many of the tests using the real implementation, as is done in |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Yes, although the coverage isn't perfect, we test the real implementation in |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@FFY00 - since this change also fixes a bug which affects Similarly, for #128670 a fix there would ideally be backported as far as we can. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Not really, the reason this works is because of GH-127972, which cannot be backported. This PR just syncs |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
test_embed.test_init_pyvenv_cfg() started to fail in some cases since this change: see #128690 issue. I wrote #129137 to update test_embed for this change. Please have a look. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.