yihui / knitr

A general-purpose tool for dynamic report generation in R
https://yihui.org/knitr/
2.37k stars 874 forks source link

Correctly check ghostscript for cropping on Windows #2246

Closed cderv closed 1 year ago

cderv commented 1 year ago

Follows discussions in https://github.com/rstudio/tinytex/issues/391

We need to make knitr::plot_crop() conditional on windows to use the ghotscript internal to TeX Live. The internal ghostscript is not found by our detection because it is not expose in path (which is on purpose by Tex Live)

https://github.com/yihui/knitr/blame/1239f36c5dee8b0e029958a793ae71558902f52c/R/plot.R#L374

This is an adaptation of previous fix #954

But we may also need to reconsider in rmarkdown as we also check for ghostcript using tools::find_gs_cmd() https://github.com/rstudio/rmarkdown/blob/0e794e35717c3be9637e2b0468bb97010d84b527/R/util.R#L318-L331

My understand is that

However, the use of internal ghostcript is conditional to Tex Live version as from https://github.com/rstudio/tinytex/issues/391 this is not working and an external program should be used.

Maybe using grepl("TeX Live 2023", tinytex::tlmgr_version(FALSE)) ?

remlapmot commented 1 year ago

If helpful, I'm happy to help test proposed changes.

remlapmot commented 1 year ago

And here is where the existing cropping logic is included in the Quarto CLI:

https://github.com/quarto-dev/quarto-cli/blob/2246187f1aa0c4bee2b68c677b8efa55a0f8c779/src/resources/rmd/execute.R#L272

cderv commented 1 year ago

Oh thanks, they indeed do not use an exported or internal function from knitr ... too bad... 😞

yihui commented 1 year ago

I've implemented the logic in knitr:::has_crop_tools() in https://github.com/yihui/knitr/commit/4953bec489479bcef6eeb2f52cb9be40daabe47b. I hope I understood it correctly. If it was correct, we'll need to replace the internal function has_crop_tools() in rmarkdown with this one, and Quarto should use it, too.

Thanks!

cderv commented 1 year ago

Looks good !

If helpful, I'm happy to help test proposed changes.

@remlapmot can you test it to confirm ?

remlapmot commented 1 year ago

Thanks indeed @yihui. This is now working well for me.

Sorry that I have been slow to reply @cderv, I am a lecturer and I was teaching and then marking.

I have checked this by uninstalling my external Ghostscript, and under the development version of knitr (1.42.10) on Windows, using TinyTeX (tlmgr 66798, TeX Live 2023). My test Rmd file was the following.

---
output: pdf_document
---

```{r include=FALSE}
if (!tinytex::check_installed('pdfcrop')) tinytex::tlmgr_install("pdfcrop")
knitr::knit_hooks$set(crop = knitr::hook_pdfcrop)
plot(1:10)
plot(1:10)


Which produced the following pdf as expected (i.e., with the first figure cropped and the second figure uncropped).

<img width="884" alt="2023-05-18 23_24_35-test-windows-rmd-fix pdf - SumatraPDF" src="https://github.com/yihui/knitr/assets/3777473/9649cdcd-392f-4795-adc6-dd99a57aa3fc">
github-actions[bot] commented 8 months ago

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.