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

install packages that do not yet satisfy version requirements #355

Merged
merged 5 commits into from Oct 22, 2013

Conversation

Projects
None yet
3 participants
@yihui
Contributor

yihui commented Oct 15, 2013

at the moment, install_deps() does not check the version requirements, which is particularly bad when a package under development increases its dependency versions, and here is a patch that hopefully fills out the gap; please let me know if you have any questions or want me to do anything else

@hadley

View changes

R/install.r Outdated
# Remove packages that are already installed
not.installed <- function(x) length(find.package(x, quiet = TRUE)) == 0
deps <- Filter(not.installed, deps)
info <- parse_deps(paste(c(pkg$depends, pkg$linkingto, pkg$imports), collapse = ','))

This comment has been minimized.

@hadley

hadley Oct 15, 2013

Member

Since you're in here, would you mind making the dependencies argument work? i.e. if it's true add in suggests

This comment has been minimized.

@yihui

yihui Oct 16, 2013

Contributor

okay, done

@hadley

View changes

R/install.r Outdated
# Packages that are not already installed or without required versions
filter_deps <- function(pkg, compare, version) {
deps <- character(0)
for (i in seq_along(pkg)) {

This comment has been minimized.

@hadley

hadley Oct 15, 2013

Member

Could you write this in a more functional style? i.e. basically by improving the old not.installed package?

This comment has been minimized.

@yihui

yihui Oct 16, 2013

Contributor

the old not.installed function only takes one argument (package names); now I need to take three, and I guess Filter() is no longer an elegant solution, so I just used the dumb loop approach, which seems to be the most straightforward

This comment has been minimized.

@wch

wch Oct 16, 2013

Member

@yihui Maybe something like this? (Note that it works for the example but I haven't tested thoroughly)

needs_install <- function(pkg, compare, version) {
  if (length(find.package(pkg, quiet = TRUE)) == 0) return(TRUE)
  if (is.na(compare)) return(FALSE)
  do.call(compare, list(packageVersion(pkg), version))
}

# Example
deps <- parse_deps("httr (< 2.1),\nRCurl (>= 3)")

needed <- mapply(needs_install, deps$name, deps$compare, deps$version)
needed <- deps$name[needed]

This comment has been minimized.

@yihui

yihui Oct 16, 2013

Contributor

@wch Thanks! I can do that and test it if you two are happy with it.

This comment has been minimized.

@hadley

hadley Oct 16, 2013

Member

@wch I think it would be slightly nicer to do:

needs_install <- function(pkg, compare, version) {
  if (length(find.package(pkg, quiet = TRUE)) == 0) return(TRUE)
  if (is.na(compare)) return(FALSE)

  compare <- match.fun(compare)
  compare(packageVersion(pkg), version)
}

This comment has been minimized.

@wch

wch Oct 16, 2013

Member

@hadley agreed, match.fun is nicer.

One more thing: with the mapply, the returned object isn't always the same time, which can cause a problem when deps has zero rows. Probably best to do something like:

needed <- mapply(needs_install, deps$name, deps$compare, deps$version, SIMPLIFY = FALSE)
needed <- deps$name[as.logical(needed)]

Which works for both of these cases, at least:

deps <- parse_deps("httr (< 2.1),\nRCurl (>= 3)")
deps <- parse_deps("")

This comment has been minimized.

@hadley

hadley Oct 16, 2013

Member

FYI Map(...) is a shorter way of writing mappy(..., SIMPLIFY = FALSE)

@yihui

This comment has been minimized.

Contributor

yihui commented Oct 16, 2013

okay, anything else to do?

@hadley

This comment has been minimized.

Member

hadley commented Oct 17, 2013

Looks good. Can you please add a line to the NEWS crediting yourself? (Add it under 1.4 since that hasn't quite made it to cran yet)

@yihui

This comment has been minimized.

Contributor

yihui commented Oct 18, 2013

Sure. I'll do that.

@yihui

This comment has been minimized.

Contributor

yihui commented Oct 20, 2013

Done.

hadley added a commit that referenced this pull request Oct 22, 2013

Merge pull request #355 from yihui/feature/install-version
install packages that do not yet satisfy version requirements

@hadley hadley merged commit 3ef4a80 into r-lib:master Oct 22, 2013

@hadley

This comment has been minimized.

Member

hadley commented Oct 22, 2013

Thanks!

@yihui yihui deleted the yihui:feature/install-version branch Oct 23, 2013

krlmlr pushed a commit to krlmlr-archive/r-travis-old that referenced this pull request Oct 24, 2013

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