diff --git a/NEWS.md b/NEWS.md index c81ec11d..48fda94a 100644 --- a/NEWS.md +++ b/NEWS.md @@ -12,6 +12,14 @@ manifest created for an renv project references the `renv.lock` in the `manifest.json`. (#926) +* Use the environment variable `RSCONNECT_PACKRAT` to analyze dependencies + using packrat, as was done prior to rsconnect-1.0.0. Use of the + `rsconnect.packrat` option is discouraged, as it is not effective when using + push-button deployment in the RStudio IDE. (#935) + +* The `renv.lock` is ignored when the `RSCONNECT_PACKRAT` environment variable + or the `rsconnect.packrat` option is set. (#936) + # rsconnect 1.0.1 * `deployDoc()` includes `.Rprofile`, `requirements.txt` and `renv.lock` when diff --git a/R/bundle.R b/R/bundle.R index e170392e..b902a09b 100644 --- a/R/bundle.R +++ b/R/bundle.R @@ -1,9 +1,9 @@ # Given a path to an directory and a list of files in that directory, copies - # those files to a new temporary directory. Performes some small modifications - # in this process, including renaming single-file Shiny apps to "app.R" and - # stripping packrat and renv commands from .Rprofile. Returns the path to the - # temporary directory. - bundleAppDir <- function(appDir, appFiles, appPrimaryDoc = NULL, verbose = FALSE) { +# those files to a new temporary directory. Performs some small modifications +# in this process, including renaming single-file Shiny apps to "app.R" and +# stripping packrat and renv commands from .Rprofile. Returns the path to the +# temporary directory. +bundleAppDir <- function(appDir, appFiles, appPrimaryDoc = NULL, verbose = FALSE) { logger <- verboseLogger(verbose) logger("Creating tempfile for appdir") diff --git a/R/bundlePackage.R b/R/bundlePackage.R index cad17ed4..43857457 100644 --- a/R/bundlePackage.R +++ b/R/bundlePackage.R @@ -29,12 +29,30 @@ bundlePackages <- function(bundleDir, packages_list } +usePackrat <- function() { + # Use RSCONNECT_PACKRAT when it has any value; fall-back to rsconnect.packrat when the environment + # variable is unset. + value <- Sys.getenv("RSCONNECT_PACKRAT", unset = NA) + if (is.na(value)) { + value <- getOption("rsconnect.packrat", default = FALSE) + } + + return(truthy(value)) +} + computePackageDependencies <- function(bundleDir, extraPackages = character(), quiet = FALSE, verbose = FALSE) { - if (file.exists(renvLockFile(bundleDir))) { + if (usePackrat()) { + taskStart(quiet, "Capturing R dependencies with packrat") + # Remove renv.lock so the packrat call to renv::dependencies does not report an implicit renv + # dependency. Mirrors rsconnect before 1.0.0, which did not include renv.lock in bundles. + # https://github.com/rstudio/rsconnect/blob/v0.8.29/R/bundle.R#L96 + removeRenv(bundleDir) + deps <- snapshotPackratDependencies(bundleDir, extraPackages, verbose = verbose) + } else if (file.exists(renvLockFile(bundleDir))) { # This ignores extraPackages; if you're using a lockfile it's your # responsibility to install any other packages you need taskStart(quiet, "Capturing R dependencies from renv.lock") @@ -42,13 +60,10 @@ computePackageDependencies <- function(bundleDir, # Once we've captured the deps, we can remove the renv directory # from the bundle (retaining the renv.lock). removeRenv(bundleDir, lockfile = FALSE) - } else if (isFALSE(getOption("rsconnect.packrat", FALSE))) { + } else { taskStart(quiet, "Capturing R dependencies with renv") # TODO: give user option to choose between implicit and explicit deps <- snapshotRenvDependencies(bundleDir, extraPackages, verbose = verbose) - } else { - taskStart(quiet, "Capturing R dependencies with packrat") - deps <- snapshotPackratDependencies(bundleDir, extraPackages, verbose = verbose) } taskComplete(quiet, "Found {nrow(deps)} dependenc{?y/ies}") diff --git a/R/utils.R b/R/utils.R index 56b97f02..e5cdde30 100644 --- a/R/utils.R +++ b/R/utils.R @@ -161,3 +161,12 @@ toJSON <- function(x, ...) { ... ) } + +truthy <- function(value, default = FALSE) { + if (!is.atomic(value) || length(value) != 1 || is.na(value)) + default + else if (is.character(value)) + value %in% c("TRUE", "True", "true", "T", "1") + else + as.logical(value) +} diff --git a/tests/testthat/_snaps/bundlePackage.md b/tests/testthat/_snaps/bundlePackage.md index 7f852171..f383e09e 100644 --- a/tests/testthat/_snaps/bundlePackage.md +++ b/tests/testthat/_snaps/bundlePackage.md @@ -4,15 +4,23 @@ pkgs <- bundlePackages(app_dir) Message i Capturing R dependencies with renv - v Found 1 dependency + v Found 2 dependencies -# can snapshot deps with packrat +# can snapshot deps with packrat (option) Code pkgs <- bundlePackages(app_dir) Message i Capturing R dependencies with packrat - v Found 1 dependency + v Found 2 dependencies + +# can snapshot deps with packrat (env var) + + Code + pkgs <- bundlePackages(app_dir) + Message + i Capturing R dependencies with packrat + v Found 2 dependencies # can capture deps from renv lockfile @@ -22,6 +30,14 @@ i Capturing R dependencies from renv.lock v Found 3 dependencies +# can capture deps with packrat even when renv lockfile present + + Code + pkgs <- bundlePackages(app_dir) + Message + i Capturing R dependencies with packrat + v Found 2 dependencies + # error if can't find source Code diff --git a/tests/testthat/test-bundlePackage.R b/tests/testthat/test-bundlePackage.R index 21e00ea2..bf0520cb 100644 --- a/tests/testthat/test-bundlePackage.R +++ b/tests/testthat/test-bundlePackage.R @@ -1,18 +1,41 @@ test_that("can snapshot deps with renv", { - app_dir <- local_temp_app(list("foo.R" = "library(MASS)")) + app_dir <- local_temp_app(list(foo.R = "library(foreign); library(MASS)")) + expect_snapshot(pkgs <- bundlePackages(app_dir)) - expect_named(pkgs, "MASS") + + # renv is not a dependency + expect_named(pkgs, c("foreign", "MASS"), ignore.order = TRUE) + expect_named(pkgs$foreign, c("Source", "Repository", "description")) expect_named(pkgs$MASS, c("Source", "Repository", "description")) # No renv lockfile left behind expect_equal(list.files(app_dir), "foo.R") }) -test_that("can snapshot deps with packrat", { +test_that("can snapshot deps with packrat (option)", { withr::local_options(rsconnect.packrat = TRUE) - app_dir <- local_temp_app(list("foo.R" = "library(MASS)")) + + app_dir <- local_temp_app(list(foo.R = "library(foreign); library(MASS)")) + expect_snapshot(pkgs <- bundlePackages(app_dir)) - expect_named(pkgs, "MASS") + + expect_named(pkgs, c("foreign", "MASS"), ignore.order = TRUE) + expect_named(pkgs$foreign, c("Source", "Repository", "description")) + expect_named(pkgs$MASS, c("Source", "Repository", "description")) + + # No packrat lockfile left behind + expect_equal(list.files(app_dir), "foo.R") +}) + +test_that("can snapshot deps with packrat (env var)", { + withr::local_envvar(RSCONNECT_PACKRAT = "TRUE") + + app_dir <- local_temp_app(list(foo.R = "library(foreign); library(MASS)")) + + expect_snapshot(pkgs <- bundlePackages(app_dir)) + + expect_named(pkgs, c("foreign", "MASS"), ignore.order = TRUE) + expect_named(pkgs$foreign, c("Source", "Repository", "description")) expect_named(pkgs$MASS, c("Source", "Repository", "description")) # No packrat lockfile left behind @@ -24,7 +47,10 @@ test_that("can capture deps from renv lockfile", { app_dir <- local_temp_app(list(foo.R = "library(foreign); library(MASS)")) renv::snapshot(app_dir, prompt = FALSE) + expect_snapshot(pkgs <- bundlePackages(app_dir)) + + # renv is included by the renv.lock expect_named(pkgs, c("foreign", "MASS", "renv"), ignore.order = TRUE) expect_named(pkgs$foreign, c("Source", "Repository", "description")) expect_named(pkgs$MASS, c("Source", "Repository", "description")) @@ -33,6 +59,24 @@ test_that("can capture deps from renv lockfile", { expect_equal(list.files(app_dir), c("foo.R", "renv.lock")) }) +test_that("can capture deps with packrat even when renv lockfile present", { + withr::local_envvar(RSCONNECT_PACKRAT = "TRUE") + withr::local_options(renv.verbose = FALSE) + + app_dir <- local_temp_app(list(foo.R = "library(foreign); library(MASS)")) + renv::snapshot(app_dir, prompt = FALSE) + + expect_snapshot(pkgs <- bundlePackages(app_dir)) + + # renv is not a dependency (not using renv.lock) + expect_named(pkgs, c("foreign", "MASS"), ignore.order = TRUE) + expect_named(pkgs$foreign, c("Source", "Repository", "description")) + expect_named(pkgs$MASS, c("Source", "Repository", "description")) + + # The (incoming) renv.lock is discarded, mirroring rsconnect pre 1.0.0. + expect_equal(list.files(app_dir), "foo.R") +}) + # ------------------------------------------------------------------------- test_that("error if can't find source", { @@ -57,3 +101,35 @@ test_that("error if can't find source", { error = TRUE ) }) + +test_that("usePackrat respects the environment variable", { + expect_false(usePackrat()) + + withr::local_envvar(RSCONNECT_PACKRAT = "TRUE") + expect_true(usePackrat()) + + withr::local_envvar(RSCONNECT_PACKRAT = "FALSE") + expect_false(usePackrat()) +}) + +test_that("usePackrat respects the option", { + expect_false(usePackrat()) + + withr::local_options(rsconnect.packrat = TRUE) + expect_true(usePackrat()) + + withr::local_options(rsconnect.packrat = FALSE) + expect_false(usePackrat()) +}) + +test_that("usePackrat prefers the environment variable over the option", { + expect_false(usePackrat()) + + withr::local_envvar(RSCONNECT_PACKRAT = "TRUE") + withr::local_options(rsconnect.packrat = FALSE) + expect_true(usePackrat()) + + withr::local_envvar(RSCONNECT_PACKRAT = "FALSE") + withr::local_options(rsconnect.packrat = TRUE) + expect_false(usePackrat()) +}) diff --git a/tests/testthat/test-utils.R b/tests/testthat/test-utils.R index ecd276a4..5ed9da8e 100644 --- a/tests/testthat/test-utils.R +++ b/tests/testthat/test-utils.R @@ -78,3 +78,31 @@ test_that("we can hash a file with well known contents", { expect_equal(fileMD5(path), "52d2daa95d288f3c01e4d4d87f85727e") }) + +test_that("truthy is truthy", { + # fallback-to-default checks + expect_false(truthy(c())) + expect_true(truthy(c(), default = TRUE)) + expect_false(truthy(NA)) + expect_true(truthy(NA, default = TRUE)) + + # true value checks + expect_true(truthy(TRUE, default = FALSE)) + expect_true(truthy("TRUE", default = FALSE)) + expect_true(truthy("True", default = FALSE)) + expect_true(truthy("true", default = FALSE)) + expect_true(truthy("T", default = FALSE)) + expect_true(truthy("1", default = FALSE)) + expect_true(truthy(1, default = FALSE)) + expect_true(truthy(42, default = FALSE)) + + # false value checks + expect_false(truthy(FALSE, default = TRUE)) + expect_false(truthy("FALSE", default = TRUE)) + expect_false(truthy("False", default = TRUE)) + expect_false(truthy("false", default = TRUE)) + expect_false(truthy("F", default = TRUE)) + expect_false(truthy("0", default = TRUE)) + expect_false(truthy(0, default = TRUE)) + expect_false(truthy("nonsense", default = TRUE)) +})