Skip to content

Conversation

@kehach07
Copy link

@kehach07 kehach07 commented Jan 7, 2026

Fixed issue :- Docs intermittently mix translations of Sphinx labels #2416

Run each Sphinx build in a separate subprocess to avoid global state leakage
between documentation builds.

@pauloxnet
Copy link
Member

I would suggest removing old lines of code instead of commenting.

@kehach07
Copy link
Author

kehach07 commented Jan 7, 2026

I would suggest removing old lines of code instead of commenting.

thank you for the feedback, i have implemented the suggestion.

@medmunds
Copy link
Contributor

medmunds commented Jan 8, 2026

This seems to me like a safer way to use Sphinx, and removing the monkeypatching and undocumented Sphinx internals feels much cleaner.

One important test would be whether this generates the same built docs as before. (I'm not set up to run djangoproject.com locally, and it doesn't seem like the PR checks deploy a preview site.)

FWIW, Copilot had some concerns:

Summary — will the built docs match previous builds (aside from the bug being fixed)?

Not with the code as written. The subprocess approach is the right direction (it properly isolates Sphinx/docutils global state), but the current way extensions are passed to sphinx-build is likely to change behaviour and break loading of extensions, so output may differ. Other behavioral changes (single-threaded build, doctree location) are less likely to change rendered content, but could affect build speed and doctree reuse.

The point about extensions seems valid: they need to be full dotted paths in a command line arg, and there are potential shell quoting issues. But I question some of its other concerns: e.g., preventing doctree reuse was intentional. (Also, the AI mixing British and American spelling in the same paragraph doesn't inspire confidence 😄 .)

Copilot also pointed out logging behavior would change.

[To clarify: I'm not a maintainer on this project, just a human who uses docs.djangoproject.com and reported a bug.]

@kehach07
Copy link
Author

kehach07 commented Jan 9, 2026

This seems to me like a safer way to use Sphinx, and removing the monkeypatching and undocumented Sphinx internals feels much cleaner.

One important test would be whether this generates the same built docs as before. (I'm not set up to run djangoproject.com locally, and it doesn't seem like the PR checks deploy a preview site.)

FWIW, Copilot had some concerns:

Summary — will the built docs match previous builds (aside from the bug being fixed)?
Not with the code as written. The subprocess approach is the right direction (it properly isolates Sphinx/docutils global state), but the current way extensions are passed to sphinx-build is likely to change behaviour and break loading of extensions, so output may differ. Other behavioral changes (single-threaded build, doctree location) are less likely to change rendered content, but could affect build speed and doctree reuse.

The point about extensions seems valid: they need to be full dotted paths in a command line arg, and there are potential shell quoting issues. But I question some of its other concerns: e.g., preventing doctree reuse was intentional. (Also, the AI mixing British and American spelling in the same paragraph doesn't inspire confidence 😄 .)

Copilot also pointed out logging behavior would change.

[To clarify: I'm not a maintainer on this project, just a human who uses docs.djangoproject.com and reported a bug.]

Thanks for the detailed review!

I’ve updated the PR and the implementation to avoid passing extensions via the command line and now rely entirely on conf.py, so extension loading and rendered output should match previous builds (aside from the bug fix).

The subprocess isolation is intentional to avoid shared Sphinx/docutils state; changes like separate doctrees are expected but shouldn’t affect rendered content. Logging differences are noted, but behaviorally this should now be much closer to the existing builds.

@sarahboyce
Copy link
Contributor

the current way extensions are passed to sphinx-build is likely to change behaviour and break loading of extensions

☝️ these changes currently mean the custom extension is no longer used as it roughly reverts the use added in #1947

How to test this works locally:

  • Build the docs (python -m manage update_docs)
  • Search for "select"
  • You should see code reference search results such as QuerySet.select_for_update (currently with these changes, these results are lost)

@kehach07
Copy link
Author

kehach07 commented Jan 12, 2026

☝️ these changes currently mean the custom extension is no longer used as it roughly reverts the use added in #1947

How to test this works locally:

  • Build the docs (python -m manage update_docs)
  • Search for "select"
  • You should see code reference search results such as QuerySet.select_for_update (currently with these changes, these results are lost)

Thanks for the pointer @sarahboyce . I’ve updated the PR to rely entirely on conf.py for extension loading (no overrides), so the custom extension is used again.
I built the docs locally with python manage.py update_docs and verified that searching for “select” returns code reference results like QuerySet.select_for_update, so the search is working as expected now.

@sarahboyce
Copy link
Contributor

I get the following error when running python manage.py update_docs --force on this branch:

Traceback (most recent call last):
  File "<frozen runpy>", line 198, in _run_module_as_main
  File "<frozen runpy>", line 88, in _run_code
  File "/usr/local/lib/python3.12/site-packages/sphinx/__main__.py", line 7, in <module>
    raise SystemExit(main(sys.argv[1:]))
                     ^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/sphinx/cmd/build.py", line 546, in main
    locale.setlocale(locale.LC_ALL, '')
  File "/usr/local/lib/python3.12/locale.py", line 615, in setlocale
    return _setlocale(category, locale)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
locale.Error: unsupported locale setting
sphinx-build failed (release=fr/5.2, builder=json): Command '['/usr/local/bin/python', '-m', 'sphinx', '-b', 'json', '-D', 'language=fr', '-c', 'data/djangodocs/sources/5.2/docs', '-d', 'data/djangodocs/fr/5.2/_doctrees/fr/json', 'data/djangodocs/sources/5.2/docs', 'data/djangodocs/fr/5.2/_build/json']' returned non-zero exit status 1. 

Also I still have the same issue on this branch. I think I didn't explain it well enough.
On the main branch, this is what the results look like when searching for select:

Screenshot from 2026-01-25 10-27-54

This is what the results look like on this branch:

Screenshot from 2026-01-25 10-12-25

QuerySet.select_for_update etc. is still missing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants