prefer RSCONNECT_PACKRAT over rsconnect.packrat (#937)

* 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 * spelling * trailing whitespace lint * feedback
rstudio · Jul 28, 2023 · 2f07fde · 2f07fde
1 parent 66053e8
commit 2f07fde
Show file tree

Hide file tree

Showing 7 changed files with 170 additions and 18 deletions.
diff --git 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
@@ -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
@@ -29,26 +29,41 @@ 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")
     deps <- parseRenvDependencies(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
@@ -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
@@ -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
@@ -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
@@ -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))
+})