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.

Sign up for GitHub

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

Refine auto-loading behaviour #1915

Merged
merged 8 commits into from
Jul 2, 2024
Merged

Refine auto-loading behaviour #1915

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
beb8d61
Re-order these checks
jennybc May 29, 2024
2fb9e65
Put some DOTS on it
jennybc May 31, 2024
f24aba7
Be precise about the recommended action
jennybc May 31, 2024
9d8635f
Don't message about cancellation
jennybc Jun 4, 2024
1989c79
Refine action around renv::restore() during autoloading
jennybc Jun 7, 2024
a11676f
Make `renv::restore()` (maybe) a runnable hyperlink if cli is available
jennybc Jun 7, 2024
ec7bf57
Introduce `cancel(verbose =)`
jennybc Jun 24, 2024
f23c248
Back out of the cli experiment
jennybc Jun 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 21 additions & 13 deletions R/load.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,8 @@ load <- function(project = NULL, quiet = FALSE, profile = NULL, ...) {

action <- renv_load_action(project)
if (action[[1L]] == "cancel") {
cancel()
autoloading <- getOption("renv.autoloader.running", default = FALSE)
cancel(verbose = !autoloading)
Copy link
Member Author

@jennybc jennybc Jun 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Allows us to see the "- Operation canceled." message after overt cancelation by the user, but suppresses such a message when deferring the project load to the session init hook. Discussed elsewhere.

} else if (action[[1L]] == "init") {
return(init(project))
} else if (action[[1L]] == "alt") {
Expand Down Expand Up @@ -146,22 +147,28 @@ renv_load_action <- function(project) {
if (!interactive())
return("load")

autoloading <- getOption("renv.autoloader.running", default = FALSE)
# if we're auto-loading, it's too early to interact with the user, which is
# often advisable, i.e. if we detect that the user needs to run renv::restore()
# https://github.com/rstudio/renv/issues/1650
#
# if the frontend is known to support a session init hook, defer loading until
# R is fully initialized, at which time it will be possible to interact with
# the user (currently this just applies to RStudio)
#
# otherwise, proceed with the knowledge that, if the user needs to run
# renv::restore(), a message to that effect will be emitted
if (autoloading && renv_rstudio_available()) {
setHook("rstudio.sessionInit", function(...) { renv::load(project) })
Copy link
Member Author

@jennybc jennybc May 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Before, renv::load(project) was definitely being appended to the "rstudio.sessionInit" hook. I can see that with getHook("rstudio.sessionInit"). However it wasn't actually being executed and I think it must have failed some pre-check re: its signature. In my hands, adding ... makes this feature start to work as intended.

Copy link
Member Author

@jennybc jennybc May 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think this is the explanation:

https://github.com/rstudio/rstudio/blob/8ebe5431ca2fb3c9719b70d4fbc45201d226c79c/src/cpp/session/modules/SessionRHooks.R#L26-L29

RStudio forwards arguments and the whole thing happens inside tryCatch(). So I guess if the hook function does not accept any arguments, the attempt to run the hook fails. Silently.

return("cancel")
}

# if this project already contains an 'renv' folder, assume it's
# already been initialized and we can directly load it
renv <- renv_paths_renv(project = project, profile = FALSE)
if (dir.exists(renv))
return("load")

# if we're running within RStudio at this point, and we're running
# within the auto-loader, we need to defer execution here so that
# the console is able to properly receive user input and update
# https://github.com/rstudio/renv/issues/1650
autoloading <- getOption("renv.autoloader.running", default = FALSE)
if (autoloading && renv_rstudio_available()) {
setHook("rstudio.sessionInit", function() { renv::load(project) })
return("cancel")
}

# check and see if we're being called within a sub-directory
path <- renv_file_find(dirname(project), function(parent) {
if (file.exists(file.path(parent, "renv")))
Expand Down Expand Up @@ -876,12 +883,13 @@ renv_load_report_synchronized <- function(project = NULL, lockfile = NULL) {
if (length(intersect(lockpkgs, libpkgs)) == 0 && length(lockpkgs) > 0L) {

caution("- None of the packages recorded in the lockfile are currently installed.")
if (renv_rstudio_autoloading()) {
autoloading <- getOption("renv.autoloader.running", default = FALSE)
if (autoloading) {
caution("- Use `renv::restore()` to restore the project library.")
return(FALSE)
}

response <- ask("Would you like to restore the project library?", default = FALSE)
response <- ask("Would you like to run `renv::restore()` to restore the project library?", default = FALSE)
Copy link
Member Author

Choose a reason for hiding this comment

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

It feels like this could ease decision-making for the user, i.e. it's easier to answer this question confidently if you know exactly what's being proposed. Also might have some value in terms of user education.

if (!response)
return(FALSE)

Expand Down
4 changes: 0 additions & 4 deletions R/rstudio.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,6 @@ renv_rstudio_available <- function() {

}

renv_rstudio_autoloading <- function() {
renv_rstudio_available() && getOption("renv.autoloader.running", default = FALSE)
}

Comment on lines -13 to -16
Copy link
Member Author

Choose a reason for hiding this comment

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

I removed the sole usage of this function.

renv_rstudio_initialize <- function(project) {

tools <- catch(as.environment("tools:rstudio"))
Expand Down
19 changes: 11 additions & 8 deletions R/utils.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,13 +126,15 @@ ask <- function(question, default = FALSE) {
if (!interactive())
return(default)

# can't prompt for input when running autoloader in RStudio
# can't prompt for input when autoloading; code run from `.Rprofile` should
# not attempt to interact with the user
# from `?Startup`:
# "It is not intended that there be interaction with the user during startup
# code. Attempting to do so can crash the R process."
# https://github.com/rstudio/renv/issues/1879
if (renv_rstudio_available()) {
autoloading <- getOption("renv.autoloader.running", default = FALSE)
if (autoloading)
return(default)
}
autoloading <- getOption("renv.autoloader.running", default = FALSE)
if (autoloading)
return(default)
Comment on lines +135 to +137
Copy link
Member Author

Choose a reason for hiding this comment

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

More simplification now that we've decided to never attempt interaction when autoloading, regardless of the frontend.


# be verbose in this scope, as we're asking the user for input
renv_scope_options(renv.verbose = TRUE)
Expand Down Expand Up @@ -519,13 +521,14 @@ take <- function(data, index = NULL) {
if (is.null(index)) data else .subset2(data, index)
}

cancel <- function() {
cancel <- function(verbose = TRUE) {

renv_snapshot_auto_suppress_next()
if (testing())
stop("Operation canceled", call. = FALSE)

message("- Operation canceled.")
if (verbose)
message("- Operation canceled.")
invokeRestart("abort")

}
Expand Down
Loading