From 5703b34f85bcbfb3ae19e4952b1ceb22012f3f96 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Thu, 25 Apr 2024 17:03:40 -0500 Subject: [PATCH 1/7] Fix up knitr paths relative to the output directory For some unknown reason, `knitr_print()` methods sometimes produces paths are relative to the input directory, rather than to the output directory. This PR re-parents those to paths to be relative to the output directory making them work again. Fixes #2334. Fixes #2341. --- R/rmarkdown.R | 3 ++- R/tweak-page.R | 24 +++++++++++++++++++++--- tests/testthat/test-tweak-page.R | 2 +- vignettes/img-path.Rmd | 29 +++++++++++++++++++++++++++++ 4 files changed, 53 insertions(+), 5 deletions(-) create mode 100644 vignettes/img-path.Rmd diff --git a/R/rmarkdown.R b/R/rmarkdown.R index d8641d986..5f0122505 100644 --- a/R/rmarkdown.R +++ b/R/rmarkdown.R @@ -56,7 +56,8 @@ render_rmarkdown <- function(pkg, input, output, ..., seed = NULL, copy_images = update_html( path, tweak_rmarkdown_html, - input_path = path_dir(input_path), + output_dir = path_dir(output_path), + input_dir = path_dir(input_path), pkg = pkg ) } diff --git a/R/tweak-page.R b/R/tweak-page.R index 1deecea4f..c2de0a1e4 100644 --- a/R/tweak-page.R +++ b/R/tweak-page.R @@ -37,19 +37,37 @@ tweak_page <- function(html, name, pkg = list(bs_version = 3)) { } } -tweak_rmarkdown_html <- function(html, input_path, pkg = list(bs_version = 3)) { +tweak_rmarkdown_html <- function(html, input_dir, output_dir, pkg = list(bs_version = 3)) { # Tweak classes of navbar toc <- xml2::xml_find_all(html, ".//div[@id='tocnav']//ul") xml2::xml_attr(toc, "class") <- "nav nav-pills nav-stacked" - # Make sure all images use relative paths img <- xml2::xml_find_all(html, "//img") src <- xml2::xml_attr(img, "src") + + # Drop the logo and any inline images + is_ok <- !grepl("^data:", src) & xml2::xml_attr(img, "class", default = "") != "logo" + img <- img[is_ok] + src <- src[is_ok] + + # Fix up absoluate paths to be relative to input_dir abs_src <- is_absolute_path(src) if (any(abs_src)) { purrr::walk2( img[abs_src], - path_rel(src[abs_src], input_path), + path_rel(src[abs_src], input_dir), + xml2::xml_set_attr, + attr = "src" + ) + } + + # Fix up paths that are relative to input_dir instead of output_dir + input_abs_path <- path_tidy(path(input_dir, src)) + up_path <- !abs_src & path_has_parent(input_abs_path, output_dir) + if (any(up_path)) { + purrr::walk2( + img[up_path], + path_rel(path(input_dir, src[up_path]), output_dir), xml2::xml_set_attr, attr = "src" ) diff --git a/tests/testthat/test-tweak-page.R b/tests/testthat/test-tweak-page.R index 5db30340c..fe8c3fbeb 100644 --- a/tests/testthat/test-tweak-page.R +++ b/tests/testthat/test-tweak-page.R @@ -125,7 +125,7 @@ test_that("h1 section headings adjusted to h2 (and so on)", {

2

") - tweak_rmarkdown_html(html) + tweak_rmarkdown_html(html, input_dir = ".", output_dir = ".") expect_equal(xpath_text(html, ".//h1"), "Title") expect_equal(xpath_text(html, ".//h2"), c("1", "2")) expect_equal(xpath_text(html, ".//h3"), "1.1") diff --git a/vignettes/img-path.Rmd b/vignettes/img-path.Rmd new file mode 100644 index 000000000..ba34b712a --- /dev/null +++ b/vignettes/img-path.Rmd @@ -0,0 +1,29 @@ +--- +title: "imgpathdebug" +--- + +```{r} +plot(1:5) +``` + +```{r} +knitr::include_graphics("test/bacon.jpg") +``` + +```{r} +# magick:::`knit_print.magick-image`() +magick::image_read("https://jeroen.github.io/images/frink.png") +``` + +```{r} +library(ggplot2) +library(gganimate) + +p <- ggplot(mtcars, aes(factor(cyl), mpg)) + + geom_point() + + # Here comes the gganimate code + transition_states(gear) + +animate(p, nframes = 1) +``` + From 5cf47c1400417f14264b26918a8e4cb0999319d9 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 30 Apr 2024 12:35:39 +1200 Subject: [PATCH 2/7] Add an automated test And resolve issue with using tempdir --- R/tweak-page.R | 5 +-- tests/testthat/_snaps/build-articles.md | 17 ---------- tests/testthat/assets/README.md | 2 ++ .../.gitignore | 0 .../DESCRIPTION | 0 .../README.md | 0 .../_pkgdown.yml | 0 .../man-figures.Rproj | 0 .../man/figures/kitten.jpg | Bin .../man/kitten.Rd | 0 .../vignettes/.gitignore | 0 .../vignettes/another-kitten.jpg | Bin .../vignettes/kitten.Rmd | 18 +++++++++-- tests/testthat/test-build-articles.R | 17 +++++++--- tests/testthat/test-package.R | 2 +- tests/testthat/test-preview.R | 2 +- vignettes/img-path.Rmd | 29 ------------------ 17 files changed, 35 insertions(+), 57 deletions(-) create mode 100644 tests/testthat/assets/README.md rename tests/testthat/assets/{man-figures => articles-images}/.gitignore (100%) rename tests/testthat/assets/{man-figures => articles-images}/DESCRIPTION (100%) rename tests/testthat/assets/{man-figures => articles-images}/README.md (100%) rename tests/testthat/assets/{man-figures => articles-images}/_pkgdown.yml (100%) rename tests/testthat/assets/{man-figures => articles-images}/man-figures.Rproj (100%) rename tests/testthat/assets/{man-figures => articles-images}/man/figures/kitten.jpg (100%) rename tests/testthat/assets/{man-figures => articles-images}/man/kitten.Rd (100%) rename tests/testthat/assets/{man-figures => articles-images}/vignettes/.gitignore (100%) rename tests/testthat/assets/{man-figures => articles-images}/vignettes/another-kitten.jpg (100%) rename tests/testthat/assets/{man-figures => articles-images}/vignettes/kitten.Rmd (70%) delete mode 100644 vignettes/img-path.Rmd diff --git a/R/tweak-page.R b/R/tweak-page.R index c2de0a1e4..c7ed6c13e 100644 --- a/R/tweak-page.R +++ b/R/tweak-page.R @@ -62,12 +62,13 @@ tweak_rmarkdown_html <- function(html, input_dir, output_dir, pkg = list(bs_vers } # Fix up paths that are relative to input_dir instead of output_dir - input_abs_path <- path_tidy(path(input_dir, src)) + output_dir <- path_real(output_dir) + input_abs_path <- purrr::map_chr(src, ~ path_abs(., input_dir)) up_path <- !abs_src & path_has_parent(input_abs_path, output_dir) if (any(up_path)) { purrr::walk2( img[up_path], - path_rel(path(input_dir, src[up_path]), output_dir), + path_rel(path_abs(src[up_path], input_dir), output_dir), xml2::xml_set_attr, attr = "src" ) diff --git a/tests/testthat/_snaps/build-articles.md b/tests/testthat/_snaps/build-articles.md index 30f177da6..195642b07 100644 --- a/tests/testthat/_snaps/build-articles.md +++ b/tests/testthat/_snaps/build-articles.md @@ -1,20 +1,3 @@ -# links to man/figures are automatically relocated - - Code - copy_figures(pkg) - Message - Copying man/figures/kitten.jpg to reference/figures/kitten.jpg - ---- - - Code - build_articles(pkg, lazy = FALSE) - Message - -- Building articles ----------------------------------------------------------- - Writing `articles/index.html` - Reading vignettes/kitten.Rmd - Writing `articles/kitten.html` - # warns about missing images Code diff --git a/tests/testthat/assets/README.md b/tests/testthat/assets/README.md new file mode 100644 index 000000000..507c7cb4e --- /dev/null +++ b/tests/testthat/assets/README.md @@ -0,0 +1,2 @@ + +![](foo.png) diff --git a/tests/testthat/assets/man-figures/.gitignore b/tests/testthat/assets/articles-images/.gitignore similarity index 100% rename from tests/testthat/assets/man-figures/.gitignore rename to tests/testthat/assets/articles-images/.gitignore diff --git a/tests/testthat/assets/man-figures/DESCRIPTION b/tests/testthat/assets/articles-images/DESCRIPTION similarity index 100% rename from tests/testthat/assets/man-figures/DESCRIPTION rename to tests/testthat/assets/articles-images/DESCRIPTION diff --git a/tests/testthat/assets/man-figures/README.md b/tests/testthat/assets/articles-images/README.md similarity index 100% rename from tests/testthat/assets/man-figures/README.md rename to tests/testthat/assets/articles-images/README.md diff --git a/tests/testthat/assets/man-figures/_pkgdown.yml b/tests/testthat/assets/articles-images/_pkgdown.yml similarity index 100% rename from tests/testthat/assets/man-figures/_pkgdown.yml rename to tests/testthat/assets/articles-images/_pkgdown.yml diff --git a/tests/testthat/assets/man-figures/man-figures.Rproj b/tests/testthat/assets/articles-images/man-figures.Rproj similarity index 100% rename from tests/testthat/assets/man-figures/man-figures.Rproj rename to tests/testthat/assets/articles-images/man-figures.Rproj diff --git a/tests/testthat/assets/man-figures/man/figures/kitten.jpg b/tests/testthat/assets/articles-images/man/figures/kitten.jpg similarity index 100% rename from tests/testthat/assets/man-figures/man/figures/kitten.jpg rename to tests/testthat/assets/articles-images/man/figures/kitten.jpg diff --git a/tests/testthat/assets/man-figures/man/kitten.Rd b/tests/testthat/assets/articles-images/man/kitten.Rd similarity index 100% rename from tests/testthat/assets/man-figures/man/kitten.Rd rename to tests/testthat/assets/articles-images/man/kitten.Rd diff --git a/tests/testthat/assets/man-figures/vignettes/.gitignore b/tests/testthat/assets/articles-images/vignettes/.gitignore similarity index 100% rename from tests/testthat/assets/man-figures/vignettes/.gitignore rename to tests/testthat/assets/articles-images/vignettes/.gitignore diff --git a/tests/testthat/assets/man-figures/vignettes/another-kitten.jpg b/tests/testthat/assets/articles-images/vignettes/another-kitten.jpg similarity index 100% rename from tests/testthat/assets/man-figures/vignettes/another-kitten.jpg rename to tests/testthat/assets/articles-images/vignettes/another-kitten.jpg diff --git a/tests/testthat/assets/man-figures/vignettes/kitten.Rmd b/tests/testthat/assets/articles-images/vignettes/kitten.Rmd similarity index 70% rename from tests/testthat/assets/man-figures/vignettes/kitten.Rmd rename to tests/testthat/assets/articles-images/vignettes/kitten.Rmd index 0481c299b..332ecf94b 100644 --- a/tests/testthat/assets/man-figures/vignettes/kitten.Rmd +++ b/tests/testthat/assets/articles-images/vignettes/kitten.Rmd @@ -20,10 +20,24 @@ knitr::opts_chunk$set( knitr::include_graphics("../man/figures/kitten.jpg") ``` +``` {r} +knitr::include_graphics("another-kitten.jpg") +``` + ## rmarkdown ![](../man/figures/kitten.jpg) -## Another kitten - ![](another-kitten.jpg) + +## External package + +```{r magick} +magick::image_read("another-kitten.jpg") +``` + +## Plot + +```{r plot} +plot(1:3) +``` \ No newline at end of file diff --git a/tests/testthat/test-build-articles.R b/tests/testthat/test-build-articles.R index 37f6c342b..dc4c839f2 100644 --- a/tests/testthat/test-build-articles.R +++ b/tests/testthat/test-build-articles.R @@ -5,21 +5,28 @@ test_that("can recognise intro variants", { expect_true(article_is_intro("articles/pack-age", "pack.age")) }) -test_that("links to man/figures are automatically relocated", { +test_that("image links relative to output", { # weird path differences that I don't have the energy to dig into skip_on_cran() - pkg <- local_pkgdown_site(test_path("assets/man-figures")) + pkg <- local_pkgdown_site(test_path("assets/articles-images")) - expect_snapshot(copy_figures(pkg)) - expect_snapshot(build_articles(pkg, lazy = FALSE)) + suppressMessages(copy_figures(pkg)) + suppressMessages(build_article("kitten", pkg)) html <- xml2::read_html(path(pkg$dst_path, "articles", "kitten.html")) src <- xpath_attr(html, "//img", "src") expect_equal(src, c( + # knitr::include_graphics() "../reference/figures/kitten.jpg", + "another-kitten.jpg", + # rmarkdown image "../reference/figures/kitten.jpg", - "another-kitten.jpg" + "another-kitten.jpg", + # magick::image_read() + "kitten_files/figure-html/magick-1.png", + # figure + "kitten_files/figure-html/plot-1.jpg" )) # And files aren't copied diff --git a/tests/testthat/test-package.R b/tests/testthat/test-package.R index d2f4528a4..ce3dd158a 100644 --- a/tests/testthat/test-package.R +++ b/tests/testthat/test-package.R @@ -55,7 +55,7 @@ test_that("package_vignettes() sorts articles alphabetically by file name", { }) test_that("override works correctly for as_pkgdown", { - pkgdown <- as_pkgdown("assets/man-figures") + pkgdown <- as_pkgdown(test_path("assets/articles-images")) expected_list <- list(figures = list(dev = "jpeg", fig.ext = "jpg", fig.width = 3, fig.asp = 1)) expect_equal(pkgdown$meta, expected_list) modified_pkgdown <- as_pkgdown(pkgdown, override = list(figures = list(dev = "png"))) diff --git a/tests/testthat/test-preview.R b/tests/testthat/test-preview.R index 35ee01f93..74e261285 100644 --- a/tests/testthat/test-preview.R +++ b/tests/testthat/test-preview.R @@ -1,5 +1,5 @@ test_that("checks its inputs", { - pkg <- local_pkgdown_site(test_path("assets/man-figures")) + pkg <- local_pkgdown_site(test_path("assets/articles-images")) expect_snapshot(error = TRUE, { preview_site(pkg, path = 1) diff --git a/vignettes/img-path.Rmd b/vignettes/img-path.Rmd deleted file mode 100644 index ba34b712a..000000000 --- a/vignettes/img-path.Rmd +++ /dev/null @@ -1,29 +0,0 @@ ---- -title: "imgpathdebug" ---- - -```{r} -plot(1:5) -``` - -```{r} -knitr::include_graphics("test/bacon.jpg") -``` - -```{r} -# magick:::`knit_print.magick-image`() -magick::image_read("https://jeroen.github.io/images/frink.png") -``` - -```{r} -library(ggplot2) -library(gganimate) - -p <- ggplot(mtcars, aes(factor(cyl), mpg)) + - geom_point() + - # Here comes the gganimate code - transition_states(gear) - -animate(p, nframes = 1) -``` - From 0b83f9bceab3b6fa2a4e7bac56435203a41acf1c Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 30 Apr 2024 12:36:44 +1200 Subject: [PATCH 3/7] Remove accidentally added file --- tests/testthat/assets/README.md | 2 -- 1 file changed, 2 deletions(-) delete mode 100644 tests/testthat/assets/README.md diff --git a/tests/testthat/assets/README.md b/tests/testthat/assets/README.md deleted file mode 100644 index 507c7cb4e..000000000 --- a/tests/testthat/assets/README.md +++ /dev/null @@ -1,2 +0,0 @@ - -![](foo.png) From d95b548dbeabb2ea464095c088c980aef44e47fe Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 30 Apr 2024 14:57:56 +1200 Subject: [PATCH 4/7] Needs magick dep --- DESCRIPTION | 1 + 1 file changed, 1 insertion(+) diff --git a/DESCRIPTION b/DESCRIPTION index 379a92463..154831fcb 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -50,6 +50,7 @@ Suggests: htmlwidgets, knitr, lifecycle, + magick, methods, openssl, pkgload (>= 1.0.2), From 9bdd04f8288d9879980ed0d89a54d7cab181144b Mon Sep 17 00:00:00 2001 From: Christophe Dervieux Date: Tue, 30 Apr 2024 15:03:28 +0200 Subject: [PATCH 5/7] Revert "Fix up knitr paths relative to the output directory" from r-lib/pkgdown#2502 This reverts commit 5703b34f85bcbfb3ae19e4952b1ceb22012f3f96. --- R/rmarkdown.R | 3 +-- R/tweak-page.R | 25 +++---------------------- tests/testthat/test-tweak-page.R | 2 +- 3 files changed, 5 insertions(+), 25 deletions(-) diff --git a/R/rmarkdown.R b/R/rmarkdown.R index 5f0122505..d8641d986 100644 --- a/R/rmarkdown.R +++ b/R/rmarkdown.R @@ -56,8 +56,7 @@ render_rmarkdown <- function(pkg, input, output, ..., seed = NULL, copy_images = update_html( path, tweak_rmarkdown_html, - output_dir = path_dir(output_path), - input_dir = path_dir(input_path), + input_path = path_dir(input_path), pkg = pkg ) } diff --git a/R/tweak-page.R b/R/tweak-page.R index c7ed6c13e..1deecea4f 100644 --- a/R/tweak-page.R +++ b/R/tweak-page.R @@ -37,38 +37,19 @@ tweak_page <- function(html, name, pkg = list(bs_version = 3)) { } } -tweak_rmarkdown_html <- function(html, input_dir, output_dir, pkg = list(bs_version = 3)) { +tweak_rmarkdown_html <- function(html, input_path, pkg = list(bs_version = 3)) { # Tweak classes of navbar toc <- xml2::xml_find_all(html, ".//div[@id='tocnav']//ul") xml2::xml_attr(toc, "class") <- "nav nav-pills nav-stacked" + # Make sure all images use relative paths img <- xml2::xml_find_all(html, "//img") src <- xml2::xml_attr(img, "src") - - # Drop the logo and any inline images - is_ok <- !grepl("^data:", src) & xml2::xml_attr(img, "class", default = "") != "logo" - img <- img[is_ok] - src <- src[is_ok] - - # Fix up absoluate paths to be relative to input_dir abs_src <- is_absolute_path(src) if (any(abs_src)) { purrr::walk2( img[abs_src], - path_rel(src[abs_src], input_dir), - xml2::xml_set_attr, - attr = "src" - ) - } - - # Fix up paths that are relative to input_dir instead of output_dir - output_dir <- path_real(output_dir) - input_abs_path <- purrr::map_chr(src, ~ path_abs(., input_dir)) - up_path <- !abs_src & path_has_parent(input_abs_path, output_dir) - if (any(up_path)) { - purrr::walk2( - img[up_path], - path_rel(path_abs(src[up_path], input_dir), output_dir), + path_rel(src[abs_src], input_path), xml2::xml_set_attr, attr = "src" ) diff --git a/tests/testthat/test-tweak-page.R b/tests/testthat/test-tweak-page.R index fe8c3fbeb..5db30340c 100644 --- a/tests/testthat/test-tweak-page.R +++ b/tests/testthat/test-tweak-page.R @@ -125,7 +125,7 @@ test_that("h1 section headings adjusted to h2 (and so on)", {

2

") - tweak_rmarkdown_html(html, input_dir = ".", output_dir = ".") + tweak_rmarkdown_html(html) expect_equal(xpath_text(html, ".//h1"), "Title") expect_equal(xpath_text(html, ".//h2"), c("1", "2")) expect_equal(xpath_text(html, ".//h3"), "1.1") From 5cde1178057a6a64dae66bd6a66d9de46f0d0a77 Mon Sep 17 00:00:00 2001 From: Christophe Dervieux Date: Tue, 30 Apr 2024 15:19:36 +0200 Subject: [PATCH 6/7] Ensure `knitr::include_graphics()` does not do any absolute to relative path processing. This processing is by default, and paths are made relative to input directory and not output. It should be fixed in later knitr version yihui/knitr#2171 --- R/rmarkdown.R | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/R/rmarkdown.R b/R/rmarkdown.R index d8641d986..7cf93ecd3 100644 --- a/R/rmarkdown.R +++ b/R/rmarkdown.R @@ -29,7 +29,10 @@ render_rmarkdown <- function(pkg, input, output, ..., seed = NULL, copy_images = BSTINPUTS = bst_paths(input_path), TEXINPUTS = tex_paths(input_path), BIBINPUTS = bib_paths(input_path), - R_CLI_NUM_COLORS = 256 + R_CLI_NUM_COLORS = 256, + # Ensure paths from output are not made relative to input + # https://github.com/yihui/knitr/issues/2171 + R_KNITR_OPTIONS = "knitr.graphics.rel_path=FALSE" ) if (new_process) { @@ -108,7 +111,7 @@ rmarkdown_render_with_seed <- function(..., seed = NULL) { # envir$.Random.seed <- .GlobalEnv$.Random.seed # } } - + rmarkdown::render(envir = globalenv(), ...) } From bbe5bff55a69eafb64abcc6cafcdc53db374e686 Mon Sep 17 00:00:00 2001 From: Christophe Dervieux Date: Tue, 30 Apr 2024 21:22:53 +0200 Subject: [PATCH 7/7] Use dev rmarkdown --- DESCRIPTION | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/DESCRIPTION b/DESCRIPTION index 154831fcb..3bbef16bd 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -34,7 +34,7 @@ Imports: purrr (>= 1.0.0), ragg, rlang (>= 1.1.0), - rmarkdown (>= 1.1.9007), + rmarkdown (>= 2.26.2), tibble, whisker, withr (>= 2.4.3), @@ -72,3 +72,5 @@ Encoding: UTF-8 Roxygen: list(markdown = TRUE) RoxygenNote: 7.3.1 SystemRequirements: pandoc +Remotes: + rstudio/rmarkdown