From 0c6adfec1cb14212c9bd27d1a695cc547a32341b Mon Sep 17 00:00:00 2001 From: Aron Atkins Date: Thu, 27 Jul 2023 08:53:01 -0400 Subject: [PATCH] prefer RSCONNECT_PACKRAT over rsconnect.packrat Environment variables are visible in the RStudio IDE R session used for push-button deployment. Not all R options are mirrored into that session. These settings indicate that packrat should be used, even in the presence of an renv.lock. When using packrat, the renv directory and renv.lock file are removed from the bundle directory, as would happen prior to rsconnect 1.0.0. Fixes #935 Fixes #936 --- NEWS.md | 8 +++ R/bundle.R | 10 +-- R/bundlePackage.R | 25 ++++++-- R/utils.R | 13 ++++ tests/testthat/_snaps/bundlePackage.md | 22 ++++++- tests/testthat/test-bundlePackage.R | 86 ++++++++++++++++++++++++-- tests/testthat/test-utils.R | 28 +++++++++ 7 files changed, 174 insertions(+), 18 deletions(-) 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..df534c6a 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. 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) { logger <- verboseLogger(verbose) logger("Creating tempfile for appdir") diff --git a/R/bundlePackage.R b/R/bundlePackage.R index cad17ed4..e6613ddd 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. + env_value <- Sys.getenv("RSCONNECT_PACKRAT", unset = NA) + if (is.na(env_value)) { + return(truthy(getOption("rsconnect.packrat", default = FALSE))) + } + + return(truthy(env_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..261a8cfe 100644 --- a/R/utils.R +++ b/R/utils.R @@ -161,3 +161,16 @@ toJSON <- function(x, ...) { ... ) } + +truthy <- function(value, default = FALSE) { + if (length(value) == 0) + default + else if (is.character(value)) + value %in% c("TRUE", "True", "true", "T", "1") + else if (is.symbol(value)) + as.character(value) %in% c("TRUE", "True", "true", "T", "1") + else if (is.na(value)) + default + 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)) +})