Skip to content

[py] replace rules_python sphinxdocs with local sphinx_docs rule#17461

Merged
titusfortner merged 1 commit into
trunkfrom
sphinxdocs
May 15, 2026
Merged

[py] replace rules_python sphinxdocs with local sphinx_docs rule#17461
titusfortner merged 1 commit into
trunkfrom
sphinxdocs

Conversation

@titusfortner

@titusfortner titusfortner commented May 15, 2026

Copy link
Copy Markdown
Member

🔗 Related Issues

Fixes #17404

💥 What does this PR do?

Move sphinxdocs into our repo since rules_python removes it

🔧 Implementation Notes

Used py_console_script_binary instead of a script wrapper since it seems more straightforward

🤖 AI assistance

  • No substantial AI assistance used
  • AI assisted (complete below)
    • Tool(s): Claude
    • What was generated: sphinx_docs.bzl
    • I reviewed all AI output and can explain the change

🔄 Types of changes

  • New feature (non-breaking change which adds functionality and tests!)

@titusfortner titusfortner requested a review from cgoldberg May 15, 2026 12:32
@selenium-ci selenium-ci added C-py Python Bindings B-build Includes scripting, bazel and CI integrations labels May 15, 2026
@qodo-code-review

Copy link
Copy Markdown
Contributor

Review Summary by Qodo

Replace rules_python sphinxdocs with local sphinx_docs rule

✨ Enhancement

Grey Divider

Walkthroughs

Description
• Replace external rules_python sphinxdocs with local sphinx_docs rule
• Implement custom Bazel rule for building Sphinx documentation
• Switch sphinx-build from sphinx_build_binary to py_console_script_binary
• Simplify sphinx_docs target configuration and dependencies
Diagram
flowchart LR
  A["rules_python sphinxdocs"] -->|"removed"| B["Local sphinx_docs.bzl"]
  C["sphinx_build_binary"] -->|"replaced with"| D["py_console_script_binary"]
  B -->|"used in"| E["py/BUILD.bazel"]
  D -->|"used in"| E
Loading

Grey Divider

File Changes

1. py/private/sphinx_docs.bzl ✨ Enhancement +46/-0

New local sphinx_docs Bazel rule implementation

• New Bazel rule implementation for building Sphinx HTML documentation
• Defines _sphinx_docs_impl function that executes sphinx-build with proper arguments
• Configures rule attributes for config file, sphinx executable, and source files
• Returns DefaultInfo with html output directory as tree artifact

py/private/sphinx_docs.bzl


2. py/BUILD.bazel ✨ Enhancement +6/-5

Update BUILD.bazel to use local sphinx_docs rule

• Remove load of sphinx_docs from rules_python and add local sphinx_docs import
• Replace sphinx_build_binary with py_console_script_binary for sphinx-build target
• Update sphinx-build target to use pkg and script attributes instead of deps
• Simplify sphinx_docs target by removing formats attribute and srcs glob filter

py/BUILD.bazel


Grey Divider

Qodo Logo

@qodo-code-review

qodo-code-review Bot commented May 15, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0)

Context used

Grey Divider


Remediation recommended

1. CWD-dependent Sphinx config 🐞 Bug ☼ Reliability
Description
py/docs/source/conf.py mutates sys.path based on os.getcwd(), but the new sphinx_docs Bazel rule
does not guarantee the action’s working directory, making autodoc import resolution depend on the
execution environment and potentially breaking or changing doc output under Bazel sandboxes/RBE.
Code

py/private/sphinx_docs.bzl[R7-22]

+    config_dir = ctx.file.config.dirname
+
+    args = ctx.actions.args()
+    args.add("-b", "html")
+    args.add("-d", doctrees_dir.path)
+    args.add(config_dir)
+    args.add(html_dir.path)
+
+    ctx.actions.run(
+        inputs = ctx.files.srcs + [ctx.file.config],
+        outputs = [html_dir, doctrees_dir],
+        executable = ctx.executable.sphinx,
+        arguments = [args],
+        mnemonic = "SphinxBuild",
+        progress_message = "Building Sphinx HTML docs for %{label}",
+    )
Evidence
The new Bazel rule runs sphinx-build without setting any working directory, while the Sphinx
configuration explicitly derives sys.path from the process CWD; that makes imports (used by
autosummary/autodoc) sensitive to where the process starts.

py/private/sphinx_docs.bzl[7-22]
py/docs/source/conf.py[19-28]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`py/docs/source/conf.py` currently does `sys.path.insert(0, os.path.join(os.getcwd(), "..", ".."))`, which makes module resolution depend on the process working directory at runtime. The new Bazel Sphinx action does not set/guarantee the working directory, so the docs build can become environment-dependent.

### Issue Context
Under Bazel, the action may run with a different CWD than local/dev tooling. Even if builds often succeed due to other PYTHONPATH/runfiles behavior, this is fragile and can cause failures or accidental imports from the wrong location when the CWD differs.

### Fix Focus Areas
- py/docs/source/conf.py[19-28]
- py/private/sphinx_docs.bzl[7-22]

### Suggested fix
Update `conf.py` to compute paths from the config file location instead of `os.getcwd()`, e.g.:
```python
DOCS_DIR = os.path.dirname(os.path.abspath(__file__))
sys.path.insert(0, os.path.abspath(os.path.join(DOCS_DIR, "..", "..")))
```
This keeps behavior consistent across Bazel, tox, and ReadTheDocs without relying on a particular working directory.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

@titusfortner titusfortner merged commit 7a6f869 into trunk May 15, 2026
43 checks passed
@titusfortner titusfortner deleted the sphinxdocs branch May 15, 2026 13:44
This was referenced Jun 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-build Includes scripting, bazel and CI integrations C-py Python Bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[🐛 Bug]: [py] Fix python build so we can upgrade rules_python without using sphinxdocs

3 participants