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

Suggests dependencies included by default #1019

Open
bollard opened this issue May 31, 2022 · 20 comments · Fixed by #1399
Open

Suggests dependencies included by default #1019

bollard opened this issue May 31, 2022 · 20 comments · Fixed by #1399
Labels
dependencies 👶 feature a feature request or enhancement
Milestone

Comments

@bollard
Copy link

bollard commented May 31, 2022

Hi,

I'm experiencing (I think), the same issue as #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
Copy link
Collaborator

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
Copy link
Author

bollard commented Jun 5, 2022

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:

  • Improving the documentation of renv::settings$package.dependency.fields to make the current behaviour clearer (i.e. Suggests are included in R package workspaces)
  • Adding Suggests to the list of returned fields when renv::settings$package.dependency.fields() is called inside an R package workspace
  • Changing the behaviour of renv::settings$package.dependency.fields to not include this special handing of R package workspaces (and only include the packages specified by that option, a la principle of least surprise)

Thanks

@kevinushey
Copy link
Collaborator

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 hadley added feature a feature request or enhancement dependencies 👶 labels Apr 25, 2023
@hadley hadley changed the title Suggests dependencies not being ignored by default on renv 0.15.5 Suggests dependencies included by default May 1, 2023
@hadley
Copy link
Member

hadley commented May 12, 2023

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
Copy link
Member

hadley commented May 25, 2023

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

@kevinushey
Copy link
Collaborator

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

@hadley
Copy link
Member

hadley commented Jun 1, 2023

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
Copy link
Member

hadley commented Jun 7, 2023

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
Copy link

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
Copy link
Member

hadley commented Jul 14, 2023

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link

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
Copy link

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
Copy link
Member

hadley commented Jul 20, 2023

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
Copy link

Fuco1 commented Sep 27, 2023

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:

  • I want to ensure that the entire team is using same development dependencies (as you can imagine there can be some breaking changes in for example pkgdown or devtools which would disrupt the workflow)
  • I don't want to include these in the production docker image for my project as they can eat up to 500 megabytes and also slow down builds.

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 kevinushey reopened this Oct 25, 2023
@kevinushey kevinushey added this to the 1.1.0 milestone Oct 25, 2023
@kevinushey
Copy link
Collaborator

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

@Fuco1
Copy link

Fuco1 commented May 7, 2024

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
Copy link
Collaborator

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
Copy link

jpanfil commented Oct 3, 2024

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies 👶 feature a feature request or enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants