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

Fix up knitr paths relative to the output directory #2502

Merged
merged 11 commits into from
Apr 30, 2024
Merged

Conversation

hadley
Copy link
Member

@hadley hadley commented Apr 25, 2024

For some unknown reason, knitr_print() methods sometimes produces paths are relative to the input directory, rather than to the output directory. This PR re-parents those to paths to be relative to the output directory making them work again.

Fixes #2334. Fixes #2341.

@thomasp85 would you mind checking that this fixes the gganimate site?

(To do: add a test so I don't accidentally break this again in the future)

For some unknown reason, `knitr_print()` methods sometimes produces paths are relative to the input directory, rather than to the output directory. This PR re-parents those to paths to be relative to the output directory making them work again.

Fixes #2334. Fixes #2341.
@hadley hadley requested a review from thomasp85 April 25, 2024 22:04
Copy link
Member

@thomasp85 thomasp85 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - this fixes the gganimate website and, I assume, other sites relying on gganimate

@cderv
Copy link
Contributor

cderv commented Apr 29, 2024

For some unknown reason, knitr_print() methods sometimes produces paths are relative to the input directory, rather than to the output directory.

About this, my understanding is the following

  • magick and gganimate both have knit_print methods that write their produced file in knitr::fig_path(). In rmarkdown context, this will be a path inside the output directory.
  • knitr::include_graphics() is used to include that path in the document, though it will be used with absolute path (as pkgdown use absolute path to call render() IIUC)
  • knitr::include_graphics() used on external resources will often be used on local resources relative to input.

This could lead to both input and output relative resources.

knitr::include_graphics() tries to handle the absolute to relative conversion, but it does not handle this output relative resources. This is to be fixed so that what magic and gganimate are doing works ok.

Clearly something to fix in knitr and/or rmarkdown

@cderv
Copy link
Contributor

cderv commented Apr 29, 2024

This may also be a breakage in rmarkdown 2.26. Absolute paths in HTML using output directory should have been made relative by rmarkdown::html_document_base post processor, and pkgdown should not need to deal with absolute path;

It should be fix soon and we'll do a patch release.

@hadley
Copy link
Member Author

hadley commented Apr 29, 2024

@cderv I think this problem is separate (if related), because it's a long standing issue.

@cderv
Copy link
Contributor

cderv commented Apr 29, 2024

Oh ok. From my quick tries of the examples of the two issues, I was really under the impression that the path handling absolute to relative to output-dir should have been handled by markdown post-processing. I'll recheck no matter the state of this PR to be sure there is nothing hidden in R Markdown or knitr

@hadley
Copy link
Member Author

hadley commented Apr 30, 2024

Awesome, thanks @cderv!

@hadley
Copy link
Member Author

hadley commented Apr 30, 2024

@cderv the windows failure is interesting — knitr certainly shouldn't be generating a path like ../../../../../../../../../../../C:/Users/runneradmin/AppData/Local/Temp/RtmpaaQWna/working_dir/RtmpusN7Up/file1434ec52ec2/articles/kitten_files/figure-html/magick-1.png (assuming that's not because I'm mangling the path in pkgdown).

@cderv
Copy link
Contributor

cderv commented Apr 30, 2024

So I looked closer into all the examples in pkgdown issue and the problem we have in knitr.

Considering that the 2.26 regression is fixed (rstudio/rmarkdown#2554), we are left with issue of include_graphics() default path processing. This is now explained in

I do believe that setting rel_path = FALSE for include_graphics() should solve your issue here. It can be done using knitr.graphics.rel_path = FALSE options to apply on all call to the function.

This relative path processing is unaware of the full rendering context enough to do the right work. It will only make path relative to the input directory, and so do not account for usage of include_graphics() with knitr::fig_path(), which both gganimate and magick knit_print() method are using.

pkgdown makes this issue surface because it uses absolute path for input and output in its call to rmarkdown::render()

Also when output_dir is not a subfolder of the input directory, this will lead to relative path with lots of ../. And this is what happens in CI

  • By default on Windows runner, the temp directory will be in another drive (c:/) than the working directory (d:).
  • So here, your pkgdown site is in your test folder, but pkgdown output dir is in a temp directory in another drive.
  • The relative path processing creates this long relative path (valid on windows I think) from output temp dir to the assets in tests folder.
  • I don't know if knitr should detect weird path like this that are on different drives. It is a combination of input and output dir being set to very different location. It needs to be better handling path in output directory though, and this would probably avoid this kind of path problem.

All comes to include_graphics(rel_path = FALSE). I believe if this is set

I'll try a forked version of this PR to verify my understanding of this.

@cderv
Copy link
Contributor

cderv commented Apr 30, 2024

FWIW my test in #2508 is successful after a few tries.

Two things are needed to solve all issues

  • Setting the option for knitr rel_path
  • Using dev rmarkdown to have the regression fix

Remaining CI failure here seems unrelated to rmarkdown / knitr path handling

@hadley hadley merged commit df6e6ea into main Apr 30, 2024
12 checks passed
@hadley hadley deleted the knitr-rel-path branch April 30, 2024 20:45
SebKrantz pushed a commit to SebKrantz/pkgdown that referenced this pull request Jun 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants