jbms / sphinx-immaterial

Adaptation of the popular mkdocs-material material design theme to the sphinx documentation system
https://jbms.github.io/sphinx-immaterial/
Other
196 stars 29 forks source link

Add graphviz integration #144

Closed jbms closed 2 years ago

2bndy5 commented 2 years ago

need to add graphviz to the readthedocs.yml dependencies:

--- a/.readthedocs.yaml
+++ b/.readthedocs.yaml
@@ -12,6 +12,9 @@ build:
     python: "3.9"
     nodejs: "16"

+  apt_packages:
+    - graphviz
+
 # Build documentation in the docs/ directory with Sphinx
 sphinx:
   configuration: docs/conf.py
jbms commented 2 years ago

need to add graphviz to the readthedocs.yml dependencies:

--- a/.readthedocs.yaml
+++ b/.readthedocs.yaml
@@ -12,6 +12,9 @@ build:
     python: "3.9"
     nodejs: "16"

+  apt_packages:
+    - graphviz
+
 # Build documentation in the docs/ directory with Sphinx
 sphinx:
   configuration: docs/conf.py

Thanks, just added that.

2bndy5 commented 2 years ago

xrefs in dot syntax?! I'll never use another theme again!

A while ago, I looked into changing the colors of a graph based on CSS vars, but I gave up when I saw the rendered output used a iframe element in the DOM.

jbms commented 2 years ago

xrefs in dot syntax?! I'll never use another theme again!

A bit hacky but I think it works well enough. This part could be upstreamed into the sphinx.ext.graphviz extension itself.

A while ago, I looked into changing the colors of a graph based on CSS vars, but I gave up when I saw the rendered output used a iframe element in the DOM.

Yeah there are many ways to embed the graphviz output, but just inlining the svg directly into the HTML provides the most control.

jbms commented 2 years ago

One difficulty with the fonts is that graphviz needs the font to be available at build time so that it can properly compute the size of the labels. This specifies the same font used by the theme, but it is hard to ensure that font will be available locally. Per #111 it would be nice to download the font as part of the build anyway, but the additional challenge is that graphviz seems to get the fonts in several different ways depending on how it is built.

2bndy5 commented 2 years ago

inlining the svg directly into the HTML provides the most control

I think there was a reason they chose to use an iframe in upstream. It was a little more than a while ago since I looked into it.

Per #111 it would be nice to download the font as part of the build anyway, but the additional challenge is that graphviz seems to get the fonts in several different ways depending on how it is built.

I was thinking the same thing.

jbms commented 2 years ago

inlining the svg directly into the HTML provides the most control

I think there was a reason they chose to use an iframe in upstream. It was a little more than a while ago since I looked into it.

sphinx.ext.graphviz doesn't use an iframe, but it does embed the svg as an <object> which prevents links from linking. I think it might be for older browser support (https://caniuse.com/?search=svg), or simply that it was implemented before inline svg was supported. In any case it seems inline svg is supported by all common browsers for several years.

jbms commented 2 years ago

I have now added support for automatically downloading the fonts, both to fix #111 and also so that they can provided in TTF format to Graphviz.

2bndy5 commented 2 years ago

This may be off topic, but I figured I'd mention it now that this aims to solve #111. I think the fetching of mermaid.js should be added to external_resource_cache. Currently, the sphinx_immaterial.mermaid_diagrams ext is on by default because the theme src seems to be fetching the mermaid.js resource regardless.

jbms commented 2 years ago

This may be off topic, but I figured I'd mention it now that this aims to solve #111. I think the fetching of mermaid.js should be added to external_resource_cache. Currently, the sphinx_immaterial.mermaid_diagrams ext is on by default because the theme src seems to be fetching the mermaid.js resource regardless.

Agreed, and also for MathJax. But that could be deferred for a separate PR.

2bndy5 commented 2 years ago

This runs fine locally on my Windows machine. Maybe there's an extra step that we need to do in CI to make it work like an end-user machine (like using a venv to ensure PATH env var is adequate??).

I get no warnings in local (fresh and consecutive) builds. Fetching the fonts only happens on fresh doc builds (as expected). Just for completeness, this is what I see when the fonts are fetched:

Fetching: https://fonts.gstatic.com/s/roboto/v30/KFOlCnqEu92Fr1MmSU5fBxc4EsA.woff2 with {}

The commit you're using to install graphviz: ts-graphviz/setup-graphviz@f1590cf4e989dd1429bff4fd6339e098050a1657 seems a bit out of date. That action has seen a release 2 days ago that would allow specifying a specific version of graphviz for different OS runners, but I'm not sure if that is helpful.

2bndy5 commented 2 years ago

For the record, that last comment was running graphviz v2.50. I just installed graphviz v5.0, and it still ran fine (from a fresh doc build).

2bndy5 commented 2 years ago

This error may actually be related to the contents of the dot file. Our .gitattributes doesn't enforce LF for dot files... Maybe the dot exe expects LF, but the Windows runner checks out the dot file with CRLF?

Additionally, I'd like to propose changing the dot file to use a radial layout to distinguish the advantage of using graphviz over mermaid.js (aside from latex support and well-seasoned age).

Can I push a commit to this branch?

jbms commented 2 years ago

This error may actually be related to the contents of the dot file. Our .gitattributes doesn't enforce LF for dot files... Maybe the dot exe expects LF, but the Windows runner checks out the dot file with CRLF?

I'll have to look into it more. It appears that dot exited with return code of 0, but generated non-empty stderr after stripping whitespace, which is shown as a warning, but the output looks empty in the logs. Maybe there is a special character getting in somehow.

Additionally, I'd like to propose changing the dot file to use a radial layout to distinguish the advantage of using graphviz over mermaid.js (aside from latex support and well-seasoned age).

In general I think graphviz is more powerful and much more standard, but mermaid has some nice built-in graph styles. I wonder if there is a mermaid to dot converter.

Can I push a commit to this branch?

Feel free to push commits.

2bndy5 commented 2 years ago

I'm not entirely sure the font is being used correctly (on Windows) to calculate the sizes.

RTD output Local (Windows) output
image image
image image
jbms commented 2 years ago

The reason for using a junction of windows is that my understanding is that symlinks aren't supported on most/all versions of windows by default, they have to be explicitly enabled globally first. In contrast, junctions are always available. However, I think there might be some restrictions on junctions across filesystems that don't apply to symlinks, so there could still be some advantage in using symlinks on Windows when supported.

2bndy5 commented 2 years ago

they have to be explicitly enabled globally first

I've never had to do this. It might be true for peculiar editions of Windows (like "enterprise" or whatever they call the build that gets sold to corporate entities).

I think there might be some restrictions on junctions across filesystems that don't apply to symlinks

This might be what was failing in CI because the github runner's use docker containers.

jbms commented 2 years ago

Are you still observing bad font metrics in your local build on Windows?

2bndy5 commented 2 years ago

Yes. I'm not sure how to get graphviz to resolve the font from the given path. See https://github.com/jbms/sphinx-immaterial/pull/144#discussion_r950749998

jbms commented 2 years ago

The problem appears to be that the normal build of Graphviz for Windows excludes the "libgd" plugin, while on Linux builds it is normally excluded. Instead, on Windows there is a gdiplus plugin that provides text layout based on the Windows GDI+ API.

It looks like there are two possible ways to inject custom fonts:

2bndy5 commented 2 years ago

I would first try using the AppData approach. Instead of using %HOME%, I think it'd be better to use %USERPROFILE% which is the older equivalent to ~ on Linux.

I've never compiled anything with the GDI lib, so that seems like an interesting challenge (assuming you need me for my windows platform). The DLL injection sounds more like an exploit than a hack though.

jbms commented 2 years ago

I filed this issue with graphviz: https://gitlab.com/graphviz/graphviz/-/issues/2267

It sounds like the issue is that they are relying on some pre-built binaries for all of the dependencies on Windows, and are missing libgd for x86_64 Windows, so it is excluded. Ideally they would setup a continuous build for graphviz that builds all of the dependencies from source, at least on Windows where they have to be bundled. It looks like currently there is no automated process for building the dependencies; the binaries were just manually obtained somehow at some point and rarely updated.

Regarding adding the fonts the the AppData directory, I'm somewhat hesitant to go that way because then we will be persistently installing a font for the user, which is an unexpected outcome of just doing a documentation build. I believe the DLL injection is sometimes used for exploits, but here I think of it as essentially just monkey patching taken to the next level.

2bndy5 commented 2 years ago

I was thinking about raising an issue over there too. It shouldn't be this hard to specify a font.

It looks like currently there is no automated process for building the dependencies; the binaries were just manually obtained somehow at some point and rarely updated.

The docs say that the dev env for Windows build uses submodules, but I haven't tried to build it myself.


So, I tried manually copying the Roboto.ttf font into my %USERPROFILE%/local/Microsoft/Windows/Fonts. But, I'm still getting the "unable to resolve font" in verbose output. I did narrow down the source of that prompt to -Nfontname option BTW.

2bndy5 commented 2 years ago

I'm ok with merging this as long as there's a warning about experimental/developmental support for Windows. The dev cycle for graphviz seems sparse, and I don't see a lot of feature or patch releases over the last few major versions.

As it is now the docs build on Windows and use the correct font in HTML (despite the inaccurate node sizes).

jbms commented 2 years ago

This still doesn't correctly handle the fonts on Windows, but the option to suppress the warning there works correctly and CI passes.