-
Notifications
You must be signed in to change notification settings - Fork 335
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
Set lib path for quarto render #2864
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
@@ -108,6 +108,13 @@ quarto_render <- function(pkg, path, quiet = TRUE, frame = caller_env()) { | |||
write_yaml(quarto_format(pkg), metadata_path) | |||
|
|||
output_dir <- withr::local_tempdir("pkgdown-quarto-", .local_envir = frame) | |||
|
|||
# set the lib path to the pkgdown installation directory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This explanation makes me wonder if this might actually be a bug in quarto_render()
. @cderv it's possible that the quarto package should be doing this to ensure that you use the same library as the current R session.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is what I was trying to get at here #2830 (comment).
I would have expected quarto to use the library path of the session it's being called in. Is there a situation where this shouldn't be the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, as a general rule, whenever you run code in a new process I think it should inherit the libpaths of the current process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see now more clearly.
Yes, I think the quarto R package should do that directly. It's using processx::run()
, and I should probably do something like callr::r()
and inherit .libPaths()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though I don't see exactly how using withr::local_libpaths()
does change things here. 🤔
- first item in
.libPaths()
is retrieved inlib_path
- and then assigned as library path using equivalent of
.libPaths(lib_path)
Isn't it a roundtrip ?
So if quarto_render()
is not getting .libPaths()
info in the first place, how does using withr::local_libpaths()
helps here ?
I was thinking I should do something more like callr::r()
when I need to assign the main process .libPaths()
to environment variable, so that they are passed to new subprocess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested with
---
title: "Untitled"
format: gfm
---
```{r}
#| echo: false
.libPaths()
```
And then
> withr::local_temp_libpaths()
Setting global deferred event(s).
i These will be run:
* Automatically, when the R session ends.
* On demand, if you call `withr::deferred_run()`.
i Use `withr::deferred_clear()` to clear them without executing.
> .libPaths()
[1] "C:/Users/chris/AppData/Local/Temp/RtmpK6lR0o/temp_libpathfe84337b7af2" "C:/Users/chris/AppData/Local/R/win-library/4.4" "C:/Program Files/R/R-4.4.2/library"
> quarto::quarto_render("test.qmd", quiet = TRUE)
> xfun::file_string("test.md")
# Untitled
[1] "C:/Users/chris/AppData/Local/R/win-library/4.4"
[2] "C:/Program Files/R/R-4.4.2/library"
So the temporary library is not see for Quarto rendering.
I think R_LIBS
needs to be set so that it works for sub process (that will inherit env var)
> .libPaths()
[1] "C:/Users/chris/AppData/Local/Temp/RtmpK6lR0o/temp_libpathfe84337b7af2" "C:/Users/chris/AppData/Local/R/win-library/4.4" "C:/Program Files/R/R-4.4.2/library"
> withr::with_envvar(
+ list(R_LIBS = paste(.libPaths(), collapse = .Platform$path.sep)),
+ quarto::quarto_render("test.qmd", quiet = TRUE)
+ )
> xfun::file_string("test.md")
# Untitled
[1] "C:/Users/chris/AppData/Local/Temp/RtmpK6lR0o/temp_libpathfe84337b7af2"
[2] "C:/Users/chris/AppData/Local/R/win-library/4.4"
[3] "C:/Program Files/R/R-4.4.2/library"
So I am not this PR does help. I am looking at making the env var setting in quarto R package directly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TLDR I think setting lib path in quarto via callr::r
is the more robust solution.
The intended behavior of this PR:
- When
pkgdown::build_site(install = FALSE)
is called, the standard library path is used (i.e., what you see above). - But when
pkgdown::build_site(install = TRUE)
is called, the library is installed in a temporary location in a callr session, so we fetch the temporary lib path, which is prepended to.libPaths()
(hence the.libPaths[1]
).
This was reported to fix the issue, but I could never figure out how to trigger this on GHA, so didn't have a test case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TLDR I think setting lib path in quarto via callr::r is the more robust solution.
I can't use callr directly, but I can mimic it. I tried
But it did not solve the issue I have in quarto for windows build
So I am still trying to understand.
For pkgdown issue I could not reproduce either yet.
so we fetch the temporary lib path, which is prepended to .libPaths() (hence the .libPaths[1])
This is what I understood. But then passing it to withr::local_libpaths()
will put this temporary library back in .libPaths()
. As shown in example above quarto_render()
will call quarto
CLI, which will call R directly and this .libPaths()
from initial R session will not be passed to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For pkgdown issue I could not reproduce either yet.
Ok I got it to reproduce locally now, and I can check that quarto-dev/quarto-r#228 solves it.
I use your repo as an example: https://github.com/jayhesselberth/pkgdown.quarto.issue in a fork. This PR does not solve the problem for me.
I just found another issue from
pkgdown/R/build-quarto-articles.R
Lines 47 to 50 in 7645a47
built_path <- path(output_dir, path_rel(output_file, "articles")) | |
if (!file_exists(built_path)) { | |
cli::cli_abort("No built file found for {.file {input_file}}") | |
} |
── Building articles ───────────────────────────────────────────────────────────
Writing articles/index.html
Reading vignettes/test-rmarkdown.Rmd
Reading vignettes/articles/test-article.qmd
Reading vignettes/test.qmd
Running `quarto render`
Error:
! in callr subprocess.
Caused by error in `.f(.x[[i]], .y[[i]], ...)`:
! No built file found for vignettes/articles/test-article.qmd
ℹ See `$stdout` and `$stderr` for standard output and error.
Type .Last.error to see the more details.
articles\test-article.qmd
is built in <temp>\pkgdown-quarto-<hash>\articles\test-article.html
, but path check above is <temp>\pkgdown-quarto-<hash>\test-article.html
which does not exists.
I'll open a new issue about this.
Closes #2830