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

files created in tmpdir can not always be deleted on windows (paths over 260 characters) #6754

Closed
torcolvin opened this issue Feb 17, 2020 · 5 comments
Labels
platform: windows windows platform-specific problem plugin: tmpdir related to the tmpdir builtin plugin type: bug problem that needs to be addressed

Comments

@torcolvin
Copy link
Contributor

torcolvin commented Feb 17, 2020

It possible to create files or directories inside tmpdir fixture that are not deletable with automated cleanup. In practice this can happen two ways: if the filename is used as a relative path or it is explicitly created in a way that using paths prefixed with \\?\

I am using pytest 5.3.5, with python 3.6, and Windows 10, but this could happen anywhere. Here is an excerpt from our test env that reproduces the issue:

import os

def dir_length(size=100):
    return str('a' * size)

def test_large_tmp_path(tmp_path):
    custom_tmp_path = tmp_path / dir_length(size=160)
    custom_tmp_path.mkdir()

    test_file = custom_tmp_path / "foo.txt"
    test_file.write_text('bar')

The output is that this test will pass, but if you run it more than 3x times it will produce the following warning output:

test_windows_pytest_tmp_path.py::test_large_tmp_path
  C:\msys64\scr\colvin\builds\2020-2\build\internal\lib\site-packages\_pytest\pathlib.py:56: PytestWarning: (rm_rf) error removing C:\Users\colvin\AppData\Local\Temp\2\pytest-of-colvin\garbage-8bf1668b-708f-47b6-9477-bb50c8169e89
  <class 'OSError'>: [WinError 145] The directory is not empty: 'C:\\Users\\colvin\\AppData\\Local\\Temp\\2\\pytest-of-colvin\\garbage-8bf1668b-708f-47b6-9477-bb50c8169e89'
    "(rm_rf) error removing {}\n{}: {}".format(path, exctype, excvalue)

This issue has been dismissed by cpython, in favor of applying a registry fix (https://docs.python.org/3/using/windows.html#removing-the-max-path-limitation). We do not want to enable the registry fix, since we are trying to replicate the environment of users who may not be able to enable this fix.

I have a proposed PR to solve this issue.

@Zac-HD Zac-HD added platform: windows windows platform-specific problem plugin: tmpdir related to the tmpdir builtin plugin type: bug problem that needs to be addressed labels Feb 18, 2020
@symonk
Copy link
Member

symonk commented Apr 13, 2020

This is also a problem for developing (windows) for pytest. our tox test suites often break things on windows:

/AppData/Local/Temp has all sorts of occassional OS WinError 145s

@symonk
Copy link
Member

symonk commented Apr 15, 2020

@torcolvin please open the PR i would like to have a look :) I am not sure if theres an awful lot we can do, we could maybe do something like:

shutil.rmtree(str(path) if not isinstance(path, WindowsPath) else WindowsPath(r'\\?\%s' % path), onerror=onerror)

This might help a bit? but it won't solve the issue, think its also not going to really work pre-py36, tmp_path yields a simple POSIX/Windows Path i believe based on platform, calling shutil.rmtree (which is what we do).

I think once any directory directly exceeds 256 chars, its going to fail regardless, for example:

quick hack:

def rm_rf(path: Path) -> None:
    """Remove the path contents recursively, even if some elements
    are read-only.
    """
    onerror = partial(on_rm_rf_error, start_path=path)
    shutil.rmtree(str(path) if not isinstance(path, WindowsPath) else WindowsPath(r'\\?\%s' % path), onerror=onerror)
# This will pass on win10
def test_tmp_dir_cleanup_handles_long_file_names(testdir, tmp_path):
    testdir.makepyfile("""
        def test_file_writing(tmp_path):
                custom_tmp_path = tmp_path / ('a'* 255)
                custom_tmp_path.mkdir()
                test_file = custom_tmp_path / "foo.txt"
                test_file.write_text('bar')
        """)
    testdir.runpytest()
    testdir.runpytest()
    testdir.runpytest()
    should_work = testdir.runpytest()
    assert should_work.ret == ExitCode.OK

with the following:

                custom_tmp_path = tmp_path / ('a'* 256)

this will fail again, so its a hack at best.

Maybe there is a better solution, I would recommend the registry fix for now (i know its not ideal for your customers etc)

we are deferring to mkdir() as tmp_path yields a standard type of WindowsPath for example, so the client would still be responsible for \?\ on their strings prior to using tmp_path.mkdir() and im not sure that everything would work smoothly still at that point

@torcolvin
Copy link
Contributor Author

I posted a PR here #6755 and have been using this patch locally to solve this issue.

@symonk
Copy link
Member

symonk commented Jun 11, 2020

@nicoddemus we should close this? #6755 was merged

@nicoddemus
Copy link
Member

Ahh yes thanks for catching it @symonk!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform: windows windows platform-specific problem plugin: tmpdir related to the tmpdir builtin plugin type: bug problem that needs to be addressed
Projects
None yet
Development

No branches or pull requests

4 participants