Only install from if a different sha from current #903

jimhester · 2015-08-25T21:34:53Z

This modifies the behavior of install_github to only download and install a package if the sha has changed from the currently installed version.

It needs to do two requests from the GitHub API

To get the sha of the 'ref' commit ('master' by default)
To retrieve the package DESCRIPTION (so we can disambiguate if a repository name is different than the package name)

It then looks up the existing package DESCRIPTION (which will have a RemoteSha if it was previously installed using install_github()) and compares it to the GitHub version.

If the installed DESCRIPTION does not have a RemoteSha or they are different, install_github proceeds as normal, otherwise nothing happens.

This works well with #902, especially if a given package has a lot of heavy dependencies that take a while to compile.

A couple of places I wanted input

I named the parameter skip_same, but this feels awkward, perhaps there is a better name?
Personally I would prefer this was the default behavior so skip_same = TRUE, but maybe you would prefer it to default to FALSE.
If we assume that all packages have the same repository name as the package name we can cut down the number of requests to just the sha. This is probably valid ~90% or more of the time, but not always.
We can probably do analogous things with the other remote types, if you find this beneficial I can make pull requests for them as well.
This has no tests yet, I wanted to settle on an implementation before doing so, but I can add them before it is accepted.

hadley · 2015-09-02T15:59:27Z

R/github.R

+    file.path("repos", username, repo, "contents", "DESCRIPTION"),
+    httr::write_memory())
+
+  content <- rawToChar(base64enc::base64decode(response$content))


Why do you need to do the decoding yourself here?

The github API returns a JSON object with a content field that is base64 encoded. I think httr will decode automatically if the entire response is base64 encoded, but only part of it is in this case.

If there is a httr function I can use to do this decoding that would be great, I just missed it!

jimhester · 2015-09-03T15:10:33Z

So my main worry with this is having to query the GitHub API twice, when unauthenticated the rate limit is only 60 requests per hour, so if someone is doing testing using install_github() with 5 github packages in Remotes: they only have to run install_github() 5 times before they hit the rate limit. (6 * 2 * 5 = 60).

I think I can get the same information without querying the GitHub API using e.g.

git ls-remote git@github.com:jimhester/lintr.git
# aaf61208cf877fe3a077c5146126a0f3867205a2        HEAD
# 50ab86db4ebc7272d95990f8d15b3fabe0278936        refs/heads/chunk_parsing
# fc80f59b2051ed3043c572bd808e53368123a529        refs/heads/comment
# cc0e600b043eac2d3753ca8058c160c5d690adfd        refs/heads/linter_names
# aaf61208cf877fe3a077c5146126a0f3867205a2        refs/heads/master
# f88a4804df4c7cccb1585b1cf08a5601c41e490f        refs/heads/single_line
# a73b1762d795a67fda02475b9a02a4a4151850e9        refs/pull/109/head
# 5e4a4f295744bfb02b0f85a7da1a5f620c0e3b6f        refs/pull/110/head
# 7b3ac8f985251063ca25a3be21d803abfb6ff2ad        refs/pull/19/head
# b1a9d4bc5d501859769c498aceaef30b9a2ed23b        refs/pull/28/head
# 67b8681335482359bc0b6a6bd4ca01596e920be3        refs/pull/37/head
# 517a4df59f9dab8bfb736a9f73e2efe3021720a2        refs/pull/46/head
# 427e177926421819b917c4dd55cd3c42cb231525        refs/pull/49/head
# 7b07088df347352b0da75eebc82ecc2e7cc1ab23        refs/pull/51/head
# fc80f59b2051ed3043c572bd808e53368123a529        refs/pull/52/head
# 7b422f6bc02b6d014ee1260d0f7d8d59d185dba6        refs/pull/69/head
# 97f5e794e8c0b379ca9945c23200dbbca0e101af        refs/pull/70/head
# 7d27c930710696c91688c610a634efa3490699aa        refs/pull/70/merge
# b828e3333c2fc2e7172116d6f6de44436651c72e        refs/pull/71/head
# ab973a26e330677f91b39da9f9ca423f6a36eb90        refs/pull/74/head
# cc1ee872613fc9a527f5680f6ca491b29ce3fa41        refs/pull/78/head
# 71d6d2f58612f291838b74ad3992e8886cd1b928        refs/pull/80/head
# 6d10a4250cda301926e8462be7e67001d8f10f7d        refs/pull/83/head
# 6f17692c940ac2b81cd6648194a0ca2753abbb1a        refs/pull/90/head
# 4722335176ba6601f7a0178f821c42dbd23ba53b        refs/pull/93/head
# 207f0fd1f679ce04e85a703189e6dd7ffffca12c        refs/pull/93/merge
# b09b3ed1132983eccb61c4cf32987a28243f95ba        refs/pull/94/head
# 975a404bab2ea2aa3d094f0ffd4db1abef29b049        refs/pull/95/head
# 3335be532798f4bff0373873b37a81495b25a01e        refs/pull/95/merge
# bb932678932a1b3196b06194509019c459ea312d        refs/pull/97/head
# 3d9e70b15559c460bc60e8961dd94cdaca8dc094        refs/pull/99/head
# f7e6ba13dca58bb612f6bba0a6875a5ba53ca84f        refs/tags/0.1
# f8fcbe89c7da9cb491bc3c0cd9377ee8f68d1d0a        refs/tags/0.1^{}
# d7eaa02fb7f31c049279797eb6bcaa183d1a7898        refs/tags/0.1.0
# 7aa7d9943e57c992b3a702c69950125ddefe89f6        refs/tags/0.1.0^{}
# 00e8748ff27d631ebfb01b038c20695b2cba8138        refs/tags/0.2.0
# 2f3b60b9432f7a79cf907df24dd0dbf95a11df2d        refs/tags/0.2.0^{}
# 3ef07e7f38e7c2da73652e2a04ba82f5d654b143        refs/tags/0.2.0.9000
# b095569b69e445fccdb6e9865f10270f38bff4c0        refs/tags/0.2.0.9000^{}
# 88ad2aad380341286fce189be484e075bc60f21e        refs/tags/0.2.1.9000
# 96962306a005e84fe217a19b018bfd64ec433a28        refs/tags/0.2.1.9000^{}
# 7c46d350e1877d857b0586b74aa1274592147c4b        refs/tags/0.3.0.9000
# 2b9d30fcf7e74420b564889d08ee1af3f9518a3a        refs/tags/0.3.0.9000^{}
# a98a234dafbb76a1622ad2a22a8eebdbe5fbae7d        refs/tags/0.3.0.9001
# 0fb087149e45aeeb5f1f23d4a11cff980deb4feb        refs/tags/0.3.0.9001^{}
# 15ca21869654706f44c6d1659822b7d3760a3eec        refs/tags/0.3.0.9002
# 780be8ea5cc9ba9321f8d09f084b6efac55ad17f        refs/tags/0.3.0.9002^{}
# 3e6cb3d4f9d617533f4f6a107e2f05f028299286        refs/tags/0.3.1
# 1df8c3daf61a026565380d59b8ae7a3fe64434b9        refs/tags/v0.2.0

And the DESCRIPTION with
https://raw.githubusercontent.com/jimhester/covr/master/DESCRIPTION

This would let this work without any API requests.

It also gives us a avenue to use the same approach for the other git based install_* functions. We can use git archive --remote=git://github.com/jimhester/lintr.git HEAD:. DESCRIPTION to get just the description from a git repository without cloning the entire repository (GitHub does not support this method however).

Will try to incorporate your comments and work on this some more.

jimhester · 2015-09-03T21:29:00Z

jimhester@98016e4 uses the alternative implementation I discussed above and adds support for git and bitbucket backends.

gitorious seems to have been shutdown / acquired by GitLab (https://gitorious.org/, https://en.wikipedia.org/wiki/Gitorious) so I don't think it is worth or possible to get it working. We may want to deprecate that backend...

I could add support for SVN repositories as well if we stored the revision in the DESCRIPTION metadata, however it looks like we do not ATM.

I think I addressed all of your comments that still applied however the code changed quite a bit so it could use another review.

It also needs some tests, although it seems to work via interactive testing.

hadley · 2015-09-04T11:41:28Z

R/install-git.r

@@ -69,3 +76,44 @@ remote_metadata.git_remote <- function(x, bundle = NULL, source = NULL) {
    RemoteSha = sha
  )
 }
+
+remote_sha.git_remote <- function(remote, ...) {
+  res <- system(


Is it possible to do this with git2r? devtools traditionally hasn't assumed that you have git installed.

ropensci/git2r#130 would seem to suggest no, I can try and hunt down the code in libgit2 and see how much work it would be to add to git2r

ropensci/git2r#172 adds ls_remote() to git2r, which exposes this functionality to us.

This PR also uses git archive to download the file for plain git remotes. I imagine git2r doesn't have this either. I think it is lower priority though as plain git is used less frequently than github or bitbucket and people will likely already have a git client installed in that case. They can always just set force = TRUE to revert to the current behavior of always installing the package.

jimhester · 2015-09-04T15:22:46Z

Ok 72edbbf should now be using git2r::ls_remote(). It is also the first use of Remotes: in the wild, hopefully it works!

jimhester · 2015-09-08T14:10:41Z

2425180 adds tests , note to NEWS and re-based, however it is broken because Travis uses CRAN devtools to install the dependencies, so the Remotes field isn't picked up.

git2r::remotes_ls() which I added to git2r with ropensci/git2r#172 has been merged, but I don't know when @stewid is planning the next CRAN release of git2r, so this PR likely won't make it in the upcoming devtools release anyway.

stewid · 2015-09-08T17:41:02Z

I have not set a date, but git2r v 0.11.0 was released only 27 days ago.

hadley · 2015-11-10T16:56:06Z

@jimhester any updates on this? Now that I'm using caching on travis this would be nice :)

stewid · 2015-11-17T21:26:31Z

@jimhester and @hadley I'm working on a new release ropensci/git2r#191

jimhester · 2016-01-11T20:02:42Z

This should now be ready to merge as well. jimhester@a00cd18 passes without any new NOTES being generated.

hadley · 2016-01-12T00:44:58Z

R/install.r

@@ -45,6 +45,8 @@
 #' @param threads number of concurrent threads to use for installing
 #'   dependencies.
 #'   It defaults to the option \code{"Ncpus"} or \code{1} if unset.
+#' @param force whether to force installation even if the reference hasn't
+#'   changed.


I think this could be reworded to be a bit more clear

I think force might be too generic - does it all to specified packages or their deps or both? Maybe the description of the function also needs to change to emphasize that it never reinstalls packages if already present

The allows you to force install of packages defined in `Remotes:`

jimhester · 2016-01-15T21:59:21Z

@hadley PTAL at this when you can, particularly jimhester@65e4fe5. If you think that looks good this should be ok to merge.

hadley · 2016-01-16T20:38:09Z

R/install-git.r

  if (!missing(args))
    warning("`args` is deprecated", call. = FALSE)

  remotes <- lapply(url, git_remote, subdir = subdir, branch = branch)
+
+  if (!isTRUE(force)) {


I think the logic here is round the wrong way - isFALSE(force) would be better

unfortunately there is no isFALSE function in base R, but I can write one easily enough of course.

Also this begs the question what the definition of isFALSE() should be. isTRUE() is defined as identical(TRUE, x), so anything but TRUE is FALSE. So we do not want identical(FALSE, x) but !identical(TRUE, x), so isFALSE() is a bit of a misnomer.

It is a small difference so I am happy to define isFALSE as !identical(TRUE, x), but it a slightly inaccurate name.

Hmmmm, thinking it through again, I think your original code is correct.

jimhester · 2016-01-19T16:14:07Z

jimhester@ce7869d adds the early return. Let me know if you see anything else that needs to change. If not I think this can be merged.

hadley · 2016-01-19T20:49:34Z

R/install-git.r

+#' @export
+remote_sha.git_remote <- function(remote, ...) {
+  if (!is.null(remote$sha)) {
+    return(remote$sha)


The point of the early return was so you could eliminate the big else block ;)

Wow, I clearly was too tired when I looked at it this morning.

hadley · 2016-01-19T20:50:59Z

LGTM - feel free to merge

Only install from if a different sha from current

hadley reviewed Sep 2, 2015
View reviewed changes

hadley reviewed Sep 4, 2015
View reviewed changes

jimhester force-pushed the github_new branch from 98016e4 to 72edbbf Compare September 4, 2015 15:20

jimhester force-pushed the github_new branch from d9f5d74 to 2425180 Compare September 8, 2015 13:17

jimhester force-pushed the github_new branch from 2425180 to a00cd18 Compare January 11, 2016 19:52

hadley reviewed Jan 12, 2016
View reviewed changes

jimhester added 11 commits January 13, 2016 12:26

Only install from if a different sha from current

b233b05

Minor cleanup

a4cbd2a

Clarify logic

432ae17

SHA caching implementation for github, git and bitbucket

60c6624

Explicitly return NULL

970878b

Use git2r::ls_remote

804f11a

Pass force down to install_dev_remotes

31ea21c

The allows you to force install of packages defined in `Remotes:`

No need for explict return as last statement

921d34e

Use remote_sha rather than querying the GitHub API

7193b1f

remote_ls is now merged into devel git2r

e8e3e21

Add tests

8af12be

jimhester added 3 commits January 13, 2016 12:26

Add note to NEWS.md

fc98a40

Pass force all the way up to install

7409f30

Use force_deps rather than force

65e4fe5

jimhester force-pushed the github_new branch from a00cd18 to 65e4fe5 Compare January 13, 2016 17:46

jimhester mentioned this pull request Jan 14, 2016

Install sha even on local install #1024

Closed

hadley reviewed Jan 16, 2016
View reviewed changes

Early return if remote is defined

ce7869d

hadley reviewed Jan 19, 2016
View reviewed changes

Use tryCatch rather than try

f4da9b9

jimhester added a commit that referenced this pull request Jan 19, 2016

Merge pull request #903 from jimhester/github_new

050c964

Only install from if a different sha from current

jimhester merged commit 050c964 into r-lib:master Jan 19, 2016

gaborcsardi mentioned this pull request Jan 22, 2016

Merge https://github.com/hadley/devtools/pull/903 r-lib/remotes#28

Closed

gaborcsardi mentioned this pull request Nov 4, 2016

WIP: Use "remotes" package #1310

Closed

gaborcsardi mentioned this pull request May 19, 2017

Port features from devtools r-lib/remotes#66

Closed

19 tasks

hadley mentioned this pull request Aug 1, 2017

Plan to transition devtools to use remotes #1555

Closed

19 tasks

tidy-bot mentioned this pull request Aug 2, 2017

Plan to transition devtools to use remotes r-lib/remotes#102

Closed

19 tasks

jimhester mentioned this pull request Feb 21, 2018

install_github always installs even if no change r-lib/remotes#122

Closed

dracodoc mentioned this pull request Mar 13, 2018

install_github does not skip package installation with same SHA in case "ref" is defined #1624

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only install from if a different sha from current #903

Only install from if a different sha from current #903

jimhester commented Aug 25, 2015

hadley Sep 2, 2015

jimhester Sep 2, 2015

hadley Sep 2, 2015

jimhester commented Sep 3, 2015

jimhester commented Sep 3, 2015

hadley Sep 4, 2015

jimhester Sep 4, 2015

jimhester Sep 4, 2015

jimhester commented Sep 4, 2015

jimhester commented Sep 8, 2015

stewid commented Sep 8, 2015

hadley commented Nov 10, 2015

stewid commented Nov 17, 2015

jimhester commented Jan 11, 2016

hadley Jan 12, 2016

hadley Jan 12, 2016

jimhester commented Jan 15, 2016

hadley Jan 16, 2016

jimhester Jan 19, 2016

jimhester Jan 19, 2016

hadley Jan 19, 2016

jimhester commented Jan 19, 2016

hadley Jan 19, 2016

jimhester Jan 19, 2016

hadley commented Jan 19, 2016

Only install from if a different sha from current #903

Only install from if a different sha from current #903

Conversation

jimhester commented Aug 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimhester commented Sep 3, 2015

jimhester commented Sep 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimhester commented Sep 4, 2015

jimhester commented Sep 8, 2015

stewid commented Sep 8, 2015

hadley commented Nov 10, 2015

stewid commented Nov 17, 2015

jimhester commented Jan 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimhester commented Jan 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimhester commented Jan 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hadley commented Jan 19, 2016