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

Pandoc args conditional to version should be set in pre_processor #2513

Open
cderv opened this issue Sep 8, 2023 · 6 comments
Open

Pandoc args conditional to version should be set in pre_processor #2513

cderv opened this issue Sep 8, 2023 · 6 comments
Assignees
Labels
bug an unexpected problem or unintended behavior next to consider for next release

Comments

@cderv
Copy link
Collaborator

cderv commented Sep 8, 2023

What we describe in https://bookdown.org/yihui/rmarkdown-cookbook/install-pandoc.html to run rmarkdown::find_pandoc() in setup chunk does not work in some case.

My pandoc version is

rmarkdown::pandoc_version()
# [1] ‘3.1.7’

But now I am trying to use an old one.

---
title: test
output: 
  html_document:
    self_contained: true
---

```{r}
rmarkdown::find_pandoc(cache = FALSE, dir = dirname(pandoc::pandoc_bin("2.7.3")))
```

This will throw an error

==> rmarkdown::render('C:/Users/chris/Documents/DEV_R/rmarkdown/test.Rmd',  encoding = 'UTF-8');
++ Activating rlang global_entrace



processing file: test.Rmd
                                                                                                            
"C:/Users/chris/AppData/Local/r-pandoc/r-pandoc/2.7.3/pandoc" +RTS -K512m -RTS test.knit.md --to html4 --from markdown+autolink_bare_uris+tex_math_single_backslash --output test.html --lua-filter "C:\Users\chris\AppData\Local\R\win-library\4.3\rmarkdown\rmarkdown\lua\pagebreak.lua" --lua-filter "C:\Users\chris\AppData\Local\R\win-library\4.3\rmarkdown\rmarkdown\lua\latex-div.lua" --embed-resources --standalone --variable bs3=TRUE --section-divs --template "C:\Users\chris\AppData\Local\R\win-library\4.3\rmarkdown\rmd\h\default.html" --no-highlight --variable highlightjs=1 --variable theme=bootstrap --mathjax --variable "mathjax-url=https://mathjax.rstudio.com/latest/MathJax.js?config=TeX-AMS-MML_HTMLorMML" --include-in-header "C:\Users\chris\AppData\Local\Temp\RtmpKmpkq0\rmarkdown-str7ab8779d3ac2.html" 
output file: test.knit.md

Unknown option --embed-resources.
Try pandoc.exe --help for more information.
Error:
! pandoc document conversion failed with error 2
Backtrace:
    ▆
 1. └─rmarkdown::render(...)
 2.   └─rmarkdown (local) convert(output_file, run_citeproc)
 3.     └─rmarkdown (local) convert_it(output)
 4.       └─rmarkdown (local) convert_fun(...)
 5.         └─rmarkdown:::stop2(...)
Exécution arrêtée

As we can see above Pandoc 2.7.3 is correctly use but --embed-resources flag is still used. Though we have

rmarkdown/R/pandoc.R

Lines 846 to 849 in 8d2d9b8

# Pandoc 2.19 deprecated --self-contained
self_contained_args <- function() {
if (pandoc_available('2.19')) c('--embed-resources', '--standalone') else '--self-contained'
}

but this is evaluated when format function are evaluated

# self contained document
if (self_contained) {
if (copy_resources)
stop("Local resource copying is incompatible with self-contained documents.")
validate_self_contained(math)
args <- c(args, self_contained_args())
}

rmarkdown/R/render.R

Lines 474 to 484 in 8d2d9b8

# if we haven't been passed a fully formed output format then
# resolve it by looking at the yaml
if (!is_output_format(output_format)) {
output_format <- output_format_from_yaml_front_matter(input_lines,
output_options,
output_format,
output_yaml,
output_file)
output_format <- create_output_format(output_format$name,
output_format$options)
}

So it is too soon for the change of pandoc version to happen in the setup chunk. That is why rmarkdown::pandoc_available("2.19") returns TRUE (on a system where 3.1.2 is available)

So we should

  • Either put any pandoc args conditional to version into pre_processor step
  • Either disallow any change in Pandoc version during rmarkdown rendering (we could track version before knitr and after knitr)

The second case would be breaking change. The first seems fine to me.

Weirdly, no one reported. So should not be that often. But I hit that when debugging pagedown and this when changing pandoc version is useful.

@cderv cderv self-assigned this Sep 8, 2023
@cderv cderv added the bug an unexpected problem or unintended behavior label Sep 8, 2023
@cderv cderv moved this from Backlog to To discuss / To plan in R Markdown Team Projects Sep 8, 2023
@cderv cderv added the next to consider for next release label Sep 8, 2023
@cderv
Copy link
Collaborator Author

cderv commented Sep 12, 2023

Doing this change could be breaking, if any other package is looking into pandoc_args value of the output format.

rmarkdown::html_document()$pandoc$args
#>  [1] "--embed-resources"                                                                     
#>  [2] "--standalone"                                                                          
#>  [3] "--variable"                                                                            
#>  [4] "bs3=TRUE"                                                                              
#>  [5] "--section-divs"                                                                        
#>  [6] "--template"                                                                            
#>  [7] "C:\\Users\\chris\\AppData\\Local\\R\\win-library\\4.3\\rmarkdown\\rmd\\h\\default.html"
#>  [8] "--no-highlight"                                                                        
#>  [9] "--variable"                                                                            
#> [10] "highlightjs=1"

If we move some of the args into pre_processor then this would not be here, because pre_processor is modifying args later.

It seems that it is common practice to do this:
https://github.com/search?q=%22%24pandoc%24args%22+language%3AR&type=code&l=R

Even distill does it
https://github.com/rstudio/distill/blob/ac5e3bf1dca2054a5bf61cfe81b59d7bdb5e3705/R/distill_website.R#L183-L204

But can't find on github any other occurrence of this on Github search.

A different approach could to revisit this: https://bookdown.org/yihui/rmarkdown-cookbook/install-pandoc.html

The problem is when changing the pandoc version from within knitr chunk itself. In a way, this is a bit too late to do that here. Doing it in R console, before rendering works as expected.

pandoc::with_pandoc_version("2.7.3", rmarkdown::render("test.Rmd"))

From within the document, following the logic of changing pandoc version before rmarkdown happen, we could also use custom knit function

---
title: test
output: 
  html_document:
    self_contained: true
knit: ( \(input, ...) pandoc::with_pandoc_version("2.7.3", rmarkdown::render(input)) )
---

# Header

I could even provide this type of function in the Pandoc package to be used that way.

knit: pandoc::render_with_pandoc_version

So we could also either document it differently, for one of those solution.

or propose a new mechanism to allow changing the pandoc version used for rmarkdown, like a specific YAML option (and Suggest usage of pandoc package behind the scene).

I believe we could also detect any change of Pandoc version during rmarkdown::render() to error or warn that this is not advice

  • Running rmarkdown::find_pandoc() before knit
  • and compare after knitting

Anyhow, there are several ways it seems, and not sure that moving every conditional args definition is possible - it could be too late for doing that in rmarkdown

@yihui
Copy link
Member

yihui commented Sep 12, 2023

I guess it should not be common to change the Pandoc version dynamically inside Rmd. Supporting this is indeed tricky. If the goal is to be able to test different versions of Pandoc, it seems pandoc::with_pandoc_version() has done a perfect job. I'm not sure if it's worth the effort to support the Knit button working with a particular version of Pandoc. If that really needs to be done, I tend to use a top-level YAML field like pandoc_version, and let render() read it. That might be a little easier for users than providing a custom knit field, but the latter is totally fine. As I said, this use case may not be common, so we don't really have to consider which implementation is easier for users.

@cderv cderv moved this from To discuss / To plan to Next / Ready for Dev in R Markdown Team Projects Sep 12, 2023
@atusy
Copy link
Collaborator

atusy commented Sep 12, 2023

I agree with yihui.

@cderv
Copy link
Collaborator Author

cderv commented Sep 13, 2023

Thanks for you feedback. This confirms my intuitions. I'll handle that with a new function in pandoc package (cderv/pandoc#37)

believe we could also detect any change of Pandoc version during rmarkdown::render() to error or warn that this is not advice

And in rmarkdown I will do that so that users are aware this is not adviced to change Pandoc version from a code chunk, maybe using xfun::do_once() to only warn once per session when this is detected.

@lfmcmillan
Copy link

lfmcmillan commented Feb 29, 2024

Please would you update the Rmarkdown bookdown site (https://bookdown.org/yihui/rmarkdown-cookbook/install-pandoc.html) to reflect the latest advice on this. Just now on a work machine I tried setting the pandoc version and directory to use a different version of pandoc and found that it wasn't enough to fix the error; I had to also set RSTUDIO_PANDOC to the other version to get the .Rmd file to knit correctly.
That is, when I ran find_pandoc from the console pointing at the other directory that worked, but find_pandoc inside the directory still kept failing even when I had the code chunk telling it to point elsewhere.
This was my failing .Rmd that knitted after I changed RSTUDIO_PANDOC:

title: "Rmarkdown demo"
author: "Louise"
date: '2024-02-29'
output: html_document
---

```{r, setup, include=FALSE}
rmarkdown::find_pandoc(version="3.1.12.1",dir="C:/Users/mcmilllo/AppData/Local/r-pandoc/r-pandoc/3.1.12.1/")
```

A````

@cderv
Copy link
Collaborator Author

cderv commented Feb 29, 2024

Thanks for the reminder @lfmcmillan - I added an update

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug an unexpected problem or unintended behavior next to consider for next release
Projects
Status: Next / Ready for Dev
Development

No branches or pull requests

4 participants