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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add Windows CI Runners #61

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1b0ef86
Initial commit
Javagedes Oct 25, 2023
e767745
Add python 3.12, fully resolve temporary directory
Javagedes Oct 25, 2023
861f19f
switch to posix, rather than str for performing search
Javagedes Oct 25, 2023
c1fa2cb
update
Javagedes Oct 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,11 @@ on:

jobs:
test:
runs-on: ubuntu-latest
runs-on: ${{ matrix.os }}
strategy:
matrix:
python-version: ['3.11', '3.10', '3.9', '3.8', '3.7']
python-version: ['3.12', '3.11', '3.10', '3.9', '3.8', '3.7']
os: [ubuntu-latest, windows-latest]
steps:
- uses: actions/checkout@v2
- name: Set up Python ${{ matrix.python-version }}
Expand Down
21 changes: 19 additions & 2 deletions gitignore_parser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

from os.path import abspath, dirname
from pathlib import Path
import sys
from typing import Reversible, Union

def handle_negation(file_path, rules: Reversible["IgnoreRule"]):
Expand Down Expand Up @@ -125,9 +126,14 @@ def __repr__(self):
def match(self, abs_path: Union[str, Path]):
matched = False
if self.base_path:
rel_path = str(_normalize_path(abs_path).relative_to(self.base_path))
rel_path = _normalize_path(abs_path).relative_to(self.base_path).as_posix()
else:
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)

if sys.platform.startswith('win'):
rel_path += ' ' * _count_trailing_symbol(' ', abs_path)
rel_path += '.' * _count_trailing_symbol('.', abs_path)
# Path() strips the trailing slash, so we need to preserve it
# in case of directory-only negation
if self.negation and type(abs_path) == str and abs_path[-1] == '/':
Expand Down Expand Up @@ -218,3 +224,14 @@ def _normalize_path(path: Union[str, Path]) -> Path:
`Path.resolve()` does.
"""
return Path(abspath(path))


def _count_trailing_symbol(symbol: str, text: str) -> int:
"""Count the number of trailing characters in a string."""
count = 0
for char in reversed(str(text)):
if char == symbol:
count += 1
else:
break
return count
16 changes: 11 additions & 5 deletions tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

from gitignore_parser import parse_gitignore

from unittest import TestCase, main
from unittest import TestCase, main, SkipTest


class Test(TestCase):
Expand Down Expand Up @@ -188,13 +188,19 @@ def test_symlink_to_another_directory(self):
This test ensures that the issue is now fixed.
"""
with TemporaryDirectory() as project_dir, TemporaryDirectory() as another_dir:
project_dir = Path(project_dir).resolve()
another_dir = Path(another_dir).resolve()
matches = _parse_gitignore_string('link', fake_base_dir=project_dir)

# Create a symlink to another directory.
link = Path(project_dir, 'link')
target = Path(another_dir, 'target')
link.symlink_to(target)

link = project_dir / 'link'
target = another_dir / 'target'

try:
link.symlink_to(target)
except OSError:
e = "Current user does not have permissions to perform symlink."
raise SkipTest(e)
# Check the intended behavior according to
# https://git-scm.com/docs/gitignore#_notes:
# Symbolic links are not followed and are matched as if they were regular
Expand Down