Introduce pathlib on init_virtualenv. #12548
Conversation
IPython/core/interactiveshell.py
Outdated
| p_exe_up2 = os.path.dirname(os.path.dirname(p)) | ||
| if p_exe_up2 and os.path.exists(p_venv) and os.path.samefile(p_exe_up2, p_venv): | ||
| # Our exe is inside the virtualenv, don't need to do anything. | ||
| elif os.environ['VIRTUAL_ENV'] == '': |
There was a problem hiding this comment.
I'm changing this because of this issue #11100
which was resolved this way #11172
The code I'm using here was suggested on the issue, and it seems better to me,
specially now that we are using Pathlib, since Path('') actualy means the path in which you currently are
So, as is, we would get a True when we should get a False
In [4]: Path('').exists()
Out[4]: True
If the warning is a bit excessive I can remove it and merge with the upwards if statement
| # In Cygwin paths like "c:\..." and '\cygdrive\c\...' are possible | ||
| if p_venv.startswith('\\cygdrive'): | ||
| p_venv = p_venv[11:] | ||
| elif len(p_venv) >= 2 and p_venv[1] == ':': | ||
| p_venv = p_venv[2:] | ||
|
|
||
| if any(p_venv in p for p in paths): | ||
| # Running properly in the virtualenv, don't need to do anything | ||
| if any(os.fspath(p_venv) in os.fspath(p) for p in paths): |
There was a problem hiding this comment.
Found no pathlibian way to do this.
Can change to str(p_venv) if it's better, we can even forget running pathlib on this section, since the previous code seemed a little better to me
| while p.is_symlink(): | ||
| p = Path(os.readlink(p)) | ||
| paths.append(p.resolve()) | ||
|
|
||
| # In Cygwin paths like "c:\..." and '\cygdrive\c\...' are possible |
There was a problem hiding this comment.
Didn't change this code because I couldn't test it on my linux, currently looking for some VM thing
|
|
||
| # executable path should end like /bin/python or \\scripts\\python.exe | ||
| p_exe_up2 = os.path.dirname(os.path.dirname(p)) | ||
| if p_exe_up2 and os.path.exists(p_venv) and os.path.samefile(p_exe_up2, p_venv): |
There was a problem hiding this comment.
This condicional is already met on line 928, I have changed it's comments to reflect this change
| paths.append(p) | ||
|
|
||
| while p.is_symlink(): | ||
| p = Path(os.readlink(p)) |
There was a problem hiding this comment.
According to this link
I believe it's not possible to replicate this on pathlib
If someone sees a better solution please let me know.
There was a problem hiding this comment.
👍 we are 3.7+ so p.resolve() might work, but we can check this later.
There was a problem hiding this comment.
p.resolve() seems to follow symlinks all the way. Here we would like to follow them one by one to check later if any of them are inside the scope for any(os.fspath(p_venv) in os.fspath(p) for p in paths)
|
Thanks that's looks great. If there are breakage we can still fix later and make sure there is a test case in the windows CI. |
Sure, you are a core dev, you don't need my authorisation :-) @meeseeksdev backport to 7.x |
|
@Carreau I know, but I was worried that there are reasons behind not doing so that I'm not seeing. Thanks :)! |
…548-on-7.x Backport PR #12548 on branch 7.x (Introduce pathlib on init_virtualenv.)
I think It's also because i have less time than before and try to backport only what is critical and that was close to a year ago and I thoughts 8.0 was not that far down the line. |
Motivated by #12515
I'm still looking for a way to test the code on a windows VM, and will try to introduce automated tests later