Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't overwrite input files specified in knitr::include_graphics() #1800

Closed
3 tasks done
salim-b opened this issue Jan 21, 2020 · 9 comments
Closed
3 tasks done

Don't overwrite input files specified in knitr::include_graphics() #1800

salim-b opened this issue Jan 21, 2020 · 9 comments

Comments

@salim-b
Copy link
Contributor

salim-b commented Jan 21, 2020

I also ran into #1797 after updating to knitr 1.27. Then I updated to the dev version (1.27.2) and stumpled upon another issue I'd call a bug, too:

It seems that knitr is always overwriting the file included through knitr::include_graphics(). I don't know if this was already the case before knitr 1.27. I noticed it because I tried to build an MVE for the original bug using knitr's reference card PDF vignette as follows:

knitr::include_graphics(paste0(find.package(package = "knitr"), "/doc/knitr-refcard.pdf"))

Since I've installed knitr through Michael Rutter's PPA (Ubuntu Linux), on my system the above is equivalent to:

knitr::include_graphics("/usr/lib/R/site-library/knitr/doc/knitr-refcard.pdf")

When I try to build this, the following error is thrown:

!!! Error: Cannot move `tmp-pdfcrop-6024.pdf' to `/usr/lib/R/site-library/knitr/doc/knitr-refcard.pdf'!

This is of course because knitr isn't run as root, but the file is owned by root and knitr doesn't have write permission:

$ ls -1l /usr/lib/R/site-library/knitr/doc/knitr-refcard.pdf
-rw-r--r-- 1 root root 79531 Jan 19 04:04 /usr/lib/R/site-library/knitr/doc/knitr-refcard.pdf

I verified this with a file knitr actually has write permission to – it gets overwritten when included through knitr::include_graphics()! The documentation of knitr::include_graphics() doesn't say anything about possible file modifications of param path and I believe it was never really intended to overwrite path, right?


Addendum: I've just read about the crop option in #1796 and setting knitr::opts_chunk$set(crop = NULL) avoids knitr's attempt to overwrite the included PDF.

I think it's fine that knitr automatically crops white space around the files specified in knitr::include_graphics() but it should still never overwrite the original files!

And is there a specific reason why crop isn't a "proper" chunk options like e.g. fig.path? @yihui 's answer:

Because cropping images require additional software packages, and I tend to let users opt-in after they know how to install these packages, instead of "officially" supporting this feature.


> xfun::session_info('knitr')
R version 3.6.2 (2019-12-12)
Platform: x86_64-pc-linux-gnu (64-bit)
Running under: Ubuntu 16.04.6 LTS, RStudio 1.2.5033

Locale:
  LC_CTYPE=de_CH.UTF-8       LC_NUMERIC=C               LC_TIME=de_CH.UTF-8        LC_COLLATE=de_CH.UTF-8     LC_MONETARY=de_CH.UTF-8   
  LC_MESSAGES=de_CH.UTF-8    LC_PAPER=de_CH.UTF-8       LC_NAME=C                  LC_ADDRESS=C               LC_TELEPHONE=C            
  LC_MEASUREMENT=de_CH.UTF-8 LC_IDENTIFICATION=C       

Package version:
  evaluate_0.14   glue_1.3.1      graphics_3.6.2  grDevices_3.6.2 highr_0.8       knitr_1.27      magrittr_1.5    markdown_1.1    methods_3.6.2  
  mime_0.7        stats_3.6.2     stringi_1.4.5   stringr_1.4.0   tools_3.6.2     utils_3.6.2     xfun_0.12       yaml_2.2.0     

By filing an issue to this repo, I promise that

  • I have fully read the issue guide at https://yihui.name/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('knitr'). 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('yihui/knitr').
    • 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.

@yihui
Copy link
Owner

yihui commented Jan 21, 2020

As you seem to have discovered, this issue is not relevant to knitr::include_graphics(), but from the fact that you enabled plot cropping (perhaps inadvertently by using other packages such as BiocStyle or rmarkdown; all output formats in BiocStyle have this feature enabled, and rmarkdown's pdf_document also enabled this feature if pdfcrop is available).

As you have also discovered in #1796, the fix is to disable cropping.

You are right that this behavior should be more clearly documented on the help page ?knitr::hook_pdfcrop. Thanks!

@salim-b
Copy link
Contributor Author

salim-b commented Jan 21, 2020

As you seem to have discovered, this issue is not relevant to knitr::include_graphics(), but from the fact that you enabled plot cropping (perhaps inadvertently by using other packages such as BiocStyle or rmarkdown; all output formats in BiocStyle have this feature enabled, and rmarkdown's pdf_document also enabled this feature if pdfcrop is available).

Completely "inadvertently", yes. I don't use BiocStyle, but of course rmarkdown to render my R Markdown documents to PDF.

As you have also discovered in #1796, the fix is to disable cropping.

I don't consider this to be a proper fix. I strongly think it should be possible to use knitr's built-in PDF cropping without touching the original input files specified in knitr::include_graphics() (knitr creates intermediate files anyways, so why overwrite the original files in the first place?)

You are right that this behavior should be more clearly documented on the help page ?knitr::hook_pdfcrop.

That's only half the battle, I think. Before today, I didn't even know about knitr::hook_pdfcrop() but obviously my plots have been cropped that way all the time. At first I didn't notice the cropping because I already cropped the white space around them myself.

@yihui
Copy link
Owner

yihui commented Jan 21, 2020

I was not the person who brought this feature to rmarkdown originally. If I were to write the pdf_document function, I wouldn't enable this feature by default. I'll think more about whether I should disable it.

@salim-b
Copy link
Contributor Author

salim-b commented Jan 21, 2020

I was not the person who brought this feature to rmarkdown originally.

I meant no offence! I absolutely appreciate all the amazing work you put into the R ecosystem!

If I were to write the pdf_document function, I wouldn't enable this feature by default. I'll think more about whether I should disable it.

I don't know much about the internals, but wouldn't it be possible (and if so, the best solution) to rework this cropping feature in such a way that the original input files specified in knitr::include_graphics() aren't modified at all?

@yihui
Copy link
Owner

yihui commented Jan 22, 2020

That sounds reasonable. Please give me a little more time to think about it. Thanks for the excellent suggestion! (BTW, I didn't feel offended at all 😄)

@salim-b
Copy link
Contributor Author

salim-b commented Jan 23, 2020

Just a small addendum:

I'm almost sure that before knitr 1.27, knitr::hook_pdfcrop() was not triggered, i.e. the plots I've included through knitr::include_graphics() were not cropped by the CLI pdfcrop. I can tell because now Git always detects changes in the plot files after knitting which it didn't with knitr 1.26 and below.

@yihui
Copy link
Owner

yihui commented Jan 23, 2020

I'm almost sure that before knitr 1.27, knitr::hook_pdfcrop() was not triggered

You are correct!

@yihui yihui closed this as completed in ae9348c Feb 5, 2020
@yihui
Copy link
Owner

yihui commented Feb 5, 2020

Images included via include_graphics() will no longer be cropped. Thanks!

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants