-
Notifications
You must be signed in to change notification settings - Fork 40
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
Add Windows CI Runners #61
base: master
Are you sure you want to change the base?
Conversation
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: ' ', '.' |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
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 testingIssues with using
Path
which broke windows support:Path
strips some trailing characters. Currently caught are' '
and'.'
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:
/home/michael/.venv/folder
asPath
turns it into.venv\\folder
and current regex fails/home/michael/hello.
asPath
strips the '.' on WindowsOther 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
andanother_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:
rel_path
to posix form withas_posix()
rather than to the current OS's form withstr
Closes #60