executablebooks / sphinx-book-theme

A clean book theme for scientific explanations and documentation with Sphinx
https://sphinx-book-theme.readthedocs.io
BSD 3-Clause "New" or "Revised" License
432 stars 197 forks source link

FIX: use `config-inited` event to register config #814

Closed agoose77 closed 7 months ago

agoose77 commented 7 months ago

Fixes #768, obviates need for executablebooks/jupyter-book#2111

I'll restate the rationale here:

The registration order is as follows:

Crucially:

  1. Extensions are registered first.
    • Any theme variables set in setup() are subsequently clobbered by sphinx::Config.init_values.
    • setup() is called exclusively once for a theme/extension, so these variables are never set again.
    • Extensions must use e.g. config-inited to set this value before the theme is required.
  2. Themes are registered later.
    • Setting theme config directly in setup() ensures that the theme search path (in create_template_bridge is correct.
    • We can't use an event for this because the theme is set-up immediately (in sphinx::Registry.load_extension) before the path is required (in sphinx::Builder.create_template_bridge) i.e. before any further events are dispatched.

What's happening in JB and other users is that they're using SBT as both a theme and an extension, so we start at the top of the registration order. Thus, we need to support both use cases, i.e. set config twice. This is acceptable because due to the registration system we either see

Finally, it should be noted that SBT needs to be in extensions whilst it adds features (the margin directive) that are non-HTML focused.

agoose77 commented 7 months ago

@ghisvail, @adam-grant-hendry, @choldgraf I'm pinging you three as the most recent investigators of the issue with SBT's use in extensions.

This PR makes the changes suggested by @adam-grant-hendry in a closed PR, which is to convert the builder-inited listener to a config-inited listener. Above in the PR description I've outlined why I think that's the appropriate solution over removing SBT from extensions, but I'd appreciate some additional input here to ensure I've not missed anything.

If you don't have any bandwidth for this, please let me know! I'll just make a release and we'll deal with the fall-out through normal channels (if there is any!).

ghisvail commented 7 months ago

I am not familiar with the specifics of Sphinx et al.

All I can offer is to test whether things work appropriately with the new release.

frgigr commented 7 months ago

Dear all, sorry to reopen this thread, but I'm quite sure this MR breaks the Jupyter-Book compilation. The error message is the following one:

Extension error (sphinx_book_theme):
Handler <function update_general_config at 0x7f09a7474790> for event 'config-inited' threw an exception (exception: update_general_config() takes 1 positional argument but 2 were given)
Traceback (most recent call last):
  File "/home/user/.local/lib/python3.9/site-packages/sphinx/events.py", line 97, in emit
    results.append(listener.handler(self.app, *args))
TypeError: update_general_config() takes 1 positional argument but 2 were given

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/home/user/.local/lib/python3.9/site-packages/jupyter_book/sphinx.py", line 114, in build_sphinx
    app = Sphinx(
  File "/home/user/.local/lib/python3.9/site-packages/sphinx/application.py", line 257, in __init__
    self.events.emit('config-inited', self.config)
  File "/home/user/.local/lib/python3.9/site-packages/sphinx/events.py", line 108, in emit
    raise ExtensionError(__("Handler %r for event %r threw an exception") %
sphinx.errors.ExtensionError: Handler <function update_general_config at 0x7f09a7474790> for event 'config-inited' threw an exception (exception: update_general_config() takes 1 positional argument but 2 were given)

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/home/user/.local/bin/jupyter-book", line 8, in <module>
    sys.exit(main())
  File "/home/user/.local/lib/python3.9/site-packages/click/core.py", line 1130, in __call__
    return self.main(*args, **kwargs)
  File "/home/user/.local/lib/python3.9/site-packages/click/core.py", line 1055, in main
    rv = self.invoke(ctx)
  File "/home/user/.local/lib/python3.9/site-packages/click/core.py", line 1657, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/home/user/.local/lib/python3.9/site-packages/click/core.py", line 1404, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/home/user/.local/lib/python3.9/site-packages/click/core.py", line 760, in invoke
    return __callback(*args, **kwargs)
  File "/home/user/.local/lib/python3.9/site-packages/jupyter_book/cli/main.py", line 317, in build
    builder_specific_actions(
  File "/home/user/.local/lib/python3.9/site-packages/jupyter_book/cli/main.py", line 528, in builder_specific_actions
    raise RuntimeError(_message_box(msg, color="red", doprint=False)) from result
RuntimeError: 
===============================================================================

There was an error in building your book. Look above for the cause.

===============================================================================

Steps to reproduce:

No issues found when reverting to 1.1.0 version. Thanks in advance and apologies if the culprit is not here!

agoose77 commented 7 months ago

@frgigr that's a new bug! Thanks for reporting it, will issue a fix.