Respect renv lockfiles #791

hadley · 2023-03-21T22:07:25Z

@aronatkins and @kevinushey: this is still an early draft, but I thought it would be useful to get some feedback now that I've put a couple of hours into it. It still needs a bunch of testing, and I doubt i have all the details of the renv -> packrat translation are fully correct yet, but this overall this feels like a solid approach to me. What do you think?

Fixes #671. Fixes #809.

R/bundleFiles.R

NEWS.md

R/bundleFiles.R

R/renv.R

aronatkins

Could we renv->manifest rather than renv->packrat->manifest?

NEWS.md

R/renv.R

Consistent name for renv file

hadley · 2023-04-22T13:33:17Z

I'm still fighting some buglet with BioC packages. In a project that uses Biobase, I get the following error:

# Installing required R packages with `packrat::restore()` ---------------------
Installing BiocGenerics (0.44.0) ... 
Using cached BiocGenerics.
	OK (symlinked cache)
Installing BiocManager (1.30.18) ... 
Using cached BiocManager.
	OK (symlinked cache)
Installing BiocVersion (3.16.0) ... 
curl: HTTP 200 https://bioconductor.org/packages/3.16/data/annotation/src/contrib/PACKAGES.rds
curl: HTTP 200 https://bioconductor.org/packages/3.16/data/experiment/src/contrib/PACKAGES.rds
curl: HTTP 200 https://bioconductor.org/packages/3.16/bioc/src/contrib/PACKAGES.rds
curl: HTTP 200 https://bioconductor.org/packages/3.16/workflows/src/contrib/PACKAGES.rds
curl: HTTP 200 https://colorado.posit.co/rspm/all/latest/src/contrib/PACKAGES.rds
curl: HTTP 404 https://bioconductor.org/packages/3.16/data/annotation/src/contrib/Archive/BiocVersion/BiocVersion_3.16.0.tar.gz
curl: (22) The requested URL returned error: 404 Not Found
curl: HTTP 404 https://bioconductor.org/packages/3.16/data/annotation/src/contrib/Archive/BiocVersion/BiocVersion_3.16.0.tar.gz

The manifest looks ok to me:

    "BiocVersion": {
      "Package": "BiocVersion",
      "Version": "3.16.0",
      "Source": "Bioconductor",
      "Repository": "https://bioconductor.org/packages/3.16/bioc",

I suspect there's some small mistake that will hopefully be obvious when I look at it on Monday.

R/bundlePackageRenv.R

R/writeManifest.R

hadley · 2023-05-02T13:44:48Z

BioC install problem was caused by colorado only having 4.1 installed. Works on rstudioservices.

toph-allen

In general, I think this change makes sense, especially because packrat is itself using a vendored version of renv to snapshot dependencies.

It seems like a lot of the complexity here is within the blocks that standardize the different ways packrat, renv, and pak record package metadata, as well as assumptions they make about packages that aren't just structural differences (and how these have evolved over time). I appreciate having links to the relevant source in the comments. (The one that stood out to me was the check for isDevVersion()).

The Bitbucket name capitalization is the only required change I noticed. But I only noticed it because I spent long enough with Bitbucket when I was implementing support for restoring those packages in Connect. We'll probably want to do some real-world tire-kicking of those sorts of scenarios in case there are other small things like that we missed. Either that or build out more examples in tests.

R/bundlePackageRenv.R

tests/testthat/test-bundlePackageRenv.R

R/bundlePackagePackrat.R

aronatkins

Some questions about package+repository handling, but this is probably far enough along to merge and get folks to start actively using it.

aronatkins · 2023-05-02T18:42:30Z

R/appDependencies.R

+#' 2. Otherwise, rsconnect will call `renv::snapshot()` to find all packages
+#'    used by your code. If you'd instead prefer to only use the packages
+#'    declared in a `DESCRIPTION` file, run
+#'    `renv::settings$snapshot.type("explicit")` to activate renv's "explicit"


Do you think that folks will need an explicit snapshot type for deploys and another snapshot type for all other workflows? Put another way: Do we need a way to toggle the snapshot type just for deploys?

renv::snapshot() does accept a type argument so this would be straightforward to wire up, at least.

My sense is that once we enable support for renv lockfile, we'll get quite a few feature requests so we might as well just let them accumulate for a while to get a sense for what people actually want, rather than doing too much prospectively.

R/appDependencies.R

aronatkins · 2023-05-02T18:45:47Z

R/bundleFiles.R

@@ -179,7 +179,7 @@ ignoreBundleFiles <- function(dir, contents) {
    # rsconnect packages
    "rsconnect", "rsconnect-python", "manifest.json",
    # packrat + renv,
-    "renv", "renv.lock", "packrat",
+    "renv", "packrat",


The renv/settings.json isn't included; does this mean that a snapshot.type set in a previous session will not be available?

Yes, but I don't think that should matter because the settings only really affect interactive usage, and the consequence of those settings will be encoded in the lockfile.

The recommendation to use renv::settings$snapshot.type("explicit") presents a challenge, then, if we expect folks to set that ahead of each deploy operation but do so in a way that does not modify their existing renv project settings. What is your suggested workflow?

aronatkins · 2023-05-02T18:46:28Z

R/bundlePackage.R

-  packages_list <- lapply(seq_len(nrow(packages)), function(i) {
-    out <- as.list(packages[i, , drop = FALSE])
+  # Manifest packages used to generate packrat file on Connect
+  # https://github.com/rstudio/connect/blob/v2023.03.0/src/connect/manifest/convert.go#L261-L320


nit: This is an internal source reference.

aronatkins · 2023-05-02T18:56:15Z

R/bundleFiles.R

@@ -179,7 +179,7 @@ ignoreBundleFiles <- function(dir, contents) {
    # rsconnect packages
    "rsconnect", "rsconnect-python", "manifest.json",
    # packrat + renv,
-    "renv", "renv.lock", "packrat",
+    "renv", "packrat",


Does this cause renv.lock to be included in the bundle?

Yes; that's necessary so that we can process it in bundlePackages(). Do you foresee a problem with including this in the deployed bundle? If so, I can delete. (But I think you could also renv::restore() directly from that file without having to convert the manifest back into a packrat spec).

I am torn on this one.

On the one hand, including only the manifest.json in the bundle is quite nice because it means we have a single source-of-truth. Folks have been confused by the presence of both manifest.json and packrat.lock in our bundles, as well as the relationship between the two (and duplication of information).

On the other hand, I like including the renv.lock, as it gives us a future option of directly using that file without reconstructing it from the manifest.

Mostly, my struggle is that I had thought about this change as "manifest-only, no packrat.lock" rather than "manifest plus renv.lock; except when we create the renv.lock on the fly".

Let's go forward with the (sometimes) inclusion, but probably document the change.

It feels like it would be a bit more consistent to include lock file, even when we generate it? I'm currently explicitly deleting it, but it seems reasonable to remove that code and preserve the lock file.

A generated lock file isn't on the set of "bundle files" that was computed much earlier in the workflow... I'm OK either way, but if we do include a generated lock file, let's include it only when we have tracked dependencies (e.g. not for static).

aronatkins · 2023-05-02T19:22:06Z

R/bundlePackageRenv.R

+    } else {
+      # $Repository comes from DESCRIPTION and is set by repo, so can be
+      # anything. So we must look up from the package name
+      pkg$Repository <- findRepoUrl(pkg$Package, availablePackages)


nit: during restore, renv first favors a matching repository name before falling back to "any" repository. not sure if we want that same pattern here or want to rely available.packages order like we do for Packrat.

Maybe this is: prefer the Repository named in renv.lock and prefer available.packages for snapshots.

Also worth considering: Should we prefer the renv.lock repository list or the list in the current R session?

https://github.com/rstudio/renv/blob/81fecb07fcc78cabb61b5309875e59484727b090/R/retrieve.R#L875-L880

kevinushey

Overall LGTM!

kevinushey · 2023-05-02T20:24:53Z

DESCRIPTION

@@ -28,13 +28,15 @@ Imports:
    lifecycle,
    openssl (>= 2.0.0),
    packrat (>= 0.6),
+    renv,


Not sure if you need it for this PR, but if you'd like to depend on renv internals without risking breakage from new renv releases, you can also use renv::embed() to embed a local copy of renv here.

Related thought: Do we need a version constraint on renv?

We don't need to embed because we're only using exported functions so far, but will keep it in mind for the future.

I've add a requirement for the latest version from CRAN.

kevinushey · 2023-05-02T20:26:27Z

R/appDependencies.R

+#' 2. Otherwise, rsconnect will call `renv::snapshot()` to find all packages
+#'    used by your code. If you'd instead prefer to only use the packages
+#'    declared in a `DESCRIPTION` file, run
+#'    `renv::settings$snapshot.type("explicit")` to activate renv's "explicit"


renv::snapshot() does accept a type argument so this would be straightforward to wire up, at least.

R/bundlePackagePackrat.R

kevinushey · 2023-05-02T20:42:57Z

R/bundlePackagePackrat.R

+  local_version <- record$Version
+  repo_version <- availablePackages[record$Package, "Version"]
+
+  package_version(local_version) > package_version(repo_version)


Should we also detect whether the trailing version component is "large"? In case a local version is 1.0.0-9000 but CRAN has 1.0.1?

That's not a version used by every package, so I worry that would dilute the impact of this change.

R/bundlePackageRenv.R

R/utils.R

Co-authored-by: Toph Allen <[email protected]>

Co-authored-by: Kevin Ushey <[email protected]>

Co-authored-by: Toph Allen <[email protected]>

#Conflicts: # NEWS.md

hadley · 2023-05-03T15:11:53Z

I think this is done, and I've finally managed to knock of the CI failure. Unless anyone spots any problems, I'll aim to merge tomorrow.

toph-allen

Looked through the changes since my last review, and they all look good.

hadley added 5 commits March 20, 2023 17:36

Sketch out main changes

462da70

Add basic translation code

8a928ea

Add renv example

3796044

Improve testing infrastructure

0928e7a

Add news bullet

5dbb07c

hadley commented Mar 21, 2023

View reviewed changes

R/bundleFiles.R Show resolved Hide resolved

More comments

24fe5ab

kevinushey reviewed Mar 22, 2023

View reviewed changes

NEWS.md Outdated Show resolved Hide resolved

R/bundleFiles.R Show resolved Hide resolved

R/renv.R Outdated Show resolved Hide resolved

R/renv.R Outdated Show resolved Hide resolved

R/renv.R Outdated Show resolved Hide resolved

kevinushey reviewed Mar 22, 2023

View reviewed changes

R/renv.R Outdated Show resolved Hide resolved

aronatkins reviewed Mar 22, 2023

View reviewed changes

NEWS.md Outdated Show resolved Hide resolved

R/renv.R Outdated Show resolved Hide resolved

hadley added 20 commits March 23, 2023 09:30

Simplify showDcf()

024be49

Restore packrat/renv directory ignoring

8c7a828

Don't need to support packrat lockfiles

93e6522

Hack into new shape

46d3c05

Standardise repos like packrat snapshot

9fe6c68

Delete lockfile when we're done with it

88aeced

Improve message

adb2b84

Use renv for snapshotting too

5dfcb80

Clean up tests

e4fcac7

Move packrat functions to own file

3a60c58

Consistent name for renv file

Update writeManifest() docs

5b8f9b5

Move tests around

ef4e237

Massiaing renv tests

fedb322

Trick R CMD check

b93ae24

Update snapshot

3c0f2a6

More info about manifest fields

52d5fd9

Complete tests of standardizeRenvPackage()

1386887

Move empty dir test to individual impls

8129386

Test bundlePackage() paths

c29339b

Eliminate unused code

f7d42fd

hadley added 3 commits April 21, 2023 07:29

Add manual test scripts

c6a88cd

Proof reading

bfaa7ea

Lazily get default biocRepos, if needed

b386d8a

hadley requested review from aronatkins and kevinushey April 22, 2023 13:31

hadley commented Apr 22, 2023

View reviewed changes

R/bundlePackageRenv.R Show resolved Hide resolved

jthomasmock reviewed Apr 23, 2023

View reviewed changes

R/writeManifest.R Outdated Show resolved Hide resolved

Silence renv output

1c6cef8

aronatkins mentioned this pull request May 2, 2023

packrat gives very little information when a package is not compatible with R rstudio/packrat#302

Open

toph-allen requested changes May 2, 2023

View reviewed changes

R/bundlePackageRenv.R Outdated Show resolved Hide resolved

tests/testthat/test-bundlePackageRenv.R Outdated Show resolved Hide resolved

R/bundlePackagePackrat.R Outdated Show resolved Hide resolved

aronatkins approved these changes May 2, 2023

View reviewed changes

kevinushey approved these changes May 2, 2023

View reviewed changes

hadley and others added 11 commits May 3, 2023 08:25

Update R/bundlePackagePackrat.R

2d4c4d3

Co-authored-by: Toph Allen <[email protected]>

Apply suggestions from code review

8fe29c6

Co-authored-by: Kevin Ushey <[email protected]>

Update tests/testthat/test-bundlePackageRenv.R

957c1a5

Co-authored-by: Toph Allen <[email protected]>

Update R/bundlePackageRenv.R

5198d45

Co-authored-by: Toph Allen <[email protected]>

Set required versions

36d0d50

Match on Packages field rather than rownames

af72cd0

Fix misunderstood fix

d7db6dd

Finish off rowname -> Package col changes

f53703d

Merge commit '25b93da28be464a58aaf3f3ac36d6ca50ece67c9'

3ec221a

#Conflicts: # NEWS.md

Switch to package that's not under active development

48ecfa5

And another test case

6e7a8b2

toph-allen approved these changes May 4, 2023

View reviewed changes

Merge branch 'main' into renv-support

752857f

hadley merged commit 003fdae into main May 8, 2023

hadley deleted the renv-support branch May 8, 2023 12:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respect renv lockfiles #791

Respect renv lockfiles #791

hadley commented Mar 21, 2023 •

edited

Loading

aronatkins left a comment

hadley commented Apr 22, 2023

hadley commented May 2, 2023

toph-allen left a comment

aronatkins left a comment

aronatkins May 2, 2023

kevinushey May 2, 2023

hadley May 3, 2023

aronatkins May 2, 2023

hadley May 3, 2023

aronatkins May 3, 2023

aronatkins May 2, 2023

aronatkins May 2, 2023

hadley May 3, 2023

aronatkins May 3, 2023

hadley May 3, 2023

aronatkins May 3, 2023

aronatkins May 2, 2023

kevinushey left a comment

kevinushey May 2, 2023

aronatkins May 2, 2023

hadley May 3, 2023

kevinushey May 2, 2023

kevinushey May 2, 2023

hadley May 3, 2023

hadley commented May 3, 2023

toph-allen left a comment

Respect renv lockfiles #791

Respect renv lockfiles #791

Conversation

hadley commented Mar 21, 2023 • edited Loading

aronatkins left a comment

Choose a reason for hiding this comment

hadley commented Apr 22, 2023

hadley commented May 2, 2023

toph-allen left a comment

Choose a reason for hiding this comment

aronatkins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinushey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hadley commented May 3, 2023

toph-allen left a comment

Choose a reason for hiding this comment

hadley commented Mar 21, 2023 •

edited

Loading