Default to the project library, if it exists #1586
Conversation
| @@ -63,8 +63,8 @@ test_that("multiple libraries can be used when snapshotting", { | |||
| lockfile <- snapshot(lockfile = NULL, library = libs, type = "all") | |||
| records <- renv_lockfile_records(lockfile) | |||
|
|
|||
| expect_length(records, 2L) | |||
| expect_setequal(names(records), c("bread", "toast")) | |||
| expect_length(records, 17) # 2 + 15 base/recommended packages | |||
There was a problem hiding this comment.
Maybe we should change type above and just supply an explicit dependency list?
| * If `library` is unsupplied, renv functions default to using the project | ||
| library, if `project` is supplied and the project library exists. |
There was a problem hiding this comment.
This reads pretty awkwardly to me. Can we give a more concrete example of the consequences of this change, or where users might see a change in behavior?
| #' @param library The \R library to be used. When `NULL`, the active project | ||
| #' library will be used instead. | ||
| #' library will be used instead. If no project is active, will fall back | ||
| #' to the system library. |
There was a problem hiding this comment.
I wonder if this could just be something like "When NULL, the current library paths will be used."? IIRC almost all places in renv just use the current library paths, with the assumption that load() would have set up the default library paths.
|
|
||
| unique(c(library, .Library)) | ||
| if (!is.null(project)) { | ||
| proj_lib <- renv_paths_library(project = project) |
There was a problem hiding this comment.
I'm not sure if this is the desired behavior. Suppose I do something like:
setwd("~/test/project")
renv::install("rlang")
And ~/test/project does happen to have an renv library, but I haven't actually activated or loaded that project. IMHO, we should install into the current library paths, not the renv project library. That is, I think loading a project explicitly should be a pre-requisite to having renv::install() use the project library.
There was a problem hiding this comment.
But this PR doesn't change that code — this only affects that behaviour when you explicitly supply a project. In that case, I think it's reasonable to say that you want to operate on that project, regardless of what project is currently loaded in the current session (if any).
i.e. if you do renv::install("rlang", project = "another-project") don't you want to install into use another-projects library?
But maybe only installing to that project's library if it exists is wrong — if you say you want to install into a given project, and that project doesn't have a library, it seems like it would be better to error?
There was a problem hiding this comment.
I think my concern is that project is usually going to be non-NULL internally, since we fall back to the current working directory when no project is active. For example:
> renv:::renv_project_resolve(NULL)
[1] "/Users/kevin"
And we use that pretty commonly when resolving the "active" project, e.g. we do that here:
I think my fears would be abated if we distinguished between the case where project was explicitly supplied by the user, versus we just used the default / current value.
There was a problem hiding this comment.
Could we add some tests (with multiple renv projects) just to confirm the behavior we want here as well? I guess the matrix here is (project A x project B) / (loaded x unloaded) / (explicit project / implicit project).
There was a problem hiding this comment.
Ah got it; so we generally need to do the library resolution before the project resolution. Let me see if it's possible to do this in a reliable way, and if not, I can give up on this PR.
No description provided.