rstudio / renv

renv: Project environments for R.
https://rstudio.github.io/renv/
MIT License
1.02k stars 155 forks source link

Should `install()` respect the `ignored.packages` setting? #1743

Open klmr opened 1 year ago

klmr commented 1 year ago

I am setting ignored.packages to make ‘renv’ ignore a package that’s used in a project. The reason is that this package gets loaded (from the global library) before the renv gets activated (consequently, the package is only referred to in the project’s .Rprofile, nowhere else).

This seems to work insofar as renv::status() and renv::snapshot() ignore that package.

However, renv::dependencies() still lists it, and renv::install() still attempts to install it. For me, this seems to defeat the purpose of the setting. Is this an oversight, or is this working as intended?

If so, is there a way of making ‘renv’ truly ignore a package, and not attempt to install it?

(renv::settings$ignored.packages() includes the package, so it was definitely set correctly.)

kevinushey commented 11 months ago

Sorry for the late reply -- this feels like an oversight to me, but would you be able to share a minimal reproducible example just so I'm certain I understand the issue?

It feels like certain requests should override that option, though -- e.g. if you tried to ignore a transitive dependency, or something similar. For example:

renv::settings$ignored.packages("rlang")
renv::install("ggplot2")

should still necessarily install rlang, I think?

klmr commented 11 months ago

Hi Kevin,

I agree that transitive dependencies must still be installed, even if they are listed in the ignored packages. I don’t think anybody would expect anything different, since that would completely break ‘renv’.

Regarding an MWE, consider a project which uses the ‘rprofile’ package. The intended use of this package is to set up projects via a project-local .Rprofile file before ‘renv’ is loaded.

As such, the package is used by putting the following line into the project .Rprofile file:

rprofile::load()

This happens instead of (not in addition to)source("renv/activate.R"), and internally causes ‘renv’ to be loaded.

Since this package will be loaded before ‘renv’ is loaded, it obviously makes no sense to include it into the ‘renv’ library or lockfile (and doing so would have no effect) — it needs to be installed in the site-wide or user library. That’s why it should never be installed by renv::install().

hutch3232 commented 9 months ago

I'm experiencing this issue with renv::install() and renv::dependencies() as well. renv::status() is totally fine. I also agree transitive dependencies must be installed. This issue comes about in a different way for me or at least a different use-case.

I use the import package to load some helper functions from a script. In my code, I'm storing the script name in a variable, 'x', and because renv knows how to interpret dependencies from import::from it's picking up my variable and is incorrectly thinking it's a package.

Full example:

R --no-save

R version 4.3.1 (2023-06-16) -- "Beagle Scouts"
Copyright (C) 2023 The R Foundation for Statistical Computing
Platform: x86_64-pc-linux-gnu (64-bit)

R is free software and comes with ABSOLUTELY NO WARRANTY.
You are welcome to redistribute it under certain conditions.
Type 'license()' or 'licence()' for distribution details.

  Natural language support but running in an English locale

R is a collaborative project with many contributors.
Type 'contributors()' for more information and
'citation()' on how to cite R or R packages in publications.

Type 'demo()' for some demos, 'help()' for on-line help, or
'help.start()' for an HTML browser interface to help.
Type 'q()' to quit R.

- Project '/mnt/code/test-renv' loaded. [renv 1.0.3]
- The project is out-of-sync -- use `renv::status()` for details.
> renv::status()
The following package(s) are missing:

package installed recorded used
x       n         n        y  

See ?renv::status() for advice on resolving these issues.
> renv::dependencies()
Finding R package dependencies ... Done!
                         Source Package Require Version   Dev
1 /mnt/code/test-renv/.Rprofile  import                 FALSE
2 /mnt/code/test-renv/.Rprofile       x                 FALSE
3 /mnt/code/test-renv/renv.lock    renv                 FALSE
> renv::settings$ignored.packages("x")
> renv::dependencies()
Finding R package dependencies ... Done!
                         Source Package Require Version   Dev
1 /mnt/code/test-renv/.Rprofile  import                 FALSE
2 /mnt/code/test-renv/.Rprofile       x                 FALSE
3 /mnt/code/test-renv/renv.lock    renv                 FALSE
> renv::status()
The following package(s) are in an inconsistent state:

package installed recorded used
import  y         n        y  

See ?renv::status() for advice on resolving these issues.
> renv::install()
Error: package 'x' is not available
Traceback (most recent calls last):
14: renv::install()
13: retrieve(packages)
12: handler(package, renv_retrieve_impl(package))
11: renv_retrieve_impl(package)
10: records[[package]] %||% renv_retrieve_resolve(package)
9: renv_retrieve_resolve(package)
8: tryCatch(renv_snapshot_description(package = package), error = function(e) {
        renv_retrieve_missing_record(package)
    })
7: tryCatchList(expr, classes, parentenv, handlers)
6: tryCatchOne(expr, names, parentenv, handlers[[1L]])
5: value[[3L]](cond)
4: renv_retrieve_missing_record(package)
3: renv_available_packages_latest(package)
2: stopf("package '%s' is not available", package)
1: stop(sprintf(fmt, ...), call. = call.)

.Rprofile

source("renv/activate.R")
x <- "helpers.R"
import::from(.from=x, .character_only=TRUE, .all=TRUE)

helpers.R

foo <- function(){

}
klmr commented 9 months ago

@hutch3232 I think this is a different issue — namely that ‘renv’ is parsing the import::from() call incorrectly. That should be fixed (i.e. ‘renv’ needs to check the .character_only argument).

That said, let me shill for my own package as an alternative here: I believe that ‘box’ handles this better than ‘import’, and ‘renv’ parses ‘box’ declarations correctly.

With ‘box’, you’d use the following code:

box::use(./x[...])

That’s both cleaner and simpler than the import::from() call. Though ‘box’ (intentionally) does not support variables in box::use(). I’ve written a more extensive comparison explaining why I think that ‘box’ is generally superior to ‘import’.

hutch3232 commented 9 months ago

Thanks, @klmr, I do agree with your point about the dependency checking for this usage pattern of import being bugged. However, I think my post still applies here because while my main problem is incorrect dependency detection, my use of renv::settings$ignored.packages should have resolved this issue as well.

Thanks for sharing box, that does look interesting!

kevinushey commented 9 months ago

Thanks -- I wonder if the documentation here:

https://github.com/rticulate/import/blob/8649e70a9c8dbbec26504ba4476c3cad857c525d/R/from.R#L50

should be updated to indicate that .from can be a package or an R script / module.

kevinushey commented 9 months ago

The convention here is also a bit unfortunate since something like import::from(bea.R) is ambiguous, given that there is a package named bea.R on CRAN. IMHO the import package should require one or more slashes in .from to disambiguate package names from file paths.

klmr commented 8 months ago

@kevinushey

IMHO the import package should require one or more slashes in .from to disambiguate package names from file paths.

Incidentally (as you might be aware) that is exactly how ‘box’ disambiguates between package and module names.