Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update RTD to 2.0.0 and fix search #12253

Merged
merged 4 commits into from
Apr 15, 2024
Merged

Update RTD to 2.0.0 and fix search #12253

merged 4 commits into from
Apr 15, 2024

Conversation

Firehawke
Copy link
Contributor

@Firehawke Firehawke commented Apr 15, 2024

DO NOT MERGE THIS WITHOUT REVIEW

Important: Due to the way that the RTD theme imports the search script (it's different between localhost and online due to ajax handling) I was unable to completely test this. Be ready to back this back out immediately if it breaks in production. It SHOULD work, but I can't 100% guarantee it and it's not something I can easily test..

As for what changed:

Three issues needed to be resolved:

First, Sphinx no longer inherently carries JQuery, so any RTD calls to JQuery broke (e.g. search).
Secondly, RTD 2.0.0 came out back in November, and that's going to ne needed going forward for future Sphinx compatibility.
Unfortunately, RTD 2.0.0 still tries to call out to JQuery for search, as noted in #12181

The best workaround as things stand is to update our RTD to V2.0.0, make necessary adjustments to the theme and download template I created back in 2021, and hold things in this place until the RTD team actually fixes their own stuff upstream. Considering it's been three months since the last commit on their theme repo and no response to the issue filed on the search issue, it could be a while before any kind of fix happens.

I'm not entirely thrilled with the results, but it's better than having no search at all (and potentially a completely broken docs site with future Sphinx versions). I'll take a look at refactoring the download panel template at some point in the near future; the changes I had to make were related to the old version of the template completely breaking with the new theme version.

@Firehawke
Copy link
Contributor Author

Thanks to Balrog, I've been able to test the changes properly. It should be OK to merge.

Comment on lines 40 to +47
# See http://www.sphinx-doc.org/en/stable/theming.html#distribute-your-theme-as-a-python-package
def setup(app):
if python_version[0] < 3:
logger.warning("Python 2 is deprecated with sphinx_rtd_theme, update to Python 3")
app.require_sphinx('1.6')
if sphinx_version <= (2, 0, 0):
logger.warning("Sphinx 1.x is deprecated with sphinx_rtd_theme, update to Sphinx 2.x or greater")
if not app.config.html_experimental_html5_writer:
logger.warning("'html4_writer' is deprecated with sphinx_rtd_theme")
else:
if app.config.html4_writer:
logger.warning("'html4_writer' is deprecated with sphinx_rtd_theme")
logger.error("Python 2 is not supported with sphinx_rtd_theme, update to Python 3.")

app.require_sphinx('5.0')
if app.config.html4_writer:
logger.error("'html4_writer' is not supported with sphinx_rtd_theme.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. the URL is outdated, resolves to 404;
  2. https://github.com/readthedocs/sphinx_rtd_theme diverges greatly, including giving back Dockerfile option for local compiling. So it's not really using __version__ = '2.0.1alpha1'

Copy link
Contributor Author

@Firehawke Firehawke Apr 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a direct pull off their git repo for RTD 2.0.0, no other changes were made. It's safe to say at this point that the RTD 2.0.0 release is a mess at the upstream side of things.

@cuavas cuavas merged commit 7542e0a into mamedev:master Apr 15, 2024
1 check passed
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.

3 participants