No warning when Ghostsript cannot be find and fig_crop = TRUE

[netique]

I write a custom format and I've noticed there is a fig_crop argument in knitr_options_pdf, which sounds pretty neat. After inspection of the generated output of the function, there was no mention about any cropping, so I went to the definition and spotted that a user is not informed by any means that pdfcrop or ghostscript is missing, and crop is silently not applied:

rmarkdown/R/output_format.R

Lines 264 to 269 in 268c6eb

	# apply cropping if requested and we have pdfcrop and ghostscript
	crop <- fig_crop && find_program("pdfcrop") != '' && tools::find_gs_cmd() != ''
	if (crop) {
	knit_hooks = list(crop = knitr::hook_pdfcrop)
	opts_chunk$crop = TRUE
	}

Note that knitr also does those checks (e.g. knitr:::has_utility("ghostsript")) whenever knitr:::pdf_crop() is called and issues proper warnings as expected. Note further that even if user installs everything needed, Ghostscript does not register its directories to PATH (at least on Windows) and this can lead to another confusion.

---

By filing an issue to this repo, I promise that

• I have fully read the issue guide at https://yihui.org/issue/.
• I have provided the necessary information about my issue.
  • If I'm asking a question, I have already asked it on Stack Overflow or RStudio Community, waited for at least 24 hours, and included a link to my question there.
  • If I'm filing a bug report, I have included a minimal, self-contained, and reproducible example, and have also included xfun::session_info('rmarkdown'). I have upgraded all my packages to their latest versions (e.g., R, RStudio, and R packages), and also tried the development version: remotes::install_github('rstudio/rmarkdown').
  • If I have posted the same issue elsewhere, I have also mentioned it in this issue.
• I have learned the Github Markdown syntax, and formatted my issue correctly.

I understand that my issue may be closed if I don't fulfill my promises.

[cderv]

Thanks for the report.

If I understand correctly, you would have expected a warning when fig_crop = TRUE that it has been set to FALSE internally because pdfcrop and/or ghostscript are not found in PATH. The same way that knitr does.

Am I understanding it ok ?

We could definitely makes the warning more coherent.

[netique]

Yeah, absolutely. Just to be a bit more verbose. So the user does not have to be surprised why his or her figures are not cropped even if they opt for so. That is the point.

[cderv]

Thanks for the report @netique !

[mirh]

Ehrm, considering fig_crop is enabled by default, shouldn't the warn be thrown "on failed actual use" rather than "hypothetical initialization"?

[yihui]

@mirh I agree.

@cderv We could probably try this:

diff --git a/R/output_format.R b/R/output_format.R
index db90df08..c7eb0a73 100644
--- a/R/output_format.R
+++ b/R/output_format.R
@@ -262,7 +262,7 @@ knitr_options_pdf <- function(fig_width,
   knit_hooks <- NULL
 
   # apply cropping if requested and we have pdfcrop and ghostscript
-  crop <- fig_crop && has_crop_tools()
+  crop <- fig_crop && has_crop_tools(!missing(fig_crop))
   if (crop) {
     knit_hooks = list(crop = knitr::hook_pdfcrop)
     opts_chunk$crop = TRUE
diff --git a/R/util.R b/R/util.R
index e900168e..d2ade523 100644
--- a/R/util.R
+++ b/R/util.R
@@ -312,7 +312,7 @@ find_program <- function(program) {
   }
 }
 
-has_crop_tools <- function() {
+has_crop_tools <- function(warn = TRUE) {
   tools <- c(
     pdfcrop = unname(find_program("pdfcrop")),
     ghostcript = unname(tools::find_gs_cmd())
@@ -320,7 +320,7 @@ has_crop_tools <- function() {
   missing <- tools[tools == ""]
   if (length(missing) == 0) return(TRUE)
   x <- paste0(names(missing), collapse = ", ")
-  warning(
+  if (warn) warning(
     sprintf("\nTool(s) not installed or not in PATH: %s", x),
     "\n-> As a result, figure cropping will be disabled."
   )

In general, I don't really like testing missing(arg) since it's not the best way to tell if an argument value was provided explicitly, but I'm afraid this is the only way to go now. If I were to write this function again, I'd probably default to fig_crop = "auto" (TRUE is cropping tools exist, otherwise FALSE). It may be too late to change the default, so the only way to tell if users explicitly opted in is !missing(fig_crop).

[cderv]

yes I think fig.crop = TRUE is not a good default. I think it is the source of issues (like #1934).

I wonder if this is really too late to change:

• Currently, it is TRUE - but it is set to FALSE if the tools are not present, with a warning.
• If we set to "auto" this means,
  • TRUE if crop tools are there
  • FALSE if not

Before, if users did not have the crop tools then fig_crop was implicitly set to FALSE as default.
It seems the same, isn't it ?

Am I missing something ?

[mirh]

I also don't think changing a default should be a taboo, at least if it's pointed out in big enough letters in the release notes
(if not any, it would still be a drop in the bucket, compared to all the other crazy shit I had to bisect when "upgrading" this week my thesis from "2019 rstudio+rmarkdown+pandoc" to 2021 ones)
Though arguably I'm just a random peasant when it comes to R and whatnot.

But I'm wondering: why should the default option depend automatically on the tools installed more or less explicitly in one's environment?
That seems recipe for irreproducibility and people banging their heads for hours.

[cderv]

compared to all the other crazy shit I had to bisect when "upgrading" this week my thesis from "2019 rstudio+rmarkdown+pandoc" to 2021 ones

I am really interested to hear about all that. Feel free to open a new issue to share all your feedback - I thinkit would help us do better in the future.

[mirh]

Since I was using my own thesisdown template, I'm not really sure how much generalizable it could be.
I mean, there was this harmless warning, then rstudio/rstudio#8954 and jgm/citeproc#60 and jgm/pandoc#7219, but more or less most of the issues could be named "the CSLReferences saga" and some fiddling with \usepackage.
(pandoc also screwed up columns in-between, but at least updating to a newer version right-away fixed it)

At the end of the day not even something *that* huge to fix, if you measure it in "lines of code", but of course you have to understand what is going actually on first and foremost.
And then craft out the magical combo to make latex happily typeset the pre-existing layout I had originally.
(if really I had to cast a stone for.. still deeper analysis, it is if this old APA style is really supposed to look as much badly indented as all the current official latex templates make it appear)

EDIT: sigh, after actually comparing pixel-by-pixel the pages I also found out of jgm/pandoc#7347 and jgm/pandoc#7393. So TL;DR this was really a big deal to upgrade (and even then, I still have tables slightly wider since the tabcolsep switch).

[yihui]

FYI fig_crop = "auto" has become the default in rmarkdown v2.8.

[github-actions]

This old thread has been automatically locked. If you think you have found something related to this, please open a new issue by following the issue guide (https://yihui.org/issue/), and link to this old issue if necessary.