diff mbox series

[RFC,3/3] convert SVGs to PDF and PNG using rsvgconverter plugin

Message ID 20251030-fix-make-multi-target-v1-3-213616ed1f0a@cherry.de
State Superseded
Headers show
Series fix epub and latexpdf targets not finding glob images | expand

Commit Message

Quentin Schulz Oct. 30, 2025, 3:17 p.m. UTC
From: Quentin Schulz <quentin.schulz@cherry.de>

The rsvgconverter plugin allows to generate a PDF or PNG from an SVG.
This is what we already do manually via Make targets but it isn't good
enough.

Sphinx generates a cache upon first parsing and stores it in a doctrees
directory. Sphinx claims that it can be shared between all builders[1].
For glob patterns in image or figure directives, Sphinx will look for
all possible matches in the source tree and store those in a
"candidates" map for each file, along with the associated mimetype. When
building, Sphinx will then look in this map to try to find a match in
the current builder's supported_image_types. If none is found, the build
will fail.

The latexpdf (using the LaTeXBuilder) target does not support SVGs by
default[2]. We used Make to generate PDFs from them before generating
the doc PDF though (see PDFs variable and %.pdf in Makefile) and that
type is supported by default[2].

The epub (via the Epub3Builder) target does support SVGs by default[3]
but we disabled their support in commit ff3876ca4910 ("conf.py: use PNG
first in EPUB output"). We used Make to generate PNGs from them before
generating the doc epub though (see PNGs variable and %.png in Makefile)
and that type is supported (c.f. Epub3Builder.supported_image_types in
our conf.py).

The issue is that this is done transparently from Sphinx. When we
generate the PDFs or PNGs variants of the SVGs, we put them in-tree
directly along their source file. Then, when caching, Sphinx will find
both the source file and the appropriate variant. However, the cache
isn't updated if there are new files in the tree and the source rST
files aren't modified. So, the cache will not have its map updated and
we won't be able to find the new variant when building for a
non-SVG-compatible builder. Take the following scenario:

- start from a clean source file (fresh clone or git clean -ffdx)
- build the html target (which supports SVGs by default[4])
- sphinx will find all the files in the source tree matching the glob
  pattern in ".. image:: test.*", in our case only an SVG since the PDFs
  and PNGs are only generated for the latexpdf and epub targets
  respectively. The cache will only store the path to the SVG file
  because it is the only source file that matches the glob,
- attempt to build the epub target (which doesn't support SVGs, only
  PNGs)
- Sphinx checks for the file to include for '.. image:: test.*' and
  finds an SVG in the cache map and then check the list of supported
  image types for the Epub3Builder and find that it doesn't support
  that. It cannot find anything satisfying the dependency and thus fails
  to build.

This scenario can easily be reproduced by running the `make all` command
since the html builder will be used first, then epub and finally
latexpdf.

The `make publish` target works by chance, because the epub builder is
built first and will cache SVG + PNG for each glob, then the latexpdf
builder is built and supports PNGs so that's fine and then html, which
supports SVG as well.

To fix this issue, we could simply always generate PDFs and PNGs of all
SVGs in the source tree, but this isn't ideal.

Instead, let's use an ImageConverter from a Sphinx plugin. This allows
to map a plugin as being able to generate a file of type Y from a file
of type X. When Sphinx wants to build an image, it'll try to find the
image with the type the current builder supports in the cache. If it
cannot, it's going to try to find an ImageConverter plugin that is able
to convert one of the image types in cache with one of the image types
the current builder supports. Then Sphinx will call this plugin to
generate the file and put it into the build directory (not in the
source!).

This allows to simplify the Makefile as well and is a much cleaner
approach.

The epub target is removed as the catch-all target contains the same
instructions as the epub target we remove in this commit.

Ideally, we shouldn't use our in-tree fork but the upstream project
sphinxcontrib.rsvgconverter, though it currently only supports
generating PDFs from SVGs and we also need PNGs. A merge request to add
support for this is pending[5].

[1] https://www.sphinx-doc.org/en/master/man/sphinx-build.html#cmdoption-sphinx-build-d
[2] https://www.sphinx-doc.org/en/master/usage/builders/index.html#sphinx.builders.latex.LaTeXBuilder.supported_image_types
[3] https://www.sphinx-doc.org/en/master/usage/builders/index.html#sphinx.builders.epub3.Epub3Builder.supported_image_types
[4] https://www.sphinx-doc.org/en/master/usage/builders/index.html#sphinx.builders.html.StandaloneHTMLBuilder
[5] https://github.com/missinglinkelectronics/sphinxcontrib-svg2pdfconverter/pull/31

Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>
---
 documentation/Makefile | 26 +++-----------------------
 documentation/conf.py  |  5 +++++
 2 files changed, 8 insertions(+), 23 deletions(-)

Comments

Antonin Godard Nov. 7, 2025, 3:04 p.m. UTC | #1
Hi,

On Thu Oct 30, 2025 at 4:17 PM CET, Quentin Schulz wrote:
> From: Quentin Schulz <quentin.schulz@cherry.de>
>
> The rsvgconverter plugin allows to generate a PDF or PNG from an SVG.
> This is what we already do manually via Make targets but it isn't good
> enough.
>
> Sphinx generates a cache upon first parsing and stores it in a doctrees
> directory. Sphinx claims that it can be shared between all builders[1].
> For glob patterns in image or figure directives, Sphinx will look for
> all possible matches in the source tree and store those in a
> "candidates" map for each file, along with the associated mimetype. When
> building, Sphinx will then look in this map to try to find a match in
> the current builder's supported_image_types. If none is found, the build
> will fail.
>
> The latexpdf (using the LaTeXBuilder) target does not support SVGs by
> default[2]. We used Make to generate PDFs from them before generating
> the doc PDF though (see PDFs variable and %.pdf in Makefile) and that
> type is supported by default[2].
>
> The epub (via the Epub3Builder) target does support SVGs by default[3]
> but we disabled their support in commit ff3876ca4910 ("conf.py: use PNG
> first in EPUB output"). We used Make to generate PNGs from them before
> generating the doc epub though (see PNGs variable and %.png in Makefile)
> and that type is supported (c.f. Epub3Builder.supported_image_types in
> our conf.py).
>
> The issue is that this is done transparently from Sphinx. When we
> generate the PDFs or PNGs variants of the SVGs, we put them in-tree
> directly along their source file. Then, when caching, Sphinx will find
> both the source file and the appropriate variant. However, the cache
> isn't updated if there are new files in the tree and the source rST
> files aren't modified. So, the cache will not have its map updated and
> we won't be able to find the new variant when building for a
> non-SVG-compatible builder. Take the following scenario:
>
> - start from a clean source file (fresh clone or git clean -ffdx)
> - build the html target (which supports SVGs by default[4])
> - sphinx will find all the files in the source tree matching the glob
>   pattern in ".. image:: test.*", in our case only an SVG since the PDFs
>   and PNGs are only generated for the latexpdf and epub targets
>   respectively. The cache will only store the path to the SVG file
>   because it is the only source file that matches the glob,
> - attempt to build the epub target (which doesn't support SVGs, only
>   PNGs)
> - Sphinx checks for the file to include for '.. image:: test.*' and
>   finds an SVG in the cache map and then check the list of supported
>   image types for the Epub3Builder and find that it doesn't support
>   that. It cannot find anything satisfying the dependency and thus fails
>   to build.
>
> This scenario can easily be reproduced by running the `make all` command
> since the html builder will be used first, then epub and finally
> latexpdf.
>
> The `make publish` target works by chance, because the epub builder is
> built first and will cache SVG + PNG for each glob, then the latexpdf
> builder is built and supports PNGs so that's fine and then html, which
> supports SVG as well.
>
> To fix this issue, we could simply always generate PDFs and PNGs of all
> SVGs in the source tree, but this isn't ideal.
>
> Instead, let's use an ImageConverter from a Sphinx plugin. This allows
> to map a plugin as being able to generate a file of type Y from a file
> of type X. When Sphinx wants to build an image, it'll try to find the
> image with the type the current builder supports in the cache. If it
> cannot, it's going to try to find an ImageConverter plugin that is able
> to convert one of the image types in cache with one of the image types
> the current builder supports. Then Sphinx will call this plugin to
> generate the file and put it into the build directory (not in the
> source!).
>
> This allows to simplify the Makefile as well and is a much cleaner
> approach.
>
> The epub target is removed as the catch-all target contains the same
> instructions as the epub target we remove in this commit.

So with your patch will the epub target include SVGs or PNGs? Apparently there
were issues rendering SVGs in EPUBs as per Michael's commit:

    conf.py: use PNG first in EPUB output

    SVG directly included in EPUB output has multiple issues,
    in particular font size and alignment ones (tested on two
    EPUB readers). Instead, using PNG, generated from SVG when available
    as the primary format for images. GIF and JPEG are fine too.

> Ideally, we shouldn't use our in-tree fork but the upstream project
> sphinxcontrib.rsvgconverter, though it currently only supports
> generating PDFs from SVGs and we also need PNGs. A merge request to add
> support for this is pending[5].

Are you willing to send a patch to OE-Core to add a recipe for it?

Otherwise: I'm all for using this instead of our Makefile rules, this is much
cleaner.

But I can't say whether BSD / IANAL is fine to include in out repo though. I'd
rather have a recipe for it if possible! (possibly with your patch on top, if it
doesn't get applied?)

Thanks!
Antonin
Quentin Schulz Nov. 7, 2025, 3:22 p.m. UTC | #2
Hi Antonin,

On 11/7/25 4:04 PM, Antonin Godard wrote:
> Hi,
> 
> On Thu Oct 30, 2025 at 4:17 PM CET, Quentin Schulz wrote:
[...]
>> The epub target is removed as the catch-all target contains the same
>> instructions as the epub target we remove in this commit.
> 
> So with your patch will the epub target include SVGs or PNGs? Apparently there

I haven't checked but no intended change made to what the epub target 
will get compared to today, that is you should still have PNGs instead 
of SVGs. The overridden supported image/figures types are stored in 
conf.py at the end of the file, I don't modify them so should have the 
same behavior.

> were issues rendering SVGs in EPUBs as per Michael's commit:
> 
>      conf.py: use PNG first in EPUB output
> 
>      SVG directly included in EPUB output has multiple issues,
>      in particular font size and alignment ones (tested on two
>      EPUB readers). Instead, using PNG, generated from SVG when available
>      as the primary format for images. GIF and JPEG are fine too.
> 
>> Ideally, we shouldn't use our in-tree fork but the upstream project
>> sphinxcontrib.rsvgconverter, though it currently only supports
>> generating PDFs from SVGs and we also need PNGs. A merge request to add
>> support for this is pending[5].
> 
> Are you willing to send a patch to OE-Core to add a recipe for it?
> 

Yes, I will need to figure out how to build and test this docs toolchain 
first though :)

> Otherwise: I'm all for using this instead of our Makefile rules, this is much
> cleaner.
> 
> But I can't say whether BSD / IANAL is fine to include in out repo though. I'd
> rather have a recipe for it if possible! (possibly with your patch on top, if it
> doesn't get applied?)
> 

No, I will not do that as it'll prevent us from building without the 
docs toolchain.

If we want to fork the plugin, we'll need to have this made available in 
a publicly accessible git repo (on git.yoctoproject.org I guess?) so 
that we can simply pip install the git repo when building without the 
toolchain (can be done in the Pipfile and 
documentation/tools/host_packages_scripts/pip3_docs.sh). Then the recipe 
for the toolchain can be for that fork. I would rather wait for upstream 
feedback before this happens, hopefully all goes well and we get a new 
version tagged soon after.

Cheers,
Quentin
Antonin Godard Nov. 7, 2025, 3:30 p.m. UTC | #3
On Fri Nov 7, 2025 at 4:22 PM CET, Quentin Schulz wrote:
> Hi Antonin,
>
> On 11/7/25 4:04 PM, Antonin Godard wrote:
>> Hi,
>> 
>> On Thu Oct 30, 2025 at 4:17 PM CET, Quentin Schulz wrote:
> [...]
>>> The epub target is removed as the catch-all target contains the same
>>> instructions as the epub target we remove in this commit.
>> 
>> So with your patch will the epub target include SVGs or PNGs? Apparently there
>
> I haven't checked but no intended change made to what the epub target 
> will get compared to today, that is you should still have PNGs instead 
> of SVGs. The overridden supported image/figures types are stored in 
> conf.py at the end of the file, I don't modify them so should have the 
> same behavior.
>
>> were issues rendering SVGs in EPUBs as per Michael's commit:
>> 
>>      conf.py: use PNG first in EPUB output
>> 
>>      SVG directly included in EPUB output has multiple issues,
>>      in particular font size and alignment ones (tested on two
>>      EPUB readers). Instead, using PNG, generated from SVG when available
>>      as the primary format for images. GIF and JPEG are fine too.
>> 
>>> Ideally, we shouldn't use our in-tree fork but the upstream project
>>> sphinxcontrib.rsvgconverter, though it currently only supports
>>> generating PDFs from SVGs and we also need PNGs. A merge request to add
>>> support for this is pending[5].
>> 
>> Are you willing to send a patch to OE-Core to add a recipe for it?
>> 
>
> Yes, I will need to figure out how to build and test this docs toolchain 
> first though :)

It's built with this recipe:
https://git.openembedded.org/openembedded-core/tree/meta/recipes-core/meta/buildtools-docs-tarball.bb

You use it like a regular SDK afterwards.

>> Otherwise: I'm all for using this instead of our Makefile rules, this is much
>> cleaner.
>> 
>> But I can't say whether BSD / IANAL is fine to include in out repo though. I'd
>> rather have a recipe for it if possible! (possibly with your patch on top, if it
>> doesn't get applied?)
>> 
>
> No, I will not do that as it'll prevent us from building without the 
> docs toolchain.
>
> If we want to fork the plugin, we'll need to have this made available in 
> a publicly accessible git repo (on git.yoctoproject.org I guess?) so 
> that we can simply pip install the git repo when building without the 
> toolchain (can be done in the Pipfile and 
> documentation/tools/host_packages_scripts/pip3_docs.sh). Then the recipe 
> for the toolchain can be for that fork. I would rather wait for upstream 
> feedback before this happens, hopefully all goes well and we get a new 
> version tagged soon after.

Let's wait for upstream feedback, but in any case I was thinking of adding to
the buildtools-docs-tarball recipe (that's for the autobuilder, mainly), and
also to documentation/tools/host_packages_scripts/pip3_docs.sh.

Thanks,
Antonin
diff mbox series

Patch

diff --git a/documentation/Makefile b/documentation/Makefile
index bade78fe8..2d6baed65 100644
--- a/documentation/Makefile
+++ b/documentation/Makefile
@@ -13,8 +13,6 @@  SPHINXLINTDOCS ?= $(SOURCEDIR)
 IMAGEDIRS      = */svg
 BUILDDIR       = _build
 DESTDIR        = final
-SVG2PNG        = rsvg-convert
-SVG2PDF        = rsvg-convert
 
 ifeq ($(shell if which $(SPHINXBUILD) >/dev/null 2>&1; then echo 1; else echo 0; fi),0)
 $(error "The '$(SPHINXBUILD)' command was not found. Make sure you have Sphinx installed")
@@ -24,7 +22,7 @@  endif
 help:
 	@$(SPHINXBUILD) -M help "$(SOURCEDIR)" "$(BUILDDIR)" $(SPHINXOPTS) $(O)
 
-.PHONY: all checks help Makefile clean stylecheck publish epub latexpdf
+.PHONY: all checks help Makefile clean stylecheck publish latexpdf
 
 publish: Makefile checks epub latexpdf html singlehtml
 	rm -rf $(BUILDDIR)/$(DESTDIR)/
@@ -35,22 +33,8 @@  publish: Makefile checks epub latexpdf html singlehtml
 	cp $(BUILDDIR)/singlehtml/index.html $(BUILDDIR)/$(DESTDIR)/singleindex.html
 	sed -i -e 's@index.html#@singleindex.html#@g' $(BUILDDIR)/$(DESTDIR)/singleindex.html
 
-# Build a list of SVG files to convert to PDFs
-PDFs := $(foreach dir, $(IMAGEDIRS), $(patsubst %.svg,%.pdf,$(wildcard $(SOURCEDIR)/$(dir)/*.svg)))
-
-# Build a list of SVG files to convert to PNGs
-PNGs := $(foreach dir, $(IMAGEDIRS), $(patsubst %.svg,%.png,$(wildcard $(SOURCEDIR)/$(dir)/*.svg)))
-
-# Pattern rule for converting SVG to PDF
-%.pdf : %.svg
-	$(SVG2PDF) --format=Pdf --output=$@ $<
-
-# Pattern rule for converting SVG to PNG
-%.png : %.svg
-	$(SVG2PNG) --format=Png --output=$@ $<
-
 clean:
-	@rm -rf $(BUILDDIR) $(PNGs) $(PDFs) poky.yaml sphinx-static/switchers.js releases.rst
+	@rm -rf $(BUILDDIR) poky.yaml sphinx-static/switchers.js releases.rst
 
 checks:
 	$(SOURCEDIR)/tools/check-glossaries --docs-dir $(SOURCEDIR)
@@ -62,14 +46,10 @@  stylecheck:
 sphinx-lint:
 	sphinx-lint $(SPHINXLINTDOCS)
 
-epub: $(PNGs)
-	$(SOURCEDIR)/set_versions.py
-	@$(SPHINXBUILD) -M $@ "$(SOURCEDIR)" "$(BUILDDIR)" $(SPHINXOPTS) $(O)
-
 # Note: we need to pass buf_size here (which is also configurable from
 # texmf.cnf), to avoid following error:
 #   Unable to read an entire line---bufsize=200000. Please increase buf_size in texmf.cnf.
-latexpdf: $(PDFs)
+latexpdf:
 	$(SOURCEDIR)/set_versions.py
 	buf_size=10000000 $(SPHINXBUILD) -M $@ "$(SOURCEDIR)" "$(BUILDDIR)" $(SPHINXOPTS) $(O)
 
diff --git a/documentation/conf.py b/documentation/conf.py
index c07b6c419..64dd911ff 100644
--- a/documentation/conf.py
+++ b/documentation/conf.py
@@ -66,6 +66,11 @@  extensions = [
     'sphinx.ext.autosectionlabel',
     'sphinx.ext.extlinks',
     'sphinx.ext.intersphinx',
+    # our fork! Do not confuse for sphinxcontrib.rsvgconverter!
+    # FIXME: migrate to upstream once
+    # https://github.com/missinglinkelectronics/sphinxcontrib-svg2pdfconverter/pull/31
+    # is merged
+    'rsvgconverter',
     'yocto-vars'
 ]
 autosectionlabel_prefix_document = True