muschellij2 commented 2 years ago

This passes the git argument all the way through on the install_remotes so that if you have any remote dependencies with a prefix of git:: and you set git = "external" on an install* (my use case of using SSH keys and no GITHUB_PAT).

Here's a reprex that shows the use case. I can turn into an issue if that is better for tracking.

library(remotes)
library(desc)
tfile = tempfile()
dir.create(tfile)
desc_file = file.path(tfile, "DESCRIPTION")
desc = desc::description$new("!new")
desc$set("Package", "temporary")
desc$del("Maintainer")
desc$set_dep(package = "stringr", type = "Imports", version = ">= 1.0.0")
desc$add_remotes("git::git@github.com:hadley/stringr")
desc$write(desc_file)

x = sessioninfo::session_info("stringr")$package
#> Warning in system("timedatectl", intern = TRUE): running command 'timedatectl'
#> had status 1
src = x$source[x$package == "stringr"]
src = sub(".*\\((.*)\\)", "\\1", src)
remove.packages("stringr")
#> Removing package from '/home/jupyter/.R/library'
#> (as 'lib' is unspecified)
remotes::install_local(path = tfile, git = "external")
#> stringr (NA -> 5f48256b5...) [Git]
#> Downloading git repo git@github.com:hadley/stringr
#> '/usr/bin/git' clone --depth 1 --no-hardlinks git@github.com:hadley/stringr /tmp/RtmpkfNJg9/file3f4d5354a09
#> 
#> * checking for file ‘/tmp/RtmpkfNJg9/file3f4d5354a09/DESCRIPTION’ ... OK
#> * preparing ‘stringr’:
#> * checking DESCRIPTION meta-information ... OK
#> * checking for LF line-endings in source and make files and shell scripts
#> * checking for empty or unneeded directories
#> * building ‘stringr_1.4.0.9000.tar.gz’
#> Installing package into '/home/jupyter/.R/library'
#> (as 'lib' is unspecified)
#> Skipping install of 'stringr' from a xgit remote, the SHA1 (5f48256b) has not changed since last install.
#>   Use `force = TRUE` to force installation
#> * checking for file ‘/tmp/RtmpkfNJg9/file3f4d70f9acd3/file3f4d650bbaaa/DESCRIPTION’ ... OK
#> * preparing ‘temporary’:
#> * checking DESCRIPTION meta-information ... OK
#> * checking for LF line-endings in source and make files and shell scripts
#> * checking for empty or unneeded directories
#> * creating default NAMESPACE file
#> * building ‘temporary_1.0.0.tar.gz’
#> Installing package into '/home/jupyter/.R/library'
#> (as 'lib' is unspecified)
remove.packages("temporary")
#> Removing package from '/home/jupyter/.R/library'
#> (as 'lib' is unspecified)
remotes::install_local(path = tfile)
#> Error: Failed to install 'temporary' from local:
#>   Error in 'git2r_remote_ls': error authenticating:
remove.packages("temporary")
#> Removing package from '/home/jupyter/.R/library'
#> (as 'lib' is unspecified)
#> Error in find.package(pkgs, lib): there is no package called 'temporary'
remotes::install_git(src, upgrade = FALSE, git = "external")
#> Downloading git repo git@github.com:tidyverse/stringr.git
#> '/usr/bin/git' clone --depth 1 --no-hardlinks git@github.com:tidyverse/stringr.git /tmp/RtmpkfNJg9/file3f4d64e1397c
#> '/usr/bin/git' fetch origin dd909b714b20ff3add2eedb0a1c917ba6938e40e
#> '/usr/bin/git' checkout FETCH_HEAD
#> * checking for file ‘/tmp/RtmpkfNJg9/file3f4d64e1397c/DESCRIPTION’ ... OK
#> * preparing ‘stringr’:
#> * checking DESCRIPTION meta-information ... OK
#> * checking for LF line-endings in source and make files and shell scripts
#> * checking for empty or unneeded directories
#> * building ‘stringr_1.4.0.9000.tar.gz’
#> Installing package into '/home/jupyter/.R/library'
#> (as 'lib' is unspecified)

res = dev_package_deps(pkgdir = tfile)
#> Error in git2r::remote_ls(remote$url, credentials = remote$credentials): Error in 'git2r_remote_ls': error authenticating:
res
#> Error in eval(expr, envir, enclos): object 'res' not found
res = dev_package_deps(pkgdir = tfile, git = "external")
res
#> Needs update -----------------------------
#>  package installed    available    is_cran remote
#>  stringr dd909b714... 5f48256b5... FALSE   Git

^{Created on 2021-12-14 by the reprex package (v2.0.1)}

Session info

``` r sessioninfo::session_info() #> ─ Session info ─────────────────────────────────────────────────────────────── #> setting value #> version R version 4.1.2 (2021-11-01) #> os Debian GNU/Linux 10 (buster) #> system x86_64, linux-gnu #> ui X11 #> language (EN) #> collate C.UTF-8 #> ctype C.UTF-8 #> tz Etc/UTC #> date 2021-12-14 #> pandoc 2.14.0.2 @ /opt/conda/bin/ (via rmarkdown) #> #> ─ Packages ─────────────────────────────────────────────────────────────────── #> package * version date (UTC) lib source #> callr 3.7.0 2021-04-20 [2] CRAN (R 4.1.0) #> cli 3.1.0.9000 2021-12-07 [1] Github (r-lib/cli@aaf7cf1) #> crayon 1.4.2 2021-10-29 [1] CRAN (R 4.1.0) #> curl 4.3.2 2021-06-23 [1] CRAN (R 4.1.0) #> desc * 1.4.0 2021-09-28 [1] CRAN (R 4.1.0) #> digest 0.6.29 2021-12-01 [1] CRAN (R 4.1.2) #> evaluate 0.14 2019-05-28 [2] CRAN (R 4.1.0) #> fastmap 1.1.0 2021-01-25 [2] CRAN (R 4.1.0) #> fs 1.5.2 2021-12-08 [1] CRAN (R 4.1.2) #> git2r 0.29.0 2021-11-22 [1] CRAN (R 4.1.0) #> glue 1.5.1 2021-11-30 [1] CRAN (R 4.1.0) #> highr 0.9 2021-04-16 [2] CRAN (R 4.1.0) #> htmltools 0.5.2 2021-08-25 [1] CRAN (R 4.1.0) #> knitr 1.36 2021-09-29 [1] CRAN (R 4.1.0) #> lifecycle 1.0.1 2021-09-24 [1] CRAN (R 4.1.0) #> magrittr 2.0.1 2020-11-17 [2] CRAN (R 4.1.0) #> pkgbuild 1.3.0 2021-12-09 [1] CRAN (R 4.1.2) #> prettyunits 1.1.1 2020-01-24 [2] CRAN (R 4.1.0) #> processx 3.5.2 2021-04-30 [2] CRAN (R 4.1.0) #> ps 1.6.0 2021-02-28 [2] CRAN (R 4.1.0) #> R6 2.5.1 2021-08-19 [1] CRAN (R 4.1.0) #> remotes * 2.4.1.9000 2021-12-14 [1] local #> reprex 2.0.1 2021-08-05 [1] CRAN (R 4.1.0) #> rlang 0.4.12 2021-10-18 [1] CRAN (R 4.1.0) #> rmarkdown 2.11 2021-09-14 [1] CRAN (R 4.1.0) #> rprojroot 2.0.2 2020-11-15 [2] CRAN (R 4.1.0) #> rstudioapi 0.13 2020-11-12 [2] CRAN (R 4.1.0) #> sessioninfo 1.2.2 2021-12-06 [1] CRAN (R 4.1.2) #> stringi 1.7.6 2021-11-29 [1] CRAN (R 4.1.0) #> stringr 1.4.0.9000 2021-12-14 [1] xgit (git@github.com:tidyverse/stringr.git@dd909b7) #> vctrs 0.3.8 2021-04-29 [2] CRAN (R 4.1.0) #> withr 2.4.3 2021-11-30 [1] CRAN (R 4.1.0) #> xfun 0.28 2021-11-04 [1] CRAN (R 4.1.0) #> yaml 2.2.1 2020-02-01 [2] CRAN (R 4.1.0) #> #> [1] /home/jupyter/.R/library #> [2] /usr/local/lib/R/site-library #> [3] /usr/lib/R/site-library #> [4] /usr/lib/R/library #> #> ────────────────────────────────────────────────────────────────────────────── ```

muschellij2 commented 2 years ago

Only failure seems to be a windows fail, not due to the code here.

muschellij2 commented 2 years ago

Only failure seems to be a windows fail, not due to the code here.

gaborcsardi commented 2 years ago

Thanks! I wonder if using an option + env var would make more sense in this case, instead of pushing this argument through everywhere, which is somewhat error prone if we also pass ... around.

muschellij2 commented 2 years ago

I think that would work better in that case but this seems to pass and didn’t raise any flags, but it’s hard to know if you’ve gone down every … rabbit hole. Without the pass through though, some of my installs fail if I don’t add a PAT in addition to the SSH key.

On Wed, Feb 2, 2022 at 3:37 AM Gábor Csárdi @.***> wrote:

Thanks! I wonder if using an option + env var would make more sense in this case, instead of pushing this argument through everywhere, which is somewhat error prone if we also pass ... around.

— Reply to this email directly, view it on GitHub https://github.com/r-lib/remotes/pull/679#issuecomment-1027700320, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIGPLRJIIMJJQBCBJFDIX3UZDUL5ANCNFSM5J7X2NWQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you authored the thread.Message ID: @.***>

-- Best, John

gaborcsardi commented 2 years ago

Without the pass through though, some of my installs fail if I don’t add a PAT in addition to the SSH key.

Why is that? If we set the default of the git argument to getOption("remotes.git", "auto") everywhere, that should work, no?

muschellij2 commented 2 years ago

I think the issue is that if you specify git in getOption, the passthrough may still be needed. The issue is that install_deps has a ... but git is not passed to dev_package_deps. If you specify git in a passthrough to dev_package_deps, it then must pass to extra_deps, which then passes to parse_one_extra. If you create a option for this, it may work, but I'm not sure if that will be sufficient.

gaborcsardi commented 2 years ago

If we use getOption() every time git is an argument, and every time we need to choose between command like git and the other one, I don't see how it would be possible that it is not used? Maybe I am missing something, though.

muschellij2 commented 2 years ago

I think I've teased this out some more. The getOption solution will work, but can have a mismatch between the option and what executes depending on specifying git in the arguments:

Scenario

You have a DESCRIPTION file for a package A with Remotes: git::git@github.com:USER/REPO. You then install package A using install_git(..., git = "external") and I would like to install USER/REPO using external git as well.

I think I can boil the issue down to this how remotes:::parse_one_extra works by default without a git argument

x = "git::git@github.com:USER/REPO"
remotes:::parse_one_extra(x)
#> $url
#> [1] "git@github.com:USER/REPO"
#> 
#> $subdir
#> NULL
#> 
#> $ref
#> NULL
#> 
#> $credentials
#> NULL
#> 
#> attr(,"class")
#> [1] "git2r_remote" "remote"

We see this is git2r_remote (as git2r is installed):

remotes:::parse_one_extra(x, git = "external")
#> $url
#> [1] "git@github.com:USER/REPO"
#> 
#> $subdir
#> NULL
#> 
#> $ref
#> NULL
#> 
#> attr(,"class")
#> [1] "xgit_remote" "remote"

^{Created on 2022-02-03 by the reprex package (v2.0.1)}

This all happens because remotes:::git_remote takes in git: https://github.com/r-lib/remotes/blob/eb15a1eb21cca47ce5601747fd4e033d8606a648/R/install-git.R#L72 If you set that option to getOption, I think this behavior would be resolved for me. The only issue with this is that you specify git in install_git and that may not be preserved/the same in remotes:::git_remote (and therefore parse_one_extra) depending on the mismatch of the default option vs. explicit git argument, which I'm not sure is an "issue" vs. just the behavior.

Rabbit Hole

Here I just run down where the pass through of git or ... is working for my proposed solution.

Starting with `install_git`

Let's say you're using install_git with git = "external": https://github.com/r-lib/remotes/blob/eb15a1eb21cca47ce5601747fd4e033d8606a648/R/install-git.R#L34. Changing that to getOption("remotes.git", "auto") would be fine, but if I pass in "external" directly, then let's see where it goes.

`install_remote`

Goes to install_remotes (this doesn't currently pass git): https://github.com/r-lib/remotes/blob/eb15a1eb21cca47ce5601747fd4e033d8606a648/R/install-git.R#L54, then install_remote: https://github.com/r-lib/remotes/blob/eb15a1eb21cca47ce5601747fd4e033d8606a648/R/install-remote.R#L91.

(Just a note, not an issue): This calls remote_sha: https://github.com/r-lib/remotes/blob/eb15a1eb21cca47ce5601747fd4e033d8606a648/R/install-remote.R#L33, but since git = "external" was given, then we know it's a xgit remote. So this preserves git option because of how the remote is created.

`install`

Then we hit the standard install (again, no git passed from install_remotes): https://github.com/r-lib/remotes/blob/eb15a1eb21cca47ce5601747fd4e033d8606a648/R/install-remote.R#L73, which passes to install_deps: https://github.com/r-lib/remotes/blob/eb15a1eb21cca47ce5601747fd4e033d8606a648/R/install.R#L22, which passes to dev_package_deps: https://github.com/r-lib/remotes/blob/eb15a1eb21cca47ce5601747fd4e033d8606a648/R/install.R#L191 Here, dev_package_deps takes only set arguments (no ...).

`dev_package_deps`

In dev_package_deps we see how the default git or would affect the installation of the package in the git::git@github.com:USER/REPO Remote.

In dev_package_deps extra_deps is called: https://github.com/r-lib/remotes/blob/eb15a1eb21cca47ce5601747fd4e033d8606a648/R/deps.R#L145 Now we have a repo, like git@github.com:USER/REPO that is private and only accessible from SSH creds (no PAT): https://github.com/r-lib/remotes/blob/eb15a1eb21cca47ce5601747fd4e033d8606a648/R/deps.R#L589

Now we see without git in parse_one_extra, it will call git_remote and use the default "auto", which forces git2r_remote, which is where we started above. So we can run install_git(..., git = "external"), but depending on getOption("remotes.git", "auto"), we would have different "gits". This isn't an issue really (sometimes you may want to install this git via external git but all deps via git2r, but I'm not sure.

muschellij2 commented 2 years ago

To summarize the behavior (with the getOption("remotes.git", "auto") proposed solution):

Minimal change required: git_remote takes in getOption("remotes.git", "auto").

Then when you run: install_git(..., git = "external"), the repos passed here will be installed via external git, but the dependencies are installed via git2r (assuming git2r installed).

All scenarios below are assuming git2r is installed

Package uses external and Deps uses external

options(remotes.git = "external")
install_git(..., git = "external")

Package uses `git2r` and Deps uses external

options(remotes.git = "external")
install_git(..., git = "auto")

Package uses external and Deps uses `git2r` (current behavior)

options(remotes.git = "auto")
install_git(..., git = "external")

Package `git2r` and Deps `git2r`

options(remotes.git = "auto")
install_git(..., git = "auto")

muschellij2 commented 2 years ago

Let me know which way you'd like me to go with this. I think the passthrough is fine, but may be harder to maintain, but the above issues exist for the getOption proposal.

gaborcsardi commented 2 years ago

Yeah, I agree that this is not ideal. OTOH if you are always using xgit or git2r, then you just need to set the option, no? So I am kind of OK with the dependencies not following the argument.

Here is another idea, though. At the beginning of the functions that take the git argument, we set the option, and it will be valid until on.exit(). This solves our issues I believe. But it does seem like a bit of an over-engineered solution.

muschellij2 commented 2 years ago

That works as well. Either way, I’m just looking for something that will work with external git only, even if git2r is installed. And I think I’ve found all the pass throughs, but would like a solution that’s easier for you to maintain while keeping functionality. Could be a getOption + on.exit

On Thu, Feb 10, 2022 at 2:28 AM Gábor Csárdi @.***> wrote:

Yeah, I agree that this is not ideal. OTOH if you are always using xgit or git2r, then you just need to set the option, no? So I am kind of OK with the dependencies not following the argument.

Here is another idea, though. At the beginning of the functions that take the git argument, we set the option, and it will be valid until on.exit(). This solves our issues I believe. But it does seem like a bit of an over-engineered solution.

— Reply to this email directly, view it on GitHub https://github.com/r-lib/remotes/pull/679#issuecomment-1034578649, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIGPLWQCF5FT33665T57MDU2NSIVANCNFSM5J7X2NWQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you authored the thread.Message ID: @.***>

-- Best, John

muschellij2 commented 2 years ago

Just seeing where this stands as we're currently relying on my branch for production

muschellij2 commented 2 years ago

So should we implement this change or implement a remotes.git option? The reason I ask is without this change, we cannot install our private git packages correctly.

fenguoerbian commented 1 year ago

May I ask will this be merged? Because I'm having the same problem here: I want to install pkgA in a private repo, which depends on pkgB in another private repo. The git repo to pkgB is presented in remotes filed of pkgA's DESCRIPTION. In our rstudio server, git2r is installed without ssh feature, so when installing pkgA I'm using

remotes::install_git("ssh://git@url_to_pkgA.git", git = "external")

But when installing the dependency pkgB, remotes uses git2r then fails the installation.

gaborcsardi commented 1 year ago

@fenguoerbian You can probably use pak::pkg_install() instead of remotes.

fenguoerbian commented 1 year ago

@fenguoerbian You can probably use pak::pkg_install() instead of remotes.

Thanks for the advice. Finally I asked the admin to install necessary dependencies and re-install git2r then everything works fine.

r-lib / remotes

allowing git to passthrough for a git external remote as compared to … #679

Scenario

Rabbit Hole

Starting with `install_git`

`install_remote`

`install`

`dev_package_deps`

Package uses external and Deps uses external

Package uses `git2r` and Deps uses external

Package uses external and Deps uses `git2r` (current behavior)

Package `git2r` and Deps `git2r`

r-lib / remotes

allowing git to passthrough for a git external remote as compared to … #679

Scenario

Rabbit Hole

Starting with install_git

install_remote

install

dev_package_deps

Package uses external and Deps uses external

Package uses git2r and Deps uses external

Package uses external and Deps uses git2r (current behavior)

Package git2r and Deps git2r

Starting with `install_git`

`install_remote`

`install`

`dev_package_deps`

Package uses `git2r` and Deps uses external

Package uses external and Deps uses `git2r` (current behavior)

Package `git2r` and Deps `git2r`