diff --git a/sphinxdocs/sphinxdocs/private/BUILD.bazel b/sphinxdocs/sphinxdocs/private/BUILD.bazel index fa5ded15f1..823f07fe73 100644 --- a/sphinxdocs/sphinxdocs/private/BUILD.bazel +++ b/sphinxdocs/sphinxdocs/private/BUILD.bazel @@ -129,6 +129,12 @@ py_binary( deps = [":proto_to_markdown_lib"], ) +py_library( + name = "sphinx_build_lib", + srcs = ["sphinx_build.py"], + visibility = ["//:__subpackages__"], +) + py_library( name = "proto_to_markdown_lib", srcs = ["proto_to_markdown.py"], diff --git a/sphinxdocs/sphinxdocs/private/sphinx.bzl b/sphinxdocs/sphinxdocs/private/sphinx.bzl index b7c051a154..c42b42ebc6 100644 --- a/sphinxdocs/sphinxdocs/private/sphinx.bzl +++ b/sphinxdocs/sphinxdocs/private/sphinx.bzl @@ -360,12 +360,17 @@ def _sphinx_source_tree_impl(ctx): dest_path = paths.join(source_prefix, dest_path) if source_file.is_directory: dest_file = ctx.actions.declare_directory(dest_path) + progress_message = "Symlinking Sphinx source directory %{input} to %{output}" else: dest_file = ctx.actions.declare_file(dest_path) + progress_message = "Symlinking Sphinx source %{input} to %{output}" + + # NOTE: Sphinx/MyST will read through symlinks, which can break relative + # xref lookup. Files are copied during the action phase to prevent this. ctx.actions.symlink( output = dest_file, target_file = source_file, - progress_message = "Symlinking Sphinx source %{input} to %{output}", + progress_message = progress_message, ) sphinx_source_files.append(dest_file) return dest_file diff --git a/sphinxdocs/sphinxdocs/private/sphinx_build.py b/sphinxdocs/sphinxdocs/private/sphinx_build.py index 8b0eec8fd3..52a334d9b9 100644 --- a/sphinxdocs/sphinxdocs/private/sphinx_build.py +++ b/sphinxdocs/sphinxdocs/private/sphinx_build.py @@ -1,10 +1,14 @@ +import concurrent.futures import contextlib import io import json import logging import os +import pathlib import shutil +import stat import sys +import threading import traceback import typing @@ -29,7 +33,171 @@ def __init__(self, message, exit_code): _REQUEST_INFO_CONFIG_NAME = "bazel_worker_request_info_path" +class DirectorySyncerError(Exception): + """Raised when one or more errors occur during directory synchronization.""" + + def __init__(self, errors: typing.List[BaseException]): + self.errors = errors + message = f"Encountered {len(errors)} error(s) during sync:\n" + "\n".join( + f" - {e}" for e in errors + ) + super().__init__(message) + + +class DirectorySyncer: + """Synchronizes a working destination directory from a source directory. + + Supports concurrent SHA-aware incremental updates (via sync()) for worker + mode and concurrent full directory copying (via copytree()) for non-worker + mode, ensuring physical file materialization to prevent relative + cross-reference resolution failures. + """ + + def __init__( + self, + srcdir: pathlib.Path, + destdir: pathlib.Path, + max_workers: typing.Optional[int] = None, + ): + self._srcdir = srcdir + self._destdir = destdir + self._max_workers = max_workers or min(32, (os.cpu_count() or 4) + 4) + self._current_shas: typing.Dict[str, str] = {} + self._lock = threading.Lock() + self._finished_cond = threading.Condition(self._lock) + self._remaining = 0 + self._errors: typing.List[BaseException] = [] + self._executor: typing.Optional[concurrent.futures.ThreadPoolExecutor] = None + + def _reset_state(self) -> None: + with self._lock: + self._errors.clear() + self._remaining = 0 + + def _wait_for_completion(self) -> None: + with self._lock: + while self._remaining > 0: + self._finished_cond.wait() + if self._errors: + raise DirectorySyncerError(list(self._errors)) + + def _submit_task(self, fn, *args) -> None: + with self._lock: + self._remaining += 1 + future = self._executor.submit(fn, *args) + future.add_done_callback(self._handle_task_done) + + def _handle_task_done(self, future: concurrent.futures.Future) -> None: + exc = future.exception() + if exc: + with self._lock: + self._errors.append(exc) + + def _task_finished(self) -> None: + with self._lock: + self._remaining -= 1 + if self._remaining == 0: + self._finished_cond.notify_all() + + @contextlib.contextmanager + def _create_executor(self): + with concurrent.futures.ThreadPoolExecutor( + max_workers=self._max_workers + ) as executor: + self._executor = executor + try: + yield + finally: + self._executor = None + + def copytree(self) -> None: + """Concurrently copies srcdir to destdir without SHA tracking.""" + self._reset_state() + shutil.rmtree(self._destdir, ignore_errors=True) + with self._create_executor(): + self._submit_task(self._copy_dir, self._srcdir, self._destdir) + self._wait_for_completion() + + def sync(self, entries: typing.Dict[str, str]) -> None: + """Synchronizes destdir to match entries {relative_path: sha} concurrently.""" + self._reset_state() + + to_remove = set(self._current_shas.keys()) - set(entries.keys()) + to_copy = { + path: sha + for path, sha in entries.items() + if self._current_shas.get(path) != sha + } + + if not to_remove and not to_copy: + self._current_shas = dict(entries) + return + + with self._create_executor(): + # 1. Submit stale path removals concurrently ASAP + for rel_path in to_remove: + dest_path = self._destdir / rel_path + self._submit_task(self._remove_path, dest_path) + + # 2. Submit created/updated item copies concurrently ASAP + for rel_path in to_copy: + src_path = self._srcdir / rel_path + dest_path = self._destdir / rel_path + if src_path.is_dir(): + self._submit_task(self._copy_dir, src_path, dest_path) + else: + self._submit_task(self._copy_file, src_path, dest_path) + + self._wait_for_completion() + + self._current_shas = dict(entries) + + def _remove_path(self, dest_path: pathlib.Path) -> None: + try: + if dest_path.is_dir() and not dest_path.is_symlink(): + shutil.rmtree(dest_path) + else: + dest_path.unlink(missing_ok=True) + except BaseException as e: + e.add_note(f"Failed removing path: dest_path={dest_path}") + raise + finally: + self._task_finished() + + def _copy_file(self, src: pathlib.Path, dest: pathlib.Path) -> None: + try: + dest.parent.mkdir(parents=True, exist_ok=True) + shutil.copy2(src, dest) + mode = dest.stat().st_mode + dest.chmod(mode | stat.S_IWUSR) + except BaseException as e: + e.add_note(f"Failed copying file: src={src}, dest={dest}") + raise + finally: + self._task_finished() + + def _copy_dir(self, src: pathlib.Path, dest: pathlib.Path) -> None: + """Recursively creates destination directory and submits tasks for its entries.""" + try: + dest.mkdir(parents=True, exist_ok=True) + with os.scandir(src) as scanner: + for entry in scanner: + c_dest = dest / entry.name + c_src = pathlib.Path(entry.path) + if entry.is_dir(): + self._submit_task(self._copy_dir, c_src, c_dest) + else: + self._submit_task(self._copy_file, c_src, c_dest) + except BaseException as e: + e.add_note(f"Failed copying directory: src={src}, dest={dest}") + raise + finally: + self._task_finished() + + class Worker: + """A Bazel persistent worker for Sphinx builds.""" + def __init__( self, instream: "typing.TextIO", outstream: "typing.TextIO", exec_root: str ): @@ -51,6 +219,7 @@ def __init__( # dict[str srcdir, dict[str path, str digest]] self._digests = {} + self._syncers: typing.Dict[pathlib.Path, DirectorySyncer] = {} # Internal output directories the worker gives to Sphinx that need # to be cleaned up upon exit. @@ -124,18 +293,25 @@ def _send_response(self, response: "WorkResponse") -> None: def _prepare_sphinx(self, request): sphinx_args = request["arguments"] - srcdir = sphinx_args[0] + srcdir = pathlib.Path(sphinx_args[0]) + destdir = pathlib.Path(f"{srcdir}.worker-in.d") incoming_digests = {} - current_digests = self._digests.setdefault(srcdir, {}) + current_digests = self._digests.setdefault(str(srcdir), {}) is_first_request = not current_digests changed_paths = [] request_info = {"exec_root": self._exec_root, "inputs": request["inputs"]} + srcdir_prefix = str(srcdir) + "/" for entry in request["inputs"]: path = entry["path"] + # In persistent worker mode, request["inputs"] includes action-level + # tools (e.g. sphinx-build, sphinx_build.py) and params files that + # live outside srcdir. Only synchronize documentation sources inside srcdir. + if not path.startswith(srcdir_prefix): + continue digest = entry["digest"] # Make the path srcdir-relative so Sphinx understands it. - path = path.removeprefix(srcdir + "/") + path = path.removeprefix(srcdir_prefix) incoming_digests[path] = digest if path not in current_digests: @@ -145,31 +321,7 @@ def _prepare_sphinx(self, request): logger.info("path %s changed", path) changed_paths.append(path) - # Remove any source files that were tracked in the previous request (`current_digests`) - # but are missing from the current `request["inputs"]` (`incoming_digests`). - # Across incremental branch switches or file removals, if these stale symlinks - # remain in `srcdir` on disk, Sphinx will discover broken/unreadable files during - # `find_files()` and abort with "WARNING: Ignored unreadable document" (fatal with -W). - for path in set(current_digests) - set(incoming_digests): - removed_path = os.path.join(srcdir, path) - if os.path.exists(removed_path) or os.path.islink(removed_path): - logger.info("removing stale source file %s", removed_path) - try: - if os.path.islink(removed_path): - try: - os.remove(removed_path) - except OSError: - os.rmdir(removed_path) - elif os.path.isdir(removed_path): - shutil.rmtree(removed_path) - else: - os.remove(removed_path) - except OSError as e: - logger.warning( - "failed to remove stale source %s: %s", removed_path, e - ) - - self._digests[srcdir] = incoming_digests + self._digests[str(srcdir)] = incoming_digests self._extension.changed_paths = changed_paths request_info["changed_sources"] = changed_paths @@ -184,13 +336,22 @@ def _prepare_sphinx(self, request): # failures. So on the first request start from a clean slate. if is_first_request: shutil.rmtree(worker_outdir, ignore_errors=True) + shutil.rmtree(destdir, ignore_errors=True) for arg in sphinx_args: if arg.startswith("--doctree-dir="): shutil.rmtree(arg.partition("=")[2], ignore_errors=True) self._worker_outdirs.add(worker_outdir) sphinx_args[1] = worker_outdir - request_info_path = os.path.join(srcdir, "_bazel_worker_request_info.json") + if srcdir not in self._syncers: + self._syncers[srcdir] = DirectorySyncer(srcdir, destdir) + syncer = self._syncers[srcdir] + syncer.sync(incoming_digests) + + sphinx_args[0] = str(destdir) + request_info_path = os.path.join( + sphinx_args[0], "_bazel_worker_request_info.json" + ) with open(request_info_path, "w") as fp: json.dump(request_info, fp) sphinx_args.append(f"--define={_REQUEST_INFO_CONFIG_NAME}={request_info_path}") @@ -341,6 +502,11 @@ def _non_worker_main(): args.extend(lines) else: args.append(arg) + if len(args) > 1: + srcdir = pathlib.Path(args[1]) + destdir = pathlib.Path(f"{srcdir}.worker-in.d") + DirectorySyncer(srcdir, destdir).copytree() + args[1] = str(destdir) sys.argv[:] = args return main() diff --git a/sphinxdocs/tests/sphinx_build/BUILD.bazel b/sphinxdocs/tests/sphinx_build/BUILD.bazel new file mode 100644 index 0000000000..b9e77220df --- /dev/null +++ b/sphinxdocs/tests/sphinx_build/BUILD.bazel @@ -0,0 +1,11 @@ +load("@rules_python//python:py_test.bzl", "py_test") + +py_test( + name = "directory_syncer_test", + srcs = ["directory_syncer_test.py"], + deps = [ + "//sphinxdocs/private:sphinx_build_lib", + "@dev_pip//absl_py", + "@dev_pip//sphinx", + ], +) diff --git a/sphinxdocs/tests/sphinx_build/directory_syncer_test.py b/sphinxdocs/tests/sphinx_build/directory_syncer_test.py new file mode 100644 index 0000000000..1365e2b3ce --- /dev/null +++ b/sphinxdocs/tests/sphinx_build/directory_syncer_test.py @@ -0,0 +1,133 @@ +import pathlib +import shutil +import stat +import tempfile + +from absl.testing import absltest +from sphinxdocs.private.sphinx_build import DirectorySyncer, DirectorySyncerError + + +class DirectorySyncerTest(absltest.TestCase): + def setUp(self): + super().setUp() + self.test_dir = pathlib.Path(tempfile.mkdtemp()) + self.addCleanup(shutil.rmtree, self.test_dir, ignore_errors=True) + self.srcdir = self.test_dir / "src" + self.destdir = self.test_dir / "dest" + self.srcdir.mkdir() + + def _write_src(self, rel_path, content, mode=None): + path = self.srcdir / rel_path + path.parent.mkdir(parents=True, exist_ok=True) + if path.exists(): + path.chmod(stat.S_IRUSR | stat.S_IWUSR | stat.S_IRGRP | stat.S_IROTH) + path.write_text(content) + if mode is not None: + path.chmod(mode) + return path + + def assert_dest_equals(self, rel_path, expected_content): + dest_file = self.destdir / rel_path + self.assertTrue(dest_file.exists(), f"Expected {dest_file} to exist") + self.assertEqual(expected_content, dest_file.read_text()) + + def assert_dest_not_exists(self, rel_path): + dest_file = self.destdir / rel_path + self.assertFalse(dest_file.exists(), f"Expected {dest_file} to not exist") + + def test_copytree(self): + self._write_src("file1.txt", "hello") + self._write_src("sub/file2.txt", "world") + + syncer = DirectorySyncer(self.srcdir, self.destdir) + syncer.copytree() + + self.assert_dest_equals("file1.txt", "hello") + self.assert_dest_equals("sub/file2.txt", "world") + + def test_sync_initial_and_incremental(self): + self._write_src("doc1.md", "v1") + self._write_src("doc2.md", "v1") + self._write_src("doc3.md", "v1") + + syncer = DirectorySyncer(self.srcdir, self.destdir) + # Initial sync + syncer.sync( + { + "doc1.md": "sha-doc1-v1", + "doc2.md": "sha-doc2-v1", + "doc3.md": "sha-doc3-v1", + } + ) + + self.assert_dest_equals("doc1.md", "v1") + self.assert_dest_equals("doc2.md", "v1") + self.assert_dest_equals("doc3.md", "v1") + + # Incremental sync: + # - doc1.md: unchanged SHA + # - doc2.md: updated SHA & content + # - doc3.md: removed from entries + # - doc4.md: newly created file + self._write_src("doc2.md", "v2") + self._write_src("doc4.md", "v1") + + syncer.sync( + { + "doc1.md": "sha-doc1-v1", + "doc2.md": "sha-doc2-v2", + "doc4.md": "sha-doc4-v1", + } + ) + + self.assert_dest_equals("doc1.md", "v1") + self.assert_dest_equals("doc2.md", "v2") + self.assert_dest_not_exists("doc3.md") + self.assert_dest_equals("doc4.md", "v1") + + def test_read_only_file_becomes_writable(self): + read_only_mode = stat.S_IRUSR | stat.S_IRGRP | stat.S_IROTH + self._write_src("readonly.txt", "version 1", mode=read_only_mode) + syncer = DirectorySyncer(self.srcdir, self.destdir) + syncer.sync({"readonly.txt": "sha-v1"}) + + dest_file = self.destdir / "readonly.txt" + self.assert_dest_equals("readonly.txt", "version 1") + self.assertTrue( + dest_file.stat().st_mode & stat.S_IWUSR, + "Destination file should be writable", + ) + + # Ensure subsequent incremental updates can overwrite the file without permission error + self._write_src("readonly.txt", "version 2", mode=read_only_mode) + syncer.sync({"readonly.txt": "sha-v2"}) + self.assert_dest_equals("readonly.txt", "version 2") + + def test_sync_directory_artifact(self): + dir_artifact = self.srcdir / "tree_art" + dir_artifact.mkdir() + (dir_artifact / "page.md").write_text("content") + + syncer = DirectorySyncer(self.srcdir, self.destdir) + syncer.sync( + { + "tree_art": "sha-dir-art", + } + ) + + self.assert_dest_equals("tree_art/page.md", "content") + + def test_errors_bubble_up(self): + syncer = DirectorySyncer(self.srcdir, self.destdir) + with self.assertRaises(DirectorySyncerError) as cm: + syncer.sync( + { + "non_existent_1.txt": "sha1", + "non_existent_2.txt": "sha2", + } + ) + self.assertGreaterEqual(len(cm.exception.errors), 2) + + +if __name__ == "__main__": + absltest.main() diff --git a/sphinxdocs/tests/sphinx_docs/BUILD.bazel b/sphinxdocs/tests/sphinx_docs/BUILD.bazel index 33b98ec585..4bbaf90691 100644 --- a/sphinxdocs/tests/sphinx_docs/BUILD.bazel +++ b/sphinxdocs/tests/sphinx_docs/BUILD.bazel @@ -1,4 +1,5 @@ load("@bazel_skylib//rules:build_test.bzl", "build_test") +load("@rules_python//python:py_test.bzl", "py_test") load("//sphinxdocs:sphinx.bzl", "sphinx_build_binary", "sphinx_docs") load(":defs.bzl", "gen_directory") @@ -21,10 +22,20 @@ sphinx_docs( ], config = "conf.py", formats = ["html"], + renamed_srcs = { + ":gen_binary_asset": "binary_asset.bin", + }, sphinx = ":sphinx-build", + strip_prefix = package_name() + "/", target_compatible_with = _TARGET_COMPATIBLE_WITH, ) +genrule( + name = "gen_binary_asset", + outs = ["binary_asset.bin"], + cmd = "printf '\\x00\\xff\\xfe\\xfd\\x80\\x90\\x00\\x01\\x02\\x03\\x04' > $@", +) + gen_directory( name = "generated_directory", ) @@ -42,3 +53,10 @@ build_test( name = "docs_build_test", targets = [":docs"], ) + +py_test( + name = "sphinx_docs_output_test", + srcs = ["sphinx_docs_output_test.py"], + data = [":docs"], + deps = ["@dev_pip//absl_py"], +) diff --git a/sphinxdocs/tests/sphinx_docs/defs.bzl b/sphinxdocs/tests/sphinx_docs/defs.bzl index 2e47ecc0f7..36fd1dba4f 100644 --- a/sphinxdocs/tests/sphinx_docs/defs.bzl +++ b/sphinxdocs/tests/sphinx_docs/defs.bzl @@ -6,7 +6,9 @@ def _gen_directory_impl(ctx): ctx.actions.run_shell( outputs = [out], command = """ -echo "# Hello" > {outdir}/index.md +printf '# Hello\\n' > {outdir}/index.md +printf '# Dir Page 1\\n\\n[Dir Page 2](dir_page2.md)\\n' > {outdir}/dir_page1.md +printf '# Dir Page 2\\n' > {outdir}/dir_page2.md """.format( outdir = out.path, ), diff --git a/sphinxdocs/tests/sphinx_docs/index.md b/sphinxdocs/tests/sphinx_docs/index.md index cdce641fa1..68a5fb38c3 100644 --- a/sphinxdocs/tests/sphinx_docs/index.md +++ b/sphinxdocs/tests/sphinx_docs/index.md @@ -3,6 +3,7 @@ :::{toctree} :glob: +generated_directory/dir_page1 ** genindex ::: diff --git a/sphinxdocs/tests/sphinx_docs/sphinx_docs_output_test.py b/sphinxdocs/tests/sphinx_docs/sphinx_docs_output_test.py new file mode 100644 index 0000000000..5d00817926 --- /dev/null +++ b/sphinxdocs/tests/sphinx_docs/sphinx_docs_output_test.py @@ -0,0 +1,28 @@ +import importlib.resources +import os +from xml.etree import ElementTree + +import tests.sphinx_docs as sphinx_docs +from absl.testing import absltest + + +class SphinxDocsOutputTest(absltest.TestCase): + def test_directory_artifact_relative_xref(self): + page1_path = importlib.resources.files(sphinx_docs).joinpath( + "docs/_build/html/generated_directory/dir_page1.html" + ) + self.assertTrue(os.path.exists(str(page1_path)), f"Not found at {page1_path}") + with open(str(page1_path)) as f: + xml = f.read() + doc_elem = ElementTree.fromstring(xml) + actual = None + for elem in doc_elem.iter(): + if "href" in elem.attrib: + if "".join(elem.itertext()).strip() == "Dir Page 2": + actual = elem.attrib["href"] + break + self.assertEqual("dir_page2.html", actual) + + +if __name__ == "__main__": + absltest.main() diff --git a/sphinxdocs/tests/sphinx_stardoc/sphinx_output_test.py b/sphinxdocs/tests/sphinx_stardoc/sphinx_output_test.py index 650e0134d3..e6653b1656 100644 --- a/sphinxdocs/tests/sphinx_stardoc/sphinx_output_test.py +++ b/sphinxdocs/tests/sphinx_stardoc/sphinx_output_test.py @@ -13,7 +13,10 @@ def setUp(self): self._xmls = {} def assert_xref(self, doc, *, text, href): - match = self._doc_element(doc).find(f".//*[.='{text}']") + # Find an element with an 'href' attribute whose string content (including + # descendants like ) equals `text`. [@href] filters out ancestor + # elements like
  • or

    which also match [.='{text}']. + match = self._doc_element(doc).find(f".//*[@href][.='{text}']") if not match: self.fail(f"No element found with {text=}") actual = match.attrib.get("href", "") @@ -113,6 +116,7 @@ def _doc_element(self, doc): ("file_with_repo", "@testrepo//lang:rule.bzl", "rule.html"), ("package_absolute", "//lang", "target.html"), ("package_basename", "lang", "target.html"), + ("relative_doc_link", "Rule documentation", "rule.html"), # fmt: on ) def test_xrefs(self, text, href): diff --git a/sphinxdocs/tests/sphinx_stardoc/xrefs.md b/sphinxdocs/tests/sphinx_stardoc/xrefs.md index 9893c32023..e3a7d7b21e 100644 --- a/sphinxdocs/tests/sphinx_stardoc/xrefs.md +++ b/sphinxdocs/tests/sphinx_stardoc/xrefs.md @@ -7,6 +7,7 @@ Various tests of cross referencing support ## Short name +* [Rule documentation](rule.md) * function: {obj}`myfunc` * function arg: {obj}`myfunc.arg1` * rule: {obj}`my_rule`