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

plot_crop() and the convert command from ImageMagick #1785

Closed
3 tasks done
hpages opened this issue Dec 12, 2019 · 2 comments
Closed
3 tasks done

plot_crop() and the convert command from ImageMagick #1785

hpages opened this issue Dec 12, 2019 · 2 comments

Comments

@hpages
Copy link

hpages commented Dec 12, 2019


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.


Hi,

We use Windows Server 2012 R2 for the Bioconductor build system to build and check Bioconductor packages. When building the vignettes, we sometimes see a warning like this one for some packages:

Invalid Parameter - /figure-html
Warning in shell(paste(c(cmd, args), collapse = " ")) :
  'convert "overview_files/figure-html/unnamed-chunk-5-1.png" -trim "overview_files/figure-html/unnamed-chunk-5-1.png"' execution failed with error code 4

Tracking down this warning reveals at least 2 issues with knitr:

  • The SystemRequirements field for knitr makes no mention of the dependency on ImageMagick.
  • On Windows Server 2012 R2 (and maybe on other Windows flavors), if ImageMagick is not installed Sys.which("convert") returns:
     > Sys.which("convert")
                                 convert
    "C:\\Windows\\SYSTEM32\\convert.exe"
    
    This convert.exe command is a Microsoft utility for converting a file-system from FAT to NTFS (see https://docs.microsoft.com/en-us/windows-server/administration/windows-commands/convert). Unfortunately this is what knitr::plot_crop() picks up which is why we get the above warnings.

Since knitr already suggests CRAN package magick (since knitr 1.22), I wonder if there is any reason to not use this in plot_crop() instead of a system call to the convert command from ImageMagick. That way knitr would no longer depend on ImageMagick and the risk to pick up the wrong convert command would be eliminated, making knitr::plot_crop() more robust and platform independent.

Thanks,
H.

> xfun::session_info('knitr')
R version 3.6.1 (2019-07-05)
Platform: x86_64-w64-mingw32/x64 (64-bit)
Running under: Windows Server 2012 R2 x64 (build 9600)

Locale:
  LC_COLLATE=English_United States.1252
  LC_CTYPE=English_United States.1252
  LC_MONETARY=English_United States.1252
  LC_NUMERIC=C
  LC_TIME=English_United States.1252

Package version:
  evaluate_0.14   glue_1.3.1      graphics_3.6.1  grDevices_3.6.1
  highr_0.8       knitr_1.26      magrittr_1.5    markdown_1.1
  methods_3.6.1   mime_0.7        stats_3.6.1     stringi_1.4.3
  stringr_1.4.0   tools_3.6.1     utils_3.6.1     xfun_0.11
  yaml_2.2.0
@yihui
Copy link
Owner

yihui commented Jan 15, 2020

Thanks for the report! The plot_crop() function existed long before the magick package. That's why it didn't use magick. Now it definitely should. I'll make the change.

@yihui yihui closed this as completed in 2c7fe06 Jan 15, 2020
lcolladotor added a commit to leekgroup/regionReport that referenced this issue Apr 24, 2020
…ound taken

This is also an opportunity to link to the relevant GitHub issues and Bioc-devel
threads.

Cropping images through `magick::image_trim()` as done by default by
`BiocStyle::html_document()` can fail on Linux. This could be an ImageMagick
issue or an issue about lack of resources. The full investigative report is at
https://stat.ethz.ch/pipermail/bioc-devel/2020-April/016650.html.
This is related to:
yihui/knitr#1785 (comment)
yihui/knitr#1796
Bioconductor/BiocStyle#65 (comment)
ropensci/magick#171
ropensci/magick#194
In regionReport version 1.21.10 I have opted by using `crop = NULL` to disable
cropping of images by `BiocStyle::html_document()` and thus avoid the
issues with `ImageMagick` either coming from `magick`, from the version of
`ImageMagick` installed on the Linux Bioconductor build machine and devel
docker, or from resources in these two Linux environments as described in the
investigative report.
About a month ago I also saw failures on Windows on Bioc 3.10. Whether they were
caused by ggbio 1.35.1 or this issue will remain a mystery. But it's likely
that this `magick::image_trim()` issue also affected the Bioconductor windows
builder.
The related bioc-devel threads are:
https://stat.ethz.ch/pipermail/bioc-devel/2020-April/016538.html
https://stat.ethz.ch/pipermail/bioc-devel/2020-March/016365.html
@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