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

Empty cwd on filesystem leads to error on .absolute() paths (Windows) #673

Closed
stijnPostNL opened this issue Apr 25, 2022 · 5 comments · Fixed by #676
Closed

Empty cwd on filesystem leads to error on .absolute() paths (Windows) #673

stijnPostNL opened this issue Apr 25, 2022 · 5 comments · Fixed by #676
Labels

Comments

@stijnPostNL
Copy link

stijnPostNL commented Apr 25, 2022

Describe the bug
When using this module on a Windows environment, whenever a patched out os.getcwd is used ( i.e. due to calling a setUpPyfakefs / using FakeOsModule ), it's no longer possible to retrieve a real absolute path on any pathlib.Path object on Windows filesystem, since a drive letter will always be omitted.

See reference for .absolute() in pathlib code at https://github.com/python/cpython/blob/b28265d7e6a41a4a0e227b37f4fbbc4d03a0a707/Lib/pathlib.py#L1193-L1207

Maybe we can change the implementation on FakeOsModule.getcwd() / FakeOsModule.getcwdb()?

How To Reproduce
Please provide a unit test or a minimal code snippet that reproduces the problem.

@unittest.skipIf(not is_windows, 'Windows-specific behavior')
class TestAbsolutePathOnWindows(fake_filesystem_unittest.TestCase):
    @patchfs
    def test_is_absolute(self, fs):
        """When os.getcwd() is patched to return a path without
        drive letter, absolute() will never make an actual absolute path"""
        self.assertTrue(pathlib.Path(".").absolute().is_absolute())

Your environment

PS C:\Users\Stijn IJzermans> python -c "import platform; print(platform.platform())"
Windows-10-10.0.22000-SP0
PS C:\Users\Stijn IJzermans> python -c "import sys; print('Python', sys.version)"
Python 3.9.12 (tags/v3.9.12:b28265d, Mar 23 2022, 23:52:46) [MSC v.1929 64 bit (AMD64)]
PS C:\Users\Stijn IJzermans> python -c "from pyfakefs.fake_filesystem import __version__; print('pyfakefs', __version__)"
pyfakefs 4.6.dev0
@stijnPostNL stijnPostNL changed the title Empty cwd on filesystem leads to unexpected behaviour (Windows) Empty cwd on filesystem leads to error on .absolute() paths (Windows) Apr 25, 2022
@mrbean-bremen
Copy link
Member

Thanks for the report! There is indeed a problem with the root path under Windows, which is not a drive but just \. I did try at one point to change this, but this caused all kinds of problems (which I forgot meanwhile), so I abandoned it at the time. I just found a work branch I made for this, but this was 4 years ago, so it is likely very outdated.

Anyway, I think I have to revisit this, now that there is a real problem, but this may take some time.

@stijnPostNL
Copy link
Author

@mrbean-bremen thanks for your quick response and makes sense!

So would adjusting the getcwd to return C:\ on (or another configurable drive) win32 systems instead of \ lead to more errors than it solves? Since the actual os.getcwd() would never be able return a \ in a Windows environment.

If I can be of any further help please let me know.

@mrbean-bremen
Copy link
Member

Maybe this can be solved with a quick fix for getcwd only, without affecting the actual root used throughout the file system, I have to check this. The problem I had was with trying to change the file system root to C:\ under Windows, but maybe this is not needed to solve this issue.

@mrbean-bremen
Copy link
Member

I think I have to fix this properly, which will take some time. I started to adapt the code accordingly, and while there are several changes needed in tha actual code, more changes are needed in the tests, as a lot of them fail due to assumptions that are no longer true. I'm not sure when I will get the time to finish this (short on free time at the moment), but I think it makes sense to do this anyway.

mrbean-bremen added a commit to mrbean-bremen/pyfakefs that referenced this issue May 9, 2022
- as Windows has no root dir as such, we use the
  current drive as root dir to better emulate the fs
- as in the real fs, if a path starts with a path separator,
  it points to the current drive (e.g. the root of cwd)
- fixes pytest-dev#673
mrbean-bremen added a commit to mrbean-bremen/pyfakefs that referenced this issue May 10, 2022
- as Windows has no root dir as such, we use the
  current drive as root dir to better emulate the fs
- as in the real fs, if a path starts with a path separator,
  it points to the current drive (e.g. the root of cwd)
- fixes pytest-dev#673
mrbean-bremen added a commit to mrbean-bremen/pyfakefs that referenced this issue May 10, 2022
- as Windows has no root dir as such, we use the
  current drive as root dir to better emulate the fs
- as in the real fs, if a path starts with a path separator,
  it points to the current drive (e.g. the root of cwd)
- fixes pytest-dev#673
mrbean-bremen added a commit to mrbean-bremen/pyfakefs that referenced this issue May 11, 2022
- as Windows has no root dir as such, we use the
  current drive as root dir to better emulate the fs
- as in the real fs, if a path starts with a path separator,
  it points to the current drive (e.g. the root of cwd)
- fixes pytest-dev#673
mrbean-bremen added a commit that referenced this issue May 11, 2022
- as Windows has no root dir as such, we use the
  current drive as root dir to better emulate the fs
- as in the real fs, if a path starts with a path separator,
  it points to the current drive (e.g. the root of cwd)
- fixes #673
github-actions bot pushed a commit that referenced this issue May 11, 2022
…r as such, we use the current drive as root dir to better emulate the fs - as in the real fs, if a path starts with a path separator, it points to the current drive (e.g. the root of cwd) - fixes #673
@mrbean-bremen
Copy link
Member

Fixed in master, please check it out. I hope that I didn't introduce too many regressions, but I can't rule it out, as the handling of the root path under Windows has changed sufficiently.

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

Successfully merging a pull request may close this issue.

2 participants