From f865e56b6fef93c8e6117c5887fee9c222305753 Mon Sep 17 00:00:00 2001 From: Frost Ming Date: Tue, 31 Dec 2024 10:11:43 +0800 Subject: [PATCH] fix: use stdlib url<->path conversion (#3364) * fix: use stdlib url<->path conversion Signed-off-by: Frost Ming * fix tests Signed-off-by: Frost Ming * fix the error type Signed-off-by: Frost Ming --- news/3362.bugfix.md | 1 + src/pdm/models/backends.py | 12 ++++----- src/pdm/models/candidates.py | 3 +-- src/pdm/models/repositories/lock.py | 7 +++--- src/pdm/models/requirements.py | 39 +++++++++++++++++------------ src/pdm/project/core.py | 3 +-- src/pdm/pytest.py | 5 ++-- src/pdm/utils.py | 27 +++++--------------- tests/cli/test_add.py | 7 +++--- tests/cli/test_lock.py | 5 ++-- tests/cli/test_run.py | 4 +-- tests/conftest.py | 4 +-- tests/models/test_backends.py | 10 ++++---- tests/models/test_candidates.py | 6 ++--- tests/models/test_requirements.py | 8 +++--- tests/test_utils.py | 24 +----------------- 16 files changed, 66 insertions(+), 99 deletions(-) create mode 100644 news/3362.bugfix.md diff --git a/news/3362.bugfix.md b/news/3362.bugfix.md new file mode 100644 index 0000000000..d3eaca7228 --- /dev/null +++ b/news/3362.bugfix.md @@ -0,0 +1 @@ +Use stdlib for URL <-> Path conversions. diff --git a/src/pdm/models/backends.py b/src/pdm/models/backends.py index 0975c6109e..875bfa36c2 100644 --- a/src/pdm/models/backends.py +++ b/src/pdm/models/backends.py @@ -6,7 +6,7 @@ from pathlib import Path from typing import TYPE_CHECKING -from pdm.utils import expand_env_vars, path_to_url +from pdm.utils import expand_env_vars if TYPE_CHECKING: from typing import TypedDict @@ -24,7 +24,7 @@ def expand_line(self, line: str, expand_env: bool = True) -> str: return line def relative_path_to_url(self, path: str) -> str: - return path_to_url(os.path.join(self.root, path)) + return self.root.joinpath(path).as_uri() @classmethod @abc.abstractmethod @@ -52,14 +52,14 @@ def build_system(cls) -> BuildSystem: class PDMBackend(BuildBackend): def expand_line(self, req: str, expand_env: bool = True) -> str: - line = req.replace("file:///${PROJECT_ROOT}", path_to_url(self.root.as_posix())) + line = req.replace("file:///${PROJECT_ROOT}", self.root.as_uri()) if expand_env: line = expand_env_vars(line) return line def relative_path_to_url(self, path: str) -> str: if os.path.isabs(path): - return path_to_url(path) + return Path(path).as_uri() return f"file:///${{PROJECT_ROOT}}/{urllib.parse.quote(path)}" @classmethod @@ -79,7 +79,7 @@ def __format__(self, __format_spec: str) -> str: if not __format_spec: return self.__path.as_posix() elif __format_spec == "uri": - return path_to_url(self.__path.as_posix()) + return self.__path.as_uri() elif __format_spec == "real": return self.__path.resolve().as_posix() raise ValueError(f"Unknown format specifier: {__format_spec}") @@ -110,7 +110,7 @@ def expand_line(self, line: str, expand_env: bool = True) -> str: def relative_path_to_url(self, path: str) -> str: if os.path.isabs(path): - return path_to_url(path) + return Path(path).as_uri() return f"{{root:uri}}/{urllib.parse.quote(path)}" @classmethod diff --git a/src/pdm/models/candidates.py b/src/pdm/models/candidates.py index 2efd4f972e..e99b32043c 100644 --- a/src/pdm/models/candidates.py +++ b/src/pdm/models/candidates.py @@ -34,7 +34,6 @@ filtered_sources, get_rev_from_url, normalize_name, - path_to_url, url_without_fragments, ) @@ -353,7 +352,7 @@ def direct_url(self) -> dict[str, Any] | None: assert self._source_dir return _filter_none( { - "url": path_to_url(self._source_dir.as_posix()), + "url": self._source_dir.as_uri(), "dir_info": {"editable": True}, "subdirectory": req.subdirectory, } diff --git a/src/pdm/models/repositories/lock.py b/src/pdm/models/repositories/lock.py index 17606af88f..5c1ffc5ab4 100644 --- a/src/pdm/models/repositories/lock.py +++ b/src/pdm/models/repositories/lock.py @@ -3,6 +3,7 @@ import dataclasses import posixpath from functools import cached_property +from pathlib import Path from typing import TYPE_CHECKING, Collection, NamedTuple, cast from pdm.exceptions import CandidateNotFound, PdmException @@ -10,7 +11,7 @@ from pdm.models.markers import EnvSpec from pdm.models.repositories.base import BaseRepository, CandidateMetadata from pdm.models.requirements import FileRequirement, Requirement, parse_line -from pdm.utils import cd, path_to_url, url_to_path, url_without_fragments +from pdm.utils import cd, url_to_path, url_without_fragments if TYPE_CHECKING: from typing import Any, Callable, Iterable, Mapping @@ -89,7 +90,7 @@ def _read_lockfile(self, lockfile: Mapping[str, Any]) -> None: } req = Requirement.from_req_dict(package_name, req_dict) if req.is_file_or_url and req.path and not req.url: # type: ignore[attr-defined] - req.url = path_to_url(posixpath.join(root, req.path)) # type: ignore[attr-defined] + req.url = root.joinpath(req.path).as_uri() # type: ignore[attr-defined] can = Candidate(req, name=package_name, version=version) can.hashes = package.get("files", []) if not static_urls and any("url" in f for f in can.hashes): @@ -112,7 +113,7 @@ def _identify_candidate(self, candidate: Candidate) -> CandidateKey: url = self.environment.project.backend.expand_line(cast(str, url)) if url.startswith("file://"): path = posixpath.normpath(url_to_path(url)) - url = path_to_url(path) + url = Path(path).as_uri() return ( candidate.identify(), candidate.version if not url else None, diff --git a/src/pdm/models/requirements.py b/src/pdm/models/requirements.py index b2fd046e87..a268212e6d 100644 --- a/src/pdm/models/requirements.py +++ b/src/pdm/models/requirements.py @@ -28,8 +28,7 @@ add_ssh_scheme_to_git_uri, comparable_version, normalize_name, - path_to_url, - path_without_fragments, + split_path_fragments, url_to_path, url_without_fragments, ) @@ -309,17 +308,24 @@ def str_path(self) -> str | None: return result def _parse_url(self) -> None: - if not self.url and self.path and self.path.is_absolute(): - self.url = path_to_url(self.path.as_posix()) - if not self.path: - path = get_relative_path(self.url) - if path is None: + if self.path: + path, fragments = split_path_fragments(self.path) + if not self.url and path.is_absolute(): + self.url = path.as_uri() + fragments + self.path = path + # For relative path, we don't resolve URL now, so the path may still contain fragments, + # it will be handled in `relocate()` method. + else: + url = url_without_fragments(self.url) + relpath = get_relative_path(url) + if relpath is None: try: - self.path = path_without_fragments(url_to_path(self.url)) - except AssertionError: + self.path = Path(url_to_path(url)) + except ValueError: pass else: - self.path = path_without_fragments(path) + self.path = Path(relpath) + if self.url: self._parse_name_from_url() @@ -328,11 +334,12 @@ def relocate(self, backend: BuildBackend) -> None: if self.path is None or self.path.is_absolute(): return # self.path is relative - self.path = path_without_fragments(os.path.relpath(self.path, backend.root)) - path = self.path.as_posix() - if path == ".": - path = "" - self.url = backend.relative_path_to_url(path) + path, fragments = split_path_fragments(self.path) + self.path = Path(os.path.relpath(path, backend.root)) + relpath = self.path.as_posix() + if relpath == ".": + relpath = "" + self.url = backend.relative_path_to_url(relpath) + fragments self._root = backend.root @property @@ -492,7 +499,7 @@ def parse_requirement(line: str, editable: bool = False) -> Requirement: # We replace the {root.uri} temporarily with a dummy URL header # to make it pass through the packaging.requirement parser # and then revert it. - root_url = path_to_url(Path().as_posix()) + root_url = Path().absolute().as_uri() replaced = "{root:uri}" in line if replaced: line = line.replace("{root:uri}", root_url) diff --git a/src/pdm/project/core.py b/src/pdm/project/core.py index 5cc3d699fb..ef8e8a93f3 100644 --- a/src/pdm/project/core.py +++ b/src/pdm/project/core.py @@ -39,7 +39,6 @@ is_conda_base_python, is_path_relative_to, normalize_name, - path_to_url, ) if TYPE_CHECKING: @@ -621,7 +620,7 @@ def make_self_candidate(self, editable: bool = True) -> Candidate: from pdm.models.candidates import Candidate - req = parse_requirement(path_to_url(self.root.as_posix()), editable) + req = parse_requirement(self.root.as_uri(), editable) assert self.name req.name = self.name can = Candidate(req, name=self.name, link=Link.from_path(self.root)) diff --git a/src/pdm/pytest.py b/src/pdm/pytest.py index 1a92333e14..0bfded6446 100644 --- a/src/pdm/pytest.py +++ b/src/pdm/pytest.py @@ -63,7 +63,7 @@ from pdm.models.session import PDMPyPIClient from pdm.project.config import Config from pdm.project.core import Project -from pdm.utils import find_python_in_path, normalize_name, parse_version, path_to_url +from pdm.utils import find_python_in_path, normalize_name, parse_version if TYPE_CHECKING: from typing import Protocol @@ -501,12 +501,11 @@ def local_finder_artifacts() -> Path: @pytest.fixture def local_finder(project_no_init: Project, local_finder_artifacts: Path) -> None: - artifacts_dir = str(local_finder_artifacts) project_no_init.pyproject.settings["source"] = [ { "type": "find_links", "verify_ssl": False, - "url": path_to_url(artifacts_dir), + "url": local_finder_artifacts.as_uri(), "name": "pypi", } ] diff --git a/src/pdm/utils.py b/src/pdm/utils.py index 72e47d013d..b2b8768f67 100644 --- a/src/pdm/utils.py +++ b/src/pdm/utils.py @@ -37,8 +37,6 @@ from pdm._types import FileHash, RepositoryConfig from pdm.compat import Distribution -_egg_fragment_re = re.compile(r"(.*)[#&]egg=[^&]*") - try: _packaging_version = importlib_metadata.version("packaging") except Exception: @@ -189,7 +187,8 @@ def url_to_path(url: str) -> str: WINDOWS = sys.platform == "win32" - assert url.startswith("file:"), f"You can only turn file: urls into filenames (not {url!r})" + if not url.startswith("file:"): + raise ValueError(f"You can only turn file: urls into filenames (not {url!r})") _, netloc, path, _, _ = parse.urlsplit(url) @@ -220,16 +219,10 @@ def url_to_path(url: str) -> str: return path -def path_to_url(path: str) -> str: - """ - Convert a path to a file: URL. The path will be made absolute and have - quoted path parts. - """ - from urllib.request import pathname2url - - path = os.path.normpath(os.path.abspath(path)) - url = parse.urljoin("file:", pathname2url(path)) - return url +def split_path_fragments(path: Path) -> tuple[Path, str]: + """Split a path into fragments""" + left, sep, right = path.as_posix().partition("#egg=") + return Path(left), sep + right def expand_env_vars(credential: str, quote: bool = False, env: Mapping[str, str] | None = None) -> str: @@ -448,14 +441,6 @@ def is_pip_compatible_with_python(python_version: Version | str) -> bool: return requires_python.contains(python_version, True) -def path_without_fragments(path: str) -> Path: - """Remove egg fragment from path""" - match = _egg_fragment_re.search(path) - if not match: - return Path(path) - return Path(match.group(1)) - - def is_in_zipapp() -> bool: """Check if the current process is running in a zipapp""" return not os.path.exists(__file__) diff --git a/tests/cli/test_add.py b/tests/cli/test_add.py index 4ad5bb7bce..051c591b15 100644 --- a/tests/cli/test_add.py +++ b/tests/cli/test_add.py @@ -6,7 +6,6 @@ from pdm.models.markers import EnvSpec from pdm.models.specifiers import PySpecSet from pdm.pytest import Distribution -from pdm.utils import path_to_url from tests import FIXTURES @@ -268,9 +267,9 @@ def test_add_with_prerelease(project, working_set, pdm): def test_add_editable_package_with_extras(project, working_set, pdm): project.environment.python_requires = PySpecSet(">=3.6") - dep_path = FIXTURES.joinpath("projects/demo").as_posix() - pdm(["add", "-dGdev", "-e", f"{dep_path}[security]"], obj=project, strict=True) - assert f"-e {path_to_url(dep_path)}#egg=demo[security]" in project.use_pyproject_dependencies("dev", True)[0] + dep_path = FIXTURES.joinpath("projects/demo") + pdm(["add", "-dGdev", "-e", f"{dep_path.as_posix()}[security]"], obj=project, strict=True) + assert f"-e {dep_path.as_uri()}#egg=demo[security]" in project.use_pyproject_dependencies("dev", True)[0] assert "demo" in working_set assert "requests" in working_set assert "urllib3" in working_set diff --git a/tests/cli/test_lock.py b/tests/cli/test_lock.py index f7aeff7e5f..28fbc224e7 100644 --- a/tests/cli/test_lock.py +++ b/tests/cli/test_lock.py @@ -1,3 +1,4 @@ +from pathlib import Path from unittest.mock import ANY import pytest @@ -8,7 +9,7 @@ from pdm.models.requirements import parse_requirement from pdm.models.specifiers import PySpecSet from pdm.project.lockfile import FLAG_CROSS_PLATFORM, Compatibility -from pdm.utils import parse_version, path_to_url +from pdm.utils import parse_version from tests import FIXTURES @@ -418,7 +419,7 @@ def test_lock_for_multiple_targets(project, pdm, repository, nested): @pytest.mark.usefixtures("repository") -@pytest.mark.parametrize("constraint", [CONSTRAINT_FILE, path_to_url(CONSTRAINT_FILE)]) +@pytest.mark.parametrize("constraint", [CONSTRAINT_FILE, Path(CONSTRAINT_FILE).as_uri()]) def test_lock_with_override_file(project, pdm, constraint): project.add_dependencies(["requests"]) pdm(["lock", "--override", constraint], obj=project, strict=True) diff --git a/tests/cli/test_run.py b/tests/cli/test_run.py index 8080072856..58fc0b86f7 100644 --- a/tests/cli/test_run.py +++ b/tests/cli/test_run.py @@ -10,7 +10,7 @@ from pdm import termui from pdm.cli import actions from pdm.cli.utils import get_pep582_path -from pdm.utils import cd, path_to_url +from pdm.utils import cd @pytest.fixture @@ -972,7 +972,7 @@ def test_run_script_with_inline_metadata(project, pdm, local_finder, local_finde result = pdm(["run", "test_script.py"], obj=project) assert result.exit_code != 0 - local_artifacts_url = path_to_url(str(local_finder_artifacts)) + local_artifacts_url = local_finder_artifacts.as_uri() project.root.joinpath("test_script.py").write_text( textwrap.dedent(f"""\ diff --git a/tests/conftest.py b/tests/conftest.py index c03bccaf3b..1a01860dda 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -11,7 +11,6 @@ from pdm.models.auth import keyring from pdm.project import Project -from pdm.utils import path_to_url from tests import FIXTURES if TYPE_CHECKING: @@ -118,12 +117,11 @@ def func(project_name): copytree(source, project_no_init.root) project_no_init.pyproject.reload() if "local_finder" in request.fixturenames: - artifacts_dir = str(local_finder_artifacts) project_no_init.pyproject.settings["source"] = [ { "type": "find_links", "verify_ssl": False, - "url": path_to_url(artifacts_dir), + "url": local_finder_artifacts.as_uri(), "name": "pypi", } ] diff --git a/tests/models/test_backends.py b/tests/models/test_backends.py index c81966eb76..899f2e1a14 100644 --- a/tests/models/test_backends.py +++ b/tests/models/test_backends.py @@ -5,7 +5,7 @@ from pdm.models.backends import _BACKENDS, get_backend, get_relative_path from pdm.project import Project -from pdm.utils import cd, path_to_url +from pdm.utils import cd from tests import FIXTURES @@ -36,8 +36,8 @@ def test_project_backend(project, working_set, backend, pdm): assert "idna" in working_set assert "demo" in working_set dep = project.pyproject.metadata["dependencies"][0] - demo_path = project.root.joinpath("demo").as_posix() - demo_url = path_to_url(demo_path) + demo_path = project.root.joinpath("demo") + demo_url = demo_path.as_uri() if backend == "pdm-backend": assert dep == "demo @ file:///${PROJECT_ROOT}/demo" elif backend == "hatchling": @@ -54,7 +54,7 @@ def test_project_backend(project, working_set, backend, pdm): def test_hatch_expand_variables(monkeypatch): root = Path().absolute() - root_url = path_to_url(root.as_posix()) + root_url = root.as_uri() backend = get_backend("hatchling")(root) monkeypatch.setenv("BAR", "bar") assert backend.expand_line("demo @ {root:uri}/demo") == f"demo @ {root_url}/demo" @@ -65,7 +65,7 @@ def test_hatch_expand_variables(monkeypatch): def test_pdm_backend_expand_variables(monkeypatch): root = Path().absolute() - root_url = path_to_url(root.as_posix()) + root_url = root.as_uri() backend = get_backend("pdm-backend")(root) monkeypatch.setenv("BAR", "bar") assert backend.expand_line("demo @ file:///${PROJECT_ROOT}/demo") == f"demo @ {root_url}/demo" diff --git a/tests/models/test_candidates.py b/tests/models/test_candidates.py index 058372ff09..e9c0655168 100644 --- a/tests/models/test_candidates.py +++ b/tests/models/test_candidates.py @@ -8,7 +8,7 @@ from pdm.models.candidates import Candidate from pdm.models.requirements import Requirement, parse_requirement from pdm.models.specifiers import PySpecSet -from pdm.utils import is_path_relative_to, path_to_url +from pdm.utils import is_path_relative_to from tests import FIXTURES @@ -104,7 +104,7 @@ def test_parse_remote_link_metadata(project): "demo @ file:///${PROJECT_ROOT}/tests/fixtures/projects/demo", "-e ./tests/fixtures/projects/demo", "-e file:///${PROJECT_ROOT}/tests/fixtures/projects/demo#egg=demo", - "-e file:///${PROJECT_ROOT}/tests/fixtures/projects/demo-#-with-hash#egg=demo", + "-e file:///${PROJECT_ROOT}/tests/fixtures/projects/demo#-with-hash#egg=demo", ], ) def test_expand_project_root_in_url(req_str, core): @@ -200,7 +200,7 @@ def test_vcs_candidate_in_subdirectory(project, is_editable): @pytest.mark.usefixtures("local_finder") def test_sdist_candidate_with_wheel_cache(project, mocker): - file_link = Link(path_to_url((FIXTURES / "artifacts/demo-0.0.1.tar.gz").as_posix())) + file_link = Link((FIXTURES / "artifacts/demo-0.0.1.tar.gz").as_uri()) built_path = FIXTURES / "artifacts/demo-0.0.1-py2.py3-none-any.whl" wheel_cache = project.make_wheel_cache() cache_path = wheel_cache.get_path_for_link(file_link, project.environment.spec) diff --git a/tests/models/test_requirements.py b/tests/models/test_requirements.py index a18a1306d0..3190c2cf92 100644 --- a/tests/models/test_requirements.py +++ b/tests/models/test_requirements.py @@ -5,7 +5,7 @@ import pytest from pdm.models.requirements import RequirementError, filter_requirements_with_extras, parse_requirement -from pdm.utils import PACKAGING_22, path_to_url +from pdm.utils import PACKAGING_22 from tests import FIXTURES FILE_PREFIX = "file:///" if os.name == "nt" else "file://" @@ -35,15 +35,15 @@ ), ( (FIXTURES / "projects/demo").as_posix(), - "demo @ " + path_to_url(FIXTURES / "projects/demo"), + "demo @ " + (FIXTURES / "projects/demo").as_uri(), ), ( (FIXTURES / "artifacts/demo-0.0.1-py2.py3-none-any.whl").as_posix(), - "demo @ " + path_to_url(FIXTURES / "artifacts/demo-0.0.1-py2.py3-none-any.whl"), + "demo @ " + (FIXTURES / "artifacts/demo-0.0.1-py2.py3-none-any.whl").as_uri(), ), ( (FIXTURES / "projects/demo").as_posix() + "[security]", - "demo[security] @ " + path_to_url(FIXTURES / "projects/demo"), + "demo[security] @ " + (FIXTURES / "projects/demo").as_uri(), ), ( 'requests; python_version=="3.7.*"', diff --git a/tests/test_utils.py b/tests/test_utils.py index 2ebc26215f..de728b96dd 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -1,4 +1,3 @@ -import os import pathlib import sys import unittest.mock as mock @@ -156,7 +155,7 @@ def test_add_ssh_scheme_to_git_uri(given, expected): class TestUrlToPath: def test_non_file_url(self): - with pytest.raises(AssertionError): + with pytest.raises(ValueError): utils.url_to_path("not_a_file_scheme://netloc/path") @pytest.mark.skipif(sys.platform.startswith("win"), reason="Non-Windows test") @@ -173,27 +172,6 @@ def test_windows_localhost_local_file_url(self): assert utils.url_to_path("file://localhost/local/file/path") == "\\local\\file\\path" -# Only testing POSIX-style paths here -@pytest.mark.skipif(sys.platform.startswith("win"), reason="Non-Windows tests") -@pytest.mark.parametrize( - "given, expected", - [ - ("/path/to/my/pdm", "file:///path/to/my/pdm"), - ("/abs/path/to/my/pdm", "file:///abs/path/to/my/pdm"), - ("/path/to/my/pdm/pyproject.toml", "file:///path/to/my/pdm/pyproject.toml"), - ("../path/to/my/pdm/pyproject.toml", "file:///abs/path/to/my/pdm/pyproject.toml"), - ], -) -def test_path_to_url(given, expected): - if os.path.isabs(given): - assert utils.path_to_url(given) == expected - else: - abs_given = "abs" + str(given).replace("..", "") - - with mock.patch("pdm.utils.os.path.abspath", return_value=abs_given): - assert utils.path_to_url(given) == expected - - @pytest.mark.parametrize( "given,expected", [