docs: add bazel sphinx_build_test replicating ReadTheDocs CI#10184
docs: add bazel sphinx_build_test replicating ReadTheDocs CI#10184alokkumardalei-wq wants to merge 1 commit intoThe-OpenROAD-Project:masterfrom
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! 👍" |
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
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 |
2 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).