Support docutils 0.18

[edgarrmondragon]

Closes #1302

[edgarrmondragon]

Build step is failing but I'm not sure how to fix that

[kloczek]

docutils 0.19 has been released.

[benjaoming]

Possibly a related bug in this PR:

Two colons are still rendered with sphinx-rtd-theme and Sphinx 5.1.0.

#1302 (comment)

Please do say if you have any findings here 🙏

[benjaoming]

I'd say that the "two colons" issue is important, but that it's a very little issue and shouldn't block us from the upgrade here, since it's causing issues for other important projects.

Edit: Issue is fixed in Sphinx

[benjaoming]

Thanks for fixing this @edgarrmondragon - it's very helpful to have comments confirming that it also has been tested and works :) I'll proceed to have a look that everything in our demo build looks okay and open up any issues I might find.

[kloczek]

Any chance to make new release?
It is already quite long since last release has been made ..

[benjaoming]

@kloczek that's definitely the intention

I'll have to investigate these current build issues though:

Exception occurred:
  File "/home/circleci/project/.tox/py27-sphinxlatest/lib/python2.7/site-packages/sphinx/ext/intersphinx.py", line 365, in missing_reference
    contnode[0] = nodes.Text(newtarget, contnode[0].rawsource)
AttributeError: 'Text' object has no attribute 'rawsource'

[benjaoming]

Above error is fixed in sphinx-doc/sphinx@961f5af

This commmit is released with Sphinx 4.3.0. Hence, Sphinx releases prior to this version definitely have this issue and aren't compatible with docutils 0.18. What might be confusing is that the error isn't occurring 100% reliably.. but that's another story.

Should we handle this simply in our Tox matrix?

[benjaoming]

It turns out that the build error was because of a missing lower bound on Sphinx in tox.ini, meaning that for Python 2.7, we always got Sphinx 1.8 for everything intended to be 2.0+. In combination with docutils 0.18, this gave the error.

After specifying the versions correctly in tox.ini and .circleci/config.yml, we are fine 👍

[edgarrmondragon]

hey @benjaoming, I've been really busy so thanks for taking on the PR 😄

[benjaoming]

Not sure if #1338 might cause a need to solve conflicts, currently it has a clean cherry-pick.

[benjaoming]

@twodrops

I wanted to test this branch with
pip install git+https://github.com/edgarrmondragon/[email protected]
but somehow the docutils seems to be still set as < docutils 0.18

Can you verify the version of Sphinx and Python that was used for this?

[benjaoming]

Failing to reproduce conflict is caused by: sphinx-rtd-theme 1.0.1a1 depends on docutils<0.18 - docutils 0.18 is correctly installed in the tox environment.

Circle CI build output LGTM 👍

py310-sphinx51 installed: alabaster==0.7.12,attrs==22.1.0,Babel==2.10.3,certifi==2022.9.24,charset-normalizer==2.1.1,docutils==0.18.1,idna==3.4,imagesize==1.4.1,iniconfig==1.1.1,Jinja2==3.0.3,MarkupSafe==2.1.1,packaging==21.3,pluggy==1.0.0,py==1.11.0,Pygments==2.13.0,pyparsing==3.0.9,pytest==7.1.3,pytz==2022.4,readthedocs-sphinx-ext==2.1.9,requests==2.28.1,six==1.16.0,snowballstemmer==2.2.0,Sphinx==5.1.1,sphinx-rtd-theme @ file:///home/circleci/project/.tox/.tmp/package/1/sphinx_rtd_theme-1.0.1a1.zip,sphinxcontrib-applehelp==1.0.2,sphinxcontrib-devhelp==1.0.2,sphinxcontrib-htmlhelp==2.0.0,sphinxcontrib-httpdomain==1.8.0,sphinxcontrib-jsmath==1.0.1,sphinxcontrib-qthelp==1.0.3,sphinxcontrib-serializinghtml==1.1.5,tomli==2.0.1,urllib3==1.26.12

#1348 adds missing Sphinx versions to the test matrix, i.e. currently only Python 3.10 environment has Sphinx 5.x.

[benjaoming]

Trying to move things forwards, so I'm going to propose the following assessment and if you agree @agjohnson I think we should merge both this PR and #1336 (because the reasoning for allowing docutils 0.19 is the same as below)

1. sphinx_rtd_theme has an upper bound on docutils because of previous fallout, see: https://blog.readthedocs.com/build-errors-docutils-0-18/
2. Previous incidents following the docutils 0.18 release are now largely handled since Sphinx is ultimately responsible for upper bounds on docutils - patched versions of Sphinx was released for 1.8.x and 2.4.x series, and subsequent Sphinx release series have upper bounds on docutils.
3. Going into the future, sphinx_rtd_theme might maintain an upper bound on docutils since it may affect the HTML structure that Sphinx generates and thus what the theme is compatible with. We should discuss to completely remove our docutils dependency rely on the Sphinx dependency entirely in a separate issue.

Proposed solution: We bump the upper bound for docutils, i.e. merge this PR as-is, same with #1336

What happens then...

The upgrade to docutils<0.19 may negatively impact deployments that have pinned some variant of sphinx<1.8.6 or Sphinx>=2.0,<2.4.5.

Example breakage: A deployment with sphinx==1.8.5 (this version has no docutils upper bound and is incompatible with docutils 0.18) and sphinx_rtd_theme (no version pinning) will get docutils 0.18 and then builds will break.

How many projects are affected?

• Unaffected: Projects created or dependency-updated after April 11, 2021 are unaffected. On this date Sphinx==3.5.4 was released w/ docutils upper bound (oddly Sphinx 3.5.4 has docutils>=0.12,<0.17 whereas Sphinx 2.4.5 has docutils>=0.12,<0.18).
• Projects that use sphinx>=3 are theoretically unaffected since they are compatible with docutils 0.18 (according to RTD blog post) and do not need the upper bound. This means that projects created after April 5 2020 are very likely unaffected.
• Projects pinning Sphinx==1.8.x need to bump to 1.8.6
• Projects pinning Sphinx==2.4.x need to bump to 2.4.5
• Projects on Sphinx 1.6.x, 2.0.x, 2.1.x, 2.2.x, 2.3.x need to implement their own docutils<0.18

Mitigation? It's possible that we release sphinx_rtd_theme with sphinx>=1.8.6,sphinx!=2.0.*,sphinx!=2.1.*,sphinx!=2.2.*,sphinx!=2.3.*,sphinx!=2.4.0,sphinx!=2.4.1,sphinx!=2.4.2,sphinx!=2.4.3,sphinx!=2.4.4, such that affected projects mentioned above won't see the 1.1 upgrade of sphinx_rtd_theme. However, since the pip resolver was changed and had issues with endless backtracking, I'm not at all comfortable with this. It will also introduce a complex version specifier that will make our future dependency headaches worse, also for users trying to understand their own derived dependency graphs. If the fallout is bad, we can decide to include this in a patch update.

Restating the benefits of bumping docutils version: This change is necessary (i.e. critical) to introduce because several projects have failing builds because of failure to satisfy dependencies. Docutils 0.18 is soon 1 year old, so it's due time to support it.

Refs: sphinx-doc/sphinx#9807

[twodrops]

@twodrops

I wanted to test this branch with
pip install git+https://github.com/edgarrmondragon/[email protected]
but somehow the docutils seems to be still set as < docutils 0.18

Can you verify the version of Sphinx and Python that was used for this?

@benjaoming I tested it with your branch again and I cannot reproduce it anymore. Sorry for the confusion.
Looking forward to the quick merge of this PR which has been blocking us for a while now.

[benjaoming]

No worries! Thanks for weighing in! Looking forwards to resolving this!

[agjohnson]

Thanks again for the contribution! Unfortunately, this PR reintroduces the display issues with footnotes/citations that caused us to pin docutils<0.18 initially:

https://sphinx-rtd-theme--1304.org.readthedocs.build/en/1304/demo/demo.html#references

Until we can address those in a backwards compatible way, docutils<0.18 is the specifier we should use.

[agjohnson]

@benjaoming A couple more notes for your input:

I'm going to propose the following assessment and if you agree @agjohnson I think we should merge both this PR

In the interest of sticking to our plan for faster release cadence, I don't think we should merge these just yet. These PRs will both require bug fixes for display bugs.

I noted on #1309 that the intention was to release 1.1 as a rather minimal release -- just pinning dependencies more accurately. We'd still try to iterate quicker and release 1.2/2.0 with new features like docutils 0.18/0.19 support however.

Previous incidents following the docutils 0.18 release are now largely handled since Sphinx is ultimately responsible for upper bounds on docutils

Mostly, however our concerns are tighter than the concerns that Sphinx has. We have display issues from docutils to worry about as well, and have to support multiple versions of docutils 😞

Going into the future, sphinx_rtd_theme might maintain an upper bound on docutils

Because of above, we can say fairly concretely that we'll always have docutils specifiers in both directions.

Proposed solution: We bump the upper bound for docutils, i.e. merge this PR as-is, same with #1336

For next release (1.2/2.0), 👍 this sounds good. However, this is going to be more like "We bump the upper bounds for docutils to 0.19 and fix all display bugs from docutils 0.18 and docutils 0.19 changes."

How many projects are affected?

This list is probably even more limited than you describe, as the project would also have to pin docutils for there to be a dependency conflict. As its a transitive dependency, this isn't common. I suppose more-so, the project would have to pin an impossible dependency as well, docutils==0.17.1 will not introduce a build failure.

Mitigation?

We can probably be rather relaxed here, as it's only projects that have docutils pinned that we really need to worry about. We also have some deprecations to enforce, including old release of Sphinx, so we can take the position that Sphinx 1.8 support is phased out enough that we aren't going to worry about supporting modern docutils and crust Sphinx combinations. That is actually the point of releases like 1.1, to give a snapshot that users can use before we start culling our dependencies.

[benjaoming]

Going to look for more references about this issue - I would like to see the discussion about why docutils uses <aside> instead of <dl>, it seems wrong.

[benjaoming]

@pradyunsg re: pradyunsg/furo#447 (comment) I see that you ran into this (footnotes are now in <aside> instead of <dl>) and already fixed it in Furo. Do you have a quick take on your decisions here? Did you drop support for docutils 0.17 at the same time?

[pradyunsg]

pradyunsg/furo@d955850

I tweaked the look, preserved docutils 0.17 compat, and filed sphinx-doc/sphinx#10531.

I’ve not had bandwidth to tweak the styles in Furo since then, but it seems like it’ll work just fine to use the css in docutils with a tweak.

[benjaoming]

Thanks for getting things improved with Sphinx @pradyunsg - it's really nice that the docutils 0.19 behavior got backported to Sphinx 5.1, I think we can reduce the scope of footnote DOM variants to just 2 versions.. the old <dl> and the new <aside> from docutils 0.19.

[benjaoming]

@agjohnson the visual regression is now addressed in #1351

[benjaoming]

Support for docutils 0.18 has been added in #1381

In release notes, we might add some clarity on old versions of Sphinx potentially breaking because of docutils 0.18, see comment in #1304 (comment)

[benjaoming]

Released in 1.2.0rc1 👍

[kloczek]

In mean time docutils 0.19 has been released 😋
It would be good to move to that new version as well 😄

[benjaoming]

@kloczek - definitely, and if we can get some help with verification and testing, we might also be able to add the support quite quickly - the release notes indicate that they have changed the DOM for footnotes again => https://docutils.sourceforge.io/RELEASE-NOTES.html#release-0-19-2022-07-05

[benjaoming]

There is an issue open for docutils 0.19 support here: #1323

[kloczek]

Thank you for the update 👍 😄