Conflict between sphinxcontrib.images and various other sphinx plugins during `make epub`

Mekk commented 3 years ago

(this definitely looks like sphinx or docutils bug, but I'd like to let you know + ask whether you have some clue)

In short

Enabling simultaneously sphinxcontrib.images and spinx.ext.todo or sphinx.ext.graphviz or various other plugins causes make epub to crash with dirty NotImplementedError while processing .. todo or similar directive. Other targets (make html, make pdf) work fine.

Looks like this is caused by sole fact that sphinxcontrib.images defines explicit epub callback. This somehow changes the processing chain.

I repeated it both on python2+sphinx1.8.5 and python3+sphinx3.5.4.

Longer story

I have sphinx document which uses sphinxext.todo and contains some .. todo: directives (and also have my custom extension of similar kind, which also shows the issue). I recently added sphinxcontrib.images there. And, out of the sudden:

make html and make pdf work fine

make epub crashes during output writing with

…
File "/home/marcink/.local/lib/python3.8/site-packages/sphinx/writers/html5.py", line 795, in unknown_visit
raise NotImplementedError('Unknown node: ' + node.__class__.__name__)
NotImplementedError: Unknown node: todo_node

I found out that disabling sphinxcontrib.images (commenting it out from extensions in conf.py) resolves the issue, then tried deeper.

sphinxcontrib.images has explicit epub callback: https://github.com/sphinx-contrib/images/blob/85da3e3227822e23f0ddc87326130a41cb4693de/sphinxcontrib/images.py#L337

Both sphinxext.todo and my custom extension do not have one.. For example sphinx/ext/todo.py has the following snippet in setup.py:

app.add_node(todo_node,
                 html=(visit_todo_node, depart_todo_node),
                 latex=(latex_visit_todo_node, latex_depart_todo_node),
                 text=(visit_todo_node, depart_todo_node),
                 man=(visit_todo_node, depart_todo_node),
                 texinfo=(visit_todo_node, depart_todo_node))

Editing this code (and editing my custom extension in the same way) resolves the issue. Once I I dirty-patched sphinx/ext/todo to:

app.add_node(todo_node,
                 html=(visit_todo_node, depart_todo_node),
                 epub=(visit_todo_node, depart_todo_node),
                 latex=(latex_visit_todo_node, latex_depart_todo_node),
                 text=(visit_todo_node, depart_todo_node),
                 man=(visit_todo_node, depart_todo_node),
                 texinfo=(visit_todo_node, depart_todo_node))

(added epub= line) whole issue disappeared, make epub started to work properly again.

So looks like „by default” sphinx nicely falls back to html callbacks while generating epub, but the fact that anybody in the ecosystem (here: sphinxcontrib.images) defined explicit epub handler changes something, and makes sphinx to work differently. And out of the sudden all directives which relied on fallback to html – cause crash.

Mekk commented 3 years ago

I reported it to sphinx, https://github.com/sphinx-doc/sphinx/issues/9219 Here treat it as „problem alert”.

Mekk commented 3 years ago

And just in case, adding the following snippet to conf.py resolves (or rather works around) the issue:

def monkeypatch_method(cls, fname=None):
    def decorator(func):
        local_fname = fname
        if local_fname is None:
            local_fname = func.__name__
        setattr(func, "orig", getattr(cls, local_fname, None))
        setattr(cls, local_fname, func)
        return func
    return decorator

import sphinx.application

@monkeypatch_method(sphinx.application.Sphinx)
def add_node(self, node, override=False, **kwds):
    if 'html' in kwds and 'epub' not in kwds:
        kwds['epub'] = kwds['html']
    return add_node.orig(self, node, override, **kwds)

jonascj commented 3 years ago

Thank you for reporting this and reporting it upstream (sphinx itself).

I'm not the original author of the extension so to me the following lines are quite hard to read. Exactly what do we assign as the epub handler? I'll look into that and see if the following lines can replaced by something more readable.

https://github.com/sphinx-contrib/images/blob/85da3e3227822e23f0ddc87326130a41cb4693de/sphinxcontrib/images.py#L322-L340

This extension, to my knowledge, only makes sense for HTML output, so for any other output (maybe with the exception of epub which is html-based itself?) it should fallback to using Sphinx's standard image directive.

But yes, it seems like a Sphinx issue, a corner case where if one extension declares an epub handler all others must as well (fall back not working). Your dirty-patch comment proves I'd say:

Editing this code (and editing my custom extension in the same way) resolves the issue. Once I I dirty-patched sphinx/ext/todo to:

app.add_node(todo_node, html=(visit_todo_node, depart_todo_node), epub=(visit_todo_node, depart_todo_node),

Mekk commented 3 years ago

For epub output your extension simply generates thumbnails (which are not clickable at this time). For my use case (epub is bonus option to take docs offline on ebook-reader or tablet but html is mostly used on daily basis) that's fine, one can read docs, know that some picture is there, and have reasonable layout.

It could of course be improved, for example by making „image-footnotes” chapter, and making those small pictures clickable as links to pages in such a chapter (which would consist of necessary large images, one per page). Mayhaps there are some other options (I don't know epub standard too well). That's by far outside this issue of course. For now sphinxcontrib.images behaviour is mostly fine, reasonable degradation on less suitable backend.

Mekk commented 3 years ago

And regarding your code, it seems to subscribe rest to backend's methods named like visit_image_node_html, visit_image_node_epub etc etc (and similarly depart), and if those are not found, fallback to visit_image_node_fallback and depart_image_node_fallback. LightBox backend doesn't offer any kind of specific epub support, so subscribing epub mostly serves to avoid calling lightbox version, and use fallback there.

After reconsideration: it matters. We likely don't want to get lightbox inside epub…

jonascj commented 3 years ago

Yes, that is the name of the backend methods, I just wondered why it wasn't typed out explicitly in 10-20 lines instead of done as it is through a function used in a dictionary comprehension.

I've used epub myself and never built any docs as epub before. The output is (x)html based and the epub output from sphinx with sphinxcontrib-images enabled includes the Lightbox2 files I can see (now that I did an epub build).

── epub
│   ├── content.opf
│   ├── genindex.xhtml
│   ├── _images
│   │   ├── 190c2700c9d8aac9d61e689d50821be51cb48dbc
│   │   ├── 210a42c7a5c6dfee575823193d8068b01404a25a
│   │   ├── f453ce6fa37bfa485a753fc8b2286dee1df0f620
│   │   ├── img.jpg
│   │   ├── Wikipedia-logo-v2_1x1.png
│   │   └── Wikipedia-logo-v2_1x.png
│   ├── index.xhtml
│   ├── META-INF
│   │   └── container.xml
│   ├── mimetype
│   ├── nav.xhtml
│   ├── sphinxcontrib-images.epub
│   ├── _static
│   │   ├── basic.css
│   │   ├── custom.css
│   │   ├── doctools.js
│   │   ├── documentation_options.js
│   │   ├── epub.css
│   │   ├── file.png
│   │   ├── jquery-3.5.1.js
│   │   ├── jquery.js
│   │   ├── language_data.js
│   │   ├── minus.png
│   │   ├── plus.png
│   │   ├── pygments.css
│   │   ├── searchtools.js
│   │   ├── sphinxcontrib-images
│   │   │   └── LightBox2
│   │   │       ├── lightbox2
│   │   │       │   ├── css
│   │   │       │   │   └── lightbox.css
│   │   │       │   ├── img
│   │   │       │   │   ├── close.png
│   │   │       │   │   ├── loading.gif
│   │   │       │   │   ├── next.png
│   │   │       │   │   └── prev.png
│   │   │       │   └── js
│   │   │       │       ├── jquery-1.11.0.min.js
│   │   │       │       ├── lightbox.min.js
│   │   │       │       └── lightbox.min.map
│   │   │       └── lightbox2-customize
│   │   │           └── jquery-noconflict.js
│   │   ├── underscore-1.12.0.js
│   │   └── underscore.js
│   └── toc.ncx

sphinx-contrib / images

Conflict between sphinxcontrib.images and various other sphinx plugins during `make epub` #22

In short

Longer story