From 02cada1d6def246346a5c8edc45acf7df213e426 Mon Sep 17 00:00:00 2001
From: Hadley Wickham <hadley@posit.co>
Date: Tue, 25 Jul 2023 16:53:47 -0500
Subject: [PATCH] New strategy to retrieve DESCRIPTION if project uses renv
 (#931)

Previously we'd always restore the packages from the lockfile, which I had expected to be fast since I had expected the packages to be in the cache. However, this doesn't appear to always be the case, so I instead use a simpler approach where I just force the user to resolve the problem. This means that we can always use the `DESCRIPTION` present in the `.libPaths()` making the code much simpler.

Fixes #930
---
 NEWS.md                                    |  5 ++++
 R/bundlePackageRenv.R                      | 32 +++++++++-------------
 tests/testthat/_snaps/bundlePackageRenv.md | 10 +++++++
 tests/testthat/test-bundlePackageRenv.R    | 11 ++++++++
 4 files changed, 39 insertions(+), 19 deletions(-)
 create mode 100644 tests/testthat/_snaps/bundlePackageRenv.md

diff --git a/NEWS.md b/NEWS.md
index 16804010..42a12afa 100644
--- a/NEWS.md
+++ b/NEWS.md
@@ -1,5 +1,10 @@
 # rsconnect (development version)
 
+* `deployApp()` and `writeManifest()` now error if your library and `renv.lock`
+  are out-of-sync. Previously it always used what was defined in the `renv.lock`
+  but that was (a) slow and (b) could lead to different results than what you
+  see when running locally (#930).
+
 * Deploying from an renv project includes the `renv.lock` in the bundle. A
   manifest created for an renv project references the `renv.lock` in the
   `manifest.json`. (#926)
diff --git a/R/bundlePackageRenv.R b/R/bundlePackageRenv.R
index 85b793a9..d79cca2e 100644
--- a/R/bundlePackageRenv.R
+++ b/R/bundlePackageRenv.R
@@ -34,24 +34,18 @@ parseRenvDependencies <- function(bundleDir, snapshot = FALSE) {
     return(data.frame())
   }
 
-  if (snapshot) {
-    # Can use system libraries
-    deps$description <- lapply(deps$Package, package_record)
-  } else {
-    old <- options(renv.verbose = FALSE)
-    defer(options(old))
-
-    # Generate a library from the lockfile
-    lib_dir <- dirCreate(file.path(bundleDir, "renv_library"))
-    defer(unlink(lib_dir, recursive = TRUE))
-    renv::restore(bundleDir, library = lib_dir, prompt = FALSE)
-
-    deps$description <- lapply(
-      deps$Package,
-      package_record,
-      # Ensure we fall back to system libraries
-      lib_dir = c(lib_dir, .libPaths())
-    )
+  deps$description <- lapply(deps$Package, package_record)
+
+  if (!snapshot) {
+    lib_versions <- unlist(lapply(deps$description, "[[", "Version"))
+
+    if (any(deps$Version != lib_versions)) {
+      cli::cli_abort(c(
+        "Library and lockfile are out of sync",
+        i = "Use renv::restore() or renv::snapshot() to synchronise",
+        i = "Or ignore the lockfile by adding to you .rscignore"
+      ))
+    }
   }
 
   deps
@@ -89,7 +83,7 @@ standardizeRenvPackage <- function(pkg,
   }
 
   if (pkg$Source == "Repository") {
-    if (pkg$Repository == "CRAN") {
+    if (identical(pkg$Repository, "CRAN")) {
       if (isDevVersion(pkg, availablePackages)) {
         pkg$Source <- NA_character_
         pkg$Repository <- NA_character_
diff --git a/tests/testthat/_snaps/bundlePackageRenv.md b/tests/testthat/_snaps/bundlePackageRenv.md
new file mode 100644
index 00000000..e2c5f255
--- /dev/null
+++ b/tests/testthat/_snaps/bundlePackageRenv.md
@@ -0,0 +1,10 @@
+# errors if library and project are inconsistent
+
+    Code
+      parseRenvDependencies(app_dir)
+    Condition
+      Error in `parseRenvDependencies()`:
+      ! Library and lockfile are out of sync
+      i Use renv::restore() or renv::snapshot() to synchronise
+      i Or ignore the lockfile by adding to you .rscignore
+
diff --git a/tests/testthat/test-bundlePackageRenv.R b/tests/testthat/test-bundlePackageRenv.R
index 7b235351..988e9fc7 100644
--- a/tests/testthat/test-bundlePackageRenv.R
+++ b/tests/testthat/test-bundlePackageRenv.R
@@ -72,6 +72,17 @@ test_that("gets DESCRIPTION from renv & system libraries", {
   expect_type(deps$description[[which(deps$Package == "MASS")]], "list")
 })
 
+
+test_that("errors if library and project are inconsistent", {
+  withr::local_options(renv.verbose = FALSE)
+
+  app_dir <- local_temp_app(list("foo.R" = "library(foreign); library(MASS)"))
+  renv::snapshot(app_dir, prompt = FALSE)
+  renv::record("MASS@0.1.1", project = app_dir)
+
+  expect_snapshot(parseRenvDependencies(app_dir), error = TRUE)
+})
+
 # standardizeRenvPackage -----------------------------------------
 
 test_that("SCM get names translated", {