Use pak::local_install_deps() in install()#2651
Conversation
| build_opts <- c("--no-resave-data", "--no-manual") | ||
| # Install dependencies | ||
| local({ | ||
| if (quiet) { |
There was a problem hiding this comment.
Hack around lack of quiet argument or option for pak.
| if (build) { | ||
| install_path <- pkgbuild::build( | ||
| pkg$path, | ||
| build = build, |
There was a problem hiding this comment.
It's a bit hard to see in the diff, but we're no longer passing any build or install options when we install the dependencies. I think this is likely to be fine, since you'd generally except these to coming from CRAN and not built locally.
|
After this change, Positron should probably revert to calling |
jennybc
left a comment
jennybc
left a comment
There was a problem hiding this comment.
@hadley I took this a bit further and left some inline comments. What do you think? I'm happy for merge.
- Improved output headers to include the package name and distinguish the three phases: "Installing dependencies of {pkg}", "Building {pkg}" (only when
build = TRUE), "Installing {pkg}" - Changed
keep_sourcedefault togetOption("keep.source.pkgs") || !build, soinstall(quick = TRUE)andinstall(build = FALSE)automatically preserve working source references - Refined docs for
quick,build, andkeep_sourceto be more accurate and have less overlap
| #' @param quick if `TRUE`, skips some optional steps (e.g. help | ||
| #' pre-rendering and multi-arch builds) to make installation as fast | ||
| #' as possible. |
There was a problem hiding this comment.
I clarified this and had to do some research! I've never understood what it meant to "skip docs", because clearly help files are available after installation even with install(quick = TRUE). So I've reworded to be more clear about what --no-docs actually does.
| dependencies = NA, | ||
| upgrade = FALSE, | ||
| build_vignettes = FALSE, | ||
| keep_source = getOption("keep.source.pkgs") || !build, |
There was a problem hiding this comment.
I actually changed this default!
The previous default didn't make much sense to me, as it made it pretty hard for a developer to get srcrefs. You had to know to do build = FALSE AND keep_source = TRUE.
I think this default is going to make default behaviour much more sensible.
There was a problem hiding this comment.
I was going to ask why we don't default to keep_source = TRUE but reading the documentation made it clear 😄.
R/install.R
Outdated
|
|
||
| if (build) { | ||
| if (!quiet) { | ||
| cli::cat_rule(paste0("Building ", pkg$package), col = "cyan") |
There was a problem hiding this comment.
I added this header so it would be more obvious whether the build step is happening or not.
There was a problem hiding this comment.
I removed this again, and added a snapshot test to hopefully illustrate why.
| #' @param quick if `TRUE`, skips some optional steps (e.g. help | ||
| #' pre-rendering and multi-arch builds) to make installation as fast | ||
| #' as possible. |
| dependencies = NA, | ||
| upgrade = FALSE, | ||
| build_vignettes = FALSE, | ||
| keep_source = getOption("keep.source.pkgs") || !build, |
There was a problem hiding this comment.
I was going to ask why we don't default to keep_source = TRUE but reading the documentation made it clear 😄.
Co-authored-by: Hadley Wickham <h.wickham@gmail.com>
2 participants
Includes some general updates to the docs of
install(), and mild improvements to non-quiet output.Fixes #2486. Fixes #2572.
@jennybc if you're testing
install()interactively, it doesn't work on devtools itself, for probably obvious reasons.