docs: add bazel sphinx_build_test replicating ReadTheDocs CI#10184
docs: add bazel sphinx_build_test replicating ReadTheDocs CI#10184alokkumardalei-wq wants to merge 3 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a local Sphinx documentation build test using Bazel to replicate the ReadTheDocs CI environment. It adds several Sphinx-related dependencies to the requirements and implements a new py_test target that verifies the documentation build process. Feedback on the implementation focuses on making the test script less intrusive and more robust. Key recommendations include avoiding the destructive removal of bazel-* symlinks by utilizing Sphinx's exclude_patterns configuration, adding check=True to subprocess calls to ensure failures are properly caught, and implementing safer cleanup logic for the _readthedocs directory to prevent accidental deletion of existing user data.
| def _remove_bazel_symlinks(repo_root: str) -> list[str]: | ||
| """Remove bazel-* convenience symlinks that cause Sphinx to recurse.""" | ||
| removed = [] | ||
| for entry in os.listdir(repo_root): | ||
| path = os.path.join(repo_root, entry) | ||
| if entry.startswith("bazel-") and os.path.islink(path): | ||
| os.unlink(path) | ||
| removed.append(path) | ||
| return removed |
There was a problem hiding this comment.
The _remove_bazel_symlinks function is destructive as it permanently removes convenience symlinks (like bazel-bin, bazel-out) from the user's workspace. Since this test is intended to be run locally, this side effect is intrusive and requires the user to manually recreate the symlinks after running the test.
A better approach is to exclude these patterns in the Sphinx configuration. Since the main symlink (created at line 59) makes the repo root visible to Sphinx, you should add main/bazel-* to the exclude_patterns list in docs/conf.py instead of deleting the files.
| # Remove bazel-* symlinks to prevent recursive scanning | ||
| _remove_bazel_symlinks(repo_root) |
There was a problem hiding this comment.
As mentioned in the feedback for the function definition, this call should be removed in favor of using exclude_patterns in docs/conf.py to avoid destructive side effects on the user's workspace.
| subprocess.run( | ||
| [sys.executable, "getMessages.py"], | ||
| cwd=docs_dir, | ||
| capture_output=True, | ||
| ) |
There was a problem hiding this comment.
The subprocess.run call for getMessages.py should include check=True. If the glossary generation fails, the test should stop immediately rather than proceeding with an incomplete build environment.
| subprocess.run( | |
| [sys.executable, "getMessages.py"], | |
| cwd=docs_dir, | |
| capture_output=True, | |
| ) | |
| subprocess.run( | |
| [sys.executable, "getMessages.py"], | |
| cwd=docs_dir, | |
| capture_output=True, | |
| check=True, | |
| ) |
| # Stub doxygen output directory | ||
| doxygen_dir = os.path.join(repo_root, "_readthedocs", "html", "doxygen_output") | ||
| os.makedirs(doxygen_dir, exist_ok=True) | ||
| cleanup_paths.append(os.path.join(repo_root, "_readthedocs")) |
There was a problem hiding this comment.
Adding _readthedocs to cleanup_paths unconditionally can lead to accidental deletion of existing user data if that directory already existed. It is safer to only add it to the cleanup list if the test script actually created it.
| # Stub doxygen output directory | |
| doxygen_dir = os.path.join(repo_root, "_readthedocs", "html", "doxygen_output") | |
| os.makedirs(doxygen_dir, exist_ok=True) | |
| cleanup_paths.append(os.path.join(repo_root, "_readthedocs")) | |
| # Stub doxygen output directory | |
| rt_dir = os.path.join(repo_root, "_readthedocs") | |
| rt_existed = os.path.exists(rt_dir) | |
| doxygen_dir = os.path.join(rt_dir, "html", "doxygen_output") | |
| os.makedirs(doxygen_dir, exist_ok=True) | |
| if not rt_existed: | |
| cleanup_paths.append(rt_dir) |
| subprocess.run( | ||
| [sys.executable, revert], | ||
| cwd=docs_dir, | ||
| capture_output=True, | ||
| ) |
There was a problem hiding this comment.
The revert-links.py script should also be run with check=True to ensure that any failure in reverting the documentation state is reported as a test failure.
| subprocess.run( | |
| [sys.executable, revert], | |
| cwd=docs_dir, | |
| capture_output=True, | |
| ) | |
| subprocess.run( | |
| [sys.executable, revert], | |
| cwd=docs_dir, | |
| capture_output=True, | |
| check=True, | |
| ) |
|
clang-tidy review says "All clean, LGTM! 👍" |
alokkumardalei-wq
force-pushed
the
feature/bazel-sphinx-test
branch
from
2c220e2 to
5892e47
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
alokkumardalei-wq
force-pushed
the
feature/bazel-sphinx-test
branch
from
5892e47 to
d290e9b
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
alokkumardalei-wq
force-pushed
the
feature/bazel-sphinx-test
branch
from
d290e9b to
27036d6
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
alokkumardalei-wq
force-pushed
the
feature/bazel-sphinx-test
branch
from
27036d6 to
66f8a3c
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
alokkumardalei-wq
force-pushed
the
feature/bazel-sphinx-test
branch
from
66f8a3c to
90a03ce
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
alokkumardalei-wq
force-pushed
the
feature/bazel-sphinx-test
branch
from
90a03ce to
7a55efa
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
alokkumardalei-wq
force-pushed
the
feature/bazel-sphinx-test
branch
from
7a55efa to
a405d28
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
alokkumardalei-wq
force-pushed
the
feature/bazel-sphinx-test
branch
from
a405d28 to
2c7d8e3
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
|
Hello @oharboe and @maliberty , all checks are passing please have a look when you get time . Happy to get any feedback on this . Thank you ! |
|
Thank you for doing this! I love having all tests reproducible locally rather than digging into mystical severs. Also this should make CI more robust: it depends on fewer servers... I'll leave review to @maliberty |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c7d8e3a1c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| from the workspace, the script lives in the real ``docs/`` directory. | ||
| In both cases the docs content sits alongside this file. | ||
| """ | ||
| return os.path.dirname(os.path.realpath(__file__)) |
There was a problem hiding this comment.
Using os.path.realpath(__file__) here can escape Bazel’s runfiles tree and point docs_dir at the live workspace when sources are symlinked, so this test may read/write real repo files instead of declared data inputs. In that mode the sanity check for staged runfiles becomes ineffective, and the test can mutate local files (README.md, docs/main) as part of setup/cleanup, which makes the check non-hermetic and can leave a dirty workspace if the run is interrupted.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolve docs path without dereferencing runfile symlinks
Using
os.path.realpath(__file__)here can escape Bazel’s runfiles tree and pointdocs_dirat the live workspace when sources are symlinked, so this test may read/write real repo files instead of declareddatainputs. In that mode the sanity check for staged runfiles becomes ineffective, and the test can mutate local files (README.md,docs/main) as part of setup/cleanup, which makes the check non-hermetic and can leave a dirty workspace if the run is interrupted.Useful? React with 👍 / 👎.
Hello @maliberty @luarss , thanks for the feedback . I have have addressed this, please have a look whrn yo get time. Happy to address any feedback on this further.
Thank you !
Adds a hermetic Bazel `py_test` target that runs the same Sphinx documentation build performed by readthedocs.org/openroad, catching broken cross-references, malformed markup, missing files, and toc.yml issues locally before they reach CI. - `//docs:sphinx_build_test` runs `sphinx-build -b html` with pinned Python deps from `@openroad-pip` (no system Sphinx required). - Tagged `doc_check` for filtering: `bazelisk test --test_tag_filters=doc_check //docs/...`. - Mirrors conf.py setup (main symlink, README2.md prefix swap, messages glossary generation, doxygen stub) and strips `bazel-*` symlinks to prevent recursive scanning. Fixes The-OpenROAD-Project#9885 Signed-off-by: alokkumardalei-wq <alokkumardalei2@gmail.com>
alokkumardalei-wq
force-pushed
the
feature/bazel-sphinx-test
branch
from
2c7d8e3 to
589c095
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
| tclint==0.7.0 | ||
|
|
||
| # Sphinx documentation build (replicates ReadTheDocs CI check) | ||
| # See: https://github.com/The-OpenROAD-Project/OpenROAD/issues/9885 |
There was a problem hiding this comment.
which python version is this requirements.in for? also can you please lock them with ==
There was a problem hiding this comment.
@luarss done, pinned all Sphinx deps with == in 8aac486 and added a header clarifying this is the Python 3.13 toolchain's input (the lock file is bazel/requirements_lock_3_13.txt). Regenerated via bazelisk run //bazel:requirements.update; the lock is unchanged since the pins match the previously resolved versions.
| "main/src/odb/src/def/doc/README.md", | ||
| "main/src/odb/src/lef/README.md", | ||
| "main/docs", | ||
| "main/bazel-*", |
There was a problem hiding this comment.
Thanks @luarss ,good catch removed in 8aac486. This was defensive for local-dev: running sphinx-build docs/ from a workspace that's been bazel-built makes Sphinx recurse into the bazel-bin/bazel-out/bazel-testlogs convenience symlinks (the main/docs exclusion doesn't cover them). It isn't needed for this PR — ReadTheDocs CI doesn't run Bazel, and runfiles don't contain bazel-* symlinks. Happy to send it as a small follow-up if you'd like that kept for the local workflow.
| def _find_docs_dir() -> str: | ||
| """Locate the docs directory at test runtime. | ||
|
|
||
| Under ``bazel test``, sources declared as ``data`` are staged in the | ||
| runfiles tree next to this script. Under a plain Python invocation | ||
| from the workspace, the script lives in the real ``docs/`` directory. | ||
| In both cases the docs content sits alongside this file. ``abspath`` | ||
| (not ``realpath``) is used so the script stays inside the runfiles | ||
| tree instead of resolving symlinks back to the live workspace. | ||
| """ | ||
| return os.path.dirname(os.path.abspath(__file__)) |
There was a problem hiding this comment.
why is this not just a simple line for variable DOCS_DIR?
|
|
||
| try: | ||
| # --- Pre-build setup (mirrors conf.py setup()) --- | ||
|
|
There was a problem hiding this comment.
You should not mirror conf.py setup. This leads to divergent sources of truth. Instead - find a way to reference that file directly, or reference their main() function if applicable.
edit: Do not simply just copy, use a symlink or reuse utilities from that conf.py file.
There was a problem hiding this comment.
@luarss you're right refactored in 8aac486. The test no longer mirrors any of conf.py's setup(app) logic. It just chdirs into docs/ and calls sphinx.cmd.build.main(...), which loads conf.py and Sphinx invokes setup(app) for us. The only piece I had to add was staging //:README.md in the runfiles tree (so conf.py's shutil.copy("main/README.md", ...) can resolve it), and I still invoke revert-links.py post-build to undo the in-place swap_prefix mutations.
Simplify the test per @luarss's review (#4148341864): - Pin Sphinx deps with `==` in bazel/requirements.in and clarify the Python 3.13 target; regenerated lock file is unchanged. - Drop "main/bazel-*" from conf.py exclude_patterns (scope creep for this PR; defensive only for local-dev sphinx-build from a workspace that has been bazel-built). - Replace _find_docs_dir() helper with a DOCS_DIR module constant. - Stop mirroring conf.py's setup() in the test. Stage //:README.md in runfiles and rely on Sphinx to call conf.py's setup(app) directly; revert-links.py is still invoked post-build to undo the in-place swap_prefix mutations on README.md / README2.md. Verified locally: bazelisk test //docs:sphinx_build_test //docs:sphinx_build_test PASSED in 2.8s Signed-off-by: alokkumardalei-wq <alokkumardalei2@gmail.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
|
Hello @luarss @maliberty I have tried to address all your feedback please have look when you get time. Happy to get any feedback on this further. Thank you ! |
|
Hello @luarss and @maliberty , all checks are passing now please have a look when you get time . Happy to take any further feedback . Thank you ! |
|
@luarss please look when you can |
luarss
left a comment
luarss
left a comment
There was a problem hiding this comment.
The test runs, but no-sandbox + local break Bazel's sandbox isolation: conf.py's setup(app) mutates workspace source files (README.md) through runfiles symlinks. Two suggestions below to fix this and let the test run fully hermetically.
| tags = [ | ||
| "doc_check", | ||
| "local", | ||
| "no-sandbox", | ||
| ], |
| import os | ||
| import shutil | ||
| import subprocess | ||
| import sys | ||
| import tempfile | ||
|
|
||
| # abspath (not realpath) so the path stays inside the runfiles tree instead | ||
| # of resolving symlinks back to the live workspace. | ||
| DOCS_DIR = os.path.dirname(os.path.abspath(__file__)) | ||
|
|
||
|
|
||
| def main() -> int: | ||
| build_output = tempfile.mkdtemp(prefix="sphinx_build_") | ||
| saved_cwd = os.getcwd() | ||
| try: | ||
| # conf.py's setup(app) hook uses cwd-relative paths like "./main" | ||
| # and "../README.md", so it must run with docs/ as cwd. Sphinx | ||
| # invokes setup(app) automatically when it loads conf.py below. | ||
| os.chdir(DOCS_DIR) | ||
|
|
||
| # Calling sphinx via subprocess would fail because a child python3 | ||
| # doesn't inherit the Bazel runfiles PYTHONPATH used by this test. | ||
| from sphinx.cmd.build import main as sphinx_main | ||
|
|
||
| args = ["-b", "html", "-T", "-q"] | ||
| if not shutil.which("mmdc"): | ||
| args.extend(["-D", "mermaid_output_format=raw"]) | ||
| args.extend([DOCS_DIR, build_output]) | ||
|
|
||
| print("Running sphinx-build...", flush=True) | ||
| returncode = sphinx_main(args) | ||
|
|
||
| # conf.py's setup(app) rewrites README.md and README2.md in place. | ||
| # revert-links.py undoes those mutations so the test leaves no trace. | ||
| revert = os.path.join(DOCS_DIR, "revert-links.py") | ||
| if os.path.isfile(revert): | ||
| subprocess.run( | ||
| [sys.executable, revert], | ||
| cwd=DOCS_DIR, | ||
| capture_output=True, | ||
| check=True, | ||
| ) | ||
|
|
||
| if returncode != 0: | ||
| print( | ||
| f"FAILED: sphinx-build exited with status {returncode}", | ||
| file=sys.stderr, | ||
| ) | ||
| return 1 | ||
| print("PASSED: Sphinx documentation built successfully.") | ||
| return 0 | ||
| finally: | ||
| os.chdir(saved_cwd) | ||
| shutil.rmtree(build_output, ignore_errors=True) |
There was a problem hiding this comment.
@maliberty LGTM. Please ignore my previous review - it was about refactoring to bazel-syntax, but in doing so led to multiple sources of truth for docs/conf.py logic - we should use a single source like what is done here.
|
conflict to resolve |
|
@maliberty Shall I start a new PR to salvage this? |
Signed-off-by: Alok Kumar Dalei <alokkumardalei2@gmail.com>
|
@alokkumardalei-wq Almost there - please fix bazel.lock |
4 participants
What does this PR do?
Adds a hermetic Bazel
py_testtarget that runs the same Sphinx documentation build performed by readthedocs.org/openroad, catching broken cross-references, malformed markup, missing files, and toc.yml issues locally before they reach CI.//docs:sphinx_build_testrunssphinx-build -b htmlwith pinned Python deps from@openroad-pip(no system Sphinx required).doc_checkfor filtering:bazelisk test --test_tag_filters=doc_check //docs/....bazel-*symlinks to prevent recursive scanning.Fixes #9885
Type of Change
Verification
./etc/Build.sh).