Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Respect renv lockfiles #791

Merged
merged 57 commits into from
May 8, 2023
Merged
Show file tree
Hide file tree
Changes from 56 commits
Commits
Show all changes
57 commits
Select commit Hold shift + click to select a range
462da70
Sketch out main changes
hadley Mar 20, 2023
8a928ea
Add basic translation code
hadley Mar 21, 2023
3796044
Add renv example
hadley Mar 21, 2023
0928e7a
Improve testing infrastructure
hadley Mar 21, 2023
5dbb07c
Add news bullet
hadley Mar 21, 2023
24fe5ab
More comments
hadley Mar 21, 2023
024be49
Simplify showDcf()
hadley Mar 23, 2023
8c7a828
Restore packrat/renv directory ignoring
hadley Mar 23, 2023
93e6522
Don't need to support packrat lockfiles
hadley Mar 23, 2023
46d3c05
Hack into new shape
hadley Mar 23, 2023
9fe6c68
Standardise repos like packrat snapshot
hadley Mar 23, 2023
88aeced
Delete lockfile when we're done with it
hadley Mar 23, 2023
adb2b84
Improve message
hadley Mar 23, 2023
5dfcb80
Use renv for snapshotting too
hadley Mar 23, 2023
e4fcac7
Clean up tests
hadley Mar 23, 2023
3a60c58
Move packrat functions to own file
hadley Mar 23, 2023
5b8f9b5
Update writeManifest() docs
hadley Mar 23, 2023
ef4e237
Move tests around
hadley Mar 23, 2023
fedb322
Massiaing renv tests
hadley Mar 23, 2023
b93ae24
Trick R CMD check
hadley Mar 23, 2023
3c0f2a6
Update snapshot
hadley Mar 23, 2023
52d5fd9
More info about manifest fields
hadley Mar 23, 2023
1386887
Complete tests of standardizeRenvPackage()
hadley Mar 24, 2023
8129386
Move empty dir test to individual impls
hadley Mar 24, 2023
c29339b
Test bundlePackage() paths
hadley Mar 24, 2023
f7d42fd
Eliminate unused code
hadley Mar 24, 2023
de2efd6
Document rsconnect.packrat option
hadley Mar 24, 2023
d83f36e
Start thinking about DESCRIPTIONs
hadley Mar 24, 2023
156e637
Correct strategy for getting renv library
hadley Mar 24, 2023
f8a7d71
Merge commit '516fb9cad24568ea502af784dad0e670f1f2f78b'
hadley Mar 24, 2023
f868fe3
Remove now unneeded helper
hadley Mar 24, 2023
91c8a24
Fix R CMD check failure
hadley Mar 24, 2023
0f77dd2
Restore original order
hadley Mar 24, 2023
4bee9a5
Linting
hadley Mar 24, 2023
f014634
Merge commit '72bcf625ca0805629838a41755b7fe5bfd8d12e7'
hadley Apr 20, 2023
b667f1f
Compute source/repository in correct order
hadley Apr 20, 2023
3cbe9e0
Repository can be NULL
hadley Apr 20, 2023
ae60d88
Work around renv/pak buglet
hadley Apr 20, 2023
e81c51e
Match existing manifest.json structure
hadley Apr 20, 2023
48c4e6a
Polishing; adding more docs
hadley Apr 20, 2023
08fff5d
Apply same fix to lockfile path
hadley Apr 21, 2023
c6a88cd
Add manual test scripts
hadley Apr 21, 2023
bfaa7ea
Proof reading
hadley Apr 21, 2023
b386d8a
Lazily get default biocRepos, if needed
hadley Apr 22, 2023
1c6cef8
Silence renv output
hadley May 2, 2023
2d4c4d3
Update R/bundlePackagePackrat.R
hadley May 3, 2023
8fe29c6
Apply suggestions from code review
hadley May 3, 2023
957c1a5
Update tests/testthat/test-bundlePackageRenv.R
hadley May 3, 2023
5198d45
Update R/bundlePackageRenv.R
hadley May 3, 2023
36d0d50
Set required versions
hadley May 3, 2023
af72cd0
Match on Packages field rather than rownames
hadley May 3, 2023
d7db6dd
Fix misunderstood fix
hadley May 3, 2023
f53703d
Finish off rowname -> Package col changes
hadley May 3, 2023
3ec221a
Merge commit '25b93da28be464a58aaf3f3ac36d6ca50ece67c9'
hadley May 3, 2023
48ecfa5
Switch to package that's not under active development
hadley May 3, 2023
6e7a8b2
And another test case
hadley May 3, 2023
752857f
Merge branch 'main' into renv-support
hadley May 8, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion DESCRIPTION
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ License: GPL-2
URL: https://github.com/rstudio/rsconnect
BugReports: https://github.com/rstudio/rsconnect/issues
Depends:
R (>= 3.0.0)
R (>= 3.5.0)
Imports:
cli,
curl,
Expand All @@ -28,13 +28,16 @@ Imports:
lifecycle,
openssl (>= 2.0.0),
packrat (>= 0.6),
renv (>= 0.17.0),
rlang (>= 1.0.0),
rstudioapi (>= 0.5),
tools,
yaml (>= 2.1.5)
Suggests:
knitr,
Biobase,
BiocManager,
foreign,
MASS,
mockr,
plumber (>= 0.3.2),
Expand Down
10 changes: 10 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,15 @@
# rsconnect (development version)

* `deployApp()` and `writeManifest()` now respect renv lock files, if present.
If you don't want to use these lockfiles, and instead return the previous
behaviour of snapshotting on every deploy, add your `renv.lock` to
`.rscignore` (#671). Learn more `?appDependencies()`.

* `deployApp()` and `writeManifest()` now use renv to capture app dependencies,
rather than packrat. If this causes a previously working deploy to fail,
please file an issue then set `options(rsconnect.packrat = TRUE)` to revert
to the previous behaviour.

* `deployApp()`'s `quarto` argument now takes values `TRUE`, `FALSE` or
`NA`. The previous value (a path to a quarto binary) is now deprecated,
and instead we automatically figure out the packge from `QUARTO_PATH` and
Expand Down
23 changes: 22 additions & 1 deletion R/appDependencies.R
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,27 @@
#' and adds all recursive dependencies to create a complete manifest of
#' package packages need to be installed to run the app.
#'
#' # Dependency discovery
#'
#' rsconnect use one of three mechanisms to find which packages your application
#' uses:
#'
#' 1. If `renv.lock` is present, it will use the versions and sources defined in
#' that file. If you're using the lockfile for some other purpose and
#' don't want it to affect deployment, add `renv.lock` to `.rscignore`.
#'
#' 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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#' mode.
#'
#' 3. Dependency resolution using renv is a new feature in rsconnect 1.0.0, and
hadley marked this conversation as resolved.
Show resolved Hide resolved
#' while we have done our best to test it, it still might fail for your app.
#' If this happens, please [file an issue](http://github.com/rstudio/rsconnect/issues)
#' then set `options(rsconnect.packrat = TRUE)` to revert to the old
#' dependency discovery mechanism.
#'
#' # Remote installation
#'
#' When deployed, the app must first install all of these packages, and
Expand Down Expand Up @@ -105,7 +126,7 @@ appDependencies <- function(appDir = getwd(), appFiles = NULL) {
on.exit(unlink(bundleDir, recursive = TRUE), add = TRUE)

extraPackages <- inferRPackageDependencies(appMetadata)
deps <- snapshotRDependencies(bundleDir, extraPackages)
deps <- computePackageDependencies(bundleDir, extraPackages, quiet = TRUE)
deps[c("Package", "Version", "Source", "Repository")]
}

Expand Down
6 changes: 4 additions & 2 deletions R/bundle.R
Original file line number Diff line number Diff line change
Expand Up @@ -112,15 +112,17 @@ createAppManifest <- function(appDir,
retainPackratDirectory = TRUE,
isCloudServer = FALSE,
image = NULL,
verbose = FALSE) {
verbose = FALSE,
quiet = FALSE) {

if (needsR(appMetadata)) {
extraPackages <- inferRPackageDependencies(appMetadata)
# provide package entries for all dependencies
packages <- bundlePackages(
bundleDir = appDir,
extraPackages = extraPackages,
verbose = verbose
verbose = verbose,
quiet = quiet
)
} else {
packages <- list()
Expand Down
2 changes: 1 addition & 1 deletion R/bundleFiles.R
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ ignoreBundleFiles <- function(dir, contents) {
# rsconnect packages
"rsconnect", "rsconnect-python", "manifest.json",
# packrat + renv,
"renv", "renv.lock", "packrat",
hadley marked this conversation as resolved.
Show resolved Hide resolved
"renv", "packrat",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

# version control
".git", ".gitignore", ".svn",
# R/RStudio
Expand Down
Loading