From 9e89ecab3d69db4452e57ebf14d3cc028e5df813 Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Mon, 3 Aug 2026 06:04:13 +0000 Subject: [PATCH 01/13] sphinxdocs: add reproduction test case for relative markdown xref resolution bug (#3977) --- sphinxdocs/tests/sphinx_stardoc/xrefs.md | 1 + 1 file changed, 1 insertion(+) 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` From 5724a6ce53f021b8cb36172923d8724a2d815f4d Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Mon, 3 Aug 2026 06:16:50 +0000 Subject: [PATCH 02/13] sphinxdocs: add automated test asserting relative markdown xref resolution (#3977) --- sphinxdocs/tests/sphinx_stardoc/sphinx_output_test.py | 1 + 1 file changed, 1 insertion(+) diff --git a/sphinxdocs/tests/sphinx_stardoc/sphinx_output_test.py b/sphinxdocs/tests/sphinx_stardoc/sphinx_output_test.py index 650e0134d3..8fff5b42f5 100644 --- a/sphinxdocs/tests/sphinx_stardoc/sphinx_output_test.py +++ b/sphinxdocs/tests/sphinx_stardoc/sphinx_output_test.py @@ -113,6 +113,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): From 00c81cfd4cede9990f65fcf3db67b86aa9917f32 Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Mon, 3 Aug 2026 06:25:41 +0000 Subject: [PATCH 03/13] test(sphinxdocs): improve assert_xref to match elements with href attributes directly --- sphinxdocs/tests/sphinx_stardoc/sphinx_output_test.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/sphinxdocs/tests/sphinx_stardoc/sphinx_output_test.py b/sphinxdocs/tests/sphinx_stardoc/sphinx_output_test.py index 8fff5b42f5..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", "") From 87c555ab5a234d166a8abf1e3f68648694acbf55 Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Mon, 3 Aug 2026 06:36:15 +0000 Subject: [PATCH 04/13] test(sphinxdocs): add test coverage for directory artifact relative xrefs and binary assets (#3977) --- sphinxdocs/tests/sphinx_docs/BUILD.bazel | 18 ++++++++++++ sphinxdocs/tests/sphinx_docs/defs.bzl | 4 ++- sphinxdocs/tests/sphinx_docs/index.md | 1 + .../sphinx_docs/sphinx_docs_output_test.py | 28 +++++++++++++++++++ 4 files changed, 50 insertions(+), 1 deletion(-) create mode 100644 sphinxdocs/tests/sphinx_docs/sphinx_docs_output_test.py 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() From 2b468108a7a8be2682128788ceae5903d03121c6 Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Mon, 3 Aug 2026 15:35:15 +0000 Subject: [PATCH 05/13] sphinxdocs: materialize source symlinks concurrently into work-private directory Sphinx/MyST relative cross-reference resolution fails when input files are symlinks because os.path.realpath() resolves outside the source directory. Add ConcurrentCopyTree in sphinx_build.py to recursively copy the source tree into a work-private '{srcdir}.worker-in.d' directory concurrently using a ThreadPoolExecutor before invoking Sphinx. --- sphinxdocs/sphinxdocs/private/sphinx.bzl | 7 +- sphinxdocs/sphinxdocs/private/sphinx_build.py | 91 ++++++++++++++++++- 2 files changed, 95 insertions(+), 3 deletions(-) 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..26ad2bedbb 100644 --- a/sphinxdocs/sphinxdocs/private/sphinx_build.py +++ b/sphinxdocs/sphinxdocs/private/sphinx_build.py @@ -1,3 +1,4 @@ +import concurrent.futures import contextlib import io import json @@ -5,6 +6,7 @@ import os import shutil import sys +import threading import traceback import typing @@ -29,6 +31,81 @@ def __init__(self, message, exit_code): _REQUEST_INFO_CONFIG_NAME = "bazel_worker_request_info_path" +class ConcurrentCopyTree: + def __init__( + self, + max_workers: typing.Optional[int] = None, + ): + self._max_workers = max_workers or min(32, (os.cpu_count() or 4) + 4) + self._remaining = 0 + self._lock = threading.Lock() + self._finished_cond = threading.Condition(self._lock) + self._errors: typing.List[BaseException] = [] + self._executor: typing.Optional[concurrent.futures.ThreadPoolExecutor] = None + + def copytree(self, srcdir: str, destdir: str) -> None: + shutil.rmtree(destdir, ignore_errors=True) + with concurrent.futures.ThreadPoolExecutor( + max_workers=self._max_workers + ) as executor: + self._executor = executor + self._submit_task(self._copy_dir, srcdir, destdir) + with self._lock: + while self._remaining > 0: + self._finished_cond.wait() + if self._errors: + raise self._errors[0] + + def _submit_task(self, fn, *args) -> None: + with self._lock: + self._remaining += 1 + future = self._executor.submit(fn, *args) + future.add_done_callback(self._check_exception) + + def _check_exception(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() + + def _copy_file(self, src: str, dest: str) -> None: + try: + shutil.copy2(src, dest) + 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: str, dest: str) -> None: + try: + os.mkdir(dest) + real_src = os.path.realpath(src) if os.path.islink(src) else src + with os.scandir(real_src) as scanner: + for entry in scanner: + c_dest = os.path.join(dest, entry.name) + c_src = entry.path + if entry.is_dir(): + self._submit_task(self._copy_dir, c_src, c_dest) + else: + self._submit_task( + self._copy_file, + os.path.realpath(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: def __init__( self, instream: "typing.TextIO", outstream: "typing.TextIO", exec_root: str @@ -189,8 +266,13 @@ def _prepare_sphinx(self, request): 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") + srcdir = sphinx_args[0] + destdir = f"{srcdir}.worker-in.d" + ConcurrentCopyTree().copytree(srcdir, destdir) + sphinx_args[0] = 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 +423,11 @@ def _non_worker_main(): args.extend(lines) else: args.append(arg) + if len(args) > 1: + srcdir = args[1] + destdir = f"{srcdir}.worker-in.d" + ConcurrentCopyTree().copytree(srcdir, destdir) + args[1] = destdir sys.argv[:] = args return main() From 93dad97fb6744ecd644f1c2d9c8d0f9f9e5837bf Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Tue, 4 Aug 2026 04:10:40 +0000 Subject: [PATCH 06/13] sphinxdocs: add SHA-aware concurrent DirectorySyncer for source materialization Sphinx/MyST relative cross-reference resolution fails when input files are symlinks because os.path.realpath() resolves outside the source directory. Add multi-threaded DirectorySyncer in sphinx_build.py supporting SHA-aware incremental sync(entries) and non-worker copytree(), forcing copied files to be user-writable and collecting errors into DirectorySyncerError. Add comprehensive unit test suite in tests/sphinx_build/. --- sphinxdocs/sphinxdocs/private/BUILD.bazel | 6 + sphinxdocs/sphinxdocs/private/sphinx_build.py | 189 ++++++++++++------ sphinxdocs/tests/sphinx_build/BUILD.bazel | 11 + .../sphinx_build/directory_syncer_test.py | 133 ++++++++++++ 4 files changed, 275 insertions(+), 64 deletions(-) create mode 100644 sphinxdocs/tests/sphinx_build/BUILD.bazel create mode 100644 sphinxdocs/tests/sphinx_build/directory_syncer_test.py 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_build.py b/sphinxdocs/sphinxdocs/private/sphinx_build.py index 26ad2bedbb..e7b3f2f07b 100644 --- a/sphinxdocs/sphinxdocs/private/sphinx_build.py +++ b/sphinxdocs/sphinxdocs/private/sphinx_build.py @@ -4,7 +4,9 @@ import json import logging import os +import pathlib import shutil +import stat import sys import threading import traceback @@ -31,38 +33,53 @@ def __init__(self, message, exit_code): _REQUEST_INFO_CONFIG_NAME = "bazel_worker_request_info_path" -class ConcurrentCopyTree: +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: 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._remaining = 0 + 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 copytree(self, srcdir: str, destdir: str) -> None: - shutil.rmtree(destdir, ignore_errors=True) - with concurrent.futures.ThreadPoolExecutor( - max_workers=self._max_workers - ) as executor: - self._executor = executor - self._submit_task(self._copy_dir, srcdir, destdir) - with self._lock: - while self._remaining > 0: - self._finished_cond.wait() - if self._errors: - raise self._errors[0] + 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._check_exception) + future.add_done_callback(self._handle_task_done) - def _check_exception(self, future: concurrent.futures.Future) -> None: + def _handle_task_done(self, future: concurrent.futures.Future) -> None: exc = future.exception() if exc: with self._lock: @@ -74,31 +91,91 @@ def _task_finished(self) -> None: if self._remaining == 0: self._finished_cond.notify_all() - def _copy_file(self, src: str, dest: str) -> None: + def copytree(self) -> None: + """Concurrently copies srcdir to destdir without SHA tracking.""" + self._reset_state() + shutil.rmtree(self._destdir, ignore_errors=True) + with concurrent.futures.ThreadPoolExecutor( + max_workers=self._max_workers + ) as executor: + self._executor = 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 concurrent.futures.ThreadPoolExecutor( + max_workers=self._max_workers + ) as executor: + self._executor = 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: str, dest: str) -> None: + def _copy_dir(self, src: pathlib.Path, dest: pathlib.Path) -> None: + """Recursively creates destination directory and submits tasks for its entries.""" try: - os.mkdir(dest) - real_src = os.path.realpath(src) if os.path.islink(src) else src - with os.scandir(real_src) as scanner: + dest.mkdir(parents=True, exist_ok=True) + with os.scandir(src) as scanner: for entry in scanner: - c_dest = os.path.join(dest, entry.name) - c_src = entry.path + 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, - os.path.realpath(c_src), - c_dest, - ) + 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 @@ -107,6 +184,8 @@ def _copy_dir(self, src: str, dest: str) -> None: class Worker: + """A Bazel persistent worker for Sphinx builds.""" + def __init__( self, instream: "typing.TextIO", outstream: "typing.TextIO", exec_root: str ): @@ -128,6 +207,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. @@ -201,10 +281,11 @@ 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"]} @@ -212,7 +293,7 @@ def _prepare_sphinx(self, request): path = entry["path"] digest = entry["digest"] # Make the path srcdir-relative so Sphinx understands it. - path = path.removeprefix(srcdir + "/") + path = path.removeprefix(str(srcdir) + "/") incoming_digests[path] = digest if path not in current_digests: @@ -222,31 +303,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 @@ -261,15 +318,19 @@ 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 - srcdir = sphinx_args[0] - destdir = f"{srcdir}.worker-in.d" - ConcurrentCopyTree().copytree(srcdir, destdir) - sphinx_args[0] = destdir + + 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" ) @@ -424,10 +485,10 @@ def _non_worker_main(): else: args.append(arg) if len(args) > 1: - srcdir = args[1] - destdir = f"{srcdir}.worker-in.d" - ConcurrentCopyTree().copytree(srcdir, destdir) - args[1] = destdir + 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() From 1cc168f7c73314b895db6018dacbc8fac9d58839 Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Wed, 5 Aug 2026 06:12:04 +0000 Subject: [PATCH 07/13] sphinxdocs: refine DirectorySyncer with pathlib, stat constants, and _create_executor helper Improve DirectorySyncer in sphinx_build.py by using pathlib.Path objects directly, replacing numeric permission literals with stat module constants, making copied files user-writable via stat.S_IWUSR, and factoring ThreadPoolExecutor creation into a _create_executor context manager. --- sphinxdocs/sphinxdocs/private/sphinx_build.py | 30 +++++++++++++------ 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/sphinxdocs/sphinxdocs/private/sphinx_build.py b/sphinxdocs/sphinxdocs/private/sphinx_build.py index e7b3f2f07b..332631fab6 100644 --- a/sphinxdocs/sphinxdocs/private/sphinx_build.py +++ b/sphinxdocs/sphinxdocs/private/sphinx_build.py @@ -45,6 +45,14 @@ def __init__(self, errors: typing.List[BaseException]): 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, @@ -91,14 +99,22 @@ def _task_finished(self) -> None: if self._remaining == 0: self._finished_cond.notify_all() - def copytree(self) -> None: - """Concurrently copies srcdir to destdir without SHA tracking.""" - self._reset_state() - shutil.rmtree(self._destdir, ignore_errors=True) + @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() @@ -117,11 +133,7 @@ def sync(self, entries: typing.Dict[str, str]) -> None: self._current_shas = dict(entries) return - with concurrent.futures.ThreadPoolExecutor( - max_workers=self._max_workers - ) as executor: - self._executor = executor - + with self._create_executor(): # 1. Submit stale path removals concurrently ASAP for rel_path in to_remove: dest_path = self._destdir / rel_path From 7c47b0c5d90353dfa24e3d66ef12cee15d9ee358 Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Wed, 5 Aug 2026 06:32:32 +0000 Subject: [PATCH 08/13] fix(sphinxdocs): include transitive original sources in worker action inputs --- news/3977.fixed.md | 3 +++ sphinxdocs/sphinxdocs/private/sphinx.bzl | 15 ++++++++++++++- sphinxdocs/sphinxdocs/private/sphinx_build.py | 5 ++++- 3 files changed, 21 insertions(+), 2 deletions(-) create mode 100644 news/3977.fixed.md diff --git a/news/3977.fixed.md b/news/3977.fixed.md new file mode 100644 index 0000000000..8794921726 --- /dev/null +++ b/news/3977.fixed.md @@ -0,0 +1,3 @@ +(sphinxdocs) Fixed relative cross-reference resolution when source files +are symlinks by materializing source directories concurrently in a +work-private location ([#3977](https://github.com/bazel-contrib/rules_python/issues/3977)). diff --git a/sphinxdocs/sphinxdocs/private/sphinx.bzl b/sphinxdocs/sphinxdocs/private/sphinx.bzl index c42b42ebc6..8b59924590 100644 --- a/sphinxdocs/sphinxdocs/private/sphinx.bzl +++ b/sphinxdocs/sphinxdocs/private/sphinx.bzl @@ -35,6 +35,11 @@ Runfiles-root relative path of the root directory for the source files. :type: str Exec-root relative path of the root directory for the source files (which are in DefaultInfo.files) +""", + "transitive_sources": """ +:type: depset[File] + +Original target files for the source tree symlinks. """, }, ) @@ -204,7 +209,12 @@ def sphinx_docs( def _sphinx_docs_impl(ctx): source_tree_info = ctx.attr.source_tree[_SphinxSourceTreeInfo] source_dir_path = source_tree_info.source_root - inputs = ctx.attr.source_tree[DefaultInfo].files + inputs = depset( + transitive = [ + ctx.attr.source_tree[DefaultInfo].files, + source_tree_info.transitive_sources, + ], + ) per_format_args = {} outputs = {} @@ -351,9 +361,11 @@ def _sphinx_source_tree_impl(ctx): # we need to merge the two into a single directory. source_prefix = ctx.label.name sphinx_source_files = [] + transitive_sources = [] # Materialize a file under the `_sources` dir def _relocate(source_file, dest_path = None): + transitive_sources.append(source_file) if not dest_path: dest_path = source_file.short_path.removeprefix(ctx.attr.strip_prefix) @@ -416,6 +428,7 @@ def _sphinx_source_tree_impl(ctx): _SphinxSourceTreeInfo( source_root = sphinx_source_dir_path, source_dir_runfiles_path = paths.dirname(source_conf_file.short_path), + transitive_sources = depset(transitive_sources), ), ] diff --git a/sphinxdocs/sphinxdocs/private/sphinx_build.py b/sphinxdocs/sphinxdocs/private/sphinx_build.py index 332631fab6..833b463833 100644 --- a/sphinxdocs/sphinxdocs/private/sphinx_build.py +++ b/sphinxdocs/sphinxdocs/private/sphinx_build.py @@ -301,11 +301,14 @@ def _prepare_sphinx(self, request): is_first_request = not current_digests changed_paths = [] request_info = {"exec_root": self._exec_root, "inputs": request["inputs"]} + prefix = str(srcdir) + "/" for entry in request["inputs"]: path = entry["path"] + if not path.startswith(prefix): + continue digest = entry["digest"] # Make the path srcdir-relative so Sphinx understands it. - path = path.removeprefix(str(srcdir) + "/") + path = path.removeprefix(prefix) incoming_digests[path] = digest if path not in current_digests: From 17c1218f53bedaf8f3dba0cd9506066c616314e7 Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Wed, 5 Aug 2026 06:34:18 +0000 Subject: [PATCH 09/13] revert(sphinxdocs): remove news entry --- news/3977.fixed.md | 3 --- 1 file changed, 3 deletions(-) delete mode 100644 news/3977.fixed.md diff --git a/news/3977.fixed.md b/news/3977.fixed.md deleted file mode 100644 index 8794921726..0000000000 --- a/news/3977.fixed.md +++ /dev/null @@ -1,3 +0,0 @@ -(sphinxdocs) Fixed relative cross-reference resolution when source files -are symlinks by materializing source directories concurrently in a -work-private location ([#3977](https://github.com/bazel-contrib/rules_python/issues/3977)). From a2b0cf519eb9ff883c949ae03d1fdc1f2daefe31 Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Wed, 5 Aug 2026 06:35:01 +0000 Subject: [PATCH 10/13] revert(sphinxdocs): revert worker action input changes --- sphinxdocs/sphinxdocs/private/sphinx.bzl | 15 +- sphinxdocs/sphinxdocs/private/sphinx_build.py | 216 ++++++------------ 2 files changed, 66 insertions(+), 165 deletions(-) diff --git a/sphinxdocs/sphinxdocs/private/sphinx.bzl b/sphinxdocs/sphinxdocs/private/sphinx.bzl index 8b59924590..c42b42ebc6 100644 --- a/sphinxdocs/sphinxdocs/private/sphinx.bzl +++ b/sphinxdocs/sphinxdocs/private/sphinx.bzl @@ -35,11 +35,6 @@ Runfiles-root relative path of the root directory for the source files. :type: str Exec-root relative path of the root directory for the source files (which are in DefaultInfo.files) -""", - "transitive_sources": """ -:type: depset[File] - -Original target files for the source tree symlinks. """, }, ) @@ -209,12 +204,7 @@ def sphinx_docs( def _sphinx_docs_impl(ctx): source_tree_info = ctx.attr.source_tree[_SphinxSourceTreeInfo] source_dir_path = source_tree_info.source_root - inputs = depset( - transitive = [ - ctx.attr.source_tree[DefaultInfo].files, - source_tree_info.transitive_sources, - ], - ) + inputs = ctx.attr.source_tree[DefaultInfo].files per_format_args = {} outputs = {} @@ -361,11 +351,9 @@ def _sphinx_source_tree_impl(ctx): # we need to merge the two into a single directory. source_prefix = ctx.label.name sphinx_source_files = [] - transitive_sources = [] # Materialize a file under the `_sources` dir def _relocate(source_file, dest_path = None): - transitive_sources.append(source_file) if not dest_path: dest_path = source_file.short_path.removeprefix(ctx.attr.strip_prefix) @@ -428,7 +416,6 @@ def _sphinx_source_tree_impl(ctx): _SphinxSourceTreeInfo( source_root = sphinx_source_dir_path, source_dir_runfiles_path = paths.dirname(source_conf_file.short_path), - transitive_sources = depset(transitive_sources), ), ] diff --git a/sphinxdocs/sphinxdocs/private/sphinx_build.py b/sphinxdocs/sphinxdocs/private/sphinx_build.py index 833b463833..7cd3c7c2ca 100644 --- a/sphinxdocs/sphinxdocs/private/sphinx_build.py +++ b/sphinxdocs/sphinxdocs/private/sphinx_build.py @@ -4,9 +4,7 @@ import json import logging import os -import pathlib import shutil -import stat import sys import threading import traceback @@ -33,61 +31,38 @@ 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. - """ - +class ConcurrentCopyTree: 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._remaining = 0 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 copytree(self, srcdir: str, destdir: str) -> None: + shutil.rmtree(destdir, ignore_errors=True) + with concurrent.futures.ThreadPoolExecutor( + max_workers=self._max_workers + ) as executor: + self._executor = executor + self._submit_task(self._copy_dir, srcdir, destdir) + with self._lock: + while self._remaining > 0: + self._finished_cond.wait() + if self._errors: + raise self._errors[0] 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) + future.add_done_callback(self._check_exception) - def _handle_task_done(self, future: concurrent.futures.Future) -> None: + def _check_exception(self, future: concurrent.futures.Future) -> None: exc = future.exception() if exc: with self._lock: @@ -99,95 +74,31 @@ def _task_finished(self) -> None: 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: + def _copy_file(self, src: str, dest: str) -> 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.""" + def _copy_dir(self, src: str, dest: str) -> None: try: - dest.mkdir(parents=True, exist_ok=True) - with os.scandir(src) as scanner: + os.mkdir(dest) + real_src = os.path.realpath(src) if os.path.islink(src) else src + with os.scandir(real_src) as scanner: for entry in scanner: - c_dest = dest / entry.name - c_src = pathlib.Path(entry.path) + c_dest = os.path.join(dest, entry.name) + c_src = 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) + self._submit_task( + self._copy_file, + os.path.realpath(c_src), + c_dest, + ) except BaseException as e: e.add_note(f"Failed copying directory: src={src}, dest={dest}") raise @@ -196,8 +107,6 @@ def _copy_dir(self, src: pathlib.Path, dest: pathlib.Path) -> None: class Worker: - """A Bazel persistent worker for Sphinx builds.""" - def __init__( self, instream: "typing.TextIO", outstream: "typing.TextIO", exec_root: str ): @@ -219,7 +128,6 @@ 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. @@ -293,22 +201,18 @@ def _send_response(self, response: "WorkResponse") -> None: def _prepare_sphinx(self, request): sphinx_args = request["arguments"] - srcdir = pathlib.Path(sphinx_args[0]) - destdir = pathlib.Path(f"{srcdir}.worker-in.d") + srcdir = sphinx_args[0] incoming_digests = {} - current_digests = self._digests.setdefault(str(srcdir), {}) + current_digests = self._digests.setdefault(srcdir, {}) is_first_request = not current_digests changed_paths = [] request_info = {"exec_root": self._exec_root, "inputs": request["inputs"]} - prefix = str(srcdir) + "/" for entry in request["inputs"]: path = entry["path"] - if not path.startswith(prefix): - continue digest = entry["digest"] # Make the path srcdir-relative so Sphinx understands it. - path = path.removeprefix(prefix) + path = path.removeprefix(srcdir + "/") incoming_digests[path] = digest if path not in current_digests: @@ -318,7 +222,31 @@ def _prepare_sphinx(self, request): logger.info("path %s changed", path) changed_paths.append(path) - self._digests[str(srcdir)] = incoming_digests + # 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._extension.changed_paths = changed_paths request_info["changed_sources"] = changed_paths @@ -333,19 +261,15 @@ 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 - - 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) + srcdir = sphinx_args[0] + destdir = f"{srcdir}.worker-in.d" + ConcurrentCopyTree().copytree(srcdir, destdir) + sphinx_args[0] = destdir request_info_path = os.path.join( sphinx_args[0], "_bazel_worker_request_info.json" ) @@ -425,19 +349,9 @@ def _process_request(self, request: "WorkRequest") -> "WorkResponse | None": # implicily bring along what the symlinks point to. shutil.copytree(worker_outdir, bazel_outdir, dirs_exist_ok=True) - # Include both stdout and stderr in the response output so that Sphinx - # warnings or diagnostic messages written to stderr are reported to the - # Bazel console even when the build succeeds. - stdout_output = stdout.getvalue() - stderr_output = stderr.getvalue() - if stderr_output: - output = f"--- STDOUT ---\n{stdout_output}\n--- STDERR ---\n{stderr_output}" - else: - output = stdout_output - response = { "requestId": request.get("requestId", 0), - "output": output, + "output": stdout.getvalue(), "exitCode": 0, } return response @@ -500,10 +414,10 @@ def _non_worker_main(): 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) + srcdir = args[1] + destdir = f"{srcdir}.worker-in.d" + ConcurrentCopyTree().copytree(srcdir, destdir) + args[1] = destdir sys.argv[:] = args return main() From 97f672a36f23a50b84df9e4e5b0a21bc01b07e17 Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Wed, 5 Aug 2026 07:13:56 +0000 Subject: [PATCH 11/13] fix(sphinxdocs): filter action inputs by srcdir prefix in worker mode --- sphinxdocs/sphinxdocs/private/sphinx_build.py | 216 ++++++++++++------ 1 file changed, 151 insertions(+), 65 deletions(-) diff --git a/sphinxdocs/sphinxdocs/private/sphinx_build.py b/sphinxdocs/sphinxdocs/private/sphinx_build.py index 7cd3c7c2ca..833b463833 100644 --- a/sphinxdocs/sphinxdocs/private/sphinx_build.py +++ b/sphinxdocs/sphinxdocs/private/sphinx_build.py @@ -4,7 +4,9 @@ import json import logging import os +import pathlib import shutil +import stat import sys import threading import traceback @@ -31,38 +33,61 @@ def __init__(self, message, exit_code): _REQUEST_INFO_CONFIG_NAME = "bazel_worker_request_info_path" -class ConcurrentCopyTree: +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._remaining = 0 + 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 copytree(self, srcdir: str, destdir: str) -> None: - shutil.rmtree(destdir, ignore_errors=True) - with concurrent.futures.ThreadPoolExecutor( - max_workers=self._max_workers - ) as executor: - self._executor = executor - self._submit_task(self._copy_dir, srcdir, destdir) - with self._lock: - while self._remaining > 0: - self._finished_cond.wait() - if self._errors: - raise self._errors[0] + 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._check_exception) + future.add_done_callback(self._handle_task_done) - def _check_exception(self, future: concurrent.futures.Future) -> None: + def _handle_task_done(self, future: concurrent.futures.Future) -> None: exc = future.exception() if exc: with self._lock: @@ -74,31 +99,95 @@ def _task_finished(self) -> None: if self._remaining == 0: self._finished_cond.notify_all() - def _copy_file(self, src: str, dest: str) -> None: + @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: str, dest: str) -> None: + def _copy_dir(self, src: pathlib.Path, dest: pathlib.Path) -> None: + """Recursively creates destination directory and submits tasks for its entries.""" try: - os.mkdir(dest) - real_src = os.path.realpath(src) if os.path.islink(src) else src - with os.scandir(real_src) as scanner: + dest.mkdir(parents=True, exist_ok=True) + with os.scandir(src) as scanner: for entry in scanner: - c_dest = os.path.join(dest, entry.name) - c_src = entry.path + 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, - os.path.realpath(c_src), - c_dest, - ) + 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 @@ -107,6 +196,8 @@ def _copy_dir(self, src: str, dest: str) -> None: class Worker: + """A Bazel persistent worker for Sphinx builds.""" + def __init__( self, instream: "typing.TextIO", outstream: "typing.TextIO", exec_root: str ): @@ -128,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. @@ -201,18 +293,22 @@ 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"]} + prefix = str(srcdir) + "/" for entry in request["inputs"]: path = entry["path"] + if not path.startswith(prefix): + continue digest = entry["digest"] # Make the path srcdir-relative so Sphinx understands it. - path = path.removeprefix(srcdir + "/") + path = path.removeprefix(prefix) incoming_digests[path] = digest if path not in current_digests: @@ -222,31 +318,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 @@ -261,15 +333,19 @@ 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 - srcdir = sphinx_args[0] - destdir = f"{srcdir}.worker-in.d" - ConcurrentCopyTree().copytree(srcdir, destdir) - sphinx_args[0] = destdir + + 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" ) @@ -349,9 +425,19 @@ def _process_request(self, request: "WorkRequest") -> "WorkResponse | None": # implicily bring along what the symlinks point to. shutil.copytree(worker_outdir, bazel_outdir, dirs_exist_ok=True) + # Include both stdout and stderr in the response output so that Sphinx + # warnings or diagnostic messages written to stderr are reported to the + # Bazel console even when the build succeeds. + stdout_output = stdout.getvalue() + stderr_output = stderr.getvalue() + if stderr_output: + output = f"--- STDOUT ---\n{stdout_output}\n--- STDERR ---\n{stderr_output}" + else: + output = stdout_output + response = { "requestId": request.get("requestId", 0), - "output": stdout.getvalue(), + "output": output, "exitCode": 0, } return response @@ -414,10 +500,10 @@ def _non_worker_main(): else: args.append(arg) if len(args) > 1: - srcdir = args[1] - destdir = f"{srcdir}.worker-in.d" - ConcurrentCopyTree().copytree(srcdir, destdir) - args[1] = destdir + 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() From 50f4b074572b7209285235a429eed8435c55f904 Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Wed, 5 Aug 2026 07:14:07 +0000 Subject: [PATCH 12/13] docs(sphinxdocs): add comment explaining srcdir input filtering in worker mode --- sphinxdocs/sphinxdocs/private/sphinx_build.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/sphinxdocs/sphinxdocs/private/sphinx_build.py b/sphinxdocs/sphinxdocs/private/sphinx_build.py index 833b463833..40c1a78526 100644 --- a/sphinxdocs/sphinxdocs/private/sphinx_build.py +++ b/sphinxdocs/sphinxdocs/private/sphinx_build.py @@ -304,6 +304,9 @@ def _prepare_sphinx(self, request): 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(prefix): continue digest = entry["digest"] From 251a1094a9e48374c6992000760872f99892960c Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Wed, 5 Aug 2026 07:15:59 +0000 Subject: [PATCH 13/13] style(sphinxdocs): rename prefix variable to srcdir_prefix in worker mode --- sphinxdocs/sphinxdocs/private/sphinx_build.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sphinxdocs/sphinxdocs/private/sphinx_build.py b/sphinxdocs/sphinxdocs/private/sphinx_build.py index 40c1a78526..52a334d9b9 100644 --- a/sphinxdocs/sphinxdocs/private/sphinx_build.py +++ b/sphinxdocs/sphinxdocs/private/sphinx_build.py @@ -301,17 +301,17 @@ def _prepare_sphinx(self, request): is_first_request = not current_digests changed_paths = [] request_info = {"exec_root": self._exec_root, "inputs": request["inputs"]} - prefix = str(srcdir) + "/" + 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(prefix): + if not path.startswith(srcdir_prefix): continue digest = entry["digest"] # Make the path srcdir-relative so Sphinx understands it. - path = path.removeprefix(prefix) + path = path.removeprefix(srcdir_prefix) incoming_digests[path] = digest if path not in current_digests: