GH-66409: check if exec_prefix is the same as prefix before searching executable_dir

[FFY00]

• Issue: Make pyvenv style virtual environments easier to configure when embedding Python #66409

[pelson]

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.

[pelson]

Actually, this might have also introduced a bug in that it now over-resolves the executable.

[FFY00]

Please open an issue with reproduction instructions and I'll have a look 👍

[pelson]

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:

AssertionError: {'exe[132 chars]': '/some/_internal/prefix', 'base_executable'[206 chars]ad']} != {'exe[132 chars]': '/usr', 'base_executable': '/some/_internal[188 chars]ad']}
- {'base_exec_prefix': '/some/_internal/prefix',
+ {'base_exec_prefix': '/usr',
-  'base_executable': '/usr/bin/python3',
?                       -

+  'base_executable': '/some/_internal/prefix/bin/python3',
?                        +++++++++ ++++++++++

   'base_prefix': '/some/_internal/prefix',
   'exec_prefix': '/venv',
   'executable': '/venv/bin/python3',
   'module_search_paths': ['/some/_internal/prefix/lib/python98.zip',
                           '/some/_internal/prefix/lib/python9.8',
-                          '/some/_internal/prefix/lib/python9.8/lib-dynload'],
?                             --------- ----------

+                          '/usr/lib/python9.8/lib-dynload'],
?                            +

   'module_search_paths_set': 1,
   'prefix': '/venv'}

After your change, it fails with:

  {'base_exec_prefix': '/some/_internal/prefix',
-  'base_executable': '/usr/bin/python3',
?                       -

+  'base_executable': '/some/_internal/prefix/bin/python3',
?                        +++++++++ ++++++++++

   'base_prefix': '/some/_internal/prefix',
   'exec_prefix': '/venv',
   'executable': '/venv/bin/python3',
   'module_search_paths': ['/some/_internal/prefix/lib/python98.zip',
                           '/some/_internal/prefix/lib/python9.8',
                           '/some/_internal/prefix/lib/python9.8/lib-dynload'],
   'module_search_paths_set': 1,
   'prefix': '/venv'}

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:

One thing to note: I think the calculation of sys._base_executable is over-resolving symlinks today - this is a separate bug which I guess means that homebrew users can't take a venv from a venv and have it survive beyond the next patch update.

I'll report it now.

[pelson]

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 test_getpath as the only place for testing the startup behaviour:

Imagine that we add a test in test_getpath which validates that a prefix /some/public/prefix is a symlink to another real one /some/_internal/prefix. If we write a test that ensures that the prefix is not resolved to the internal one, it should pass today. If we then update getpath.c to do recursive path readlink, instead of file only readlink, then the test will continue to pass even though the underlying implementation changed. This is because there is a mocked implementation of realpath which meets our assumptions today, but doesn't guarantee to be the same as getpath_realpath.

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 test_venv. Are there equivalent tests which use subprocess for the non venv case?

[FFY00]

Yes, although the coverage isn't perfect, we test the real implementation in test_embed.

[pelson]

@FFY00 - since this change also fixes a bug which affects venv and virtualenv, is this something that we can backport to 3.12 & 3.13, or do you consider this change to actually be a new feature for 3.14 onwards?

Similarly, for #128670 a fix there would ideally be backported as far as we can.

[FFY00]

Not really, the reason this works is because of GH-127972, which cannot be backported. This PR just syncs exec_prefix up with prefix.

[vstinner]

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.