From f28dfc8b1b678a7fb45b45f1d49bd3eb2edf9f0b Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Fri, 12 Apr 2024 08:09:37 -0500 Subject: [PATCH 1/9] Improve rmarkdown error forwarding Part of #2433 --- R/rmarkdown.R | 6 ++++-- tests/testthat/_snaps/rmarkdown.md | 2 -- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/R/rmarkdown.R b/R/rmarkdown.R index 1dd09d73d..14eea2f87 100644 --- a/R/rmarkdown.R +++ b/R/rmarkdown.R @@ -25,7 +25,7 @@ render_rmarkdown <- function(pkg, input, output, ..., seed = NULL, copy_images = quiet = quiet ) - path <- tryCatch( + path <- withCallingHandlers( callr::r_safe( function(seed, envir, ...) { if (!is.null(seed)) { @@ -51,12 +51,14 @@ render_rmarkdown <- function(pkg, input, output, ..., seed = NULL, copy_images = ) ), error = function(cnd) { + # browser() cli::cli_abort( c( "Failed to render RMarkdown document.", x = gsub("\r", "", cnd$stderr, fixed = TRUE) ), - parent = cnd + parent = cnd$parent, + trace = cnd$parent$trace ) } ) diff --git a/tests/testthat/_snaps/rmarkdown.md b/tests/testthat/_snaps/rmarkdown.md index aa84604f5..b6d077fce 100644 --- a/tests/testthat/_snaps/rmarkdown.md +++ b/tests/testthat/_snaps/rmarkdown.md @@ -18,8 +18,6 @@ ! Failed to render RMarkdown document. x [WARNING] Could not fetch resource path-to-image.png Failing because there were warnings. Caused by error: - ! in callr subprocess. - Caused by error: ! pandoc document conversion failed with error 3 # render_rmarkdown styles ANSI escapes From 8725e3cb1d7dbe57abf38e75b3c82ff88fc33c9f Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Mon, 15 Apr 2024 08:27:19 -0500 Subject: [PATCH 2/9] Tweaks from code review --- R/rmarkdown.R | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/R/rmarkdown.R b/R/rmarkdown.R index 14eea2f87..7f8f44e68 100644 --- a/R/rmarkdown.R +++ b/R/rmarkdown.R @@ -51,13 +51,12 @@ render_rmarkdown <- function(pkg, input, output, ..., seed = NULL, copy_images = ) ), error = function(cnd) { - # browser() cli::cli_abort( c( "Failed to render RMarkdown document.", x = gsub("\r", "", cnd$stderr, fixed = TRUE) ), - parent = cnd$parent, + parent = cnd$parent %||% cnd, trace = cnd$parent$trace ) } From 6c3efdec3edfa3ed9d43c42a8150b17a59b99495 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Mon, 15 Apr 2024 08:36:59 -0500 Subject: [PATCH 3/9] Try snapshoting traceback too --- tests/testthat/_snaps/rmarkdown.md | 18 ++++++++++++++++++ tests/testthat/assets/r-fail.Rmd | 13 +++++++++++++ tests/testthat/test-rmarkdown.R | 9 +++++++++ 3 files changed, 40 insertions(+) create mode 100644 tests/testthat/assets/r-fail.Rmd diff --git a/tests/testthat/_snaps/rmarkdown.md b/tests/testthat/_snaps/rmarkdown.md index b6d077fce..b9ba50fcd 100644 --- a/tests/testthat/_snaps/rmarkdown.md +++ b/tests/testthat/_snaps/rmarkdown.md @@ -20,6 +20,24 @@ Caused by error: ! pandoc document conversion failed with error 3 +--- + + Code + render_rmarkdown(pkg, "assets/r-fail.Rmd", "test.html") + Message + Reading assets/r-fail.Rmd + Condition + Error in `render_rmarkdown()`: + ! Failed to render RMarkdown document. + x Quitting from lines at lines 6-13 [unnamed-chunk-1] (r-fail.Rmd) + Caused by error: + ! Error! + Code + last_trace() + Condition + Error: + ! Can't show last error because no error was recorded yet + # render_rmarkdown styles ANSI escapes Code diff --git a/tests/testthat/assets/r-fail.Rmd b/tests/testthat/assets/r-fail.Rmd new file mode 100644 index 000000000..101dc46a0 --- /dev/null +++ b/tests/testthat/assets/r-fail.Rmd @@ -0,0 +1,13 @@ +--- +title: "r-fail" +--- + +```{r} +f <- function() g() +g <- function() h() +h <- function() { + rlang::abort("Error!") +} + +f() +``` \ No newline at end of file diff --git a/tests/testthat/test-rmarkdown.R b/tests/testthat/test-rmarkdown.R index 24b3af222..e35e87307 100644 --- a/tests/testthat/test-rmarkdown.R +++ b/tests/testthat/test-rmarkdown.R @@ -21,12 +21,21 @@ test_that("render_rmarkdown yields useful error", { tmp <- dir_create(file_temp()) pkg <- list(src_path = test_path("."), dst_path = tmp, bs_version = 3) + # For pandoc expect_snapshot(error = TRUE, { render_rmarkdown(pkg, "assets/pandoc-fail.Rmd", "test.html", output_format = rmarkdown::html_document(pandoc_args = "--fail-if-warnings")) }) + + expect_snapshot(error = TRUE, { + render_rmarkdown(pkg, "assets/r-fail.Rmd", "test.html") + last_trace() + }) + }) + + test_that("render_rmarkdown styles ANSI escapes", { skip_if_no_pandoc() local_edition(3) From 3c30089e0bba31930f27597a7339eaadb031c2ef Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Mon, 15 Apr 2024 08:38:11 -0500 Subject: [PATCH 4/9] WS --- tests/testthat/test-rmarkdown.R | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/testthat/test-rmarkdown.R b/tests/testthat/test-rmarkdown.R index e35e87307..6e12d4794 100644 --- a/tests/testthat/test-rmarkdown.R +++ b/tests/testthat/test-rmarkdown.R @@ -34,8 +34,6 @@ test_that("render_rmarkdown yields useful error", { }) - - test_that("render_rmarkdown styles ANSI escapes", { skip_if_no_pandoc() local_edition(3) From 512b2f01598572d2da83c8b43904b559ee9f723c Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Mon, 15 Apr 2024 09:02:45 -0500 Subject: [PATCH 5/9] Option to build in process --- R/rmarkdown.R | 84 ++++++++++++++++-------------- tests/testthat/_snaps/rmarkdown.md | 32 +++++++----- tests/testthat/test-rmarkdown.R | 13 +++-- 3 files changed, 70 insertions(+), 59 deletions(-) diff --git a/R/rmarkdown.R b/R/rmarkdown.R index 7f8f44e68..fc22fc66d 100644 --- a/R/rmarkdown.R +++ b/R/rmarkdown.R @@ -1,7 +1,7 @@ #' Render RMarkdown document in a fresh session #' #' @noRd -render_rmarkdown <- function(pkg, input, output, ..., seed = NULL, copy_images = TRUE, quiet = TRUE) { +render_rmarkdown <- function(pkg, input, output, ..., seed = NULL, copy_images = TRUE, new_process = TRUE, quiet = TRUE, call = caller_env()) { input_path <- path_abs(input, pkg$src_path) output_path <- path_abs(output, pkg$dst_path) @@ -19,49 +19,39 @@ render_rmarkdown <- function(pkg, input, output, ..., seed = NULL, copy_images = output_dir = path_dir(output_path), intermediates_dir = tempdir(), encoding = "UTF-8", - envir = globalenv(), seed = seed, ..., quiet = quiet ) - path <- withCallingHandlers( - callr::r_safe( - function(seed, envir, ...) { - if (!is.null(seed)) { - # since envir is copied from the parent fn into callr::r_safe(), - # set.seed() sets the seed in the wrong global env and we have to - # manually copy it over - set.seed(seed) - envir$.Random.seed <- .GlobalEnv$.Random.seed - if (requireNamespace("htmlwidgets", quietly = TRUE)) { - htmlwidgets::setWidgetIdSeed(seed) - } - } - rmarkdown::render(envir = envir, ...) - }, - args = args, - show = !quiet, - env = c( - callr::rcmd_safe_env(), - BSTINPUTS = bst_paths(input_path), - TEXINPUTS = tex_paths(input_path), - BIBINPUTS = bib_paths(input_path), - R_CLI_NUM_COLORS = 256 - ) - ), - error = function(cnd) { - cli::cli_abort( - c( - "Failed to render RMarkdown document.", - x = gsub("\r", "", cnd$stderr, fixed = TRUE) - ), - parent = cnd$parent %||% cnd, - trace = cnd$parent$trace - ) - } + withr::local_envvar( + callr::rcmd_safe_env(), + BSTINPUTS = bst_paths(input_path), + TEXINPUTS = tex_paths(input_path), + BIBINPUTS = bib_paths(input_path), + R_CLI_NUM_COLORS = 256 ) + if (new_process) { + path <- withCallingHandlers( + callr::r_safe(rmarkdown_render_with_seed, args = args, show = !quiet), + error = function(cnd) { + lines <- strsplit(cnd$stderr, "\r?\n")[[1]] + cli::cli_abort( + c( + x = "Failed to render RMarkdown document.", + set_names(lines, " ") + ), + parent = cnd$parent %||% cnd, + trace = cnd$parent$trace, + call = call + ) + } + ) + } else { + path <- inject(rmarkdown_render_with_seed(!!!args)) + } + if (identical(path_ext(path)[[1]], "html")) { update_html( path, @@ -103,6 +93,25 @@ render_rmarkdown <- function(pkg, input, output, ..., seed = NULL, copy_images = invisible(path) } + +rmarkdown_render_with_seed <- function(..., seed = NULL) { + if (!is.null(seed)) { + set.seed(seed) + if (requireNamespace("htmlwidgets", quietly = TRUE)) { + htmlwidgets::setWidgetIdSeed(seed) + } + + # since envir is copied from the parent fn into callr::r_safe(), + # set.seed() sets the seed in the wrong global env and we have to + # manually copy it over + # if (!identical(envir, globalenv())) { + # envir$.Random.seed <- .GlobalEnv$.Random.seed + # } + } + + rmarkdown::render(envir = globalenv(), ...) +} + # adapted from tools::texi2dvi bst_paths <- function(path) { paths <- c( @@ -126,5 +135,4 @@ bib_paths <- function(path) { tex_paths(path) ) paste(paths, collapse = .Platform$path.sep) - } diff --git a/tests/testthat/_snaps/rmarkdown.md b/tests/testthat/_snaps/rmarkdown.md index b9ba50fcd..afa816a05 100644 --- a/tests/testthat/_snaps/rmarkdown.md +++ b/tests/testthat/_snaps/rmarkdown.md @@ -9,34 +9,38 @@ # render_rmarkdown yields useful error Code - render_rmarkdown(pkg, "assets/pandoc-fail.Rmd", "test.html", output_format = rmarkdown::html_document( - pandoc_args = "--fail-if-warnings")) + # Pandoc error + render_rmarkdown(pkg, "assets/pandoc-fail.Rmd", "test.html", output_format = format) Message Reading assets/pandoc-fail.Rmd Condition - Error in `render_rmarkdown()`: - ! Failed to render RMarkdown document. - x [WARNING] Could not fetch resource path-to-image.png Failing because there were warnings. + Error: + x Failed to render RMarkdown document. + [WARNING] Could not fetch resource path-to-image.png + Failing because there were warnings. Caused by error: ! pandoc document conversion failed with error 3 - ---- - Code + # R error render_rmarkdown(pkg, "assets/r-fail.Rmd", "test.html") Message Reading assets/r-fail.Rmd Condition - Error in `render_rmarkdown()`: - ! Failed to render RMarkdown document. - x Quitting from lines at lines 6-13 [unnamed-chunk-1] (r-fail.Rmd) + Error: + x Failed to render RMarkdown document. + + Quitting from lines at lines 6-13 [unnamed-chunk-1] (r-fail.Rmd) Caused by error: ! Error! Code - last_trace() + render_rmarkdown(pkg, "assets/r-fail.Rmd", "test.html", new_process = FALSE) + Message + Reading assets/r-fail.Rmd + + Quitting from lines at lines 6-13 [unnamed-chunk-1] (r-fail.Rmd) Condition - Error: - ! Can't show last error because no error was recorded yet + Error in `h()`: + ! Error! # render_rmarkdown styles ANSI escapes diff --git a/tests/testthat/test-rmarkdown.R b/tests/testthat/test-rmarkdown.R index 6e12d4794..d2bea3629 100644 --- a/tests/testthat/test-rmarkdown.R +++ b/tests/testthat/test-rmarkdown.R @@ -21,17 +21,16 @@ test_that("render_rmarkdown yields useful error", { tmp <- dir_create(file_temp()) pkg <- list(src_path = test_path("."), dst_path = tmp, bs_version = 3) - # For pandoc - expect_snapshot(error = TRUE, { - render_rmarkdown(pkg, "assets/pandoc-fail.Rmd", "test.html", - output_format = rmarkdown::html_document(pandoc_args = "--fail-if-warnings")) - }) + format <- rmarkdown::html_document(pandoc_args = "--fail-if-warnings") expect_snapshot(error = TRUE, { + "Pandoc error" + render_rmarkdown(pkg, "assets/pandoc-fail.Rmd", "test.html", output_format = format) + + "R error" render_rmarkdown(pkg, "assets/r-fail.Rmd", "test.html") - last_trace() + render_rmarkdown(pkg, "assets/r-fail.Rmd", "test.html", new_process = FALSE) }) - }) test_that("render_rmarkdown styles ANSI escapes", { From f7fefb0ae91428741964d40a9dfa2779f17d2fea Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Mon, 15 Apr 2024 09:07:41 -0500 Subject: [PATCH 6/9] Work around xfun issue --- tests/testthat/_snaps/rmarkdown.md | 4 ++-- tests/testthat/test-rmarkdown.R | 20 ++++++++++++-------- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/tests/testthat/_snaps/rmarkdown.md b/tests/testthat/_snaps/rmarkdown.md index afa816a05..91d04fd76 100644 --- a/tests/testthat/_snaps/rmarkdown.md +++ b/tests/testthat/_snaps/rmarkdown.md @@ -29,7 +29,7 @@ Error: x Failed to render RMarkdown document. - Quitting from lines at lines 6-13 [unnamed-chunk-1] (r-fail.Rmd) + Quitting from lines 6-13 [unnamed-chunk-1] (r-fail.Rmd) Caused by error: ! Error! Code @@ -37,7 +37,7 @@ Message Reading assets/r-fail.Rmd - Quitting from lines at lines 6-13 [unnamed-chunk-1] (r-fail.Rmd) + Quitting from lines 6-13 [unnamed-chunk-1] (r-fail.Rmd) Condition Error in `h()`: ! Error! diff --git a/tests/testthat/test-rmarkdown.R b/tests/testthat/test-rmarkdown.R index d2bea3629..ddda6f87e 100644 --- a/tests/testthat/test-rmarkdown.R +++ b/tests/testthat/test-rmarkdown.R @@ -23,14 +23,18 @@ test_that("render_rmarkdown yields useful error", { format <- rmarkdown::html_document(pandoc_args = "--fail-if-warnings") - expect_snapshot(error = TRUE, { - "Pandoc error" - render_rmarkdown(pkg, "assets/pandoc-fail.Rmd", "test.html", output_format = format) - - "R error" - render_rmarkdown(pkg, "assets/r-fail.Rmd", "test.html") - render_rmarkdown(pkg, "assets/r-fail.Rmd", "test.html", new_process = FALSE) - }) + expect_snapshot( + { + "Pandoc error" + render_rmarkdown(pkg, "assets/pandoc-fail.Rmd", "test.html", output_format = format) + + "R error" + render_rmarkdown(pkg, "assets/r-fail.Rmd", "test.html") + render_rmarkdown(pkg, "assets/r-fail.Rmd", "test.html", new_process = FALSE) + }, + error = TRUE, + # work around xfun bug + transform = function(x) gsub("lines ?at lines", "lines", x)) }) test_that("render_rmarkdown styles ANSI escapes", { From 85961d32e048f4c19c12ec997ab59d471b7deaa5 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Mon, 15 Apr 2024 09:13:32 -0500 Subject: [PATCH 7/9] Refactor tests; including traceback --- tests/testthat/_snaps/rmarkdown.md | 26 +++++++++++++++++--------- tests/testthat/test-rmarkdown.R | 29 ++++++++++++++++++++--------- 2 files changed, 37 insertions(+), 18 deletions(-) diff --git a/tests/testthat/_snaps/rmarkdown.md b/tests/testthat/_snaps/rmarkdown.md index 91d04fd76..7ed6b9cef 100644 --- a/tests/testthat/_snaps/rmarkdown.md +++ b/tests/testthat/_snaps/rmarkdown.md @@ -6,10 +6,9 @@ Reading assets/vignette-with-img.Rmd Writing `test.html` -# render_rmarkdown yields useful error +# render_rmarkdown yields useful error if pandoc fails Code - # Pandoc error render_rmarkdown(pkg, "assets/pandoc-fail.Rmd", "test.html", output_format = format) Message Reading assets/pandoc-fail.Rmd @@ -20,27 +19,36 @@ Failing because there were warnings. Caused by error: ! pandoc document conversion failed with error 3 + +# render_rmarkdown yields useful error if R fails + Code - # R error - render_rmarkdown(pkg, "assets/r-fail.Rmd", "test.html") + # Test traceback + summary(expect_error(render_rmarkdown(pkg, "assets/r-fail.Rmd", "test.html"))) Message Reading assets/r-fail.Rmd - Condition + Output + Error: x Failed to render RMarkdown document. Quitting from lines 6-13 [unnamed-chunk-1] (r-fail.Rmd) Caused by error: ! Error! + --- + Backtrace: + x + 1. \-global f() + 2. \-global g() + 3. \-global h() Code - render_rmarkdown(pkg, "assets/r-fail.Rmd", "test.html", new_process = FALSE) + # Just test that it works; needed for browser() etc + expect_error(render_rmarkdown(pkg, "assets/r-fail.Rmd", "test.html", + new_process = FALSE)) Message Reading assets/r-fail.Rmd Quitting from lines 6-13 [unnamed-chunk-1] (r-fail.Rmd) - Condition - Error in `h()`: - ! Error! # render_rmarkdown styles ANSI escapes diff --git a/tests/testthat/test-rmarkdown.R b/tests/testthat/test-rmarkdown.R index ddda6f87e..8e618c1c9 100644 --- a/tests/testthat/test-rmarkdown.R +++ b/tests/testthat/test-rmarkdown.R @@ -13,7 +13,7 @@ test_that("render_rmarkdown copies image files in subdirectories", { ) }) -test_that("render_rmarkdown yields useful error", { +test_that("render_rmarkdown yields useful error if pandoc fails", { local_edition(3) skip_on_cran() # fragile due to pandoc dependency skip_if_no_pandoc("2.18") @@ -22,19 +22,30 @@ test_that("render_rmarkdown yields useful error", { pkg <- list(src_path = test_path("."), dst_path = tmp, bs_version = 3) format <- rmarkdown::html_document(pandoc_args = "--fail-if-warnings") + expect_snapshot( + render_rmarkdown(pkg, "assets/pandoc-fail.Rmd", "test.html", output_format = format), + error = TRUE + ) +}) + +test_that("render_rmarkdown yields useful error if R fails", { + local_edition(3) + skip_if_no_pandoc() + + tmp <- dir_create(file_temp()) + pkg <- list(src_path = test_path("."), dst_path = tmp, bs_version = 3) expect_snapshot( { - "Pandoc error" - render_rmarkdown(pkg, "assets/pandoc-fail.Rmd", "test.html", output_format = format) - - "R error" - render_rmarkdown(pkg, "assets/r-fail.Rmd", "test.html") - render_rmarkdown(pkg, "assets/r-fail.Rmd", "test.html", new_process = FALSE) + "Test traceback" + summary(expect_error(render_rmarkdown(pkg, "assets/r-fail.Rmd", "test.html"))) + + "Just test that it works; needed for browser() etc" + expect_error(render_rmarkdown(pkg, "assets/r-fail.Rmd", "test.html", new_process = FALSE)) }, - error = TRUE, # work around xfun bug - transform = function(x) gsub("lines ?at lines", "lines", x)) + transform = function(x) gsub("lines ?at lines", "lines", x) + ) }) test_that("render_rmarkdown styles ANSI escapes", { From 5f28931cd3f213d3f7c0d753a24d73405c7cc9e1 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Mon, 15 Apr 2024 09:25:27 -0500 Subject: [PATCH 8/9] Add `new_process` argument to build_article() --- R/build-articles.R | 5 +++++ man/build_articles.Rd | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/R/build-articles.R b/R/build-articles.R index 8d858e41f..f82005c01 100644 --- a/R/build-articles.R +++ b/R/build-articles.R @@ -200,11 +200,15 @@ build_articles <- function(pkg = ".", #' @param name Name of article to render. This should be either a path #' relative to `vignettes/` without extension, or `index` or `README`. #' @param data Additional data to pass on to template. +#' @param new_process Build the article in a clean R process? The default, +#' `TRUE`, ensures that every article is build in a fresh environment, but +#' you may want to set it to `FALSE` to make debugging easier. build_article <- function(name, pkg = ".", data = list(), lazy = FALSE, seed = 1014L, + new_process = TRUE, quiet = TRUE) { pkg <- as_pkgdown(pkg) @@ -293,6 +297,7 @@ build_article <- function(name, output_format = format, output_options = options, seed = seed, + new_process = new_process, quiet = quiet ) } diff --git a/man/build_articles.Rd b/man/build_articles.Rd index 3f5b9c072..c1753785e 100644 --- a/man/build_articles.Rd +++ b/man/build_articles.Rd @@ -21,6 +21,7 @@ build_article( data = list(), lazy = FALSE, seed = 1014L, + new_process = TRUE, quiet = TRUE ) @@ -48,6 +49,10 @@ freshly generated section in browser.} relative to \verb{vignettes/} without extension, or \code{index} or \code{README}.} \item{data}{Additional data to pass on to template.} + +\item{new_process}{Build the article in a clean R process? The default, +\code{TRUE}, ensures that every article is build in a fresh environment, but +you may want to set it to \code{FALSE} to make debugging easier.} } \description{ \code{build_articles()} renders each R Markdown file underneath \verb{vignettes/} and From 6f30f5d25f8a8ed3b91f7319f6c65784186ed621 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Mon, 15 Apr 2024 09:28:12 -0500 Subject: [PATCH 9/9] Add news bullet --- NEWS.md | 1 + 1 file changed, 1 insertion(+) diff --git a/NEWS.md b/NEWS.md index 68ec2564f..80117fa44 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,6 @@ # pkgdown (development version) +* `build_article()` gains a new `new_process` argument which allows to build a vignette in the current process for debugging purposes. We've also improved the error messages and tracebacks if an article fails to build, hopefully also making debugging easier (#2438). * Preview links now work once again (#2435). * `build_home()` no longer renders Github issue and pull request templates (@hsloot, #2362) * It is now easier to preview parts of the website locally interactively. `build_reference_index()` and friends will call `init_site()` internally instead of erroring (@olivroy, #2329).