Check readme images after building reference and articles (#2494)

We can't know if the images in a readme exist until we have built the rest of the site. We worked around this a bit for images in `man/figures` but that strategy doesn't work for articles since we actually need to build them. So now we just check in a new step at the end of `build_site()`.

Fixes #2194

Author: hadley

NEWS.md

@@ -1,5 +1,6 @@
 # pkgdown (development version)
 
+* `build_home()` no longer checks if the README is missing any images. This check is now performed in `build_site()`, after `build_articles()` so you can refer to images created by vignettes with warnings (#2194).
 * `build_home()` now includes the contents of `inst/AUTHORS` on the authors page (#2506).
 * If you put a dropdown menu (e.g. articles) on the right hand side of the navbar, it will now be right aligned. This makes longer titles more likely to stay on the page (#2421).
 * The title for the "Reference" page is now "Package index" since this page might contain more than just function details (#2181).

R/build-home-index.R

@@ -3,13 +3,7 @@
 build_home_index <- function(pkg = ".", quiet = TRUE) {
   pkg <- section_init(pkg, depth = 0L)
 
-  src_path <- path_first_existing(
-    pkg$src_path,
-    c("pkgdown/index.md",
-      "index.md",
-      "README.md"
-    )
-  )
+  src_path <- path_index(pkg)
   dst_path <- path(pkg$dst_path, "index.html")
   data <- data_home(pkg)
 
@@ -34,12 +28,19 @@ build_home_index <- function(pkg = ".", quiet = TRUE) {
     logo = logo_path(pkg, depth = 0)
   )
 
-  copy_figures(pkg)
-  check_missing_images(pkg, path_rel(src_path, pkg$src_path), "index.html")
-
   invisible()
 }
 
+path_index <- function(pkg) {
+  path_first_existing(
+    pkg$src_path,
+    c("pkgdown/index.md",
+      "index.md",
+      "README.md"
+    )
+  )
+}
+
 data_home <- function(pkg = ".") {
   pkg <- as_pkgdown(pkg)
 
@@ -206,21 +207,3 @@ cran_link <- memoise(function(pkg) {
 
   NULL
 })
-
-
-check_missing_images <- function(pkg, src_path, dst_path) {
-  html <- xml2::read_html(path(pkg$dst_path, dst_path), encoding = "UTF-8")
-  src <- xml2::xml_attr(xml2::xml_find_all(html, ".//img"), "src")
-
-  rel_src <- src[xml2::url_parse(src)$scheme == ""]
-  rel_path <- fs::path_norm(path(fs::path_dir(dst_path), rel_src))
-  exists <- fs::file_exists(path(pkg$dst_path, rel_path))
-
-  if (any(!exists)) {
-    paths <- rel_src[!exists]
-    cli::cli_warn(c(
-      "Missing images in {.file {src_path}}: {.file {paths}}",
-      i = "pkgdown can only use images in {.file man/figures} and {.file vignettes}"
-    ))
-  }
-}

R/build.R

@@ -467,6 +467,8 @@ build_site_local <- function(pkg = ".",
     build_search(pkg, override = override)
   }
 
+  check_built_site(pkg)
+  
   cli::cli_rule("Finished building pkgdown site for package {.pkg {pkg$package}}")
   preview_site(pkg, preview = preview)
 }

R/check.R

@@ -21,3 +21,30 @@ check_pkgdown <- function(pkg = ".") {
 
   cli::cli_inform(c("v" = "No problems found."))
 }
+
+check_built_site <- function(pkg = ".") {
+  pkg <- as_pkgdown(pkg)
+
+  cli::cli_rule("Checking for problems")
+  index_path <- path_index(pkg)
+  if (!is.null(index_path)) {
+    check_missing_images(pkg, index_path, "index.html")
+  }
+}
+
+check_missing_images <- function(pkg, src_path, dst_path) {
+  html <- xml2::read_html(path(pkg$dst_path, dst_path), encoding = "UTF-8")
+  src <- xml2::xml_attr(xml2::xml_find_all(html, ".//img"), "src")
+
+  rel_src <- src[xml2::url_parse(src)$scheme == ""]
+  rel_path <- fs::path_norm(path(fs::path_dir(dst_path), rel_src))
+  exists <- fs::file_exists(path(pkg$dst_path, rel_path))
+
+  if (any(!exists)) {
+    paths <- rel_src[!exists]
+    cli::cli_warn(c(
+      "Missing images in {.file {path_rel(src_path, pkg$src_path)}}: {.file {paths}}",
+      i = "pkgdown can only use images in {.file man/figures} and {.file vignettes}"
+    ))
+  }
+}

R/rmarkdown.R

@@ -88,7 +88,7 @@ render_rmarkdown <- function(pkg, input, output, ..., seed = NULL, copy_images =
     dir_create(unique(path_dir(dst)))
     file_copy(src, dst, overwrite = TRUE)
   }
-  check_missing_images(pkg, input, output)
+  check_missing_images(pkg, input_path, output)
 
   invisible(path)
 }

tests/testthat/_snaps/build-articles.md

@@ -9,7 +9,7 @@
       Writing `articles/html-vignette.html`
     Condition
       Warning:
-      Missing images in 'vignettes/html-vignette.Rmd': 'foo.png'
+      Missing images in 'vignettes/html-vignette.Rmd': 'kitten.jpg'
       i pkgdown can only use images in 'man/figures' and 'vignettes'
 
 # articles don't include header-attrs.js script

tests/testthat/_snaps/build-home.md

@@ -16,20 +16,6 @@
       Writing `authors.html`
       Writing `404.html`
 
-# warns about missing images
-
-    Code
-      build_home(pkg)
-    Message
-      -- Building home ---------------------------------------------------------------
-      Writing `authors.html`
-    Condition
-      Warning:
-      Missing images in 'README.md': 'foo.png'
-      i pkgdown can only use images in 'man/figures' and 'vignettes'
-    Message
-      Writing `404.html`
-
 # can build site even if no Authors@R present
 
     Code

tests/testthat/_snaps/check.md

@@ -24,3 +24,14 @@
     Message
       v No problems found.
 
+# warn about missing images in readme
+
+    Code
+      check_built_site(pkg)
+    Message
+      -- Checking for problems -------------------------------------------------------
+    Condition
+      Warning:
+      Missing images in 'README.md': 'articles/kitten.jpg'
+      i pkgdown can only use images in 'man/figures' and 'vignettes'
+

tests/testthat/assets/bad-images/README.md

@@ -1,2 +1,2 @@
 
-![](foo.png)
+![](vignettes/kitten.jpg)

tests/testthat/assets/bad-images/vignettes/html-vignette.Rmd

@@ -6,4 +6,4 @@ title: "test"
 knitr::opts_chunk$set(collapse = TRUE, comment = "#>")
 ```
 
-![](foo.png)
+![](kitten.jpg)

tests/testthat/test-build-articles.R

@@ -34,6 +34,9 @@ test_that("image links relative to output", {
 })
 
 test_that("warns about missing images", {
+  # Added in #2509: I can't figure out why this is necessary :(
+  skip_on_covr()
+
   pkg <- local_pkgdown_site(test_path("assets/bad-images"))
   expect_snapshot(build_articles(pkg))
 })

tests/testthat/test-build-home.R

@@ -21,11 +21,6 @@ test_that("intermediate files cleaned up automatically", {
   )
 })
 
-test_that("warns about missing images", {
-  pkg <- local_pkgdown_site(test_path("assets/bad-images"))
-  expect_snapshot(build_home(pkg))
-})
-
 test_that("can build site even if no Authors@R present", {
   skip_if_no_pandoc()
 

tests/testthat/test-check.R

@@ -21,3 +21,26 @@ test_that("informs if everything is ok", {
   pkg <- local_pkgdown_site(test_path("assets/reference"))
   expect_snapshot(check_pkgdown(pkg))
 })
+
+# built site ---------------------------------------------------------------
+
+test_that("warn about missing images in readme", {
+  pkg <- local_pkgdown_site(test_path("assets/bad-images"))
+  suppressMessages(build_home(pkg))
+
+  expect_snapshot(check_built_site(pkg))
+})
+
+test_that("readme can use images from vignettes", {
+  pkg <- local_pkgdown_site(test_path("assets/bad-images"))
+  file_copy(
+    test_path("assets/articles-images/man/figures/kitten.jpg"),
+    path(pkg$src_path, "vignettes/kitten.jpg")
+  )
+  withr::defer(unlink(path(pkg$src_path, "vignettes/kitten.jpg")))
+
+  suppressMessages(build_home(pkg))
+  suppressMessages(build_articles(pkg))
+
+  suppressMessages(expect_no_warning(check_built_site(pkg)))
+})