Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ipython fails to start in bin folder due to Pathlib changes in interactiveshell.py #13177

Closed
bucknerns opened this issue Oct 5, 2021 · 9 comments

Comments

@bucknerns
Copy link
Contributor

bucknerns commented Oct 5, 2021

Ipython will enter an endless loop when starting from a bin folder in a virtualenv created from pyenv.

The cause is related to the fact that the pyenv virtual env has a relative link pointing from python to python3.9\

Folder structure
env/bin/python -> /home/nathan/.pyenv/versions/3.9.7/bin/python*
env/bin/python3.9 -> python\

.pyenv/versions/3.9.7/bin/python -> python3.9

Problem code

while p.is_symlink():

while p.is_symlink():
    p = Path(os.readlink(p))
    paths.append(p.resolve())

Resolve value always equals: /home/nathan/.pyenv/versions/3.9.7/bin/python3.9

First iteration p: /home/nathan/env/bin/python
After readlink: /home/nathan/.pyenv/versions/3.9.7/bin/python

Iter 2 p: /home/nathan/.pyenv/versions/3.9.7/bin/python
After readlink: python3.9   # Here the relative link wasn't handled, this was a reference to the .pyenv dir but now it will reference the current directory causing the loop

Iter 3 p: python3.9 # Will reference current dir
After: python

Iter 4 p: python
After: /home/nathan/.pyenv/versions/3.9.7/bin/python

loop steps 2-4 forever

Outside of the bin folder it works but not correctly

Iter 1 p: /home/nathan/env/bin/python
After: /home/nathan/.pyenv/versions/3.9.7/bin/python
Resolve: /home/nathan/.pyenv/versions/3.9.7/bin/python3.9

Before: /home/nathan/.pyenv/versions/3.9.7/bin/python
After: python3.9  # It will pass the is_symlink check but only because it is not looking at the current directory and the file doesn't exist.
Resolve: /home/nathan/python3.9 # appends a bad path but starts up

Edit: Fix link

@bucknerns
Copy link
Contributor Author

Seems to only affect 7.28.0

@sivel
Copy link

sivel commented Oct 15, 2021

Does this resolve your issue?

diff --git a/IPython/core/interactiveshell.py b/IPython/core/interactiveshell.py
index 37b31580d..5065ba239 100644
--- a/IPython/core/interactiveshell.py
+++ b/IPython/core/interactiveshell.py
@@ -919,8 +919,8 @@ def init_virtualenv(self):
         # So we just check every item in the symlink tree (generally <= 3)
         paths = [p]
         while p.is_symlink():
-            p = Path(os.readlink(p))
-            paths.append(p.resolve())
+            p = Path(os.readlink(p)).resolve()
+            paths.append(p)
         
         # In Cygwin paths like "c:\..." and '\cygdrive\c\...' are possible
         if p_venv.parts[1] == "cygdrive":
@@ -953,7 +953,7 @@ def init_virtualenv(self):
             "please install IPython inside the virtualenv."
         )
         import site
-        sys.path.insert(0, virtual_env)
+        sys.path.insert(0, str(virtual_env))
         site.addsitedir(virtual_env)
 
     #-------------------------------------------------------------------------

I'm experiencing a separate issue, where inserting PosixPath into sys.path is causing ipython to not be able to load any packages from the venv, and ran across this issue, while looking for mine.

@sivel
Copy link

sivel commented Oct 15, 2021

Actually that might not be the right fix for the while loop. I'm a little confused by the intent of the while loop. .resolve() will eliminate resolve all symlinks, whereas os.readlink() will only resolve 1, and not continue to follow additional symlinks.

In the end the list would likely contain duplicates as a result, but I feel like the intent was not to have that happen.

@bucknerns
Copy link
Contributor Author

I guess I wasn't clear when I opened the ticket, the issue is os.readlink.
It makes the assumption that the read in link is a complete path, in pyenv's case it is not.
So you go from a whole path to the read in link, which can be relative to any directory but Pathlib will assume the current working directory thus starting the loop. The fix above will fix this issue but defeats the purpose of this loop.
If you are going to add p = Path(os.readlink(p)).resolve() might as well just do
paths = [p, p.resolve()] and remove the loop.

So if the goal of this section is to add all of the python paths I think you want something like this, adjusting the cwd so your read in links get the correct absolute path.

I made a PR #13537

@bollwyvl bollwyvl reopened this Feb 26, 2022
@bollwyvl
Copy link
Contributor

the fix in #13537 apparently doesn't work for python 3.8: #13554

@bucknerns
Copy link
Contributor Author

I missed that, it can be replaced with os.readlink, I'll submit the change if no one else is

@bollwyvl
Copy link
Contributor

Fix (with test/coverage) would be good, but looks we'll be pushing out a reversion in the nearer term:

#13557

@bucknerns
Copy link
Contributor Author

#13559

@bucknerns
Copy link
Contributor Author

bucknerns commented Mar 30, 2022

This was merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants