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

[R] Add additional DESCRIPTION fields for dev dependencies and update docs accordingly #28485

Closed
asfimport opened this issue May 11, 2021 · 8 comments

Comments

@asfimport
Copy link
Collaborator

asfimport commented May 11, 2021

Edit: Blocked by a bug in {remotes} which results in second order dependencies of packages listed in Config/Needs?... not being installed.

Recent updates to the remotes package mean that additional fields can be added to a package's DESCRIPTION and remotes functions can take the field as a parameter and install these additional dependencies: https://github.com/r-lib/remotes/blob/master/NEWS.md#new-functions-and-features

We could update DESCRIPTION to use these fields and update the docs to use this functionality of remotes instead of having separate references to packages to install.

 

Reporter: Nicola Crane / @thisisnic
Assignee: Dragoș Moldovan-Grünfeld / @dragosmg

Related issues:

PRs and other links:

Note: This issue was originally created as ARROW-12743. Please see the migration documentation for further details.

@asfimport
Copy link
Collaborator Author

Ian Cook / @ianmcook:
Also see the related discussion here: #10373 (comment)

For some dev dependencies such as roxygen2, a version of the package does exist on CRAN, but we want the newer dev version of it from GitHub. In such cases, wouldn’t it be fine to just use Suggests and Remotes like this: 

Suggests: 
    roxygen2
Remotes:  
    r-lib/roxygen2

Config/Needs seems more appropriate for dependencies that don’t exist on CRAN at all. The devtools vignette says:

When you submit your package to CRAN, all of its dependencies must also be available on CRAN. For this reason, release() will warn you if you try to release a package with a Remotes field.
But that is vague on the question of what happens if all dependencies are on CRAN and there’s a Remotes field.

@asfimport
Copy link
Collaborator Author

Jonathan Keane / @jonkeane:
If I'm reading the devtools/remotes documentation correctly, if we use something like Config/Needs/docs: ... then someone would have to install the dependencies with install_deps(".", dependencies = "Config/Needs/docs") in order to have those dependencies installed. Which isn't that hard, but is an extra step beyond what is currently needed. The pro is that we don't have to trim this when sending it to CRAN.

On the other hand using Remotes: r-lib/roxygen2 all the dependencies are installed when people do the standard install_deps(".", dependencies = TRUE) which is nice for developer (especially new developer) experience, but we'll need to trim it before we send to CRAN (though we already do that with badges, so it's not that much extra work).

I think leaving it with a version number requirement is not great — we would need to confirm that the new version is released to cran before we can send it (since it would fail the version check there and complain, right?) and it's possible for someone to have installed roxygen2 from github before this feature was added, but after the number was bumped so it would appear at install time that the dependency is satisfied, but then the errors when redocumenting would be weird.

@asfimport
Copy link
Collaborator Author

Dragoș Moldovan-Grünfeld / @dragosmg:
I think the roxygen2 version is recorded in a single place in DESCRIPTION (i.e. the RoxygenNote: field). I don't think CRAN checks parse that field and check what version of roxygen2 is available. If there is a mismatch in versions between the developer's local environment and DESCRIPTION devtools::check() with not re-document. Running devtools::document() in the same scenario will overwrite the RoxygenNote field.

We could use multiple "Config/Needs/..." fields and then install all dependencies with a single call to {}remotes::install_deps(){}.

install_deps(".", dependencies = c("soft", "Config/Needs/website", "Config/Needs/dev"))

, where "soft" is the equivalent of {}TRUE{}.

I think if someone has the development version of roxygen2 installed and they attempt to install dependencies, remotes will compare shas and will suggest and upgrade even if the version number is the same.

@asfimport
Copy link
Collaborator Author

Dragoș Moldovan-Grünfeld / @dragosmg:
There seems to be a bug in {remotes} when trying to install dependencies from the "Config/Needs/..." fields. The package or packages listed there are installed, but not their dependencies.

Reprex (needs to be run inside a package that has Config/Needs/website: pkgdown in DESCRIPTION:

# fs and memoise are pkgdown dependencies
remove.packages(c("pkgdown", "fs", "memoise")) 
remotes::install_deps(".", dependencies = "Config/Needs/website")
packageVersion("pkgdown")
# [1] ‘2.0.1’
packageVersion("fs")
# Error in packageVersion("fs") : there is no package called ‘fs’
packageVersion("memoise")
# Error in packageVersion("memoise") : there is no package called ‘memoise’

adding "soft" or "hard" to dependencies works, but that seems a non-canonical use of either of those options and, thus, could change without warning.

remotes::install_deps(".", dependencies = c("hard", "Config/Needs/website"))
# or
remotes::install_deps(".", dependencies = c("soft", "Config/Needs/website"))

In conclusion, I propose we delay using the Config/Needs/... field in DESCRIPTION until either the {remotes} issue is solved or we switch to {pak} for package installation.

@asfimport
Copy link
Collaborator Author

Jonathan Keane / @jonkeane:
And, additionally, didn't you find that remotes::install_deps(".", dependencies = "soft") installs not just imports+depends+suggests, but does that recursively for all packages it touches, so ends up installing many more packages than {{remotes::install_deps(".", dependencies = TRUE)} right?

@asfimport
Copy link
Collaborator Author

Dragoș Moldovan-Grünfeld / @dragosmg:
Yes, that is right, I raised that as ARROW-15299. I'm not 100% convinced I am correct with that one, hence raised it as an investigation ticket (and not yet a {remotes} GitHub issue). 

@asfimport
Copy link
Collaborator Author

Dragoș Moldovan-Grünfeld / @dragosmg:
I am copying (from ARROW-15299) the results of my little investigation. 

What we would need is a combination between dependencies = TRUE and {}dependencies = "Config/Needs/website"{}.
I ran the experiment and the results are below:

|test|description|total_pkgs_installed|
|
|-|-|-|-|
|1|dependencies = TRUE|64|
|
|2|dependencies = 'hard'|12|
|
|3|dependencies = c('hard', 'Config/Needs/website')|61|
|
|4|dependencies = 'soft'|1819|
|
|5|dependencies = c('soft', 'Config/Needs/website')|1819|
|
|6|dependencies = c('Config/Needs/website')|1|



I excluded dependencies = 'soft' & combinations of 'soft' since that is too recursive and ends up installing an excessive number of packages.

dependencies = 'hard' & dependencies = 'Config/Needs/website' are too narrow.



I compared dependencies = TRUE to {}dependencies = c('hard', 'Config/Needs/website'){}. While the overall number of installed packages looks promising (64 vs 61), there isn't complete overlap. |

@asfimport
Copy link
Collaborator Author

Nicola Crane / @thisisnic:
Issue resolved by pull request 11921
#11921

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant