Skip to content

Conversation

@Javagedes
Copy link

@Javagedes Javagedes commented Oct 25, 2023

With recent changes to start using Path, gitignore_parser lost some support for Windows which was not caught due to CI not running on a windows runner. Also, I added python 3.12 testing

Issues with using Path which broke windows support:

  1. Path strips some trailing characters. Currently caught are ' ' and '.'
  2. Path switches path separators to the current os's path separator, breaking the regex is some instances.

Broken CI when turning on Windows Runners, before applying fixes:

  1. test_ignore_directory, specifically /home/michael/.venv/folder as Path turns it into .venv\\folder and current regex fails
  2. test_trailingspaces, as all the spaces are stripped on windows
  3. test_wildcard, specifically /home/michael/hello. as Path strips the '.' on Windows

Other Miscellaneous issue:

test_symlink_to_another_directory failed on windows - but was a test issue, not an issue with the gitignore_parser. The issue is that TemproaryDirectory() itself uses a symlink on windows, so the base_dir does not match due to the fact that in one instance its the symlink and the other is the full path. I just resolve the symlink on the project_dir and another_dir, and focus on the symlink resolution the test truly cares about.

I also added logic to automatically skip the test if the symlink fails. This happens locally if not running as an admin as you don't have permission to create symlinks.

This PR fixes the following:

  1. preserve symbols that are stripped by path: ' ' and '.'
  2. Convert rel_path to posix form with as_posix() rather than to the current OS's form with str

Closes #60

rel_path = str(_normalize_path(abs_path))
rel_path = _normalize_path(abs_path).as_posix()
# Path() strips the trailing following symbols on windows, so we need to
# preserve it: ' ', '.'
Copy link
Owner

Choose a reason for hiding this comment

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

How about we just use PurePosixPath instead of Path and special-casing Windows?

Copy link
Author

Choose a reason for hiding this comment

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

I believe I could make this work, but it would continue to require special handling per OS due to calling .relative_to.

Since Windows considers uppercase and lowercase letters the same, on a windows device C:/home/michael and c:/home/michael are the same path. However, by using PurePosixPath, the comparison in relative_to uses Posix logic, which is that uppercase and lowercase letters are different. Due to this, .relative_to will not find relative matches that it should on windows.

This can be seen on all test cases when running on Windows. You end up getting the error:
ValueError: 'c:/home/michael/o.py' is not in the subpath of 'C:/home/michael' OR one path is relative and the other is absolute.

Another issue is that PurePosixPath does not like \\. It treats it as a normal path character (not a parts separator) and thus the path parts are not actually split up as expected.

I have not done a full test, as I don't wan't to invest more time until I know this is the route you want to go, but I believe a possible solution would be to lowercase the entire path inside _normalize_path if we are on a windows system, it may get us there, but I don't garuntee it.

Something like:

def _normalize_path(path: Union[str, Path]) -> PurePosixPath
    path = abspath(path).replace('\\', '/')
    if sys.platform.startswith('win'):
        path = path.lower()
    return PurePosixPath(path)

elifarley added a commit to elifarley/file-matcher that referenced this pull request Dec 20, 2024
elifarley added a commit to elifarley/file-matcher that referenced this pull request Dec 20, 2024
elifarley added a commit to elifarley/file-matcher that referenced this pull request Dec 20, 2024
elifarley added a commit to elifarley/file-matcher that referenced this pull request Dec 20, 2024
@kdeldycke
Copy link

I confirm that this PR fix #60.

@mherrmann Any plan to merge it and make a release?

@mherrmann
Copy link
Owner

Fixed in v0.1.13 via #79. Thank you both.

@mherrmann mherrmann closed this Aug 25, 2025
kdeldycke added a commit to kdeldycke/workflows that referenced this pull request Aug 25, 2025
@kdeldycke
Copy link

I can confirm v0.1.13 fix my issue. Thanks @mherrmann for the release! 🥳

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.

unit tests fail on windows runners

3 participants