[setup] Obtain sphinx via PyPI instead of the system - #24931
Conversation
|
@drake-jenkins-bot linux-noble-unprovisioned-gcc-bazel-experimental-documentation please. |
tyler-yankee
left a comment
There was a problem hiding this comment.
@tyler-yankee reviewed 9 files and all commit messages, and made 1 comment.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge" (waiting on tyler-yankee).
tools/workspace/python/venv_upgrade line 130 at r1 (raw file):
for root in lief mypy sphinx; do package_requirements="./setup/python/${root}/requirements.txt" bazel run //setup/python/${package}:lock.update
working
typo, and still need to test this locally anyways
0d70cab to
f6a9064
Compare
tyler-yankee
left a comment
There was a problem hiding this comment.
@tyler-yankee reviewed 2 files and all commit messages, and resolved 1 discussion.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge".
|
@drake-jenkins-bot linux-noble-unprovisioned-gcc-bazel-experimental-documentation please. |
f6a9064 to
8e333c8
Compare
|
@drake-jenkins-bot linux-noble-unprovisioned-gcc-bazel-experimental-documentation please. |
tyler-yankee
left a comment
There was a problem hiding this comment.
+a:@tom-osika for feature review, please.
@tyler-yankee reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: LGTM missing from assignee tom-osika, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on tom-osika).
tyler-yankee
left a comment
There was a problem hiding this comment.
BTW I have an update to sphinx 8 locally, but I'm going to defer it to another PR, since I'd like to target this commit just towards moving the machinery around.
Sphinx 8 (8.2.3 in particular) doesn't appear to be a problem for drake. 9.1.0 on the other hand....Sphinx basically deprecated half of how pydrake_sphinx_extension.py works in that release 🙂
@tyler-yankee made 1 comment.
Reviewable status: LGTM missing from assignee tom-osika, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on tom-osika).
jwnimmer-tri
left a comment
There was a problem hiding this comment.
@jwnimmer-tri reviewed 9 files and all commit messages, and made 1 comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee tom-osika, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on tom-osika and tyler-yankee).
MODULE.bazel line 166 at r3 (raw file):
] use_repo(pip, "drake_sphinx")
nit Should probably be dev_dependency = True, which also means it needs to re-locate within this file.
tyler-yankee
left a comment
There was a problem hiding this comment.
@tyler-yankee made 1 comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee tom-osika, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on jwnimmer-tri and tom-osika).
MODULE.bazel line 166 at r3 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit Should probably be
dev_dependency = True, which also means it needs to re-locate within this file.
Fair, but isn't pip itself the dev dependency from Bazel's perspective, not sphinx? Do we dual-declare pip = use_extension(...) and pip_dev = use_extension(..., dev_dependency = True)?
jwnimmer-tri
left a comment
There was a problem hiding this comment.
@jwnimmer-tri made 1 comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee tom-osika, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on tom-osika and tyler-yankee).
MODULE.bazel line 166 at r3 (raw file):
Previously, tyler-yankee (Tyler Yankee) wrote…
Fair, but isn't
pipitself the dev dependency from Bazel's perspective, not sphinx? Do we dual-declarepip = use_extension(...)andpip_dev = use_extension(..., dev_dependency = True)?
I didn't look in detail how it works.
At the very least, we need better commentary explaining to downstream users that they shouldn't touch this. Having their bzlmod graph (their MODULE.bazel.lock file) be smaller due to dev_dependency markings would be a nice win, but I suppose the most important thing is at least to clarify that they shouldn't be touching this.
8e333c8 to
1a0164a
Compare
tyler-yankee
left a comment
There was a problem hiding this comment.
@tyler-yankee reviewed 2 files and all commit messages, and resolved 1 discussion.
Reviewable status: LGTM missing from assignee tom-osika, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on jwnimmer-tri and tom-osika).
tom-osika
left a comment
There was a problem hiding this comment.
@tom-osika made 1 comment.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers (waiting on jwnimmer-tri).
tyler-yankee
left a comment
There was a problem hiding this comment.
+a:@jwnimmer-tri for platform review or delegation, please.
@tyler-yankee made 1 comment.
Reviewable status: LGTM missing from assignee jwnimmer-tri(platform) (waiting on jwnimmer-tri and tom-osika).
|
@drake-jenkins-bot linux-noble-unprovisioned-gcc-bazel-experimental-documentation please. |
jwnimmer-tri
left a comment
There was a problem hiding this comment.
@jwnimmer-tri reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform) (waiting on tom-osika and tyler-yankee).
doc/pydrake/build.py line 162 at r4 (raw file):
# Run the documentation generator. os.environ["LANG"] = "en_US.UTF-8" check_call(
The loss of check_call loses e.g. the LC_ALL setting. We did patch in the "drake is building documentation" env var above but this spelling loses the encapsulation of check_call being the source of truth.
Is it possible to run sphinx as an external program still? I would think so.
Sphinx upgrades can now be done independently of developer platform upgrades, unblocking the latter effort which can bring substantial other work, and ensuring documentation changes are more carefully inspected upon upgrade. In general, easing the upgrade process provides a clearer modernization pathway for pydrake documentation, particularly during the switch to nanobind.
1a0164a to
a6d0ae0
Compare
tyler-yankee
left a comment
There was a problem hiding this comment.
@tyler-yankee reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform) (waiting on jwnimmer-tri and tom-osika).
doc/pydrake/build.py line 162 at r4 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
The loss of
check_callloses e.g. theLC_ALLsetting. We did patch in the "drake is building documentation" env var above but this spelling loses the encapsulation ofcheck_callbeing the source of truth.Is it possible to run sphinx as an external program still? I would think so.
Good point, python -m sphinx seems to do the job.
|
@drake-jenkins-bot linux-noble-unprovisioned-gcc-bazel-experimental-documentation please. |
jwnimmer-tri
left a comment
There was a problem hiding this comment.
@jwnimmer-tri reviewed 1 file and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status:complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),tom-osika (waiting on tom-osika).
Sphinx upgrades can now be done independently of developer platform upgrades, unblocking the latter effort which can bring substantial other work, and ensuring documentation changes are more carefully inspected upon upgrade.
In general, easing the upgrade process provides a clearer modernization pathway for pydrake documentation, particularly during the switch to nanobind.
Towards #24895.
This change is