Skip to content
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

Vignette engines for Quarto #57

Merged
merged 8 commits into from
Nov 3, 2023
Merged

Conversation

dcnorris
Copy link
Contributor

@dcnorris dcnorris commented Sep 7, 2022

I am opening this pull request pursuant to quarto-dev/quarto-cli#2307.

This indulgence in 'speculative generality' was motivated
solely by the existence of the 'output_format = "all"' option
in quarto_render. In any case, its effect on the package build
seems no different from that of "html" engine.
HenrikBengtsson added a commit to HenrikBengtsson/teeny that referenced this pull request Oct 6, 2023
@HenrikBengtsson
Copy link

This LGTM. ("Disclaimer": I was one of the persons designing and implementing support for generic vignette engines back in R 3.0.0.)

I've verified it works as expected using a minimal R package (https://github.com/HenrikBengtsson/teeny/tree/test/quarto-vignettes). Anyone can test this PR by installing:

remotes::install_github("quarto-dev/quarto-r#57")
remotes::install_github("HenrikBengtsson/teeny", ref = "test/quarto-vignettes", dependencies = TRUE, build_vignettes = TRUE)

This package has two Quarto vignettes, one in HTML and one in PDF, cf.

browseVignettes("teeny")

@dcnorris , can you please resync your PR with the latest quarto-dev/quarto-r so that this PR is a bit more relevant. FWIW, I've done that locally and verified things still work as expected.

@cderv cderv self-assigned this Oct 9, 2023
Copy link
Collaborator

@cderv cderv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the working PR already. This shows we can do it in quarto R package and not inside knitr 📦 - this quarto package seems the right place for it.

I have shared some thoughts on what we would like to do.

I'll probably work on this in a new branch, as this could be more change than just a few tweaks, and I can dedicate time on this. Would that be ok with you ?

Happy to have you thoughts (and yours too @HenrikBengtsson) on this.

Our aim is to minimize the potential problem of rendering vignette on CRAN by providing lightweight format as we did with rmarkdown.

Comment on lines +6 to +14
vignetteEngine(name = "pdf",
package = "quarto",
pattern = "[.]qmd$",
weave = function(file, ..., encoding = "UTF-8") {
quarto_render(file, ..., output_format = "pdf")
},
tangle = vignetteEngine("knitr::rmarkdown")$tangle,
aspell = vignetteEngine("knitr::rmarkdown")$aspell
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding PDF vignette, we would like to put that on hold for now.

Quarto 1.4 has typst support, with typst CLI bundled in Quarto (https://quarto.org/docs/prerelease/1.4/typst.html) and this is a good way to produce PDF faster and more reliable than using LaTeX.

This could be safer on CRAN that to rely on the LaTeX installation. Do you have experience on CRAN to build LaTeX vignette ?
If we provice a pdf engine using LaTeX, we should probably tweak the default configuration so that for example latexmk auto install does no happen when running a vignette on CRAN. (I don't know if that is really allowed, and how LaTeX dependencies are handled on CRAN servers to build vignette).

So for now, I would only provide HTML vignette

Copy link
Contributor Author

@dcnorris dcnorris Oct 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have experience on CRAN to build LaTeX vignette ?

Not with Quarto, but certainly with Rmd. This vignette for my DTAT package serves to demonstrate the reproducibility of calculations for a peer-reviewed paper, e.g.:
https://cran.r-project.org/web/packages/DTAT/vignettes/dtat.pdf

I would tend to discourage gratuitous FUD here about building LaTeX stuff on CRAN. I don't recall myself ever having to 'downgrade' a PDF vignette to HTML for any technical reason.


See also https://cran.r-project.org/doc/manuals/r-devel/R-exts.html#Writing-package-vignettes.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry if my tone was not the good one. (sometimes non native english speaker catch me ... 😞)
I didn't mean to sound like I was doubtful about it working well. It was mainly question about what can really be done considering how quarto is working by default with quarto pdf engine

format: pdf require LaTeX and some packages. Pandoc's template and some quarto feature have requirement about LaTeX packages. By default, quarto will try to install missing LaTeX packages when rendering errors are found, and then retrigger a render. By "LaTeX installation", I mean which LaTeX extension are available or not. I don't think it is seen as good practice to automatically update tlmgr and install new missing LaTeX package. So I meant it was safer to opt-out this feature, or use PDF output produced by format: typst which is quicker to render.

I don't know the list of available LaTeX component from CTAN available on CRAN build machine to compare with Pandoc's and Quarto requirement. Maybe this is full texlive install available and so no problem ever.

However, as pointed out in the manual you rightly referenced, including the output in the source would make sure to LaTeX extension installation are needed. So that is a way to make sure it would work.

My concerns is preventing any problem with package building on CRAN because of how quarto works, and what impact building vignette with quarto could have. Maybe this is nothing to worry about.

Anyhow, I hope this clarifies my initial thoughts.

R/zzz.R Show resolved Hide resolved
@dcnorris
Copy link
Contributor Author

@cderv as you consider lightweight HTML vignettes, please avoid foreclosing opportunities to embed D3 or OJS apps. You'll find a D3 app by scrolling about 2/3 of the way down this vignette from my DTAT package:
https://cran.r-project.org/web/packages/DTAT/vignettes/Designing-33PC.html

@eitsupi
Copy link

eitsupi commented Oct 28, 2023

Any update on this?

@cderv
Copy link
Collaborator

cderv commented Oct 28, 2023

I just took a week off this week, so got a bit delayed but coming back to this next week. I started by making some updates to the package to allow metadata modification from quarto_render(). Anyhow, as always, the PR will be updated once something is available.

Copy link
Collaborator

@cderv cderv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to test a few different things and scenarios before merging into main, so I will merge this PR into a vignette-engine branch and work on that one.

Thanks a lot for your contribution !

@cderv cderv changed the base branch from main to vignette-engine November 3, 2023 10:46
@cderv cderv merged commit 9dc50eb into quarto-dev:vignette-engine Nov 3, 2023
cderv pushed a commit that referenced this pull request Jan 30, 2024
cderv pushed a commit that referenced this pull request Jan 30, 2024
@cderv cderv mentioned this pull request Jan 30, 2024
2 tasks
cderv pushed a commit that referenced this pull request Jan 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants