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

Add SHA updating logic for github remotes #135

Merged
merged 66 commits into from Aug 8, 2018

Conversation

@jimhester
Copy link
Member

@jimhester jimhester commented Jul 31, 2018

This updates remotes with the logic from the current devel devtools (only for the github remote).

Since pkgman and friends seem to be a ways off it might be worth trying to update remotes further and use that on travis / appveyor etc. The process was somewhat easier than I expected originally.

R/github.R Outdated
tmp <- tempfile()
download(tmp, url, auth_token = github_pat())

rawToChar(base64enc::base64decode(gsub("\\\\n", "", fromJSONFile(tmp)$content)))
Copy link
Member Author

@jimhester jimhester Jul 31, 2018

Choose a reason for hiding this comment

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

This is the current blocker for this, we need to decode the base64 encoded content, so I guess would have to write a R only version of base64decode().

Loading

Copy link
Member Author

@jimhester jimhester Jul 31, 2018

Choose a reason for hiding this comment

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

Update: now have this

Loading

@jimhester jimhester requested a review from gaborcsardi Jul 31, 2018
@yihui
Copy link

@yihui yihui commented Aug 1, 2018

Yeah!! 🎉 🎉 🎉

Loading

@jimhester jimhester force-pushed the devtools-github branch from 2170efa to 23f86b2 Aug 2, 2018
@codecov-io
Copy link

@codecov-io codecov-io commented Aug 2, 2018

Codecov Report

No coverage uploaded for pull request base (master@e78d84e). Click here to learn what that means.
The diff coverage is 73.34%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #135   +/-   ##
=========================================
  Coverage          ?   82.02%           
=========================================
  Files             ?       27           
  Lines             ?     1524           
  Branches          ?        0           
=========================================
  Hits              ?     1250           
  Misses            ?      274           
  Partials          ?        0
Impacted Files Coverage Δ
R/json.R 93.68% <0%> (ø)
R/utils.R 86.61% <100%> (ø)
R/github.R 27.58% <13.63%> (ø)
R/install-github.R 73.64% <14.28%> (ø)
R/install-git.R 20.73% <17.14%> (ø)
R/install-local.R 84% <60%> (ø)
R/install-remote.R 71.21% <65.45%> (ø)
R/deps.R 81.63% <67.67%> (ø)
R/install-svn.R 91.89% <86.84%> (ø)
R/install-bioc.R 89.68% <89.68%> (ø)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e78d84e...d82007f. Read the comment docs.

Loading

@jimhester jimhester force-pushed the devtools-github branch 2 times, most recently from 2d551e7 to fb58c49 Aug 3, 2018
@jimhester
Copy link
Member Author

@jimhester jimhester commented Aug 6, 2018

@gaborcsardi I realize this is probably too big now to review in detail; do you have any overall comments and would you be ok with me merging this and working on the open issues?

Loading

@gaborcsardi
Copy link
Contributor

@gaborcsardi gaborcsardi commented Aug 6, 2018

Loading

Copy link
Contributor

@gaborcsardi gaborcsardi left a comment

Super cool! Left some minor comments. Thanks for doing this.

Loading

R/deps.R Outdated
}
fun <- tryCatch(get(paste0(tolower(type), "_remote"),
envir = asNamespace("remotes"), mode = "function", inherits = FALSE),
error = function(e) stop("Unknown remote type: ", type, call. = FALSE))
Copy link
Contributor

@gaborcsardi gaborcsardi Aug 6, 2018

Choose a reason for hiding this comment

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

IDK, this will find things like parse_one_remote(), etc. Not that this happens in practice, but it does not seem right to me. How about just create an object, try to dispatch, and then fail, if the dispatch fails.

Loading

R/install-git.R Outdated
#'}
install_git <- function(url, subdir = NULL, branch = NULL,
install_git <- function(url, subdir = NULL, ref = NULL,
Copy link
Contributor

@gaborcsardi gaborcsardi Aug 6, 2018

Choose a reason for hiding this comment

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

This is a breaking change. Can we re-add a branch argument and forward that to ref? If you think it is worth it. Only in the user facing function.

Loading

on.exit(unlink(tmp))
description_path <- paste0(collapse = "/", c(remote$subdir, "DESCRIPTION"))

# Try using git archive --remote to retrieve the DESCRIPTION, if the protocol
Copy link
Contributor

@gaborcsardi gaborcsardi Aug 6, 2018

Choose a reason for hiding this comment

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

Seems like this means that even for git2r remotes we need a command line git? So can't we just use that for everything and remove the git2r stuff?

Loading

Copy link
Member Author

@jimhester jimhester Aug 8, 2018

Choose a reason for hiding this comment

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

You need command line git to get the remote package name, but if you can't get it because command line git is not available the thing we then just install the remote unconditionally. So it will still work without command line git, just won't be as efficient as it could be.

I think it is still useful to have both types of remotes, although I agree it does increase the complexity considerably; the code is already written...

Loading

R/install-git.R Outdated
res <- try(silent = TRUE,
system_check(git_path(),
args = c("archive", "-o", tmp, "--remote", remote$url,
if (is.null(remote$ref)) "HEAD" else remote$ref,
Copy link
Contributor

@gaborcsardi gaborcsardi Aug 6, 2018

Choose a reason for hiding this comment

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

This is sometimes HEAD, sometimes master (see below). Which one is right?

Loading

Copy link
Member Author

@jimhester jimhester Aug 8, 2018

Choose a reason for hiding this comment

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

Probably HEAD as it is most generic

Loading

@@ -0,0 +1,138 @@
#' Instlal a package from GitLab
Copy link
Contributor

@gaborcsardi gaborcsardi Aug 6, 2018

Choose a reason for hiding this comment

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

Typo

Loading

local_sha <- local_sha(remote_package_name(remote))
}

same <- remote_sha == local_sha
Copy link
Contributor

@gaborcsardi gaborcsardi Aug 6, 2018

Choose a reason for hiding this comment

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

Do we always have a full sha here? (As opposed to the first 7 characters or sg like that?)

Loading

Copy link
Member Author

@jimhester jimhester Aug 8, 2018

Choose a reason for hiding this comment

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

Yes, IIRC all of the methods return the full untruncated sha.

Loading

package2remote(name)$sha %||% NA_character_
}

package2remote <- function(name, lib = .libPaths(), repos = getOption("repos"), type = getOption("pkgType")) {
Copy link
Contributor

@gaborcsardi gaborcsardi Aug 6, 2018

Choose a reason for hiding this comment

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

This seems like an important internal function. Do you think you could add some brief (internal) docs that explain what it is used for? Thanks!

Loading

Copy link
Member Author

@jimhester jimhester Aug 8, 2018

Choose a reason for hiding this comment

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

You are right, I wonder if it would be useful to export this actually, it can sometimes be handy, e.g. if you want to re-install a package from another library. Although this would also require exporting install_remote().

Loading

}

#' @export
`[.remotes` <- function(x,i,...) {
Copy link
Contributor

@gaborcsardi gaborcsardi Aug 6, 2018

Choose a reason for hiding this comment

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

Why is this trick needed? Can you please add it as internal docs, or a comment?

Loading

Copy link
Member Author

@jimhester jimhester Aug 8, 2018

Choose a reason for hiding this comment

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

It is possible it is not needed, I will remove it and see if anything breaks.

Loading

jimhester added 16 commits Aug 8, 2018
This seems to be due to a regression in roxygen 6.1.0
Mainly to avoid duplicating the protocol if it exists
This is about ~4 times faster than without because we can return just
the SHA by passing the appropriate header

    bench::mark(iterations=30,
        curl = github_commit("r-lib", "devtools", host = "api.github.com", use_curl = TRUE),
        base = github_commit("r-lib", "devtools", host = "api.github.com", use_curl = FALSE))
    # A tibble: 2 x 14
      expression      min     mean   median      max `itr/sec`
      <chr>      <bch:tm> <bch:tm> <bch:tm> <bch:tm>     <dbl>
    1 curl         68.6ms   84.2ms   81.4ms    164ms     11.9
    2 base        261.8ms  316.4ms  284.9ms    615ms      3.16
This was missing in the remote dependencies, which caused rbind's to
fail
It has changed in R-devel
@jimhester jimhester force-pushed the devtools-github branch from 61fe315 to d82007f Aug 8, 2018
@jimhester jimhester merged commit eb81fe6 into r-lib:master Aug 8, 2018
4 checks passed
Loading
@jimhester
Copy link
Member Author

@jimhester jimhester commented Aug 8, 2018

Thank you for the review Gabor, I appreciated the comments!

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants