Create PDF version of the OSCAR manual#5627
Create PDF version of the OSCAR manual#5627varuntrehan7 wants to merge 16 commits intooscar-system:masterfrom
Conversation
docs/make_work.jl
Outdated
| Documenter.LaTeX(platform = "docker"), | ||
| ], | ||
| sitename = "Oscar.jl", | ||
| authors = ["OSCAR Team"], |
There was a problem hiding this comment.
please keep the changes as minimal as possible. In particular, you do not need to reformat this whole thing. And the line about authors was not present before
docs/make_work.jl
Outdated
| canonical = "https://docs.oscar-system.org/stable/", | ||
| ), | ||
| # Add PDF generation via LaTeX | ||
| Documenter.LaTeX(platform = "docker"), |
There was a problem hiding this comment.
I would prefer if there was some argument to choose between html and pdf output. When just locally working on the documentation, I want to rebuild the html over and over again. But also generating and compiling latex all the time just wastes my time and resources
There was a problem hiding this comment.
I also think that it would be good to have this behind an option. We could run this on our github CI for master and provide the pdf as an artifact.
But I would prefer not to run this locally by default. I think that it is more likely that Oscar developers have latex installed than docker, so this should probably be behind a check that docker is in the path?
docs/make_work.jl
Outdated
| plugins = [bib], | ||
| ) | ||
| end | ||
| format = if get(ENV, "OSCAR_BUILD_PDF", "false") == "true" && |
There was a problem hiding this comment.
I am not sure if this place is the best one to check the condition. I would find it more natural to have this type of format as an input to this doit function. That way the parameter could be (with a bit more work) be exposed as an argument to Oscar.build_doc, so that any user can directly when building the documentation decide which versions to build (and not have to fiddle around with env variables)
There was a problem hiding this comment.
okay got it, thank you
docs/make_work.jl
Outdated
| plugins=[bib], | ||
| ) | ||
| end | ||
| format = if build_pdf && success(`docker --version`) |
There was a problem hiding this comment.
similarly, the docker check should IMO not be here. If I request a pdf, it should be built or throw an error
|
I am not quite sure that we are at the same point of understanding here. These new options should also be added to the docstring of Please also remove all formatting changes to lines that you didn't change at all. This makes it easier to review the diff in the github UI. |
thank you for explaining that, that's helpful and before I do anything I had one question - I want to confirm that |
Yes, exactly. |
Thankyou for clarifying, I'll make the changes and push shortly ( maybe tonight since I have a class right now). |
|
Thankyou for the changes, should I ready for review it? |
|
The one thing that IMO is still missing is adding a pdf build to the CI job that builds the documentation. The resulting pdf should then be made available as an action artifact |
got it, I will add and push shortly. Thankyou! |
|
The respective CI job does not run, possibly due to some syntax error in the yml file. BUt looking at you changes in general, it would be better to just adapt the script that gets called in the action file. In fact, the only use-case of |
|
I’ve now adapted BuildDoc.doit to support formats and pdf_method, and changed docs/make.jl to request [:html, :pdf] with pdf_via_latex. Locally, the PDF is not produced because the docs build aborts early due to existing @docs errors , but the HTML build starts. |
|
From a pure code perspective, this looks good. The build failure is due to latex not being available on the CI runner. @benlorenz @fingolfin do you have any preference if we install docker or latex on the CI runners to be able to produce the pdf documentation? (Depending on the answers, @varuntrehan7 could change the |
I think docker would be easier for our CI, since I think docker is already installed on the runner images (at least the github-hosted runners). |
|
latexmk is unhappy: That docker image seems quite old: https://hub.docker.com/r/juliadocs/documenter-latex/
|
It seems that the corresponding Dockerfile is not even in the Documenter.jl repo, but still in the predecessor DocumenterLatex.jl. See JuliaDocs/Documenter.jl#2073 and JuliaDocs/Documenter.jl#2092 |
|
I don't understand in how far docker would be better in CI: it still has to download the container images, right? OTOH installing latex via apt is routine and also matches more closely how we plan to use it locally. So I'd suggest to try that instead? |
I thought the docker image would make sure that we have all relevant latex packages for documenter installed and less prone to new updates causing problems. It was also just a one-word change since it docker is pre-installed. But I didn't expect a 7 year old image. So yeah let's try installing the packages directly. |
|
@lgoettgens Should i make any changes from my side? |
docs/make.jl
Outdated
| include("make_work.jl") | ||
|
|
||
| Base.invokelatest(BuildDoc.doit, Oscar; warnonly=false, local_build=false, doctest=false) | ||
| Base.invokelatest(BuildDoc.doit, Oscar; warnonly=false, local_build=false, doctest=false, formats = [:html, :pdf], pdf_method = :pdf_via_docker) |
There was a problem hiding this comment.
| Base.invokelatest(BuildDoc.doit, Oscar; warnonly=false, local_build=false, doctest=false, formats = [:html, :pdf], pdf_method = :pdf_via_docker) | |
| Base.invokelatest(BuildDoc.doit, Oscar; warnonly=false, local_build=false, doctest=false, formats = [:html, :pdf], pdf_method = :pdf_via_latex) |
@varuntrehan7 this was suggested by @fingolfin and @benlorenz. Please apply this change and then tinker with apt install statements in the action yaml file tot try to get it running
There was a problem hiding this comment.
okay got it, thanks!
|
I’ve added LaTeX installation via apt (latexmk + texlive), I also tried building the docs locally after installing LaTeX. The build still errors, but the remaining failures come from existing Documenter @docs blocks (missing docstrings in Nemo), not from the PDF/LaTeX toolchain itself. |
|
@lgoettgens can you also look into this? thanks! |
|
The actual error is not shown in the logs. I did run it locally and had a look at the So, something is using invalid TeX syntax. Using $ git grep -n -F '\R'
docs/src/CommutativeAlgebra/localizations.md:12:1 \in U \;\text{ and }\; u, v \in U \;\Rightarrow \; u\cdot v \in U.
experimental/InjectiveResolutions/src/MonoidAlgebra.jl:303:Let kQ be a monoid algebra over some semigroup $Q$. Given a face $F$ of the cone $C = \RR_{\geq 0}Q$ of the monoid algebra kQ,
experimental/InjectiveResolutions/src/MonoidAlgebra.jl:325:# OUTPUT: polyhedral cone \RR_{\geq 0}Q
experimental/IntersectionTheory/src/Main.jl:1288:Compute all the Chern numbers of `X` as a list of pairs $\lambda\Rightarrow
src/Groups/schur_index.jl:300: # Determine m_\R(chi) and use Theorem 2.2 to find a bound u \in \N
src/Groups/schur_index.jl:442: # Set u to be the LCM of m_\R(chi) and the u_p’s.
src/Groups/schur_index.jl:626: # Return m_\R(\chi) and the sequence of pairs `p => u_p` where u_p \not= 1.We can edit each of these, but the result will be less nice to read interactively... so perhaps we should just define macros. According to https://documenter.juliadocs.org/stable/man/latex/#Set-math-engine-and-define-macros-for-LaTeX something like this might work (to be adapted to our needs) Though I think it would actually be better to agree one (say, It is very likely that more errors of this kind will pop up. If you can't figure out a solution, post the error here or on slack and we can figure it out. |
okay got it, thanks! |
|
The new plan is now to add PDF output to AA; then Nemo; etc. until we have resolved all the math TeX errors in each, then we can return to this one. |
This PR adds PDF documentation generation using Documenter’s built-in
LaTeX backend. The HTML build is unchanged, the PDF is generated in
parallel using the docker LaTeX platform to avoid external toolchain
dependencies.
Resolves #5362.