Ability to install directly from a Git repository URL #169

Merged
merged 3 commits into from Dec 16, 2012

Projects

None yet

3 participants

@davidcoallier
Contributor

This pull requests allow someone to install a public repository (or a repository said person has access to) directly from the devtools package using the install_git command.

  • Added the initial install_git command that allows one to install directly from a git repository.
  • Added the documentation page for the install_git command as well.
@hadley hadley commented on an outdated diff Oct 16, 2012
R/install-git.r
@@ -0,0 +1,85 @@
+#' Install a package from a git repository
+#'
+#' This function is vectorised so you can install multiple packages in
hadley
hadley Oct 16, 2012 Owner

I think you note that this requires git installed, and there should be a specific error if it is not

@hadley hadley commented on an outdated diff Oct 16, 2012
R/install-git.r
+#'
+#' See \code{\link{install_git}} for more information about the paraemeters.
+install_git_single <- function(git_url, name = NULL, subdir = NULL, config = list(), ...) {
+ if (is.null(name)) {
+ name <- basename(git_url)
+ }
+
+ message("Installing ", name, " using the Git-URL: ", git_url)
+
+ bundle <- file.path(tempdir(), name)
+
+ # This is only to make sure we have the latest git version.
+ # It should —in-theory— be handled by \code{on.exit} but it
+ # isn't always the case, therefore we clean before ourselves.
+ if (file.exists(bundle)) {
+ unlink(bundle, force = TRUE, recursive = TRUE)
hadley
hadley Oct 16, 2012 Owner

I think you'd be better off installing to tempfile() so it's guaranteed to be unique

@hadley hadley commented on an outdated diff Oct 16, 2012
R/install-git.r
+
+ # Install
+ install(pkg_path, ...)
+}
+
+#' Retrieve the current install git-cli version installed and used.
+get_git_version <- function() {
+ # We use \code{system} here because there's no arbitrary parameter.
+ return(system('git --version', intern=TRUE))
+}
+
+#' Retrieve the current running path of the git binary.
+get_git_command_path <- function() {
+ # We use \code{system} here because there's no arbitrary parameter.
+ return(system('which git', intern=TRUE))
+}
hadley
hadley Oct 16, 2012 Owner

Sys.which would be better. On windows, you'll need to look for git.exe

@hadley hadley commented on an outdated diff Oct 16, 2012
R/install-git.r
+
+ message("Installing ", name, " using the Git-URL: ", git_url)
+
+ bundle <- file.path(tempdir(), name)
+
+ # This is only to make sure we have the latest git version.
+ # It should —in-theory— be handled by \code{on.exit} but it
+ # isn't always the case, therefore we clean before ourselves.
+ if (file.exists(bundle)) {
+ unlink(bundle, force = TRUE, recursive = TRUE)
+ }
+
+ # Clone the package file from the git repository.
+ # @TODO: Handle configs, this currently only supports public repos
+ # and repositories with the public SSH key set.
+ request <- system2('git', args=c('clone', git_url, bundle), stdout = FALSE, stderr = FALSE)
hadley
hadley Oct 16, 2012 Owner

Maybe use system_check here?

Owner
hadley commented Oct 16, 2012

This is the first time we're adding an external dependency on git, so it will need some thought.

Contributor

Great stuff thanks for all the comments :) I'll rework the patch, rebase it and let you know inline.

If, after reflexion, you realise that you don't want to use it, that's fine as well. I was thinking about adding the ability to specify your git binary in the install_git(...) parameters. What do you think?

Contributor

@hadley I've rebased and added the changes from the peer-review. The OS detection is a lot better and I've added a parameter for people that'd want to use their own git binary.

Let me know if this seems to make a bit more sense :)

@hadley hadley commented on an outdated diff Oct 16, 2012
R/install-git.r
+#' Install a package from a git repository
+#'
+#' It is important to note that this function requires "Git" to be
+#' installed on your system in order to be used.
+#'
+#' This function is vectorised so you can install multiple packages in
+#' a single command.
+#'
+#' @param git_url location of package on internet. The url should point to a
+#' public or private repository.
+#' @param name optional package name, used to provide more informative
+#' messages
+#' @param subdir A sub-directory withing a git repository that may
+#' contain the package we are interested in installing.
+#'
+#' @param git_binary Specify which Git binary to use.
hadley
hadley Oct 16, 2012 Owner

Maybe this should be an option? (see zzz.r)

@hadley hadley and 1 other commented on an outdated diff Oct 16, 2012
R/install-git.r
+install_git_single <- function(git_url, name = NULL, subdir = NULL,
+ git_binary = NULL, config = list(), ...) {
+ if (is.null(name)) {
+ name <- basename(git_url)
+ }
+
+ message("Preparing installation of ", name, " using the Git-URL: ", git_url)
+
+ bundle <- tempfile()
+
+ # We just set a dummy-fallback variable.
+ git_binary_path = "git"
+
+ if (!is.null(git_binary)) {
+ if (!file.exists(git_binary)) {
+ stop(paste(
hadley
hadley Oct 16, 2012 Owner

You don't need to use paste with stop - it accepts ... and paste together with no sep.

davidcoallier
davidcoallier Oct 16, 2012 Contributor

Saw that after, it'll be part of my next rebase alongside the other changes :-)

@hadley hadley commented on an outdated diff Oct 16, 2012
R/install-git.r
+ if (!file.exists(git_binary)) {
+ stop(paste(
+ "The Git binary you have specified ",
+ "doesn't seem to exist on this system.", sep=""
+ ), call. = FALSE)
+ }
+
+ git_binary_path <- git_binary
+ } else {
+ # No custom binary was specified therefore we are going
+ # to use the default system git-binary which is decided
+ # depending on the OS you are running. See \code{get_default_git_binary()}
+ # for more information on how we retrieve it.
+ git_binary_path <- get_default_git_binary()
+ if (git_binary_path == "") {
+ stop("We could not find a Git binary on your system.", call. = FALSE)
hadley
hadley Oct 16, 2012 Owner

I think get_default_git_binary should throw an error if it can't find one

@hadley hadley commented on an outdated diff Oct 16, 2012
R/install-git.r
+ stop("Does not appear to be an R package", call. = FALSE)
+ }
+
+ config_path <- file.path(pkg_path, "configure")
+ if (file.exists(config_path)) {
+ Sys.chmod(config_path, "777")
+ }
+
+ # Install
+ install(pkg_path, ...)
+}
+
+#' Retrieve the current install git-cli version installed and used.
+get_git_version <- function() {
+ # We use \code{system} here because there's no arbitrary parameter.
+ return(system('git --version', intern=TRUE))
hadley
hadley Oct 16, 2012 Owner

Don't need explicit return

@hadley hadley commented on an outdated diff Oct 16, 2012
R/install-git.r
+
+ # Install
+ install(pkg_path, ...)
+}
+
+#' Retrieve the current install git-cli version installed and used.
+get_git_version <- function() {
+ # We use \code{system} here because there's no arbitrary parameter.
+ return(system('git --version', intern=TRUE))
+}
+
+#' Retrieve the current running path of the git binary.
+#' @param binary_name The name of the binary depending on the OS.
+get_git_command_path <- function(binary_name) {
+ # We use \code{system} here because there's no arbitrary parameter.
+ return(as.character(Sys.which(get_default_git_binary())))
hadley
hadley Oct 16, 2012 Owner

Are you using as.character to strip the names? If so, unname would be more clear.

@hadley hadley and 1 other commented on an outdated diff Oct 16, 2012
R/install-git.r
+get_git_version <- function() {
+ # We use \code{system} here because there's no arbitrary parameter.
+ return(system('git --version', intern=TRUE))
+}
+
+#' Retrieve the current running path of the git binary.
+#' @param binary_name The name of the binary depending on the OS.
+get_git_command_path <- function(binary_name) {
+ # We use \code{system} here because there's no arbitrary parameter.
+ return(as.character(Sys.which(get_default_git_binary())))
+}
+
+#' Retrieve the default git binary name dependng on the OS.
+get_default_git_binary <- function() {
+ os_type <- .Platform$OS.type
+ if (os_type == "unix") {
hadley
hadley Oct 16, 2012 Owner

if (os_type == "unix") "git" else "git.exe" would be more idiomatic

hadley
hadley Oct 16, 2012 Owner

But I think I'd rather just have this wrapped inside of get_git_command_path (which maybe could be called git_path?)

davidcoallier
davidcoallier Oct 16, 2012 Contributor

Yeah I think git_path makes sense. I'll do the changes and rebase.

Contributor

I've left out the .onLoad options(...) setting because it really made it look awkward.

Contributor

Did you have a chance to review and think about that @hadley? I've been using install_git on a few machines now and it works pretty well :P

Owner
hadley commented Oct 29, 2012

It looks good - I'll pull next time I'm working on devtools.

Contributor

Cool thanks :)

Collaborator
wch commented Dec 13, 2012

I think it would be a good idea to make a shallow git clone, to minimize the download size.

Contributor

@wch Yeah I agree. --depth=1 or do you have something else in mind?

@wch wch and 1 other commented on an outdated diff Dec 13, 2012
R/zzz.r
@@ -68,7 +68,7 @@ current_ver <- function() {
op <- options()
op.devtools <- list(
devtools.path="~/R-dev",
- github.user="hadley"
+ github.user="hadley",
wch
wch Dec 13, 2012 Collaborator

Extra comma?

davidcoallier
davidcoallier Dec 13, 2012 Contributor

Must have been from some residual changes I had done. I'll remove and rebase.

@wch wch commented on the diff Dec 13, 2012
R/install-git.r
+ }
+
+ # Install
+ install(pkg_path, ...)
+}
+
+#' Retrieve the currently installed git-cli version.
+git_version <- function() {
+ system2(git_path(), args=c('--version'))
+}
+
+
+#' Retrieve the current running path of the git binary.
+#' @param git_binary_name The name of the binary depending on the OS.
+git_path <- function(git_binary_name = NULL) {
+ os_type <- .Platform$OS.type
wch
wch Dec 13, 2012 Collaborator

I think these two variables don't need to be defined up here -- even without these lines, they will be defined before being used by anything else.

@wch wch commented on the diff Dec 13, 2012
R/install-git.r
+}
+
+#' Retrieve the currently installed git-cli version.
+git_version <- function() {
+ system2(git_path(), args=c('--version'))
+}
+
+
+#' Retrieve the current running path of the git binary.
+#' @param git_binary_name The name of the binary depending on the OS.
+git_path <- function(git_binary_name = NULL) {
+ os_type <- .Platform$OS.type
+ binary_name <- NULL
+
+ if (!is.null(git_binary_name)) {
+ if (!file.exists(git_binary_name)) {
wch
wch Dec 13, 2012 Collaborator

Maybe use !file.exists(Sys.which(git_binary_name))? Otherwise, will users need to specify the full path?

davidcoallier
davidcoallier Dec 13, 2012 Contributor

This case is for users that did specify the path. We only validate what they said before proceeding further. The users don't need to always specify the full path, that's what the else right after takes care of.

@wch wch and 2 others commented on an outdated diff Dec 13, 2012
R/install-git.r
+ # This is only looking for an error code above 0-success
+ if (request > 0) {
+ stop("There seems to be a problem retrieving this Git-URL.", call. = FALSE)
+ }
+
+ pkg_path <- if (is.null(subdir)) bundle else file.path(bundle, subdir)
+ on.exit(unlink(bundle), add = TRUE)
+
+ # Check it's an R package
+ if (!file.exists(file.path(pkg_path, "DESCRIPTION"))) {
+ stop("Does not appear to be an R package", call. = FALSE)
+ }
+
+ config_path <- file.path(pkg_path, "configure")
+ if (file.exists(config_path)) {
+ Sys.chmod(config_path, "777")
wch
wch Dec 13, 2012 Collaborator

Does this need to be world-writeable? If not, 755 would be better. (Also, does Sys.chmod work on Windows?)

davidcoallier
davidcoallier Dec 13, 2012 Contributor

755 does make more sense. I'll change that.

With regards to Sys.chmod on Windows, I'd suspect that if it doesn't work, it'll degrade nicely. It is in the base package after all. Anyone with a Window machine could test that?

hadley
hadley Dec 14, 2012 Owner

@davidcoallier I'm confused - have you ever programmed in R before? ;) But I did just try it on a windows VM, and it worked without errors.

davidcoallier
davidcoallier Dec 14, 2012 Contributor

Hah :) It is my first public R code-contribution and I'm sure you can tell from the first version of this pull-request ;)

From having worked with many other projects, the "Sys"-like libraries are usually the only ones I've noticed with a knack to degrade nicely, now I assume no other R library is going to be that gentle :P

@wch wch and 1 other commented on an outdated diff Dec 13, 2012
R/install-git.r
+ # Check it's an R package
+ if (!file.exists(file.path(pkg_path, "DESCRIPTION"))) {
+ stop("Does not appear to be an R package", call. = FALSE)
+ }
+
+ config_path <- file.path(pkg_path, "configure")
+ if (file.exists(config_path)) {
+ Sys.chmod(config_path, "777")
+ }
+
+ # Install
+ install(pkg_path, ...)
+}
+
+#' Retrieve the currently installed git-cli version.
+git_version <- function() {
wch
wch Dec 13, 2012 Collaborator

I don't see this function used anywhere. Can it be removed?

davidcoallier
davidcoallier Dec 13, 2012 Contributor

Sure, it was for the eventuality of unit-tests for the install-git functions.

@wch wch commented on the diff Dec 13, 2012
R/install-git.r
+ invisible(mapply(install_git_single, git_url, name,
+ MoreArgs = list(
+ subdir = subdir, git_binary = git_binary, config = config, ...
+ )
+ ))
+}
+
+#' Install a single package from a git repository
+#'
+#' This function allows you to install a single package from a git repository.
+#'
+#' See \code{\link{install_git}} for more information about the paraemeters.
+install_git_single <- function(git_url, name = NULL, subdir = NULL,
+ git_binary = NULL, config = list(), ...) {
+ if (is.null(name)) {
+ name <- basename(git_url)
wch
wch Dec 13, 2012 Collaborator

I think git URLs usually have .git on the end of them, and it might be nice to strip that part off as well.

Collaborator
wch commented Dec 13, 2012

@davidcoallier yes, --depth=1 was what I was thinking. (I haven't used it before, though.)

David Coallier added some commits Oct 15, 2012
David Coallier - Added the initial `install_git` command that allows one to install
  direclty from a git repository.

- Added the documentation page for the `install_git` command as well.

- Adjusted code to reflect peer-review from Hadley
- Added the `git_binary` variable to the documentation.

Changed the name of `get_git_version` to `git_version` to be more similar to `git_path` and other nomenclature.
6bb4860
David Coallier - Removed superfluous comma at the end of `zzz.r` (Thanks @wch)
- Changes:
  - Clone with --depth 1 and --no-hardlinks. Makes it a whole lot faster.
  - Sys.chmod(755) instead of 777
  - Removed the .git from the name we display when checking out the code
    if a `.git` is present in the base name.
  - Removed the `git_version` function as there's no need for it just
    yet.
2435934
Contributor

Ok I've rebased off your master and made the changes. This version of the PR should be a lot better now.

As for the comment (#169 (comment)), I've decided to leave the variable declarations at the top of the function as it makes it easier for me to track. If you feel like removing it, feel free to commit it to my branch and I'll push it here :)

@wch wch commented on an outdated diff Dec 14, 2012
R/install-git.r
+ ))
+}
+
+#' Install a single package from a git repository
+#'
+#' This function allows you to install a single package from a git repository.
+#'
+#' See \code{\link{install_git}} for more information about the paraemeters.
+install_git_single <- function(git_url, name = NULL, subdir = NULL,
+ git_binary = NULL, config = list(), ...) {
+ if (is.null(name)) {
+ name <- basename(git_url)
+
+ # Following the feedback from @wch, we remove
+ # the "git" suffix from the name of the clone folder.
+ parts = unlist(strsplit(name, ".", fixed=TRUE))
wch
wch Dec 14, 2012 Collaborator

A simpler way to strip off .git is to do this:

name <- gsub("\\.git$", "", name)

Example cases:

gsub("\\.git$", "", "foo.git")  # should change this
# [1] foo

gsub("\\.git$", "", "foogit")  # shouldn't change
# [1] "foogit"

gsub("\\.git$", "", "foo.gitbar")  # shouldn't change
# [1] "foo.gitbar"
Collaborator
wch commented Dec 14, 2012

Looking good!

@wch wch merged commit ae95e9b into hadley:master Dec 16, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment