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 Sphinx and requirements #7443

Closed
wants to merge 1 commit into from

Conversation

mhilbrunner
Copy link
Member

@mhilbrunner mhilbrunner commented May 30, 2023

In the vein of #7436, this is a start on updating the version of Sphinx and its requirements we're using as everything is getting kinda old (we use Sphinx 4.x, current is 7.x; this PR updates it to 5.x at least).

Updating to Sphinx 6.x currently seems to break some things (like the sidebar menu toggling) which we need to investigate.

Checked in a local build:

  • General build and layout
  • Sidebar menu
  • Images
  • Code tabs
  • Code highlighting

Things that seem iffy/may need testing:

  • Search

Supersedes #6575, #6755, #6749
Sphinx changelog: https://www.sphinx-doc.org/en/master/changes.html#release-5-0-2-released-jun-17-2022

@mhilbrunner mhilbrunner added enhancement dependencies Pull requests that update a dependency file python Pull requests that update Python code cherrypick:4.0 labels May 30, 2023
@YuriSizov
Copy link
Contributor

as everything is getting kinda old

It would be nice to document somewhere, preferably in this and similar PRs, what are the main benefits from updating to the newer versions. Not to sound like a luddite, but since it works right now and it is not a security concern, we could just keep it as is.

Updating would likely require us to rewrite our custom styles and scripts to keep working with newer versions (as you mention, sidebar is broken in 6+ as an example, every time we update the theme our style overrides break in unpredictable places). And such work doesn't come for free.

@mhilbrunner
Copy link
Member Author

mhilbrunner commented May 30, 2023

but since it works right now and it is not a security concern, we could just keep it as is.

My feeling is that the longer we wait, the more effort updating is going to take (because everything will break, instead of a small breakage here or there that is easier to locate and fix), and eventually we will need to update.

Something like #7289: we are forced to update a single component (like the OS), which may then require us to live with a newer Python, which requires a newer Sphinx, or which requires newer dependencies etc.
Or a security issue is discovered, we now need to update a component, but doing that takes a lot of time as everything grew outdated.

That said, I agree this is a consideration, and currently there is nothing forcing us to update. I'd prefer to do it on our terms, instead of it coming back to haunt us :)

This PR is intended as "research how much work is needed to update dependencies", and if it takes too much work for now, we can postpone. My initial version updated to 6.x/7.x, but a lot broke; so I went with 5.x and so far not too much seems to break at first glance.

@mhilbrunner mhilbrunner marked this pull request as draft May 30, 2023 16:33
@YuriSizov
Copy link
Contributor

Yeah, that's fair. If we don't update regularly, we may be in a bad place to do that by the time it's actually required. Though I don't see anything particularly critical that might arise in the future. It's just a PITA to adjust our stuff all the time. Perhaps we should implement it in a less hacky way. For example, we can actually fork the theme and make our adjustments a native part of our forked theme.

@Piralein
Copy link
Member

  • the sphinx_rtd_theme is not compatible with sphinx 7 at the moment, the highest version we can choose is 6.2.1
  • The sidebar toggling is broken because sphinx 6.0 removed jquery, adding it manually restores the functionality. (https://www.sphinx-doc.org/en/master/changes.html#id91)

I did a complete build with 6.2.1 and got a lot of svg errors, not sure what this is about.

@YuriSizov
Copy link
Contributor

We also extend templates, both core and from the RTD theme. So we need to double-check that our overrides don't erase anything added by newer versions.

@mhilbrunner
Copy link
Member Author

@Piralein Good call on JQuery, thats indeed an easy fix. From what I can see the SVG errors are more like warnings (they added social media preview card support, which also support showing images on the page, but SVG isn't supported for that so it generates warnings for every page where it would pick a SVG as the first image to preview and does nothing; which IMO is okay)

@paddy-exe paddy-exe marked this pull request as ready for review May 31, 2023 14:18
@mhilbrunner mhilbrunner marked this pull request as draft May 31, 2023 14:20
@Calinou
Copy link
Member

Calinou commented Jun 6, 2023

Updating Sphinx could resolve the issue with make latexpdf not working due to sphinx-doc/sphinx#10903. I'd like to add PDF builds to the offline CI workflow in the long run, as the large ePubs we currently build have many issues opening on old/low-end devices.

@Piralein
Copy link
Member

Piralein commented Oct 2, 2023

I tested locally with:

sphinx==6.2.1
sphinx_rtd_theme==1.3.0
sphinx-tabs==3.4.1
sphinx-copybutton==0.5.2
sphinx-notfound-page==1.0.0
sphinxext-opengraph==0.8.2

All modules seem to be compatible. There are only 2 issues:

  1. The warnings with social cards - added by sphinxext-opengraph
    This can be disabled, see: https://github.com/wpilibsuite/sphinxext-opengraph/blob/main/docs/source/socialcards.md
ogp_social_cards = {
    "enable": False
}
  1. our config prevents the sphinx_rtd_theme to load the sphinxcontrib.jquery module.
    jQuery not loaded automatically with sphinxcontrib-jquery 4.0.0 readthedocs/sphinx_rtd_theme#1452
    It works by removing html_theme_path = [sphinx_rtd_theme.get_html_theme_path()].
    This seems to be some outdated configuration no longer used. The documentation of https://sphinx-rtd-theme.readthedocs.io/en/stable/installing.html doesn't mention it.

we could also add it to the extension ourself:

extensions = [
    "sphinxcontrib.jquery",
]

@Piralein
Copy link
Member

pygments==2.18.0

sphinx==6.2.1
sphinx_rtd_theme==1.3.0

sphinx-tabs==3.4.5
sphinx-copybutton==0.5.2
sphinx-notfound-page==1.0.2
sphinxext-opengraph==0.9.1

sphinxcontrib-applehelp==1.0.8
sphinxcontrib-htmlhelp==2.0.5
sphinxcontrib-qthelp==1.0.7
sphinxcontrib-serializinghtml==1.1.10
sphinxcontrib-devhelp==1.0.6

sphinxcontrib-video==0.2.1

My current setup, I haven't discovered any problems yet. (with the changes above)

@mhilbrunner
Copy link
Member Author

Superseded by #10158.

@mhilbrunner mhilbrunner closed this Nov 2, 2024
@mhilbrunner mhilbrunner deleted the update-req branch November 2, 2024 14:32
@mhilbrunner mhilbrunner added archived and removed enhancement dependencies Pull requests that update a dependency file python Pull requests that update Python code cherrypick:4.0 cherrypick:4.1 labels Nov 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants