From 591748e51651839a2d9258c3517ddf4e03985f9b Mon Sep 17 00:00:00 2001 From: Alex Carney Date: Sun, 20 Oct 2024 18:01:27 +0100 Subject: [PATCH] sphinx-agent: Implement a fallback html_theme The sphinx agent can now handle the case where the requested `html_theme` is not available in the environment by suppressing the raised error, overriding the value of `html_theme` and attempting to run `Sphinx.__init__` again. --- lib/esbonio/changes/916.enhancement.md | 1 + lib/esbonio/esbonio/sphinx_agent/app.py | 132 +++++++++++++++++- lib/esbonio/hatch.toml | 1 - lib/esbonio/tests/e2e/conftest.py | 4 - lib/esbonio/tests/e2e/test_e2e_diagnostics.py | 13 +- lib/esbonio/tests/sphinx-agent/conftest.py | 4 - .../sphinx-agent/handlers/test_diagnostics.py | 54 +++++-- .../{test_app.py => test_sa_database.py} | 0 .../tests/sphinx-agent/test_sa_diagnostics.py | 99 +++++++++++++ 9 files changed, 282 insertions(+), 26 deletions(-) create mode 100644 lib/esbonio/changes/916.enhancement.md rename lib/esbonio/tests/sphinx-agent/{test_app.py => test_sa_database.py} (100%) create mode 100644 lib/esbonio/tests/sphinx-agent/test_sa_diagnostics.py diff --git a/lib/esbonio/changes/916.enhancement.md b/lib/esbonio/changes/916.enhancement.md new file mode 100644 index 000000000..86358152b --- /dev/null +++ b/lib/esbonio/changes/916.enhancement.md @@ -0,0 +1 @@ +When asking for a `html_theme` that is not available in the current environment, the server will now fallback to Sphinx's `alabaster` theme and report the error as a diagnostic diff --git a/lib/esbonio/esbonio/sphinx_agent/app.py b/lib/esbonio/esbonio/sphinx_agent/app.py index 2318b7296..1b28f3534 100644 --- a/lib/esbonio/esbonio/sphinx_agent/app.py +++ b/lib/esbonio/esbonio/sphinx_agent/app.py @@ -6,6 +6,7 @@ import typing from sphinx.application import Sphinx as _Sphinx +from sphinx.errors import ThemeError from sphinx.util import console from sphinx.util import logging as sphinx_logging_module from sphinx.util.logging import NAMESPACE as SPHINX_LOG_NAMESPACE @@ -19,6 +20,9 @@ from typing import Any from typing import Literal + from docutils.nodes import Element + from docutils.parsers.rst import Directive + RoleDefinition = tuple[str, Any, list[types.Role.TargetProvider]] sphinx_logger = logging.getLogger(SPHINX_LOG_NAMESPACE) @@ -138,12 +142,22 @@ def __init__(self, *args, **kwargs): # Override sphinx's usual logging setup function sphinx_logging_module.setup = setup_logging # type: ignore - super().__init__(*args, **kwargs) + # `try_run_init` may call `__init__` more than once, this could lead to spamming + # the user with warning messages, so we will suppress these messages if the + # retry counter has been set. + self._esbonio_retry_count = 0 + try_run_init(self, super().__init__, *args, **kwargs) def add_role(self, name: str, role: Any, override: bool = False): - super().add_role(name, role, override) + super().add_role(name, role, override or self._esbonio_retry_count > 0) self.esbonio.add_role(name, role) + def add_directive(self, name: str, cls: type[Directive], override: bool = False): + super().add_directive(name, cls, override or self._esbonio_retry_count > 0) + + def add_node(self, node: type[Element], override: bool = False, **kwargs): + super().add_node(node, override or self._esbonio_retry_count > 0, **kwargs) + def setup_extension(self, extname: str): """Override Sphinx's implementation of `setup_extension` @@ -188,6 +202,106 @@ def _report_missing_extension(self, extname: str, exc: Exception): self.esbonio.diagnostics.setdefault(uri, set()).add(diagnostic) +def try_run_init(app: Sphinx, init_fn, *args, **kwargs): + """Try and run Sphinx's ``__init__`` function. + + There are occasions where Sphinx will try and throw an error that is recoverable + e.g. a missing theme. In these situations we want to suppress the error, record a + diagnostic, and try again - which is what this function will do. + + Some errors however, are not recoverable in which case we will allow the error to + proceed as normal. + + Parameters + ---------- + app + The application instance we are trying to initialize + + init_fn + The application's `__init__` method, as returned by ``super().__init__`` + + args + Positional arguments to ``__init__`` + + retries + Max number of retries, a fallback in case we end up creating infinite recursion + + kwargs + Keyword arguments to ``__init__`` + """ + + if app._esbonio_retry_count >= 100: + raise RuntimeError("Unable to initialize Sphinx: max retries exceeded") + + try: + init_fn(*args, **kwargs) + except ThemeError as exc: + # Fallback to the default theme. + kwargs.setdefault("confoverrides", {})["html_theme"] = "alabaster" + kwargs["confoverrides"]["html_theme_options"] = {} + + app._esbonio_retry_count += 1 + report_theme_error(app, exc) + try_run_init(app, init_fn, *args, **kwargs) + except Exception: + logger.exception("Unable to initialize Sphinx") + raise + + +def report_theme_error(app: Sphinx, exc: ThemeError): + """Attempt to convert the given theme error into a useful diagnostic. + + Parameters + ---------- + app + The Sphinx object being initialized + + exc + The error instance + """ + + if (config := app.esbonio.config_ast) is None: + return + + if (range_ := find_html_theme_declaration(config)) is None: + return + + diagnostic = types.Diagnostic( + range=range_, + message=f"{exc}", + severity=types.DiagnosticSeverity.Error, + ) + + uri = app.esbonio.config_uri + logger.debug("Adding diagnostic %s: %s", uri, diagnostic) + app.esbonio.diagnostics.setdefault(uri, set()).add(diagnostic) + + +def find_html_theme_declaration(mod: ast.Module) -> types.Range | None: + """Attempt to find the location in the user's conf.py file where the ``html_theme`` + was declared.""" + + for node in mod.body: + if not isinstance(node, ast.Assign): + continue + + if len(targets := node.targets) != 1: + continue + + if not isinstance(name := targets[0], ast.Name): + continue + + if name.id == "html_theme": + break + + else: + # Nothing found, abort + logger.debug("Unable to find 'html_theme' node") + return None + + return ast_node_to_range(node) + + def find_extension_declaration(mod: ast.Module, extname: str) -> types.Range | None: """Attempt to find the location in the user's conf.py file where the given ``extname`` was declared. @@ -230,15 +344,21 @@ def find_extension_declaration(mod: ast.Module, extname: str) -> types.Range | N logger.debug("Unable to find node for extension %r", extname) return None + return ast_node_to_range(element) + + +def ast_node_to_range(node: ast.stmt | ast.expr) -> types.Range: + """Convert the given ast node to a range.""" + # Finally, try and extract the source location. - start_line = element.lineno - 1 - start_char = element.col_offset + start_line = node.lineno - 1 + start_char = node.col_offset - if (end_line := (element.end_lineno or 0) - 1) < 0: + if (end_line := (node.end_lineno or 0) - 1) < 0: end_line = start_line + 1 end_char: int | None = 0 - elif (end_char := element.end_col_offset) is None: + elif (end_char := node.end_col_offset) is None: end_line += 1 end_char = 0 diff --git a/lib/esbonio/hatch.toml b/lib/esbonio/hatch.toml index ad91c6ee5..1a5d04755 100644 --- a/lib/esbonio/hatch.toml +++ b/lib/esbonio/hatch.toml @@ -30,7 +30,6 @@ sphinx = ["8"] [envs.hatch-test.overrides] matrix.sphinx.dependencies = [ - "sphinx-design", "myst-parser", { value = "sphinx>=6,<7", if = ["6"] }, { value = "sphinx>=7,<8", if = ["7"] }, diff --git a/lib/esbonio/tests/e2e/conftest.py b/lib/esbonio/tests/e2e/conftest.py index b02099189..4000ef38c 100644 --- a/lib/esbonio/tests/e2e/conftest.py +++ b/lib/esbonio/tests/e2e/conftest.py @@ -47,10 +47,6 @@ async def client(lsp_client: LanguageClient, uri_for, tmp_path_factory): workspace_uri.fs_path, str(build_dir), ], - "configOverrides": { - "html_theme": "alabaster", - "html_theme_options": {}, - }, "pythonCommand": [sys.executable], }, }, diff --git a/lib/esbonio/tests/e2e/test_e2e_diagnostics.py b/lib/esbonio/tests/e2e/test_e2e_diagnostics.py index 2c108b9ef..01a62b652 100644 --- a/lib/esbonio/tests/e2e/test_e2e_diagnostics.py +++ b/lib/esbonio/tests/e2e/test_e2e_diagnostics.py @@ -81,12 +81,19 @@ async def test_workspace_diagnostic(client: LanguageClient, uri_for): workspace_uri = uri_for("workspaces", "demo") expected = { - str(workspace_uri / "rst" / "diagnostics.rst"): {message}, - str(workspace_uri / "myst" / "diagnostics.md"): {message}, + str(workspace_uri / "conf.py"): ( + "no theme named 'furo' found", + "Could not import extension sphinx_design (exception: " + "No module named 'sphinx_design')", + ), + str(workspace_uri / "index.rst"): ('Unknown directive type "grid"'), + str(workspace_uri / "rst" / "diagnostics.rst"): (message,), + str(workspace_uri / "myst" / "diagnostics.md"): (message,), } assert len(report.items) == len(expected) for item in report.items: - assert expected[item.uri] == {d.message for d in item.items} + for diagnostic in item.items: + assert diagnostic.message.startswith(expected[item.uri]) assert len(client.diagnostics) == 0, "Server should not publish diagnostics" diff --git a/lib/esbonio/tests/sphinx-agent/conftest.py b/lib/esbonio/tests/sphinx-agent/conftest.py index 80e11a84b..d339ae362 100644 --- a/lib/esbonio/tests/sphinx-agent/conftest.py +++ b/lib/esbonio/tests/sphinx-agent/conftest.py @@ -52,10 +52,6 @@ async def client(request, uri_for, build_dir): demo_workspace.fs_path, str(build_dir), ], - config_overrides={ - "html_theme": "alabaster", - "html_theme_options": {}, - }, ) resolved = config.resolve(test_uri, workspace, logger) assert resolved is not None diff --git a/lib/esbonio/tests/sphinx-agent/handlers/test_diagnostics.py b/lib/esbonio/tests/sphinx-agent/handlers/test_diagnostics.py index 8994b44b3..4bf4ed7b7 100644 --- a/lib/esbonio/tests/sphinx-agent/handlers/test_diagnostics.py +++ b/lib/esbonio/tests/sphinx-agent/handlers/test_diagnostics.py @@ -5,7 +5,6 @@ import pytest from pygls.protocol import default_converter -from sphinx import version_info as sphinx_version from esbonio.server import Uri from esbonio.server.features.project_manager import Project @@ -26,8 +25,17 @@ def check_diagnostics( for k, ex_diags in expected.items(): actual_diags = [converter.structure(d, types.Diagnostic) for d in actual[k]] - # Order of results is not important - assert set(actual_diags) == set(ex_diags) + assert len(ex_diags) == len(actual_diags) + + for actual_diagnostic in actual_diags: + # Assumes ranges are unique + matches = [e for e in ex_diags if e.range == actual_diagnostic.range] + assert len(matches) == 1 + + expected_diagnostic = matches[0] + assert actual_diagnostic.range == expected_diagnostic.range + assert actual_diagnostic.severity == expected_diagnostic.severity + assert actual_diagnostic.message.startswith(expected_diagnostic.message) @pytest.mark.asyncio @@ -36,13 +44,40 @@ async def test_diagnostics(client: SubprocessSphinxClient, project: Project, uri that they are correctly reset when fixed.""" rst_diagnostics_uri = uri_for("workspaces/demo/rst/diagnostics.rst") myst_diagnostics_uri = uri_for("workspaces/demo/myst/diagnostics.md") + index_uri = uri_for("workspaces/demo/index.rst") + conf_uri = uri_for("workspaces/demo/conf.py") - if sphinx_version[0] >= 8: - message = "image file not readable: not-an-image.png [image.not_readable]" - else: - message = "image file not readable: not-an-image.png" + message = "image file not readable: not-an-image.png" expected = { + index_uri: [ + types.Diagnostic( + message='Unknown directive type "grid"', + severity=types.DiagnosticSeverity.Error, + range=types.Range( + start=types.Position(line=11, character=0), + end=types.Position(line=12, character=0), + ), + ) + ], + conf_uri: [ + types.Diagnostic( + message="Could not import extension sphinx_design", + severity=types.DiagnosticSeverity.Error, + range=types.Range( + start=types.Position(line=20, character=4), + end=types.Position(line=20, character=19), + ), + ), + types.Diagnostic( + message="no theme named 'furo' found", + severity=types.DiagnosticSeverity.Error, + range=types.Range( + start=types.Position(line=41, character=0), + end=types.Position(line=41, character=19), + ), + ), + ], rst_diagnostics_uri: [ types.Diagnostic( message=message, @@ -77,7 +112,10 @@ async def test_diagnostics(client: SubprocessSphinxClient, project: Project, uri ) actual = await project.get_diagnostics() - check_diagnostics({myst_diagnostics_uri: expected[myst_diagnostics_uri]}, actual) + + fixed_expected = expected.copy() + del fixed_expected[rst_diagnostics_uri] + check_diagnostics(fixed_expected, actual) # The original diagnostics should be reported when the issues are re-introduced. # diff --git a/lib/esbonio/tests/sphinx-agent/test_app.py b/lib/esbonio/tests/sphinx-agent/test_sa_database.py similarity index 100% rename from lib/esbonio/tests/sphinx-agent/test_app.py rename to lib/esbonio/tests/sphinx-agent/test_sa_database.py diff --git a/lib/esbonio/tests/sphinx-agent/test_sa_diagnostics.py b/lib/esbonio/tests/sphinx-agent/test_sa_diagnostics.py new file mode 100644 index 000000000..8995efa11 --- /dev/null +++ b/lib/esbonio/tests/sphinx-agent/test_sa_diagnostics.py @@ -0,0 +1,99 @@ +from __future__ import annotations + +import ast + +import pytest +from lsprotocol import types + +from esbonio.server.testing import range_from_str +from esbonio.sphinx_agent.app import find_extension_declaration +from esbonio.sphinx_agent.app import find_html_theme_declaration + +CONF_PY = """\ +# Configuration file for the Sphinx documentation builder. +# +# For the full list of built-in configuration values, see the documentation: +# https://www.sphinx-doc.org/en/master/usage/configuration.html +from docutils import nodes +from sphinx.application import Sphinx + +# -- Project information ----------------------------------------------------- +# https://www.sphinx-doc.org/en/master/usage/configuration.html#project-information + +project = "Esbonio Demo" +copyright = "2023, Esbonio Developers" +author = "Esbonio Developers" +release = "1.0" + +extensions = [ "a", + "sphinx.ext.intersphinx", + "myst-parser", +] + +# -- Options for HTML output ------------------------------------------------- +# https://www.sphinx-doc.org/en/master/usage/configuration.html#options-for-html-output + +html_theme = "furo" +html_title = "Esbonio Demo" +html_theme_options = { + "source_repository": "https://github.com/swyddfa/esbonio", + "source_branch": "develop", + "source_directory": "lib/esbonio/tests/workspaces/demo/", +} +""" + + +@pytest.mark.parametrize( + "src,expected", + [ + ("", None), + ("a=3", None), + (CONF_PY, range_from_str("23:0-23:19")), + ], +) +def test_find_html_theme_declaration(src: str, expected: types.Range | None): + """Ensure that we can locate the location within a ``conf.py`` + file where the ``html_theme`` is defined.""" + + mod = ast.parse(src) + actual = find_html_theme_declaration(mod) + + if expected is None: + assert actual is None + + else: + assert actual.start.line == expected.start.line + assert actual.end.line == expected.end.line + + assert actual.start.character == expected.start.character + assert actual.end.character == expected.end.character + + +@pytest.mark.parametrize( + "src,extname,expected", + [ + ("", "myst-parser", None), + ("a=3", "myst-parser", None), + ("extensions='a'", "myst-parser", None), + ("extensions=['myst-parser']", "myst-parser", range_from_str("0:12-0:25")), + (CONF_PY, "myst-parser", range_from_str("17:6-17:19")), + ], +) +def test_find_extension_declaration( + src: str, extname: str, expected: types.Range | None +): + """Ensure that we can locate the location within a ``conf.py`` + file where the ``html_theme`` is defined.""" + + mod = ast.parse(src) + actual = find_extension_declaration(mod, extname) + + if expected is None: + assert actual is None + + else: + assert actual.start.line == expected.start.line + assert actual.end.line == expected.end.line + + assert actual.start.character == expected.start.character + assert actual.end.character == expected.end.character