Add support for renv to the Packages Pane#12446
Conversation
`renv` is just like `pak` in that it is a runtime depdendency, so we have to check it each time we execute a command.
Disable interactive prompts for renv install/update operations so they run non-interactively. Sort the package list alphabetically by name (case-insensitive) so newly installed packages appear in their correct position rather than at the top. See #11660
|
E2E Tests 🚀 |
austin3dickey
left a comment
There was a problem hiding this comment.
Thanks! Works for me.
| const pkgNames = packages.map(p => p.name); | ||
| const pkgVector = this._formatRVector(pkgNames); | ||
| code = `install.packages(${pkgVector})`; | ||
| code = `renv::install(${pkgVector}, lock = TRUE, prompt = FALSE)`; |
There was a problem hiding this comment.
Does renv not support version tags?
There was a problem hiding this comment.
It does! We should use this similar to other methods:
renv::install("digest@0.6.18")You can see how this function works with ?renv::install, to see the help page for it.
juliasilge
left a comment
There was a problem hiding this comment.
I opened up some existing renv projects I have, and unfortunately they were not identified correctly as using renv. We need to iterate on how we decide whether to opt in to the renv handling here. I can start doing some digging into that, but the documentation for the R package is here: https://rstudio.github.io/renv/
| */ | ||
| private async _detectRenv(): Promise<boolean> { | ||
| try { | ||
| const result = await this._session.evaluate('!is.null(getOption("renv.project.path"))'); |
| const pkgNames = packages.map(p => p.name); | ||
| const pkgVector = this._formatRVector(pkgNames); | ||
| code = `install.packages(${pkgVector})`; | ||
| code = `renv::install(${pkgVector}, lock = TRUE, prompt = FALSE)`; |
There was a problem hiding this comment.
It does! We should use this similar to other methods:
renv::install("digest@0.6.18")You can see how this function works with ?renv::install, to see the help page for it.
| code = `pak::pkg_remove(${pkgVector})`; | ||
| if (isRenv) { | ||
| // Print a reminder to run renv::snapshot() to update lockfile | ||
| code = `renv::remove(${pkgVector}); message("Note: Run renv::snapshot() to update your lockfile.")`; |
There was a problem hiding this comment.
This is going to be quite confusing for folks. Do I remember correctly from our discussion today that we said an explicit removal should be a strong enough signal to update the lockfile? If I remember that correctly, then we should just go ahead and do that.
If not, we should make a "real", regular notification here in the VS Code sense and not use printing in the console.
Prior to this, install/update were just always using the latest. This also changes the update action to use the install function. This seems to be consistent with the API exposed by pak. But it appears that the versioned APIs are really only available for install.
renv::project() is the public API for detecting if renv is active, rather than checking the internal renv.project.path option. See #11657
Automatically update the renv lockfile after removing packages, matching the behavior of install and update which use lock=TRUE. See #11657
Run the lockfile update silently after the removal completes, rather than chaining it in the same R expression. See #11657
|
This should be good to go now. I tested by installing cowsay, note there are differences in |
juliasilge
left a comment
There was a problem hiding this comment.
I think something is not quite right in how we populate the Packages pane for renv projects with pkg_list. For example, it has presented me with a package here that is not in the renv-managed library, and when I try to uninstall it, it fails saying that the package is not installed:
That's pretty confusing; I think we should do a better job of showing the packages that are being managed by renv in the Packages pane. I think it would be something like installed.packages(lib.loc = renv::paths$library()) but could use some research to make sure it's the best option.
| let code: string; | ||
| if (method === 'pak') { | ||
| // pak supports "pkg@version" syntax directly | ||
| if (isRenv) { |
There was a problem hiding this comment.
The way the logic for these different commands is structured right now is nested something like this:
if (isRenv) {
// renv code
} else {
// get method
if (method === 'pak') {
// pak code
} else {
// base R code
}
}
Can we use early returns to make the code easier to follow? Something like this, for all the different commands:
async installPackages(packages: positron.PackageSpec[]): Promise<void> {
for (const pkg of packages) {
this._validatePackageName(pkg.name);
}
// Handle renv projects
if (await this._detectRenv()) {
const pkgSpecs = packages.map(p => p.version ? `${p.name}@${p.version}` : p.name);
const pkgVector = this._formatRVector(pkgSpecs);
await this._executeAndWait(`renv::install(${pkgVector}, lock = TRUE, prompt = FALSE)`);
this._session.invalidatePackageResourceCaches();
return;
}
// Non-renv: use pak or base R
let method: string;
if (packages.some((pkg) => pkg.name === 'pak')) {
method = await this._getPakMethod();
} else {
method = await this._ensurePak();
}
if (method === 'pak') {
const pkgSpecs = packages.map(p => p.version ? `${p.name}@${p.version}` : p.name);
const pkgVector = this._formatRVector(pkgSpecs);
await this._executeAndWait(`pak::pkg_install(${pkgVector}, ask = FALSE)`);
} else {
const pkgNames = packages.map(p => p.name);
const pkgVector = this._formatRVector(pkgNames);
await this._executeAndWait(`install.packages(${pkgVector})`);
}
this._session.invalidatePackageResourceCaches();
}There was a problem hiding this comment.
We chatted more on this, and it's true there are tradeoffs between preferring a flatter structure with early returns and having more clarity on when the code is getting executed. Sounds like your preference, @bricestacey, is the second, which is very reasonable.
juliasilge
left a comment
There was a problem hiding this comment.
This is looking good!
We need to improve how we handle failures in the multi-step commands, but that can come in a follow up. Also OK for a follow up, I don't think we should run the snapshot updating silently because of how easy it is for that to fail.
| // Update renv lockfile after removal | ||
| if (isRenv) { | ||
| await this._executeSilently('renv::snapshot(prompt = FALSE)'); | ||
| } |
There was a problem hiding this comment.
This is not a blocker for merging here, but can you log a follow up issue to handle errors differently? For example, here I tried to remove a package and renv::remove() succeeded but updating the snapshot failed. Since that code is run silently, the message is quite confusing:
The situation is that uninstalling succeeded, but the message says "Failed to uninstall package: 'httr'".
I also think I would advocate for running that code renv::snapshot() where the user can see it, rather than silently. It is very easy for this to fail.
There was a problem hiding this comment.
Here is the ticket #12627
This is also an issue for install using the lock = TRUE parameter... I don't know how to easily reproduce the error you presented, but I just make the renv.lock file read-only. Note that install succeeds, but yeah the lock file update fails.
> Sys.chmod("renv.lock", mode = "0444") # Make read-only
> renv::install(c("fortunes@1.5-4"), lock = TRUE, prompt = FALSE)
The following package(s) will be installed:
- fortunes [1.5-4]
These packages will be installed into "~/posit/qa-example-content/workspaces/packages-r-renv2/renv/library/macos/R-4.5/aarch64-apple-darwin20".
# Installing packages --------------------------------------------------------
- Installing fortunes 1.5-4 ... OK [linked from cache]
Successfully installed 1 package in 3 milliseconds.
Traceback (most recent calls last):
13: renv::install(c("fortunes@1.5-4"), lock = TRUE, prompt = FALSE) at #1
12: renv_lockfile_save(lockfile, project = project)
11: renv_lockfile_write(lockfile, file = file)
10: renv_lockfile_write_json(lockfile, file)
9: writeLines(json, con = file)
8: base::writeLines(enc2utf8(text), con = con, sep = sep, useBytes = TRUE)
7: file(con, "w")
6: .handleSimpleError(function (cnd)
{
...
}, "cannot open the connection", base::quote(file(con, "w")))
5: h(simpleError(msg, call))
4: <condition-handler>(...)
3: invoke_option_error_handler() at errors.R#32
2: withCallingHandlers(
{
# Evaluate from a promise to keep a simple call stack.
# We do evaluate from a closure wrapped in `handler()` so that R
# can infer a named call, for instance in the "Called from:"
# output of `browser()`.
error_handler <- eval(bquote(function() .(hnd)))
error_handler()
},
error = function(err) {
# Disable error handler to avoid cascading errors
options(error = NULL)
# We don't let the error propagate to avoid a confusing sequence of
# error messages from R, such as "Error during wrapup"
writeLines(
c(
"The `getOption(\"error\")` handler failed.",
"This option was unset to avoid cascading errors.",
"Caused by:",
conditionMessage(err)
),
con = stderr()
)
# Bail early
non_local_return
}
) at errors.R#221
1: error_handler() at errors.R#228
Warning message:
In file(con, "w") :
cannot open file '/Users/brice/posit/qa-example-content/workspaces/packages-r-renv2/renv.lock': Permission denied
Error in `file()`:
! cannot open the connection
Show Traceback
|
Let me know if you want any support in doing the ark building/updating dance. |
When fetching the packages for `renv`, we only want to include the packages within a given libPath... This PR is related to posit-dev/positron#12446 posit-dev/positron#11660
|
This PR includes the bump for ark #12633 |

See #11660
Release Notes
New Features
renvto the Packages PaneBug Fixes
QA Notes