rstudio / renv

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

`Suggests` dependencies included by default #1019

Open bollard opened 2 years ago

bollard commented 2 years ago

Hi,

I'm experiencing (I think), the same issue as https://github.com/rstudio/renv/issues/823. But since that is a little stale (and on an older version) I thought I'd raise the issue here again.

Using, renv 0.15.5

Reproduce as before,

  1. Create a package (for instance with usethis::create_package("suggestdemo"))
  2. Run renv::init() and restart the R session
  3. Add the Suggests-field to the DESCRIPTION file (for instance Suggests: knitr)
  4. Run renv::status()

The result is

> renv::status()
* The project is already synchronized with the lockfile.
The following package(s) are used in the project, but are not installed:

    knitr

Consider installing these packages, and then using `renv::snapshot()`
to record these packages in the lockfile.

Note also,

> renv::settings$package.dependency.fields()
[1] "Imports"   "Depends"   "LinkingTo"

And finally,

> renv::dependencies(dev = TRUE)
Finding R package dependencies ... Done!
                           Source Package Require Version   Dev
1 D:/code/suggestdemo/DESCRIPTION   knitr                 FALSE
2   D:/code/suggestdemo/renv.lock    renv                 FALSE

My expectation would be to not see knitr in renv::status and to see it listed as a development TRUE dependency in renv::dependencies

Thanks

kevinushey commented 2 years ago

For R packages managed by renv, the Suggests dependencies from the DESCRIPTION file are included by default, under the assumption that those packages would be required for testing (or other similar runtime requirements).

If you change the snapshot type to "explicit", then the issue should go away:

renv::settings$snapshot.type("explicit")
bollard commented 2 years ago

Thanks for the explanation, and yes while I can understand that they'd be needed for testing and / or building the package, they wouldn't be needed for runtime operation of doing whatever it is the package happens to do.

I think including them by default adds a lot of unnecessary bloat to the renv.lock file and, even if I can understand that being the default setting, I don't understand why that choice couldn't be made by the user (and it seems like that mechanism is already in place with renv::settings$package.dependency.fields)

Using the explicit snapshot type (as I understand) would affect how all (including non-Suggests) packages are captured and so doesn't feel like a great solution (or at least not what I'd hope to achieve).

I've seen some discussion of separate development dependency groups, but until / if that is merged, could I suggest some combination of either:

Thanks

kevinushey commented 2 years ago

Thanks; I think I agree with your assessment that even if the default is to not include Suggested packages, the user should be able to configure that behavior (probably via the package.dependency.fields setting).

hadley commented 1 year ago

I would expect renv::install() to install suggested packages but renv::snapshot() to not snapshot them (i.e. snapshot() should respect package.dependency.field but install should ignore it).

@kevinushey do you think it's possible to make this change now without breaking expected behaviour? (i.e. should I just document it?)

hadley commented 1 year ago

Per #1336, this also seems to be a problem if you explicitly specific the dependencies.

kevinushey commented 1 year ago

I'd be on board making this change even if it meant breaking some existing behaviors, since the current behavior seems unintuitive.

hadley commented 1 year ago

In install(), replace renv_project_remotes() with renv_install_dependencies() to parallel renv_snapshot_dependencies(). (It's possible we might be able to remove renv_project_remotes() afterwards, but that should be done in a separate PR).

If the default snapshot type has been switched to explicit, it should look for dependencies only in the DESCRIPTION, but default to including packages listed in the Suggests. It might makes sense to change the signature of install() to align on dev = TRUE/FALSE rather than specifying dependencies (But this will require some thought as to how it would affect settings$package.dependency.fields).

Then need to update renv_snapshot_dependencies() to ensure that Suggested packages are treated as dev. Looks like this is happening in renv_dependencies_discover_description() and renv_dependencies_discover_description_impl() where Suggested packages are only listed as dev dependencies if its not the active project. renv_snapshot_dependencies() also powers status(), so this will fix the motivating problem.

hadley commented 1 year ago

Once #1394 (the culmination of a bunch of refactoring) is merged, this should be straightforward to implement — we can add a use_dev_deps argument to renv_snapshot_dependencies(), and then only set it to TRUE in install().

chrisbrownlie commented 1 year ago

I've just upgraded to the new v1.0.0 - does this change mean that its no longer possible to have snapshot type of "implicit", and include packages from Suggests when snapshotting? I can't see how to do so as setting renv::settings$package.dependency.fields seems to have no impact on what is being considered in renv::snapshot(). The documentation for ?renv::settings also seems to suggest that package.dependency.fields is only relevant when using renv::install() as opposed to renv::restore().

I have packages in Suggests that I'd like to be included in my lockfile when snapshotting and restoring (while keeping the 'implicit' snapshot type) - what would be the way to achieve this? IOW is there a way to snapshot() development dependencies or has that possibility been removed?

hadley commented 1 year ago

No, implicit dependencies still look at any DESCRIPTION files so this should still work. Could you please file a new issue with an explanation of the problem that you're seeing along with a reprex?

If you want to snapshot development dependencies, it sounds like they might just be regular dependencies? Or at least it's not clear to me, what the difference would be.

kevinushey commented 1 year ago

Here's what I see:

> writeLines(readLines("DESCRIPTION"))
Type: Project
Suggests: rlang
> renv::dependencies(dev = TRUE)
Finding R package dependencies ... Done!
                                    Source Package Require Version   Dev
1 /Users/kevin/scratch/suggest/DESCRIPTION   rlang                  TRUE
2   /Users/kevin/scratch/suggest/renv.lock    renv                 FALSE

Because we treat these as development dependencies, they won't enter the lockfile unless they're explicitly used elsewhere in the project (e.g. in the above case you'd need library(rlang) or something similar).

kevinushey commented 1 year ago

I have packages in Suggests that I'd like to be included in my lockfile when snapshotting and restoring

I guess part of the question here is: why do you want to record these packages in Suggests if you also want them to be in the lockfile? Can you expand a bit more on your project structure?

chrisbrownlie commented 1 year ago

I guess part of the question here is: why do you want to record these packages in Suggests if you also want them to be in the lockfile? Can you expand a bit more on your project structure?

Hi, thanks both for the responses - this might just be down to my misunderstanding of how renv should be used or what development dependencies are!

I have a shiny app that is structured as a package, and have put packages under Suggests like devtools, testthat, covr etc. The reason I'd like these in my lockfile is our CI pipeline renv::restore()s packages from the lockfile before running checks/tests. I'd also like people in the team to be using the same versions etc. of those packages when developing. Is there a different way of doing this we should be using?

KoderKow commented 1 year ago

Hello! I am facing a situation in this area and I can explain my project structure and workflow.

My project was using {renv} 0.15.4 where the Suggests packages were being captured with snapshot(type = "explicit"). As mentioned above, {renv} 1.0.0 no longer adds Suggests packages to the lockfile. These Suggests packages are things like {here}, {fs}, {roxygen2}, etc. These were placed there to make sure developers had the same versions of these packages on their own system for the given project.

When we are ready to make pull requests, everything is done in Azure DevOps. I have a custom-built solution for making things work in Azure pipelines; pretty much a docker solution that will renv::restore() to reproduce the dev environment then proceeds to do a package check and run unit testing. Now that renv 1.0.0 does not include Suggests packages with explicit snapshotting, these packages are missing during installation and I am met with an error stating they are not available.

Error within Azure Devops:

❯ checking package dependencies ... ERROR
  Package suggested but not available: ‘printr’

  The suggested packages are required for a complete check.
  Checking can be attempted without them by setting the environment
  variable _R_CHECK_FORCE_SUGGESTS_ to a false value.

  See section ‘The DESCRIPTION file’ in the ‘Writing R Extensions’
  manual.

1 error ✖ | 0 warnings ✔ | 0 notes ✔
Error: R CMD check found ERRORs
Execution halted

I tried adding the Suggests field with renv::settings$package.dependency.fields, but that did not work. I am having a hard time finding a quick solution as this is stalling my updates for a project to go through. I would prefer not to downgrade {renv}, is there a set of actions I am missing or not understanding? Am I using {renv} incorrectly? Is it recommended to set _R_CHECK_FORCE_SUGGESTS_ to FALSE?

As a bandaid, I have put my Suggests packages in my R/{pkg-name}-package.R files as imports so {renv} will pick them up.

hadley commented 1 year ago

If you want packages to be tracked in the snapshot, then I think you should just list them in Imports instead of Suggests. Alternatively, you could call renv::install() in your docker pipeline.

Fuco1 commented 1 year ago

If you want packages to be tracked in the snapshot, then I think you should just list them in Imports instead of Suggests. Alternatively, you could call renv::install() in your docker pipeline.

The main ideas I think are two:

Package managers such as npm or yarn (javascript) or composer (php) work exactly this way. They make no difference between dev and production dependencies except simply separating them into two sets which you can install independently. But if a dev dependency breaks constraints of a prod one or vice-versa, the install will fail (i.e. when installing everything, the two sets are merged together and the entire set must resolve properly).

Because I guess historically R packages worked in a different way and Suggest was really just a suggestion for some (manual?) action, this logic might not completely map to the DESCRIPTION file.

In our project, I'm actually using a non-standard ImportsTest: section where I but some test dependencies and install them with some custom-logic which parses the desc file... not great solution, but it works :/

kevinushey commented 1 year ago

Re-opening for future consideration (can we include development dependencies in the lockfile? or in an alternate "dev" lockfile?)

Fuco1 commented 6 months ago

I'm in the process of upgrading to renv 1.x and some things are still quite unclear to me regarding the dev dependencies. I will describe the expected workflow as I see it.

Workflow A - working on the project

  1. Clone the repository
  2. Run renv::restore(). This I expect to install all the dependencies of the project as well as tools such as devtools, pkgdown, testthat and so on (as recorded in Suggests)
  3. I make changes to the project and decide to update one package. I run renv::update("ggplot") then renv::snapshot(). This should update the lock file with the newly updated version of ggplot

Workflow B - building a docker image

  1. Clone the repository
  2. Run renv::restore(dev = FALSE). This I expect to only restore Imports packages to versions recorded in renv.lock file and not install any dependencies from the Suggests. Currently this seems to install everything.

I think there is a disconnect between what is recorded in the lock file and what is restored. I see the restore action as taking any subset of the lockfile and installing that, of course provided the subset is consistent. That is how it works in other package managers most commonly.

Right now I see as a workaround to generate two snapshots, one renv::snapshot(dev = TRUE) and one with dev = FALSE and use one for the Workflow A and the second for Workflow B. Is this the intended mechanism?

kevinushey commented 6 months ago

Right now I see as a workaround to generate two snapshots, one renv::snapshot(dev = TRUE) and one with dev = FALSE and use one for the Workflow A and the second for Workflow B. Is this the intended mechanism?

I think that's the correct approach currently, given that lockfiles currently do not record whether a particular package is considered a development dependency or not -- the current assumption is that a lockfile should include only non-development dependencies.

jpanfil commented 1 month ago

Following up on this, what's the current best practice for a situation where I have a subset of packages that I would want restored when in a production setting, but otherwise, I can restore all of the packages?

For example, say I have these packages listed in my DESCRIPTION.

Imports: 
    checkmate,
    dplyr,
    magrittr,
    rlang
Suggests:
    ggplot2,
    scales

For production, I want only the Imports. Otherwise, I want all packages from both Imports and Suggests. What is the recommended way to set up the lockfile, renv::restore, and so on?