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

Prompt user to install missing and outdated dependencies #184

Merged
merged 3 commits into from
Dec 16, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions DESCRIPTION
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,18 @@ Imports:
crayon,
desc,
methods,
rlang (>= 0.99.0.9001),
rlang (>= 0.99.0.9003),
rprojroot,
rstudioapi,
utils,
withr (>= 2.4.3)
Suggests:
Rcpp,
bitops,
covr,
pak,
pkgbuild,
Rcpp,
remotes,
testthat (>= 3.1.0)
Remotes:
r-lib/rlang
Expand Down
8 changes: 8 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
# pkgload (development version)

* `load_all()` now calls `rlang::check_installed()` to prompt whether
to install missing packages.

Outdated and missing dependencies are installed using pak if
installed. If not, the remotes package is used if installed.
Otherwise `install.packages()` is used as a last resort but this
method does not support Remotes fields.

* User `onLoad` hooks are now run after exports have been
populated. This allows the hook to use exported functions.

Expand Down
13 changes: 1 addition & 12 deletions R/imports-env.r
Original file line number Diff line number Diff line change
Expand Up @@ -60,23 +60,12 @@ load_imports <- function(path = ".") {
return(invisible(imports))
}

res <- mapply(check_dep_version, imports$package, imports$version)
abort_for_missing_packages(res, imports$package)

deps_check_installed(path, imports)
process_imports(path)

invisible(deps)
}

abort_for_missing_packages <- function(x, pkgs) {
if (any(!x)) {
abort(
paste0("Dependency package(s) ",
paste0("'", pkgs[!x], "'", collapse = ","),
" not available."))
}
}

# Load imported objects
# The code in this function is taken and adapted from base::loadNamespace
# Setup variables were added and the for loops put in a tryCatch block
Expand Down
3 changes: 1 addition & 2 deletions R/load-depends.r
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,7 @@ load_depends <- function(path = ".", quiet = FALSE) {
return(invisible())
}

res <- mapply(check_dep_version, depends$package, depends$version)
abort_for_missing_packages(res, depends$package)
deps_check_installed(path, depends)

# The `quietly` argument of `require()` does not affect attachment
# of required packages
Expand Down
38 changes: 38 additions & 0 deletions R/package-deps.r
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,44 @@ parse_deps <- function(string) {
deps[names != "R", ]
}

# Takes a dependency data frame generated by the `desc` package.
deps_check_installed <- function(path, deps, call = caller_env()) {
if (!nrow(deps)) {
return()
}

pkg <- deps$package
ver <- deps$version

# Recreate `pkg (>= ver)` strings
has_version <- ver != "*"
pkg[has_version] <- sprintf(
"%s (%s)",
pkg[has_version],
ver[has_version]
)

# Outdated and missing dependencies are installed using pak if
# installed. If not, the remotes package is used if installed.
# Otherwise `install.packages()` is used as a last resort but this
# method does not support Remotes fields.
action <- function(pkg, ...) {
if (is_installed("pak")) {
deps <- pak::pkg_deps(path, upgrade = FALSE)
deps <- deps[deps$package %in% pkg, ]
pak::pak(deps$ref, ask = FALSE)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does two rounds of dependency lookups, which is not ideal.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a fix to suggest? This is getting the refs of the package names in pkg which are potentially Remotes refs, and then installing them. Is there a way to do these two steps in one go?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just call

pkg_install("deps::.")

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid having to reinstall dependencies from github when pak does not recognise that a local or dev install is sufficient.

} else if (is_installed("remotes")) {
deps <- remotes::dev_package_deps(path)
deps <- deps[deps$package %in% pkg, ]
update(deps, upgrade = TRUE)
} else {
utils::install.packages(pkg)
}
}

check_installed(pkg, action = action, call = call)
}


#' Check that the version of an imported package satisfies the requirements
#'
Expand Down
19 changes: 9 additions & 10 deletions tests/testthat/test-depend.r
Original file line number Diff line number Diff line change
@@ -1,20 +1,19 @@
local_load_all_quiet()

test_that("Warned about dependency versions", {
test_that("Warn about mismatched version", {
# Should give a warning about grid version
expect_warning(load_all("testImportVersion"), "Need grid >=")
expect_error(
load_all("testImportVersion"),
class = "rlib_error_package_not_found"
)
unload("testImportVersion")

# TODO: Add check for NOT giving a warning about compiler version
# Not possible with testthat?
})


test_that("Error on missing dependencies", {
# Should give a warning about missing package
expect_error(regexp = "Dependency package[(]s[)] 'missingpackage' not available",
expect_warning(regexp = "missingpackage.* not available",
load_all("testImportMissing")))
expect_error(
load_all("testImportMissing"),
class = "rlib_error_package_not_found"
)

# Loading process will be partially done; unload it
unload("testImportMissing")
Expand Down