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

Return canonical path when using %APPDATA% on NT #222

Merged
merged 7 commits into from
Aug 18, 2021

Conversation

florianbussmann
Copy link
Contributor

@florianbussmann florianbussmann commented Apr 6, 2021

This fixes starting jupyter lab and notebook on windows systems when python is installed from the Microsoft Store.
jupyterlab/jupyterlab#9250
jupyter-server/jupyter_server#435
jupyter/notebook#5996

Background

The following runtime dir is not accessible outside of python as shown in jupyterlab/jupyterlab#9250 (comment)

runtime:
    C:\Users\bussmann\AppData\Roaming\jupyter\runtime

Result

Tested on florianbussmann/notebook@5a3be03 the runtime dir used by jupyter-core in florianbussmann@ad09dbf is accessible

(.venv) C:\Users\bussmann\source\florianbussmann\notebook>jupyter --paths
config:
    C:\Users\bussmann\.jupyter
    c:\users\bussmann\source\florianbussmann\notebook\.venv\etc\jupyter
    C:\ProgramData\jupyter
data:
    C:\Users\bussmann\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.9_qbz5n2kfra8p0\LocalCache\Roaming\jupyter
    c:\users\bussmann\source\florianbussmann\notebook\.venv\share\jupyter
    C:\ProgramData\jupyter
runtime:
    C:\Users\bussmann\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.9_qbz5n2kfra8p0\LocalCache\Roaming\jupyter\runtime

Tests

(.venv) C:\Users\bussmann\source\florianbussmann\jupyter_core>py.test -rs 
=========================================================================================== test session starts ============================================================================================
platform win32 -- Python 3.9.3, pytest-6.2.3, py-1.10.0, pluggy-0.13.1
rootdir: C:\Users\bussmann\source\florianbussmann\jupyter_core
collected 52 items

jupyter_core\tests\test_application.py ........                                                                                                                                                       [ 15%]
jupyter_core\tests\test_command.py ............                                                                                                                                                       [ 38%]
jupyter_core\tests\test_migrate.py ........                                                                                                                                                           [ 53%]
jupyter_core\tests\test_paths.py .......................s                                                                                                                                             [100%]

========================================================================================= short test summary info ========================================================================================== 
SKIPPED [1] jupyter_core\tests\test_paths.py:315: does not run on windows
====================================================================================== 51 passed, 1 skipped in 3.27s ======================================================================================= 

@jasongrout
Copy link
Member

jasongrout commented Apr 6, 2021

FYI, I found out that realpath does not behave as well as Path.resolve() on Windows on Python 3.6 in jupyterlab/jupyterlab#9709. IIRC, in Python 3.6 on Windows, realpath would not correct the case of the path to a canonical case (including the case of the drive letter) where Path.resolve() would do so. I don't have a windows box up to test it again, but IIRC realpath(r'c:\users') would not return a canonically-cased filename, whereas Path(r'c:\users').resolve() would.

Not sure if something needs to change here, but I thought I'd flag a problem I saw with using realpath recently.

@jasongrout
Copy link
Member

jasongrout commented Apr 6, 2021

Yes, here is a test with python 3.6 installed via conda-forge:

Python 3.6.13 (default, Feb 19 2021, 05:17:09) [MSC v.1916 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> import os.path
>>> os.path.realpath(r'c:\users')
'c:\\users'
>>> from pathlib import Path
>>> Path(r'c:\users').resolve()
WindowsPath('C:/Users')
>>> str(Path(r'c:\users').resolve())
'C:\\Users'

@jasongrout
Copy link
Member

Here is Python 3.7 on Windows:

Python 3.7.10 | packaged by conda-forge | (default, Feb 19 2021, 15:37:01) [MSC v.1916 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> import os.path
>>> os.path.realpath(r'c:\users')
'c:\\users'

And it looks like it was fixed in Python 3.8:

Python 3.8.8 | packaged by conda-forge | (default, Feb 20 2021, 15:50:08) [MSC v.1916 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> os.path.realpath(r'c:\users')
'C:\\Users'

@florianbussmann
Copy link
Contributor Author

@jasongrout Thats very interesting, I thought windows wouldn't care about the case when resolving files.

I installed python 3.7 from the Microsoft Store and get the same file not found error for jupyter notebook as before.. the runtime dir does not resolve to the correct path created by the app installation of python although the case is correct in my case.

(.venv) C:\Users\bussmann\source\florianbussmann\notebook-37>python --version
Python 3.7.9

(.venv) C:\Users\bussmann\source\florianbussmann\notebook-37>jupyter --paths
config:
    C:\Users\bussmann\.jupyter
    c:\users\bussmann\source\florianbussmann\notebook-37\.venv\etc\jupyter
    C:\ProgramData\jupyter
data:
    C:\Users\bussmann\AppData\Roaming\jupyter
    c:\users\bussmann\source\florianbussmann\notebook-37\.venv\share\jupyter
    C:\ProgramData\jupyter
runtime:
    C:\Users\bussmann\AppData\Roaming\jupyter\runtime

pathlib resolves the path correctly from the same variable

(.venv) C:\Users\bussmann\source\florianbussmann\notebook-37>python
Python 3.7.9 (tags/v3.7.9:13c94747c7, Aug 17 2020, 16:30:00) [MSC v.1900 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> runtime_dir = r'C:\Users\bussmann\AppData\Roaming\jupyter\runtime'
>>> import os
>>> os.path.realpath(runtime_dir)    
'C:\\Users\\bussmann\\AppData\\Roaming\\jupyter\\runtime'
>>> from pathlib import Path
>>> Path(runtime_dir).resolve() 
WindowsPath('C:/Users/bussmann/AppData/Local/Packages/PythonSoftwareFoundation.Python.3.7_qbz5n2kfra8p0/LocalCache/Roaming/jupyter/runtime')

I would change my fix to

str(Path(...).resolve())

as introduced in the mentioned jupyterlab issue

On darwin systems os.path.realpath is used when resolving the homedir and I am wondering if this should be changed as well

home = get_home_dir()
if sys.platform == 'darwin':
return os.path.join(home, 'Library', 'Jupyter')

def get_home_dir():
"""Get the real path of the home directory"""
homedir = os.path.expanduser('~')
# Next line will make things work even when /home/ is a symlink to
# /usr/home as it is on FreeBSD, for example
homedir = os.path.realpath(homedir)
return homedir

@jasongrout
Copy link
Member

@florianbussmann - sorry, an update on a linked issue led to this, and I realize you might have been waiting for a reply:

I would change my fix to str(Path(...).resolve()) as introduced in the mentioned jupyterlab issue

That sounds great. It's been a while - do you still have time to make that change?

On darwin systems os.path.realpath is used when resolving the homedir and I am wondering if this should be changed as well

I don't know of a difference (off the top of my head) between Path.resolve and realpath on darwin that would affect us (unlike the bug pointed out above for Windows), so I don't think it's necessary in this PR. On the other hand, if you'd like to make that change too to be consistent, I think that would be fine too.

@CherryDT
Copy link

@jasongrout Thats very interesting, I thought windows wouldn't care about the case when resolving files.

Note that it's up to the filesystem whether it does or not. In fact current versions of NTFS allow a per-directory flag of case sensitivity. While it's true that some parts of the Win32 subsystem assume case-insensitivity and get confused if that's not the case, it's not universally true that case doesn't matter for Windows.

@jasongrout
Copy link
Member

In fact current versions of NTFS allow a per-directory flag of case sensitivity.

Oh, wow. Just wow. Thanks for the info. I wouldn't have imagined that.

@jasongrout Thats very interesting, I thought windows wouldn't care about the case when resolving files.

Note that the bug we were running into that led to the investigation was a case-sensitive cache of filenames (IIRC, Node's cache), so it didn't matter what Windows behavior was, the key there was that the paths were not normalized, so Node's cache was treating two paths different when they were the same on disk.

On Python 3.6 and 3.7, realpath does not give the canonical case for drive letters and paths, while Path().resolve() does. See the discussion at jupyter#222 (comment)
@jasongrout
Copy link
Member

@florianbussmann - oh right, I forgot about the appdata mocking! Thanks!

Copy link
Member

@jasongrout jasongrout left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. Thanks!

@jasongrout
Copy link
Member

@florianbussmann - thanks again for putting this PR in and pushing it forward. Do my changes look okay to you?

@florianbussmann
Copy link
Contributor Author

@jasongrout - looks good to me! I also verified this on my windows machine and it still works as expected on my windows-store installations of python.

@jasongrout jasongrout merged commit f5660f6 into jupyter:master Aug 18, 2021
@jasongrout jasongrout added this to the 4.8 milestone Aug 18, 2021
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

Successfully merging this pull request may close these issues.

None yet

3 participants