-
Notifications
You must be signed in to change notification settings - Fork 82
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
New strategy to retrieve DESCRIPTION if project uses renv #931
Conversation
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
now it's faster to include the lockfile 😁 > bench::mark(
+ rsconnect::writeManifest(appFiles = c("index.qmd")),
+ rsconnect::writeManifest(appFiles = c("index.qmd", "renv.lock")), iterations = 3
+ )
ℹ Capturing R dependencies with renv
✔ Found 118 dependencies
ℹ Capturing R dependencies from renv.lock
✔ Found 121 dependencies
# A tibble: 2 × 13
expression min median `itr/sec` mem_alloc `gc/sec` n_itr n_gc total_time result memory time gc
<bch:expr> <bch> <bch:> <dbl> <bch:byt> <dbl> <int> <dbl> <bch:tm> <list> <list> <list> <list>
1 "rsconnect::writeManifest(appFiles = c(\"index.qmd\"))" 8.07s 8.49s 0.118 114MB 1.14 3 29 25.4s <NULL> <Rprofmem> <bench_tm> <tibble>
2 "rsconnect::writeManifest(appFiles = c(\"index.qmd\", \"renv.… 6.38s 6.49s 0.155 113MB 0.618 3 12 19.4s <NULL> <Rprofmem> <bench_tm> <tibble> thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will re-test console and push-button deploys from renv projects before merging; the deploy-pane R environment was different than what @hadley was seeing (because I only use the system R library, not a user library).
NEWS.md
Outdated
@@ -1,5 +1,10 @@ | |||
# rsconnect (development version) | |||
|
|||
* `deployApp()` and `writeManifest()` now error if your library and lockfile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renv.lock
rather than "lockfile".
"should err" or "should error"? I prefer "err", but others see that as an "error". 😜
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like "err" means to make a mistake; whereas here we are erroring (or more technically correct, throwing an error).
R/bundlePackageRenv.R
Outdated
lib_dir = c(lib_dir, .libPaths()) | ||
) | ||
if (!snapshot) { | ||
lib_versions <- unlist(lapply(deps$Package, packageDescription, fields = "Version")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are about to load all the package descriptions just below; what about validating against deps$description
rather than loading the same information twice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but agree with the comment re: avoiding the need to read DESCRIPTION files twice.
#Conflicts: # NEWS.md # R/bundlePackageRenv.R
Confirmed deployment for an renv project from an environment that does not have a user R library. |
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
@edavidaja could you please confirm that this fixes your problem?