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 b61df93cb..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 @@ -17,6 +18,10 @@ if typing.TYPE_CHECKING: from typing import IO 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]] @@ -43,12 +48,41 @@ class Esbonio: log: DiagnosticFilter def __init__(self, dbpath: pathlib.Path, app: _Sphinx): + self.app: _Sphinx = app self.db = Database(dbpath) self.log = DiagnosticFilter(app) self._roles: list[RoleDefinition] = [] """Roles captured during Sphinx startup.""" + self._config_ast: ast.Module | Literal[False] | None = None + """The parsed AST of the user's conf.py file. + If ``False``, we already tried parsing the module and were unable to.""" + + self.diagnostics: dict[types.Uri, set[types.Diagnostic]] = {} + """Recorded diagnostics.""" + + @property + def config_uri(self) -> types.Uri: + return types.Uri.for_file(pathlib.Path(self.app.confdir, "conf.py")) + + @property + def config_ast(self) -> ast.Module | None: + """Return the AST for the user's conf.py (if possible)""" + + if self._config_ast is not None: + return self._config_ast or None # convert ``False`` to ``None`` + + try: + conf_py = pathlib.Path(self.app.confdir, "conf.py") + self._config_ast = ast.parse(source=conf_py.read_text()) + return self._config_ast + except Exception as exc: + logger.debug("Unable to parse user's conf.py: %s", exc) + self._config_ast = False + + return None + def add_role( self, name: str, @@ -108,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` @@ -135,22 +179,13 @@ def _report_missing_extension(self, extname: str, exc: Exception): Parameters ---------- extname - The name of the extension that caused the excetion + The name of the extension that caused the exception exc The exception instance """ - if not isinstance(cause := exc.__cause__, ImportError): - return - - # Parse the user's config file - # TODO: Move this somewhere more central. - try: - conf_py = pathlib.Path(self.confdir, "conf.py") - config = ast.parse(source=conf_py.read_text()) - except Exception: - logger.debug("Unable to parse user's conf.py") + if (config := self.esbonio.config_ast) is None: return # Now attempt to find the soure location of the extenison. @@ -159,13 +194,112 @@ def _report_missing_extension(self, extname: str, exc: Exception): return diagnostic = types.Diagnostic( - range=range_, message=str(cause), severity=types.DiagnosticSeverity.Error + range=range_, message=f"{exc}", severity=types.DiagnosticSeverity.Error ) - # TODO: Move the set of diagnostics somewhere more central. - uri = types.Uri.for_file(conf_py) + uri = self.esbonio.config_uri logger.debug("Adding diagnostic %s: %s", uri, diagnostic) - self.esbonio.log.diagnostics.setdefault(uri, set()).add(diagnostic) + 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: @@ -210,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/esbonio/sphinx_agent/handlers/diagnostics.py b/lib/esbonio/esbonio/sphinx_agent/handlers/diagnostics.py index 124b1affe..a18ab00a1 100644 --- a/lib/esbonio/esbonio/sphinx_agent/handlers/diagnostics.py +++ b/lib/esbonio/esbonio/sphinx_agent/handlers/diagnostics.py @@ -22,14 +22,14 @@ def init_db(app: Sphinx, config: Config): def clear_diagnostics(app: Sphinx, docname: str, source): """Clear the diagnostics assocated with the given file.""" uri = Uri.for_file(app.env.doc2path(docname, base=True)) - app.esbonio.log.diagnostics.pop(uri, None) + app.esbonio.diagnostics.pop(uri, None) def sync_diagnostics(app: Sphinx, *args): app.esbonio.db.clear_table(DIAGNOSTICS_TABLE) results = [] - diagnostics = app.esbonio.log.diagnostics + diagnostics = app.esbonio.diagnostics for uri, items in diagnostics.items(): for item in items: diff --git a/lib/esbonio/esbonio/sphinx_agent/log.py b/lib/esbonio/esbonio/sphinx_agent/log.py index ffe42b746..c40391322 100644 --- a/lib/esbonio/esbonio/sphinx_agent/log.py +++ b/lib/esbonio/esbonio/sphinx_agent/log.py @@ -30,7 +30,6 @@ def __init__(self, app, *args, **kwargs): super().__init__(*args, **kwargs) self.app = app - self.diagnostics: dict[Uri, set[types.Diagnostic]] = {} def filter(self, record: logging.LogRecord) -> bool: conditions = [ @@ -76,7 +75,7 @@ def filter(self, record: logging.LogRecord) -> bool: ), ) - self.diagnostics.setdefault(uri, set()).add(diagnostic) + self.app.esbonio.diagnostics.setdefault(uri, set()).add(diagnostic) return True 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