Skip to content

Commit

Permalink
3.13 support for Windows (#10667)
Browse files Browse the repository at this point in the history
python/cpython#113829 changed `os.path.isabs` on Windows to not consider a path starting a single backslash to be an absolute path.

We do a lot of naive `posixpath` to `ntpath` conversions, and it broke DVC in multiple places. DVC is likely broken in many other places, and these bugs are difficult to identify.

In this commit, I have tried to fix in the places where the tests failed. And mostly by trying to imitate pre-3.13 behaviour.
Some of the changes may not be correct but keeps the old behaviour in place.

Also re-enabled the CI for all Python versions supported for Windows.
  • Loading branch information
skshetry authored Jan 10, 2025
1 parent dd50155 commit 7d14acb
Show file tree
Hide file tree
Showing 4 changed files with 51 additions and 3 deletions.
36 changes: 36 additions & 0 deletions .github/workflows/tests.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,30 @@ jobs:
- os: windows-latest
pyv: "3.9"
pytestargs: "--splitting-algorithm=least_duration --splits 4 --group 4"
- os: windows-latest
pyv: "3.10"
pytestargs: "--splitting-algorithm=least_duration --splits 4 --group 1"
- os: windows-latest
pyv: "3.10"
pytestargs: "--splitting-algorithm=least_duration --splits 4 --group 2"
- os: windows-latest
pyv: "3.10"
pytestargs: "--splitting-algorithm=least_duration --splits 4 --group 3"
- os: windows-latest
pyv: "3.10"
pytestargs: "--splitting-algorithm=least_duration --splits 4 --group 4"
- os: windows-latest
pyv: "3.11"
pytestargs: "--splitting-algorithm=least_duration --splits 4 --group 1"
- os: windows-latest
pyv: "3.11"
pytestargs: "--splitting-algorithm=least_duration --splits 4 --group 2"
- os: windows-latest
pyv: "3.11"
pytestargs: "--splitting-algorithm=least_duration --splits 4 --group 3"
- os: windows-latest
pyv: "3.11"
pytestargs: "--splitting-algorithm=least_duration --splits 4 --group 4"
- os: windows-latest
pyv: "3.12"
pytestargs: "--splitting-algorithm=least_duration --splits 4 --group 1"
Expand All @@ -81,6 +105,18 @@ jobs:
- os: windows-latest
pyv: "3.12"
pytestargs: "--splitting-algorithm=least_duration --splits 4 --group 4"
- os: windows-latest
pyv: "3.13"
pytestargs: "--splitting-algorithm=least_duration --splits 4 --group 1"
- os: windows-latest
pyv: "3.13"
pytestargs: "--splitting-algorithm=least_duration --splits 4 --group 2"
- os: windows-latest
pyv: "3.13"
pytestargs: "--splitting-algorithm=least_duration --splits 4 --group 3"
- os: windows-latest
pyv: "3.13"
pytestargs: "--splitting-algorithm=least_duration --splits 4 --group 4"
steps:
- uses: actions/checkout@v4
with:
Expand Down
10 changes: 10 additions & 0 deletions dvc/config.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
"""DVC config objects."""

import ntpath
import os
import posixpath
import re
from contextlib import contextmanager
from functools import partial
Expand Down Expand Up @@ -268,12 +270,17 @@ def _resolve(conf_dir, path):
if re.match(r"\w+://", path):
return path

if os.name == "nt" and posixpath.isabs(path) and ntpath.sep not in path:
return path

if os.path.isabs(path):
return path

# on windows convert slashes to backslashes
# to have path compatible with abs_conf_dir
if os.path.sep == "\\" and "/" in path:
if path.startswith("/"):
path = path.replace("/", "\\\\", 1)
path = path.replace("/", "\\")

expanded = os.path.expanduser(path)
Expand Down Expand Up @@ -305,6 +312,9 @@ def _to_relpath(conf_dir, path):
if os.path.expanduser(path) != path:
return localfs.as_posix(path)

if os.name == "nt" and posixpath.isabs(path) and ntpath.sep not in path:
return path

if isinstance(path, RelPath) or not os.path.isabs(path):
path = relpath(path, conf_dir)
return localfs.as_posix(path)
Expand Down
5 changes: 3 additions & 2 deletions dvc/fs/dvc.py
Original file line number Diff line number Diff line change
Expand Up @@ -732,9 +732,10 @@ def repo_url(self) -> str:
return self.fs.repo_url

def from_os_path(self, path: str) -> str:
if os.path.isabs(path):
if os.path.isabs(path) or (
os.name == "nt" and posixpath.isabs(path) and ntpath.sep not in path
):
path = os.path.relpath(path, self.repo.root_dir)

return as_posix(path)

def close(self):
Expand Down
3 changes: 2 additions & 1 deletion tests/unit/test_ignore.py
Original file line number Diff line number Diff line change
Expand Up @@ -168,8 +168,9 @@ def test_match_ignore_from_file(
):
from dvc.fs import localfs

root = r"\\" if os.name == "nt" else "/"
dvcignore_path = os.path.join(
os.path.sep, "full", "path", "to", "ignore", "file", ".dvcignore"
root, "full", "path", "to", "ignore", "file", ".dvcignore"
)
dvcignore_dirname = os.path.dirname(dvcignore_path)

Expand Down

0 comments on commit 7d14acb

Please sign in to comment.