From 9887868545de48fc1763d1f66a009a544fc5220e Mon Sep 17 00:00:00 2001 From: Alex Carney Date: Tue, 24 Sep 2024 20:53:24 +0100 Subject: [PATCH] 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 | 61 ++++++++++++++++++- 6 files changed, 122 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 14c241e9..4830b7d6 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 ba3e975c..2e618742 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 f4e3262e..1bb6de22 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 bc1f158e..6dd4a72d 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 4553c307..6be3d248 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 4a393473..8559e94b 100644 --- a/lib/esbonio/tests/sphinx-agent/test_sa_unit.py +++ b/lib/esbonio/tests/sphinx-agent/test_sa_unit.py @@ -3,6 +3,7 @@ import logging import os import pathlib +import re import sys import typing from unittest import mock @@ -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,64 @@ 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", r"\w+", "html"), + doctreedir=( + os.sep + os.path.join("path", "to", "cache", r"\w+", "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", r"\w+"]), + doctreedir=os.sep + + os.path.join("path", "to", "cache", r"\w+", ".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 + + # Also test the outdir and doctree match the expected pattern + assert re.match(expected.pop("outdir"), actual.pop("outdir")) + assert re.match(expected.pop("doctreedir"), actual.pop("doctreedir")) + + 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"