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

Jump to bottom

Default to the project library, if it exists #1586

Closed
wants to merge 4 commits into from
Closed

Default to the project library, if it exists #1586

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b96704c
Default to the project library, if it exists
hadley Jul 24, 2023
b4b302f
Merge commit 'acc968a179f293b7ba531c3c2a57e1cfc755dc4b'
hadley Jul 24, 2023
62a0be2
Merge commit 'c06b9bfb526d52bc8a34b507854fe3c421161cb3'
hadley Jul 25, 2023
601666c
Adjust test for presence of system library
hadley Jul 25, 2023
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
3 changes: 3 additions & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@

# renv 1.1.0 (UNRELEASED)

* If `library` is unsupplied, renv functions default to using the project
library, if `project` is supplied and the project library exists.
Comment on lines +4 to +5
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?


* Fixed a regression in parsing expressions within R Markdown chunk options. (#1558)

* Fixed an issue that prevented `renv::install()` from functioning
Expand Down
2 changes: 1 addition & 1 deletion R/install.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ install <- function(packages = NULL,
}

# set up library paths
libpaths <- renv_libpaths_resolve(library)
libpaths <- renv_libpaths_resolve(library, project)
renv_scope_libpaths(libpaths)

# check for explicitly-provided type -- we handle this specially for PPM
Expand Down
16 changes: 11 additions & 5 deletions R/libpaths.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,11 +191,17 @@ renv_libpaths_restore <- function() {
# 'renv::restore(library = <...>)')
#
# https://github.com/rstudio/renv/issues/1544
renv_libpaths_resolve <- function(library) {

if (is.null(library))
return(renv_libpaths_all())
renv_libpaths_resolve <- function(library = NULL, project = NULL) {
if (!is.null(library)) {
return(unique(c(library, .Library)))
}

unique(c(library, .Library))
if (!is.null(project)) {
proj_lib <- renv_paths_library(project = project)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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:

https://github.com/rstudio/renv/pull/1586/files#diff-76ebee0a54b80584fcf1eff0cdf55ef6f17e47a6d80cb807b5939e8e50cac32cR128

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.

Copy link
Collaborator

@kevinushey kevinushey Jul 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

if (dir.exists(proj_lib)) {
return(unique(c(proj_lib, .Library)))
}
}

renv_libpaths_all()
}
2 changes: 1 addition & 1 deletion R/rebuild.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ rebuild <- function(packages = NULL,
project <- renv_project_resolve(project)
renv_project_lock(project = project)

libpaths <- renv_libpaths_resolve(library)
libpaths <- renv_libpaths_resolve(library, project)
library <- nth(libpaths, 1L)

# get collection of packages currently installed
Expand Down
2 changes: 1 addition & 1 deletion R/restore.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ restore <- function(project = NULL,
renv_project_lock(project = project)

# resolve library, lockfile arguments
libpaths <- renv_libpaths_resolve(library)
libpaths <- renv_libpaths_resolve(library, project)
lockfile <- lockfile %||% renv_lockfile_load(project = project, strict = TRUE)

# check and ask user if they need to activate first
Expand Down
3 changes: 2 additions & 1 deletion R/roxygen.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@
#' @param lockfile Path to a lockfile. When `NULL` (the default), the `renv.lock` located in the root of the current project will be used.
#'
#' @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.
Comment on lines 11 to +13
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

#'
#' @param prompt Boolean; prompt the user before taking any action? For backwards
#' compatibility, `confirm` is accepted as an alias for `prompt`.
Expand Down
6 changes: 2 additions & 4 deletions R/snapshot.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,6 @@ the$auto_snapshot_hash <- TRUE
#'
#' @inherit renv-params
#'
#' @param library The \R libraries to snapshot. When `NULL`, the active \R
#' libraries (as reported by `.libPaths()`) are used.
#'
#' @param lockfile The location where the generated lockfile should be written.
#' By default, the lockfile is written to a file called `renv.lock` in the
#' project directory. When `NULL`, the lockfile (as an \R object) is returned
Expand Down Expand Up @@ -146,7 +143,8 @@ snapshot <- function(project = NULL,
if (!is.null(lockfile))
renv_activate_prompt("snapshot", library, prompt, project)

libpaths <- renv_path_normalize(library %||% renv_libpaths_all())
library <- renv_libpaths_resolve(library, project)
libpaths <- renv_path_normalize(library)
if (config$snapshot.validate())
renv_snapshot_preflight(project, libpaths)

Expand Down
5 changes: 1 addition & 4 deletions R/status.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,6 @@ the$status_running <- FALSE
#'
#' @inherit renv-params
#'
#' @param library The library paths. By default, the library paths associated
#' with the requested project are used.
#'
#' @param sources Boolean; check that each of the recorded packages have a
#' known installation source? If a package has an unknown source, renv
#' may be unable to restore it.
Expand Down Expand Up @@ -111,7 +108,7 @@ status <- function(project = NULL,
project <- renv_project_resolve(project)
renv_project_lock(project = project)

libpaths <- renv_libpaths_resolve(library)
libpaths <- renv_libpaths_resolve(library, project)
lockpath <- lockfile %||% renv_lockfile_path(project)

invisible(renv_status_impl(project, libpaths, lockpath, sources, cache))
Expand Down
2 changes: 1 addition & 1 deletion R/update.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,7 @@ update <- function(packages = NULL,
renv_project_lock(project = project)

# resolve library path
libpaths <- renv_libpaths_resolve(library)
libpaths <- renv_libpaths_resolve(library, project)
library <- nth(libpaths, 1L)
renv_scope_libpaths(libpaths)

Expand Down
4 changes: 2 additions & 2 deletions man/snapshot.Rd
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions man/status.Rd
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 14 additions & 0 deletions tests/testthat/test-libpaths.R
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
test_that("resolve library with multiple fallbacks", {

path <- tempfile()

expect_equal(renv_libpaths_resolve(), .libPaths())
expect_equal(renv_libpaths_resolve(path), c(path, .Library))

dir.create(renv_paths_library(project = path), recursive = TRUE)
expect_equal(
renv_libpaths_resolve(NULL, path),
c(renv_paths_library(project = path), .Library)
)

})
4 changes: 2 additions & 2 deletions tests/testthat/test-snapshot.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should change type above and just supply an explicit dependency list?

expect_contains(names(records), c("bread", "toast"))

.libPaths(oldlibpaths)

Expand Down
Loading