From 986acef432d1013e28d4075ca43380a0ed079045 Mon Sep 17 00:00:00 2001 From: Alex Carney Date: Sun, 22 Sep 2024 19:49:16 +0100 Subject: [PATCH 1/8] sphinx-agent: Move `Uri` implementation into its own file --- .../esbonio/sphinx_agent/types/__init__.py | 328 +----------------- lib/esbonio/esbonio/sphinx_agent/types/uri.py | 325 +++++++++++++++++ 2 files changed, 331 insertions(+), 322 deletions(-) create mode 100644 lib/esbonio/esbonio/sphinx_agent/types/uri.py diff --git a/lib/esbonio/esbonio/sphinx_agent/types/__init__.py b/lib/esbonio/esbonio/sphinx_agent/types/__init__.py index bbf137496..4553c307a 100644 --- a/lib/esbonio/esbonio/sphinx_agent/types/__init__.py +++ b/lib/esbonio/esbonio/sphinx_agent/types/__init__.py @@ -1,20 +1,16 @@ """Type definitions for the sphinx agent. -This is the *only* file shared between the agent itself and the parent language server. -For this reason this file *cannot* import anything from Sphinx. +This is the *only* module shared between the agent itself and the parent language +server. For this reason this module *cannot* import anything from Sphinx. """ from __future__ import annotations import dataclasses -import os -import pathlib import re from typing import Any -from typing import Callable from typing import Optional from typing import Union -from urllib import parse from .lsp import Diagnostic from .lsp import DiagnosticSeverity @@ -25,10 +21,13 @@ from .roles import RST_DEFAULT_ROLE from .roles import RST_ROLE from .roles import Role +from .uri import IS_WIN +from .uri import Uri __all__ = ( "Diagnostic", "DiagnosticSeverity", + "IS_WIN", "Location", "MYST_ROLE", "Position", @@ -36,6 +35,7 @@ "RST_ROLE", "Range", "Role", + "Uri", ) MYST_DIRECTIVE: re.Pattern = re.compile( @@ -129,322 +129,6 @@ """ -IS_WIN = os.name == "nt" -SCHEME = re.compile(r"^[a-zA-Z][a-zA-Z\d+.-]*$") -RE_DRIVE_LETTER_PATH = re.compile(r"^(\/?)([a-zA-Z]:)") - - -# TODO: Look into upstreaming this into pygls -# - if it works out -# - when pygls drops 3.7 (Uri uses the := operator) -@dataclasses.dataclass(frozen=True) -class Uri: - """Helper class for working with URIs.""" - - scheme: str - - authority: str - - path: str - - query: str - - fragment: str - - def __post_init__(self): - """Basic validation.""" - if self.scheme is None: - raise ValueError("URIs must have a scheme") - - if not SCHEME.match(self.scheme): - raise ValueError("Invalid scheme") - - if self.authority and self.path and (not self.path.startswith("/")): - raise ValueError("Paths with an authority must start with a slash '/'") - - if self.path and self.path.startswith("//") and (not self.authority): - raise ValueError( - "Paths without an authority cannot start with two slashes '//'" - ) - - def __eq__(self, other): - if type(other) is not type(self): - return False - - if self.scheme != other.scheme: - return False - - if self.authority != other.authority: - return False - - if self.query != other.query: - return False - - if self.fragment != other.fragment: - return False - - if IS_WIN and self.scheme == "file": - # Filepaths on windows are case in-sensitive - if self.path.lower() != other.path.lower(): - return False - - elif self.path != other.path: - return False - - return True - - def __hash__(self): - if IS_WIN and self.scheme == "file": - # Filepaths on windows are case in-sensitive - path = self.path.lower() - else: - path = self.path - - return hash((self.scheme, self.authority, path, self.query, self.fragment)) - - def __fspath__(self): - """Return the file system representation of this uri. - - This makes Uri instances compatible with any function that expects an - ``os.PathLike`` object! - """ - # TODO: Should we raise an exception if scheme != "file"? - return self.as_fs_path(preserve_case=True) - - def __str__(self): - return self.as_string() - - def __truediv__(self, other): - return self.join(other) - - @classmethod - def create( - cls, - *, - scheme: str = "", - authority: str = "", - path: str = "", - query: str = "", - fragment: str = "", - ) -> Uri: - """Create a uri with the given attributes.""" - - if scheme in {"http", "https", "file"}: - if not path.startswith("/"): - path = f"/{path}" - - return cls( - scheme=scheme, - authority=authority, - path=path, - query=query, - fragment=fragment, - ) - - @classmethod - def parse(cls, uri: str) -> Uri: - """Parse the given uri from its string representation.""" - scheme, authority, path, _, query, fragment = parse.urlparse(uri) - return cls.create( - scheme=parse.unquote(scheme), - authority=parse.unquote(authority), - path=parse.unquote(path), - query=parse.unquote(query), - fragment=parse.unquote(fragment), - ) - - def resolve(self) -> Uri: - """Return the fully resolved version of this Uri.""" - - # This operation only makes sense for file uris - if self.scheme != "file": - return Uri.parse(str(self)) - - return Uri.for_file(pathlib.Path(self).resolve()) - - @classmethod - def for_file(cls, filepath: Union[str, os.PathLike[str]]) -> Uri: - """Create a uri based on the given filepath.""" - - fpath = os.fspath(filepath) - if IS_WIN: - fpath = fpath.replace("\\", "/") - - if fpath.startswith("//"): - authority, *path = fpath[2:].split("/") - fpath = "/".join(path) - else: - authority = "" - - return cls.create(scheme="file", authority=authority, path=fpath) - - @property - def fs_path(self) -> Optional[str]: - """Return the equivalent fs path.""" - return self.as_fs_path() - - def where(self, **kwargs) -> Uri: - """Return an transformed version of this uri where certain components of the uri - have been replace with the given arguments. - - Passing a value of ``None`` will remove the given component entirely. - """ - keys = {"scheme", "authority", "path", "query", "fragment"} - valid_keys = keys.copy() & kwargs.keys() - - current = {k: getattr(self, k) for k in keys} - replacements = {k: kwargs[k] for k in valid_keys} - - return Uri.create(**{**current, **replacements}) - - def join(self, path: str) -> Uri: - """Join this Uri's path component with the given path and return the resulting - uri. - - Parameters - ---------- - path - The path segment to join - - Returns - ------- - Uri - The resulting uri - """ - - if not self.path: - raise ValueError("This uri has no path") - - if IS_WIN: - fs_path = self.fs_path - if fs_path is None: - raise ValueError("Unable to join paths, fs_path is None") - - joined = os.path.normpath(os.path.join(fs_path, path)) - new_path = self.for_file(joined).path - else: - new_path = os.path.normpath(os.path.join(self.path, path)) - - return self.where(path=new_path) - - def as_fs_path(self, preserve_case: bool = False) -> Optional[str]: - """Return the file system path correspondin with this uri.""" - if self.path: - path = _normalize_path(self.path, preserve_case) - - if self.authority and len(path) > 1: - path = f"//{self.authority}{path}" - - # Remove the leading `/` from windows paths - elif RE_DRIVE_LETTER_PATH.match(path): - path = path[1:] - - if IS_WIN: - path = path.replace("/", "\\") - - return path - - return None - - def as_string(self, encode=True) -> str: - """Return a string representation of this Uri. - - Parameters - ---------- - encode - If ``True`` (the default), encode any special characters. - - Returns - ------- - str - The string representation of the Uri - """ - - # See: https://github.com/python/mypy/issues/10740 - encoder: Callable[[str], str] = parse.quote if encode else _replace_chars # type: ignore[assignment] - - if authority := self.authority: - usercred, *auth = authority.split("@") - if len(auth) > 0: - *user, cred = usercred.split(":") - if len(user) > 0: - usercred = encoder(":".join(user)) + f":{encoder(cred)}" - else: - usercred = encoder(usercred) - authority = "@".join(auth) - else: - usercred = "" - - authority = authority.lower() - *auth, port = authority.split(":") - if len(auth) > 0: - authority = encoder(":".join(auth)) + f":{port}" - else: - authority = encoder(authority) - - if usercred: - authority = f"{usercred}@{authority}" - - scheme_separator = "" - if authority or self.scheme == "file": - scheme_separator = "//" - - if path := self.path: - path = encoder(_normalize_path(path)) - - if query := self.query: - query = encoder(query) - - if fragment := self.fragment: - fragment = encoder(fragment) - - parts = [ - f"{self.scheme}:", - scheme_separator, - authority if authority else "", - path if path else "", - f"?{query}" if query else "", - f"#{fragment}" if fragment else "", - ] - return "".join(parts) - - -def _replace_chars(segment: str) -> str: - """Replace a certain subset of characters in a uri segment""" - return segment.replace("#", "%23").replace("?", "%3F") - - -def _normalize_path(path: str, preserve_case: bool = False) -> str: - """Normalise the path segment of a Uri. - - Parameters - ---------- - path - The path to normalise. - - preserve_case - If ``True``, preserve the case of the drive label on Windows. - If ``False``, the drive label will be lowercased. - - Returns - ------- - str - The normalised path. - """ - - # normalize to fwd-slashes on windows, - # on other systems bwd-slashes are valid - # filename character, eg /f\oo/ba\r.txt - if IS_WIN: - path = path.replace("\\", "/") - - # Normalize drive paths to lower case - if (not preserve_case) and (match := RE_DRIVE_LETTER_PATH.match(path)): - path = match.group(1) + match.group(2).lower() + path[match.end() :] - - return path - - # -- DB Types # # These represent the structure of data as stored in the SQLite database diff --git a/lib/esbonio/esbonio/sphinx_agent/types/uri.py b/lib/esbonio/esbonio/sphinx_agent/types/uri.py new file mode 100644 index 000000000..c6f8dbd37 --- /dev/null +++ b/lib/esbonio/esbonio/sphinx_agent/types/uri.py @@ -0,0 +1,325 @@ +from __future__ import annotations + +import dataclasses +import os +import pathlib +import re +from typing import Callable +from typing import Optional +from typing import Union +from urllib import parse + +IS_WIN = os.name == "nt" +SCHEME = re.compile(r"^[a-zA-Z][a-zA-Z\d+.-]*$") +RE_DRIVE_LETTER_PATH = re.compile(r"^(\/?)([a-zA-Z]:)") + + +# TODO: Look into upstreaming this into pygls +# - if it works out +# - when pygls drops 3.7 (Uri uses the := operator) +@dataclasses.dataclass(frozen=True) +class Uri: + """Helper class for working with URIs.""" + + scheme: str + + authority: str + + path: str + + query: str + + fragment: str + + def __post_init__(self): + """Basic validation.""" + if self.scheme is None: + raise ValueError("URIs must have a scheme") + + if not SCHEME.match(self.scheme): + raise ValueError("Invalid scheme") + + if self.authority and self.path and (not self.path.startswith("/")): + raise ValueError("Paths with an authority must start with a slash '/'") + + if self.path and self.path.startswith("//") and (not self.authority): + raise ValueError( + "Paths without an authority cannot start with two slashes '//'" + ) + + def __eq__(self, other): + if type(other) is not type(self): + return False + + if self.scheme != other.scheme: + return False + + if self.authority != other.authority: + return False + + if self.query != other.query: + return False + + if self.fragment != other.fragment: + return False + + if IS_WIN and self.scheme == "file": + # Filepaths on windows are case in-sensitive + if self.path.lower() != other.path.lower(): + return False + + elif self.path != other.path: + return False + + return True + + def __hash__(self): + if IS_WIN and self.scheme == "file": + # Filepaths on windows are case in-sensitive + path = self.path.lower() + else: + path = self.path + + return hash((self.scheme, self.authority, path, self.query, self.fragment)) + + def __fspath__(self): + """Return the file system representation of this uri. + + This makes Uri instances compatible with any function that expects an + ``os.PathLike`` object! + """ + # TODO: Should we raise an exception if scheme != "file"? + return self.as_fs_path(preserve_case=True) + + def __str__(self): + return self.as_string() + + def __truediv__(self, other): + return self.join(other) + + @classmethod + def create( + cls, + *, + scheme: str = "", + authority: str = "", + path: str = "", + query: str = "", + fragment: str = "", + ) -> Uri: + """Create a uri with the given attributes.""" + + if scheme in {"http", "https", "file"}: + if not path.startswith("/"): + path = f"/{path}" + + return cls( + scheme=scheme, + authority=authority, + path=path, + query=query, + fragment=fragment, + ) + + @classmethod + def parse(cls, uri: str) -> Uri: + """Parse the given uri from its string representation.""" + scheme, authority, path, _, query, fragment = parse.urlparse(uri) + return cls.create( + scheme=parse.unquote(scheme), + authority=parse.unquote(authority), + path=parse.unquote(path), + query=parse.unquote(query), + fragment=parse.unquote(fragment), + ) + + def resolve(self) -> Uri: + """Return the fully resolved version of this Uri.""" + + # This operation only makes sense for file uris + if self.scheme != "file": + return Uri.parse(str(self)) + + return Uri.for_file(pathlib.Path(self).resolve()) + + @classmethod + def for_file(cls, filepath: Union[str, os.PathLike[str]]) -> Uri: + """Create a uri based on the given filepath.""" + + fpath = os.fspath(filepath) + if IS_WIN: + fpath = fpath.replace("\\", "/") + + if fpath.startswith("//"): + authority, *path = fpath[2:].split("/") + fpath = "/".join(path) + else: + authority = "" + + return cls.create(scheme="file", authority=authority, path=fpath) + + @property + def fs_path(self) -> Optional[str]: + """Return the equivalent fs path.""" + return self.as_fs_path() + + def where(self, **kwargs) -> Uri: + """Return an transformed version of this uri where certain components of the uri + have been replace with the given arguments. + + Passing a value of ``None`` will remove the given component entirely. + """ + keys = {"scheme", "authority", "path", "query", "fragment"} + valid_keys = keys.copy() & kwargs.keys() + + current = {k: getattr(self, k) for k in keys} + replacements = {k: kwargs[k] for k in valid_keys} + + return Uri.create(**{**current, **replacements}) + + def join(self, path: str) -> Uri: + """Join this Uri's path component with the given path and return the resulting + uri. + + Parameters + ---------- + path + The path segment to join + + Returns + ------- + Uri + The resulting uri + """ + + if not self.path: + raise ValueError("This uri has no path") + + if IS_WIN: + fs_path = self.fs_path + if fs_path is None: + raise ValueError("Unable to join paths, fs_path is None") + + joined = os.path.normpath(os.path.join(fs_path, path)) + new_path = self.for_file(joined).path + else: + new_path = os.path.normpath(os.path.join(self.path, path)) + + return self.where(path=new_path) + + def as_fs_path(self, preserve_case: bool = False) -> Optional[str]: + """Return the file system path correspondin with this uri.""" + if self.path: + path = _normalize_path(self.path, preserve_case) + + if self.authority and len(path) > 1: + path = f"//{self.authority}{path}" + + # Remove the leading `/` from windows paths + elif RE_DRIVE_LETTER_PATH.match(path): + path = path[1:] + + if IS_WIN: + path = path.replace("/", "\\") + + return path + + return None + + def as_string(self, encode=True) -> str: + """Return a string representation of this Uri. + + Parameters + ---------- + encode + If ``True`` (the default), encode any special characters. + + Returns + ------- + str + The string representation of the Uri + """ + + # See: https://github.com/python/mypy/issues/10740 + encoder: Callable[[str], str] = parse.quote if encode else _replace_chars # type: ignore[assignment] + + if authority := self.authority: + usercred, *auth = authority.split("@") + if len(auth) > 0: + *user, cred = usercred.split(":") + if len(user) > 0: + usercred = encoder(":".join(user)) + f":{encoder(cred)}" + else: + usercred = encoder(usercred) + authority = "@".join(auth) + else: + usercred = "" + + authority = authority.lower() + *auth, port = authority.split(":") + if len(auth) > 0: + authority = encoder(":".join(auth)) + f":{port}" + else: + authority = encoder(authority) + + if usercred: + authority = f"{usercred}@{authority}" + + scheme_separator = "" + if authority or self.scheme == "file": + scheme_separator = "//" + + if path := self.path: + path = encoder(_normalize_path(path)) + + if query := self.query: + query = encoder(query) + + if fragment := self.fragment: + fragment = encoder(fragment) + + parts = [ + f"{self.scheme}:", + scheme_separator, + authority if authority else "", + path if path else "", + f"?{query}" if query else "", + f"#{fragment}" if fragment else "", + ] + return "".join(parts) + + +def _replace_chars(segment: str) -> str: + """Replace a certain subset of characters in a uri segment""" + return segment.replace("#", "%23").replace("?", "%3F") + + +def _normalize_path(path: str, preserve_case: bool = False) -> str: + """Normalise the path segment of a Uri. + + Parameters + ---------- + path + The path to normalise. + + preserve_case + If ``True``, preserve the case of the drive label on Windows. + If ``False``, the drive label will be lowercased. + + Returns + ------- + str + The normalised path. + """ + + # normalize to fwd-slashes on windows, + # on other systems bwd-slashes are valid + # filename character, eg /f\oo/ba\r.txt + if IS_WIN: + path = path.replace("\\", "/") + + # Normalize drive paths to lower case + if (not preserve_case) and (match := RE_DRIVE_LETTER_PATH.match(path)): + path = match.group(1) + match.group(2).lower() + path[match.end() :] + + return path From d5725b9ae2e772dcfe1253895b1b8009c11c22d5 Mon Sep 17 00:00:00 2001 From: Alex Carney Date: Tue, 24 Sep 2024 20:53:24 +0100 Subject: [PATCH 2/8] lsp: Implement a `${defaultBuildDir}` config variable Unless esbonio finds a `sphinx-build` command to use from the user's config it will attempt to guess something reasonable. During this process it generates a default build directory to use, in a subfolder of `platformdirs.user_cache_dir()` so that it does not interfere with the user's files. Up until now, the moment a user sets their own `sphinx-build` command this behavior is lost, which can lead to issues on some systems (see #865). This commit introduces a `${defaultBuildDir}` placeholder value that the user can use to provide their own build flags, while maintaining the default choice of build dir provided by esbonio. --- .../sphinx_manager/client_subprocess.py | 4 ++ .../server/features/sphinx_manager/config.py | 13 ++-- lib/esbonio/esbonio/sphinx_agent/config.py | 40 +++++++++++- .../esbonio/sphinx_agent/handlers/__init__.py | 8 ++- .../esbonio/sphinx_agent/types/__init__.py | 7 +++ .../tests/sphinx-agent/test_sa_unit.py | 63 ++++++++++++++++++- 6 files changed, 124 insertions(+), 11 deletions(-) diff --git a/lib/esbonio/esbonio/server/features/sphinx_manager/client_subprocess.py b/lib/esbonio/esbonio/server/features/sphinx_manager/client_subprocess.py index 14c241e9a..4830b7d64 100644 --- a/lib/esbonio/esbonio/server/features/sphinx_manager/client_subprocess.py +++ b/lib/esbonio/esbonio/server/features/sphinx_manager/client_subprocess.py @@ -11,6 +11,7 @@ import typing from uuid import uuid4 +import platformdirs from pygls.client import JsonRPCClient from pygls.protocol import JsonRPCProtocol @@ -212,6 +213,9 @@ async def start(self) -> SphinxClient: params = types.CreateApplicationParams( command=self.config.build_command, config_overrides=self.config.config_overrides, + context={ + "cacheDir": platformdirs.user_cache_dir("esbonio", "swyddfa"), + }, ) self.sphinx_info = await self.protocol.send_request_async( "sphinx/createApp", params diff --git a/lib/esbonio/esbonio/server/features/sphinx_manager/config.py b/lib/esbonio/esbonio/server/features/sphinx_manager/config.py index ba3e975cf..2e6187427 100644 --- a/lib/esbonio/esbonio/server/features/sphinx_manager/config.py +++ b/lib/esbonio/esbonio/server/features/sphinx_manager/config.py @@ -1,6 +1,5 @@ from __future__ import annotations -import hashlib import importlib.util import logging import pathlib @@ -8,7 +7,6 @@ from typing import Optional import attrs -import platformdirs from pygls.workspace import Workspace from esbonio.server import Uri @@ -227,9 +225,12 @@ def _resolve_build_command(self, uri: Uri, logger: logging.Logger) -> list[str]: conf_py = current / "conf.py" logger.debug("Trying path: %s", current) if conf_py.exists(): - cache = platformdirs.user_cache_dir("esbonio", "swyddfa") - project = hashlib.md5(str(current).encode()).hexdigest() # noqa: S324 - build_dir = str(pathlib.Path(cache, project)) - return ["sphinx-build", "-M", "dirhtml", str(current), str(build_dir)] + return [ + "sphinx-build", + "-M", + "dirhtml", + str(current), + "${defaultBuildDir}", + ] return [] diff --git a/lib/esbonio/esbonio/sphinx_agent/config.py b/lib/esbonio/esbonio/sphinx_agent/config.py index f4e3262ed..1bb6de227 100644 --- a/lib/esbonio/esbonio/sphinx_agent/config.py +++ b/lib/esbonio/esbonio/sphinx_agent/config.py @@ -1,8 +1,10 @@ from __future__ import annotations import dataclasses +import hashlib import inspect import pathlib +import re import sys from typing import Any from typing import Literal @@ -13,6 +15,8 @@ from sphinx.application import Sphinx from sphinx.cmd.build import main as sphinx_build +VARIABLE = re.compile(r"\$\{(\w+)\}") + @dataclasses.dataclass class SphinxConfig: @@ -128,7 +132,7 @@ def fromcli(cls, args: list[str]): warning_is_error=sphinx_args.get("warningiserror", False), ) - def to_application_args(self) -> dict[str, Any]: + def to_application_args(self, context: dict[str, Any]) -> dict[str, Any]: """Convert this into the equivalent Sphinx application arguments.""" # On OSes like Fedora Silverblue, `/home` is symlinked to `/var/home`. This @@ -139,6 +143,25 @@ def to_application_args(self) -> dict[str, Any]: # Resolving these paths here, should ensure that the agent always # reports the true location of any given directory. conf_dir = pathlib.Path(self.conf_dir).resolve() + self.conf_dir = str(conf_dir) + + # Resolve any config variables. + # + # This is a bit hacky, but let's go with it for now. The only config variable + # we currently support is 'defaultBuildDir' which is derived from the value + # of `conf_dir`. So we resolve the full path of `conf_dir` above, then resolve + # the configuration variables here, before finally calling resolve() on the + # remaining paths below. + for name, value in dataclasses.asdict(self).items(): + if not isinstance(value, str): + continue + + if (match := VARIABLE.match(value)) is None: + continue + + replacement = self.resolve_config_variable(match.group(1), context) + setattr(self, name, VARIABLE.sub(replacement, value)) + build_dir = pathlib.Path(self.build_dir).resolve() doctree_dir = pathlib.Path(self.doctree_dir).resolve() src_dir = pathlib.Path(self.src_dir).resolve() @@ -159,3 +182,18 @@ def to_application_args(self) -> dict[str, Any]: "warning": sys.stderr, "warningiserror": self.warning_is_error, } + + def resolve_config_variable(self, name: str, context: dict[str, str]): + """Resolve the value for the given configuration variable.""" + + if name.lower() == "defaultbuilddir": + if (cache_dir := context.get("cacheDir")) is None: + raise RuntimeError( + f"Unable to resolve config variable {name!r}, " + "missing context value: 'cacheDir'" + ) + + project = hashlib.md5(self.conf_dir.encode()).hexdigest() # noqa: S324 + return str(pathlib.Path(cache_dir, project)) + + raise ValueError(f"Unknown configuration variable {name!r}") diff --git a/lib/esbonio/esbonio/sphinx_agent/handlers/__init__.py b/lib/esbonio/esbonio/sphinx_agent/handlers/__init__.py index bc1f158e7..6dd4a72d2 100644 --- a/lib/esbonio/esbonio/sphinx_agent/handlers/__init__.py +++ b/lib/esbonio/esbonio/sphinx_agent/handlers/__init__.py @@ -106,12 +106,14 @@ class definition from the ``types`` module. def create_sphinx_app(self, request: types.CreateApplicationRequest): """Create a new sphinx application instance.""" - sphinx_config = SphinxConfig.fromcli(request.params.command) + params = request.params + + sphinx_config = SphinxConfig.fromcli(params.command) if sphinx_config is None: raise ValueError("Invalid build command") - sphinx_config.config_overrides.update(request.params.config_overrides) - sphinx_args = sphinx_config.to_application_args() + sphinx_config.config_overrides.update(params.config_overrides) + sphinx_args = sphinx_config.to_application_args(params.context) self.app = Sphinx(**sphinx_args) # Connect event handlers. diff --git a/lib/esbonio/esbonio/sphinx_agent/types/__init__.py b/lib/esbonio/esbonio/sphinx_agent/types/__init__.py index 4553c307a..6be3d248c 100644 --- a/lib/esbonio/esbonio/sphinx_agent/types/__init__.py +++ b/lib/esbonio/esbonio/sphinx_agent/types/__init__.py @@ -144,6 +144,10 @@ ] +# -- RPC Types +# +# These represent the structure of the messages sent between the Sphinx agent and the +# parent language server. @dataclasses.dataclass class CreateApplicationParams: """Parameters of a ``sphinx/createApp`` request.""" @@ -154,6 +158,9 @@ class CreateApplicationParams: config_overrides: dict[str, Any] """Overrides to apply to the application's configuration.""" + context: dict[str, str] = dataclasses.field(default_factory=dict) + """The context in which to resolve config variables.""" + @dataclasses.dataclass class CreateApplicationRequest: diff --git a/lib/esbonio/tests/sphinx-agent/test_sa_unit.py b/lib/esbonio/tests/sphinx-agent/test_sa_unit.py index 4a3934739..7e73b3f3f 100644 --- a/lib/esbonio/tests/sphinx-agent/test_sa_unit.py +++ b/lib/esbonio/tests/sphinx-agent/test_sa_unit.py @@ -1,5 +1,6 @@ from __future__ import annotations +import hashlib import logging import os import pathlib @@ -431,7 +432,7 @@ def test_cli_arg_handling(args: list[str], expected: dict[str, Any]): config = SphinxConfig.fromcli(args) assert config is not None - actual = config.to_application_args() + actual = config.to_application_args({}) # pytest overrides stderr on windows, so if we were to put `sys.stderr` in the # `expected` dict this test would fail as `sys.stderr` inside a test function has a @@ -444,6 +445,66 @@ def test_cli_arg_handling(args: list[str], expected: dict[str, Any]): assert expected == actual +@pytest.mark.parametrize( + "args, expected, context", + [ + ( + ["-M", "html", "src", "${defaultBuildDir}"], + application_args( + srcdir="src", + confdir="src", + outdir=os.sep + os.path.join("path", "to", "cache", "", "html"), + doctreedir=( + os.sep + os.path.join("path", "to", "cache", "", "doctrees") + ), + buildername="html", + ), + { + "cacheDir": os.sep + os.sep.join(["path", "to", "cache"]), + }, + ), + ( + ["-b", "html", "src", "${defaultBuildDir}"], + application_args( + srcdir="src", + confdir="src", + outdir=os.sep + os.sep.join(["path", "to", "cache", ""]), + doctreedir=( + os.sep + os.path.join("path", "to", "cache", r"", ".doctrees") + ), + buildername="html", + ), + { + "cacheDir": os.sep + os.sep.join(["path", "to", "cache"]), + }, + ), + ], +) +def test_cli_default_build_dir( + args: list[str], expected: dict[str, Any], context: dict[str, str] +): + """Ensure that we can handle ``${defaultBuildDir}`` variable correctly.""" + config = SphinxConfig.fromcli(args) + assert config is not None + + actual = config.to_application_args(context) + + # pytest overrides stderr on windows, so if we were to put `sys.stderr` in the + # `expected` dict this test would fail as `sys.stderr` inside a test function has a + # different value. + # + # So, let's test for it here instead + assert actual.pop("status") == sys.stderr + assert actual.pop("warning") == sys.stderr + + # Compute the expected hash now that the confdir has been resolved + ehash = hashlib.md5(config.conf_dir.encode()).hexdigest() + expected["outdir"] = expected["outdir"].replace("", ehash) + expected["doctreedir"] = expected["doctreedir"].replace("", ehash) + + assert expected == actual + + ROOT = pathlib.Path(__file__).parent.parent / "sphinx-extensions" / "workspace" PY_PATH = ROOT / "code" / "diagnostics.py" CONF_PATH = ROOT / "sphinx-extensions" / "conf.py" From b707ae3dcc8693e6638b86a23c8280d4f989e26f Mon Sep 17 00:00:00 2001 From: Alex Carney Date: Tue, 24 Sep 2024 21:00:37 +0100 Subject: [PATCH 3/8] lsp: mypy fixes --- lib/esbonio/esbonio/server/server.py | 4 +++- lib/esbonio/esbonio/server/setup.py | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/esbonio/esbonio/server/server.py b/lib/esbonio/esbonio/server/server.py index 11b589f3a..501cb111b 100644 --- a/lib/esbonio/esbonio/server/server.py +++ b/lib/esbonio/esbonio/server/server.py @@ -39,7 +39,9 @@ def get_text_document(self, doc_uri: str) -> TextDocument: uri = str(Uri.parse(doc_uri).resolve()) return super().get_text_document(uri) - def put_text_document(self, text_document: types.TextDocumentItem): + def put_text_document( + self, text_document: types.TextDocumentItem, notebook_uri: str | None = None + ): text_document.uri = str(Uri.parse(text_document.uri).resolve()) return super().put_text_document(text_document) diff --git a/lib/esbonio/esbonio/server/setup.py b/lib/esbonio/esbonio/server/setup.py index c05c62866..3ab8948a7 100644 --- a/lib/esbonio/esbonio/server/setup.py +++ b/lib/esbonio/esbonio/server/setup.py @@ -85,7 +85,7 @@ async def on_document_save( ): # Record the version number of the document doc = ls.workspace.get_text_document(params.text_document.uri) - doc.saved_version = doc.version or 0 + doc.saved_version = doc.version or 0 # type: ignore[attr-defined] await call_features(ls, "document_save", params) From d89845a34fadc037b5f769b8b84255146df2c4b7 Mon Sep 17 00:00:00 2001 From: Alex Carney Date: Tue, 24 Sep 2024 21:01:00 +0100 Subject: [PATCH 4/8] docs: Update dependencies --- docs/pyproject.toml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/docs/pyproject.toml b/docs/pyproject.toml index a636caea1..fe8ac1d86 100644 --- a/docs/pyproject.toml +++ b/docs/pyproject.toml @@ -12,8 +12,12 @@ dependencies = [ "furo", "myst-parser", "platformdirs", - "pytest_lsp", + "pytest_lsp>=1.0b0", + "pygls>=2.0a0", ] +[tool.hatch.envs.docs.env-vars] +UV_PRERELEASE = "allow" + [tool.hatch.envs.docs.scripts] build = "sphinx-build -M dirhtml . ./_build" From 9b52603061f90244c1934f13feb607aea4b53205 Mon Sep 17 00:00:00 2001 From: Alex Carney Date: Tue, 24 Sep 2024 21:01:39 +0100 Subject: [PATCH 5/8] docs: Use the `${defaultBuildDir}` placeholder --- docs/pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/pyproject.toml b/docs/pyproject.toml index fe8ac1d86..39eb8af42 100644 --- a/docs/pyproject.toml +++ b/docs/pyproject.toml @@ -1,5 +1,5 @@ [tool.esbonio.sphinx] -# buildCommand = ["sphinx-build", "-M", "dirhtml", ".", "./_build"] +buildCommand = ["sphinx-build", "-M", "dirhtml", ".", "${defaultBuildDir}"] pythonCommand = ["hatch", "-e", "docs", "run", "python"] [tool.hatch.envs.docs] From 39ed784faa784d39e1d89b0f4a0840c4f2b4710a Mon Sep 17 00:00:00 2001 From: Alex Carney Date: Wed, 25 Sep 2024 18:52:06 +0100 Subject: [PATCH 6/8] docs: Document the `${defaultBuildDir}` variable --- docs/lsp/howto/use-esbonio-with.rst | 2 ++ docs/lsp/reference/configuration.rst | 29 ++++++++++++++++++++++++---- 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/docs/lsp/howto/use-esbonio-with.rst b/docs/lsp/howto/use-esbonio-with.rst index 348205507..bd867db84 100644 --- a/docs/lsp/howto/use-esbonio-with.rst +++ b/docs/lsp/howto/use-esbonio-with.rst @@ -1,3 +1,5 @@ +.. _lsp-use-with: + How To Use Esbonio With... ========================== diff --git a/docs/lsp/reference/configuration.rst b/docs/lsp/reference/configuration.rst index 572d42192..87e195cd5 100644 --- a/docs/lsp/reference/configuration.rst +++ b/docs/lsp/reference/configuration.rst @@ -230,21 +230,42 @@ The following options control the creation of the Sphinx application object mana :scope: project :type: string[] - The ``sphinx-build`` command to use when invoking the Sphinx subprocess. + The ``sphinx-build`` command ``esbonio`` should use when building your documentation, for example:: + + ["sphinx-build", "-M", "dirhtml", "docs", "${defaultBuildDir}", "--fail-on-warning"] + + This can contain any valid :external+sphinx:std:doc:`man/sphinx-build` argument however, the following arguments will be ignored and have no effect. + + - ``--color``, ``-P``, ``--pdb`` + + Additionally, this option supports the following variables + + - ``${defaultBuildDir}``: Expands to esbonio's default choice of build directory .. esbonio:config:: esbonio.sphinx.pythonCommand :scope: project :type: string[] - The command to use when launching the Python interpreter for the process hosting the Sphinx application. - Use this to select the Python environment you want to use when building your documentation. + Used to select the Python environment ``esbonio`` should use when building your documentation. + This can be as simple as the full path to the Python executable in your virtual environment:: + + ["/home/user/Projects/example/venv/bin/python"] + + Or a complex command with a number of options and arguments:: + + ["hatch", "-e", "docs", "run", "python"] + + For more examples see :ref:`lsp-use-with` .. esbonio:config:: esbonio.sphinx.cwd :scope: project :type: string The working directory from which to launch the Sphinx process. - If not set, this will default to the root of the workspace folder containing the project. + If not set + + - ``esbonio`` will use the directory containing the "closest" ``pyproject.toml`` file. + - If no ``pyproject.toml`` file can be found, ``esbonio`` will use workspace folder containing the project. .. esbonio:config:: esbonio.sphinx.envPassthrough :scope: project From 5176a68f9f10e12298ff8f16a6f2d9b9bf97086b Mon Sep 17 00:00:00 2001 From: Alex Carney Date: Wed, 25 Sep 2024 19:43:46 +0100 Subject: [PATCH 7/8] lsp: Ensure filepaths are properly escaped Backslashes in Windows filepaths can cause issues ```python >>> import re >>> VARIABLE = re.compile(r"\$\{(\w+)\}") >>> VARIABLE.sub('\\path\\to\\cache', '${defaultBuildDir}/doctrees') Traceback (most recent call last): File "", line 1, in File "/usr/lib64/python3.12/re/__init__.py", line 334, in _compile_template return _sre.template(pattern, _parser.parse_template(repl, pattern)) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/lib64/python3.12/re/_parser.py", line 1075, in parse_template raise s.error('bad escape %s' % this, len(this)) from None re.error: bad escape \p at position 0 ``` Calling `re.escape` on the replacement ensures that characters like backslashes are escaped correctly ```python >>> VARIABLE.sub(re.escape('\\path\\to\\cache'), '${defaultBuildDir}/doctrees') '\\path\\to\\cache/doctrees' >>> ``` --- lib/esbonio/esbonio/sphinx_agent/config.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/esbonio/esbonio/sphinx_agent/config.py b/lib/esbonio/esbonio/sphinx_agent/config.py index 1bb6de227..e491dcd4f 100644 --- a/lib/esbonio/esbonio/sphinx_agent/config.py +++ b/lib/esbonio/esbonio/sphinx_agent/config.py @@ -160,7 +160,9 @@ def to_application_args(self, context: dict[str, Any]) -> dict[str, Any]: continue replacement = self.resolve_config_variable(match.group(1), context) - setattr(self, name, VARIABLE.sub(replacement, value)) + result = VARIABLE.sub(re.escape(replacement), value) + + setattr(self, name, result) build_dir = pathlib.Path(self.build_dir).resolve() doctree_dir = pathlib.Path(self.doctree_dir).resolve() From 4a1e84f090b09196a209964a60bed54a0083103b Mon Sep 17 00:00:00 2001 From: Alex Carney Date: Sun, 29 Sep 2024 12:28:47 +0100 Subject: [PATCH 8/8] lsp: Create a placeholder for a client at the given scope When `manager.get_client` is called many times in quick succession (such as on server restart with N files open) this can fool the `SphinxManager` into creating multiple client instances for a given configuration scope. By storing a ``None`` at the relevant scope we allow the SphinxManager to detect that the scope has already been handled, preventing the spawning of duplicated client instances. This should, finally, fix the flaky test issue (#859) --- .../server/features/sphinx_manager/manager.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/lib/esbonio/esbonio/server/features/sphinx_manager/manager.py b/lib/esbonio/esbonio/server/features/sphinx_manager/manager.py index c02462887..7cee14f1d 100644 --- a/lib/esbonio/esbonio/server/features/sphinx_manager/manager.py +++ b/lib/esbonio/esbonio/server/features/sphinx_manager/manager.py @@ -238,8 +238,18 @@ async def get_client(self, uri: Uri) -> SphinxClient | None: partial(self._create_or_replace_client, uri), scope=uri, ) - # The first few callers in a given scope will miss out, but that shouldn't matter - # too much + + # It's possible for this code path to be hit multiple times in quick + # succession e.g. on a fresh server start with N .rst files already open, + # creating the opportunity to accidentally spawn N duplicated clients! + # + # To prevent this, store a `None` at this scope, all being well it will be + # replaced with the actual client instance when the + # `_create_or_replace_client` callback runs. + self.clients[scope] = None + + # The first few callers in a given scope will miss out, but that shouldn't + # matter too much return None if (client := self.clients[scope]) is None: