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 v4.0.0 #364

Merged
merged 16 commits into from
Sep 15, 2021
Merged

UPDATE: Sphinx v4.0.0 #364

merged 16 commits into from
Sep 15, 2021

Conversation

choldgraf
Copy link
Member

@choldgraf choldgraf commented Aug 3, 2021

This PR enables sphinx>4 to be supported

@choldgraf
Copy link
Member Author

any reason not to just merge this @chrisjsewell ?

@chrisjsewell
Copy link
Member

any reason not to just merge this @chrisjsewell ?

Yeh should be fine if it’s passing tests, although it’s always good to have a quick look at https://www.sphinx-doc.org/en/master/extdev/deprecated.html and think if anything applies here

@choldgraf
Copy link
Member Author

good catch @chrisjsewell - logo is deprecated in favor of logo_url in V4, so I added a check for this and also noted when to remove it

Copy link
Member

@chrisjsewell chrisjsewell left a comment

Choose a reason for hiding this comment

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

@choldgraf I note that both the tests (https://github.com/executablebooks/sphinx-book-theme/pull/364/checks?check_run_id=3263717973) and document's build (https://readthedocs.org/projects/sphinx-book-theme/builds/14417635/) are still not being actually run with sphinx version 4.

So obviously this isn't yet testing compatibility with v4 😬 and we need to look what is still pinning to v3

@choldgraf
Copy link
Member Author

@chrisjsewell ah OK lemme take a look at the test infra, still getting the hang of tox

@choldgraf
Copy link
Member Author

choldgraf commented Aug 6, 2021

Latest commit adds Sphinx 4 to the testing matrix and removes Sphinx 2. Also updates the dependencies to be alphabetical :-)

edit: argh ReadTheDocs is failing because MyST-NB doesn't yet support Sphinx 4, so this PR is blocked on executablebooks/MyST-NB#338

@mmcky
Copy link
Member

mmcky commented Sep 2, 2021

@choldgraf there are test failures when I upgrade to myst-nb~=0.13 for testing. I will look more into this tomorrow morning but have reverted the change for now -- might be due to docutils change.

setup.py Outdated Show resolved Hide resolved
@choldgraf
Copy link
Member Author

@mmcky I got all the tests passing :-) could you give a quick review?

@choldgraf choldgraf requested a review from mmcky September 2, 2021 20:23
mmcky
mmcky previously approved these changes Sep 2, 2021
Copy link
Member

@mmcky mmcky left a comment

Choose a reason for hiding this comment

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

thanks for finishing this off @choldgraf looking good.

docs/conf.py Outdated
@@ -60,7 +60,7 @@
# "linkify",
# "substitution",
]
myst_url_schemes = ("http", "https", "mailto")
myst_url_schemes = ["http", "https", "mailto"]
Copy link
Member

Choose a reason for hiding this comment

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

@choldgraf this is now the default in myst-parser so we can remove this now.

@@ -1,7 +1,11 @@
<div class="navbar-brand-box">
<a class="navbar-brand text-wrap" href="{{ pathto('index') }}">
{% if logo %}
<img src="{{ pathto('_static/' + logo, 1) }}" class="logo" alt="logo">
<!-- `logo` is deprecated in Sphinx 4.0, so remove this when we stop supporting 3 -->
Copy link
Member

Choose a reason for hiding this comment

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

@choldgraf good find! Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

Oh cool it looks like there are html_logo and latex_logo now

https://www.sphinx-doc.org/en/master/usage/configuration.html?highlight=logo#confval-html_logo

This is something we will need to manage in jupyter-book to issue a depcration notice for logo in jupyter-book/jupyter-book#1438

Copy link
Member Author

Choose a reason for hiding this comment

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

we may not need to deprecate it if we are managing it under the hood though!

Copy link
Member

Choose a reason for hiding this comment

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

OK -- if html or latex isn't specified use logo -- I see where you are going

@mmcky
Copy link
Member

mmcky commented Sep 2, 2021

https://www.sphinx-doc.org/en/master/extdev/deprecated.html

@chrisjsewell that is a great link. Thanks 👍

@mmcky
Copy link
Member

mmcky commented Sep 2, 2021

  • @choldgraf this fails under sphinx3 against a fixture so I will add in sphinx3 and sphinx4 fixtures and will run tests againts both major sphinx versions

@chrisjsewell
Copy link
Member

chrisjsewell commented Sep 2, 2021

Guys, FYI it is going to be a bit more of change than this, to support docutils 0.17.

It now uses a bunch of semantic HTML tags: <main>, <section>, <header>, <footer>, <aside>, <figure>, and <figcaption>, instead of e.g. <div class="section"> (see sphinx-doc/sphinx#9001)

It will break lots of current CSS, like:

> div > div > div.section, .prev-next-bottom {

@choldgraf
Copy link
Member Author

choldgraf commented Sep 2, 2021

@chrisjsewell I think this means that we will need to choose to only support docutils 0.17 and up, or else we're going to have to maintain a bunch of CSS that is meant for pre/post 0.17, right?

For some reason our RTD builds are using docutils ~ 0.16: https://readthedocs.org/projects/sphinx-book-theme/builds/14623897/

that's why we weren't catching the CSS regressions

@mmcky
Copy link
Member

mmcky commented Sep 2, 2021

ah docutils==0.17!

Alternatively we could keep docutils>=0.15,<0.17 and then upgrade docutils to support 0.17 separately?

I don't think there is anything in the stack that uses 0.17 as a minimum yet.

@chrisjsewell
Copy link
Member

I think this means that we will need to choose to only support docutils 0.17 and up, or else we're going to have to maintain a bunch of CSS that is meant for pre/post 0.17, right?

I guess firstly I would have a look at what other themes are doing about this, and obviously we likely also require pydata to make updates as well, so at least for now we will likely have to pin to <0.17

You can possibly do e.g. div.section,section. to support both; don't know if there is anywhere that could cause issues

@mmcky
Copy link
Member

mmcky commented Sep 2, 2021

Opened an issue to track docutils==0.17 support #392

@mmcky
Copy link
Member

mmcky commented Sep 3, 2021

@choldgraf just updated the branch protection rules. I learnt about this yesterday from @chrisjsewell :-)

Update: Hot tip -- helps to hit save 👍

The rules were updated to include the sphinx versions in the testing matrix

@choldgraf
Copy link
Member Author

As long as we are not creating version conflicts with other parts of the stack then that works for me

@mmcky
Copy link
Member

mmcky commented Sep 3, 2021

As long as we are not creating version conflicts with other parts of the stack then that works for me

I have been copying myst-parser which is docutils>=0.15,<0.18 so sphinx-book-theme would impose 0.16 in preference to 0.17 but as I understand it should be able to resolve in the stack given the ranges. But maybe we should add #392 as a priority item.

@choldgraf
Copy link
Member Author

@mmcky anything left of this one or are you just waiting for someone to approve?

@mmcky
Copy link
Member

mmcky commented Sep 8, 2021

@choldgraf just waiting on an approval I think this is ready to 🚢

pbauer added a commit to plone/training that referenced this pull request Sep 8, 2021
@choldgraf
Copy link
Member Author

choldgraf commented Sep 15, 2021

ok let's ship this one so that we aren't holding everybody back from upgrading to Sphinx 4 :-)

THANK YOU @mmcky 🎉 and @chrisjsewell for spotting the HTML bugs that we were going to run into w/ docutils

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