sphinx_rtd_theme icon indicating copy to clipboard operation
sphinx_rtd_theme copied to clipboard

Support docutils 0.18

Open edgarrmondragon opened this issue 3 years ago β€’ 11 comments

Closes https://github.com/readthedocs/sphinx_rtd_theme/issues/1302

edgarrmondragon avatar Jul 19 '22 03:07 edgarrmondragon

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

edgarrmondragon avatar Jul 19 '22 04:07 edgarrmondragon

docutils 0.19 has been released.

kloczek avatar Jul 27 '22 14:07 kloczek

Possibly a related bug in this PR:

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

https://github.com/readthedocs/sphinx_rtd_theme/issues/1302#issuecomment-1195290758

Please do say if you have any findings here :pray:

benjaoming avatar Aug 12 '22 16:08 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 avatar Aug 19 '22 15:08 benjaoming

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

kloczek avatar Aug 19 '22 16:08 kloczek

@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 avatar Aug 19 '22 16:08 benjaoming

Above error is fixed in https://github.com/sphinx-doc/sphinx/commit/961f5af0963055f0206a437765a087a2bf03bcca

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 avatar Aug 21 '22 10:08 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 :+1:

benjaoming avatar Aug 23 '22 20:08 benjaoming

hey @benjaoming, I've been really busy so thanks for taking on the PR πŸ˜„

edgarrmondragon avatar Aug 24 '22 23:08 edgarrmondragon

Not sure if https://github.com/readthedocs/sphinx_rtd_theme/pull/1338 might cause a need to solve conflicts, currently it has a clean cherry-pick.

benjaoming avatar Aug 26 '22 19:08 benjaoming

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

See pip error: The conflict is caused by: sphinx-rtd-theme 1.0.1a1 depends on docutils<0.18

The branch from @benjaoming however installs correctly, but I do not need docutils 0.19 yet (due to incompatibility with some other packages) pip install git+https://github.com/benjaoming/[email protected] works.

Any clue why?

twodrops avatar Sep 23 '22 10:09 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 avatar Oct 03 '22 13:10 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 :+1:

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 avatar Oct 03 '22 14:10 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 https://github.com/readthedocs/sphinx_rtd_theme/pull/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 https://github.com/readthedocs/sphinx_rtd_theme/pull/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: https://github.com/sphinx-doc/sphinx/issues/9807

benjaoming avatar Oct 04 '22 12:10 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 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.

twodrops avatar Oct 04 '22 15:10 twodrops

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

benjaoming avatar Oct 04 '22 16:10 benjaoming

@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 :disappointed:

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 https://github.com/readthedocs/sphinx_rtd_theme/pull/1336

For next release (1.2/2.0), :+1: 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.

agjohnson avatar Oct 04 '22 23:10 agjohnson

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 avatar Oct 05 '22 06:10 benjaoming

@pradyunsg re: https://github.com/pradyunsg/furo/discussions/447#discussioncomment-2863617 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?

benjaoming avatar Oct 05 '22 06:10 benjaoming

https://github.com/pradyunsg/furo/commit/d955850a89310a932ecab4e84cba0c20371a6a33

I tweaked the look, preserved docutils 0.17 compat, and filed https://github.com/sphinx-doc/sphinx/issues/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.

pradyunsg avatar Oct 05 '22 08:10 pradyunsg

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 avatar Oct 05 '22 08:10 benjaoming

@agjohnson the visual regression is now addressed in https://github.com/readthedocs/sphinx_rtd_theme/pull/1351

benjaoming avatar Oct 05 '22 12:10 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 https://github.com/readthedocs/sphinx_rtd_theme/pull/1304#issuecomment-1266924478

benjaoming avatar Dec 15 '22 12:12 benjaoming

Released in 1.2.0rc1 :+1:

benjaoming avatar Dec 16 '22 15:12 benjaoming

In mean time docutils 0.19 has been released πŸ˜‹ It would be good to move to that new version as well πŸ˜„

kloczek avatar Dec 16 '22 15:12 kloczek

@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 avatar Dec 16 '22 16:12 benjaoming

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

benjaoming avatar Dec 16 '22 16:12 benjaoming

Thank you for the update πŸ‘ πŸ˜„

kloczek avatar Dec 16 '22 18:12 kloczek