-
Notifications
You must be signed in to change notification settings - Fork 122
Make docs build less platform-dependent #808
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
Conversation
|
This has to be fixed upstream: korsbo/Latexify.jl#343. Won't do much here. |
|
Once the fix is available upstream, all this PR does is remove |
These fonts are only used in the second table. The first table uses the standard fonts. Is there a specific glyph that is not “sufficiently clear” in the standard font and that only appears in the second table? IMO, having to install a font to build the docs is not ideal, especially since it does not error when the font is missing, but silently generates the table with the header and the first column just empty. |
I couldn't remember specifically what it was, but the default monospaced font has no glyph at all for pi: We could use Courier New instead (which is probably available by default on Windows and Mac), but I had a hard time ascertaining whether that would be available on the Ubuntu used by a GitHub runner (or how to conveniently install it). |
|
It looks like, as far as font for this LaTeX render, the options are:
I am happy to do whichever seems best, but lean probably away from option 1. |
|
We could use fontconfig to get the name of an installed monospace font (but I have not checked whether this font name can then be used to render the tables): It should even be possible to check whether the font has a Another option would be to render the individual cells in LaTeX and put them into a markdown table in the docs (the first column and the header would not be rendered by LaTeX, thus no font problems). But mixing the non-LaTeX text and the rendered PNGs would lead to inconsistent font sizes (depending on screen DPI). |
f5eeea7 to
965878e
Compare
|
Using Fontconfig turns out to be simple and easy and handle this perfectly-- |
|
Thanks for taking the time to improve this! Some comments:
|
I like that, would be happy to make that change.
That error on second build is a consequence of Plots still loading UnitfulLatexify as a dependency, which I can fix in JuliaPlots/Plots.jl#5174 once Unitful has a new release. (Interlinking dependencies have been a bit of a bugbear here.) |
|
I can confirm I can successfully build the docs multiple times if I If you add |
Should we add Plots to our |
I don't think so. |
|
Then I think we should retroactively mark UnitfulLatexify (all versions) as incompatible with Unitful ≥ 1.25 in the general registry. |
I intend to have new version of Plots require Unitful ≥ 1.25 and drop UnitfulLatexify, and UnitfulLatexify is now marked as having an upper bound of Unitful 1.23; would that be sufficient? |
|
No, because it would not prevent users from installing an old Plots, old UnitfulLatexify und new Unitful version. If they have those three packages in their Project.toml and run I don't know whether that is a real problem, since some users may just have Unitful and Plots in their Project.toml (with UnitfulLatexify being installed as weakdep of Plots). But if they need Latexify functionality independent from plotting, they might have UnitfulLatexify in their Project.toml. Another option might be to release a new UnitfulLatexify version that does nothing (or only prints a warning that it is no longer required) and is only compatible with upcoming Unitful versions. Then users who update an existing project would get the newest versions of everything. But it would not stop anyone from manually installing new Unitful and old UnitfulLatexify. If we want to prevent that, we need an upper bound on Unitful in all UnitfulLatexify versions. |
Fixed that! This doesn't need to be in this PR, but to make sure that UnitfulLatexify doesn't get loaded over the top of the extension, maybe the best solution is to make a non-SemVer-breaking release of UnitfulLatexify that has no code, just prints a depwarn and loads both Unitful and Latexify. That could be compatible with new Unitful. @sostock and @gustaphe , thoughts? |
|
Sure, sounds good. I could make a change like that, but not in the next week or so. |
|
Yes, sounds good! This will be helpful for upgrading. Additionally, we could still make a PR to the general registry to make older UnitfulLatexify versions incompatible with Unitful ≥1.25. What do you think about that? |
Intimidating but I took a stab with JuliaRegistries/General#138160. Very possible that I did something wrong or that I should redo it after the Unitful and UnitfulLatexify new releases are already on the registry, this is my first time working with the registry. In that case, since we would be introducing a way to block users from getting a bad installation, perhaps the new release of UnitfulLatexify should be (correctly) declared a breaking 2.0 release rather than a non-breaking 1.8? |
I’m not sure which would be better. In what way do you consider the release breaking? Because of the printed warning? |
We took this as a chance to break a little of the API from UnitfulLatexify--replacing e.g. |
|
I see – then I think it should definitely be UnitfulLatexify 2.0. |
|
I think this PR should be good to go, then. We can continue conversation about UnitfulLatexify compat at gustaphe/UnitfulLatexify.jl#22 if necessary, but once this PR and that PR are merged & released I can double check JuliaRegistries/General#138160 (pending advice from someone who knows more than I do) to make sure that all old Unitful is treated as incompatible with new UnitfulLatexify and vice versa. |


This should make it easier to build docs locally cross-platform, making explicit a Ghostscript_jll dependency that wasn't obvious in #795 . Depends on korsbo/Latexify.jl#344 getting merged and released (hi @gustaphe), and Latexify compat should get bumped accordingly indocs/project.toml.Existing caveats:
Still depends on system having theFreeSerifandFreeMonofonts installed. The default font in tectonic forLatexify.renderwas not sufficiently clear for my tastes. Fortunately, these fonts might be installed by default by some unix packages, and if not can be installed for free withsudo apt install fonts-freefont-ttf, or from places online it seems.That second can possibly be assuaged (for the meantime) by adjusting
docs/make.jlto check if the images already exist:where currently the latex images are generated either way.