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

Move more dependencies to Suggests #962

Merged
merged 13 commits into from Jan 12, 2016

Conversation

Projects
None yet
3 participants
@jimhester
Copy link
Member

jimhester commented Oct 28, 2015

This allows devtools to be installed in less time, such as when it is
used only to install packages and dependencies on Travis and other CIs.

If a given Suggested dependency is needed for functionality check_suggested() prints an informative error

check_suggested("ggplot22")
#> Error: 'ggplot22' must be installed for this functionality
check_suggested("ggplot2", "2.0.0")
#> Error: 'ggplot2' >= 2.0.0 must be installed for this functionality

This should remove the Rcpp dependency and combined with r-lib/httr@7c68198 it should lighten the dependency load for devtools considerably.

@@ -6,8 +6,7 @@ compile_rcpp_attributes <- function(pkg) {
# Only scan for attributes in packages explicitly linking to Rcpp
if (links_to_rcpp(pkg)) {

if (!requireNamespace("Rcpp", quietly = TRUE))
stop("Rcpp required for building this package")
check_suggested("Rcpp", "0.10.0")

# Only compile attributes if we know we have the function available
if (utils::packageVersion("Rcpp") >= "0.10.0")

This comment has been minimized.

Copy link
@hadley

hadley Oct 28, 2015

Member

This check could now go

@hadley

This comment has been minimized.

Copy link
Member

hadley commented Oct 28, 2015

Where does evaluate get used? In run_examples() maybe?

@hadley

This comment has been minimized.

Copy link
Member

hadley commented Oct 28, 2015

The only package I'm hesitant about removing is roxygen2, because I have some vague recollection that we deliberately made it part of devtools. I think it's basically because there's a strong tension in devtools to be batteries included because it makes teaching R package development that much easier.

@jimhester

This comment has been minimized.

Copy link
Member Author

jimhester commented Oct 28, 2015

Evaluate is from run_examples(), I had a check for it, but failed to save that file, included in 9fc052f.

Also removed the extra Rcpp check in 8b94cf4.

Unfortunately roxygen2 has dependencies for both stringr and Rcpp, which are the two biggest dependencies for devtools currently, so keeping it in Imports kind of defeats the purpose of this PR.

I agree with the batteries included approach to devtools, it just doesn't jive well when it is used for installing packages on CIs. You could always suggest people install devtools with install.packages("devtools", dependencies = TRUE) to install all the Suggested packages (including roxygen2).

The best solution would be to split out the install_ functions to their own package (installr?), which would have limited dependencies, but for now I guess this is a stop gap.

@jimhester jimhester force-pushed the jimhester:lighten_dependencies branch from 8b94cf4 to f28c702 Oct 28, 2015

@hadley

This comment has been minimized.

Copy link
Member

hadley commented Oct 28, 2015

Another option would be to have a batteriesincluded package that just had everything people needed as a dependency.

@hadley

This comment has been minimized.

Copy link
Member

hadley commented Oct 28, 2015

For symmetry if we're going to drop roxygen2, I think we should also drop testhat.

And could you add a bullet point to NEWS and think about how to change the README to tell people how to get a full environment for package development?

@gaborcsardi

This comment has been minimized.

Copy link
Member

gaborcsardi commented Oct 28, 2015

Whenever ppl try using testthat or roxygen, they could just get a message that it is missing, and it needs to be installed. Potentially devtools could also install them if interactive().

Btw. as much as I like micropackages, I personally don't like "removing" testthat and roxygen2.
Maybe we can work on making roxygen2 lighter, e.g. remove the Rcpp dependency. testthat is not too heavy imo. And/or moving the install_* functions to separate packages.

Btw.2. slightly related, https://github.com/metacran/description is almost ready. Some more docs would be nice, and validation is mostly missing. Validation is very hard, actually, for some fields, e.g. License. I'll add them, slowly, but it will take a while.

@jimhester

This comment has been minimized.

Copy link
Member Author

jimhester commented Oct 28, 2015

Actually testthat has nearly always been in Suggests (0a02d83) and is only used for test(), which we check for https://github.com/hadley/devtools/pull/962/files#diff-576140fd16c657af5df5c401b5f95bbbR21, so that should be OK.

The main issues with just using install.packages("devtools", dependencies = TRUE) is

  1. BiocInstaller, because you can't install that from CRAN (and most people aren't going to need it)
  2. Rcpp (not needed if you are doing R only) and Windows users often don't have a RTools installed.
  3. lintr - many people aren't going to use this, has a hefty igraph dependency (although it wouldn't hurt it's popularity 😄)
  4. MASS and bitops - these are just used in the tests
  5. knitr & rmarkdown (not useful if you are using old style Sweave vignettes)

I think 1. kind of makes the dependencies = TRUE approach a non-starter for general use.

Re: @gaborcsardi this implementation does generate an error with informative message about the missing packages, but offering to install them isn't a bad idea though, maybe that is the best way to solve this problem...

@jimhester

This comment has been minimized.

Copy link
Member Author

jimhester commented Oct 28, 2015

e4f1ece adds the a prompt to install the suggested packages if they are not installed, I think it will work well.

@hadley

This comment has been minimized.

Copy link
Member

hadley commented Oct 28, 2015

@gaborcsardi I think removing the Rcpp dependency from roxygen2 is going to be hard - it's just a much better tool for parsing stuff than R is. I'd rather push towards moving all the package install stuff out in a separate package.

@gaborcsardi

This comment has been minimized.

Copy link
Member

gaborcsardi commented Oct 28, 2015

@hadley I am not suggesting removing the C++ code, only the Rcpp wrapper. Rcpp is great, but the roxygen C++ code is short and seems straightforward without Rcpp, too.

Just an idea, not sure if it is worth the effort.

@jimhester

This comment has been minimized.

Copy link
Member Author

jimhester commented Oct 29, 2015

jimhester@2db88ba adds a NOTE to news.

With e4f1ece if someone doesn't have roxygen2 or testthat installed when they run document() or test() respectively it will prompt them asking if they want to install it, so it should be pretty seamless for new users I think.

Another additional thing we could do would be to add installation prompts to setup() for roxygen2 and testthat. setup() is called for create() as well, so this would ensure users have both roxygen2 and testthat if they are writing a new package. I don't know how much that adds to the current functionality though, thoughts?

@jimhester jimhester force-pushed the jimhester:lighten_dependencies branch from c0874e1 to 0d96154 Nov 5, 2015

@jimhester jimhester force-pushed the jimhester:lighten_dependencies branch from 69222fe to 8a02e54 Jan 11, 2016

@jimhester jimhester force-pushed the jimhester:lighten_dependencies branch from 8a02e54 to 7792e8e Jan 11, 2016

jimhester added some commits Jan 11, 2016

@jimhester

This comment has been minimized.

Copy link
Member Author

jimhester commented Jan 11, 2016

This is now ready jimhester@d7c94be failed only because of GitHub rate limiting on Appveyor and contains only NEWS changes from jimhester@b4bc23e, which passed.

@hadley

This comment has been minimized.

Copy link
Member

hadley commented Jan 12, 2016

This looks good but I'm concerned that the version req is now recorded in two places, and the chances are that I'll forget to update one

@jimhester

This comment has been minimized.

Copy link
Member Author

jimhester commented Jan 12, 2016

Hmm true, I can write a function to read the version number from the DESCRIPTION and use that is the default value of version for check_suggested().

R/utils.r Outdated
message(msg, "\nWould you like to install it?")
if (menu(c("Yes", "No")) == 1) {
install.packages(pkg)
}

This comment has been minimized.

Copy link
@hadley

hadley Jan 12, 2016

Member

Doesn't this also need a stop()? (i.e. if they say no)

This comment has been minimized.

Copy link
@jimhester

jimhester Jan 12, 2016

Author Member

Yes! Fixed with jimhester@ab8bae8. Having the two stop() calls right after each other looks a little ugly, but modifying the conditional to avoid them would be just as bad in other ways.

@hadley

This comment has been minimized.

Copy link
Member

hadley commented Jan 12, 2016

Thanks!

hadley added a commit that referenced this pull request Jan 12, 2016

Merge pull request #962 from jimhester/lighten_dependencies
Move more dependencies to Suggests

@hadley hadley merged commit 627a521 into r-lib:master Jan 12, 2016

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.