-
Notifications
You must be signed in to change notification settings - Fork 153
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
allowing git to passthrough for a git external remote as compared to … #679
base: main
Are you sure you want to change the base?
Conversation
Only failure seems to be a windows fail, not due to the code here. |
1 similar comment
Only failure seems to be a windows fail, not due to the code here. |
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 |
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
<#679 (comment)>, 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
|
Why is that? If we set the default of the |
I think the issue is that if you specify |
If we use |
I think I've teased this out some more. The ScenarioYou have a I think I can boil the issue down to this how x = "git::[email protected]:USER/REPO"
remotes:::parse_one_extra(x)
#> $url
#> [1] "[email protected]:USER/REPO"
#>
#> $subdir
#> NULL
#>
#> $ref
#> NULL
#>
#> $credentials
#> NULL
#>
#> attr(,"class")
#> [1] "git2r_remote" "remote" We see this is remotes:::parse_one_extra(x, git = "external")
#> $url
#> [1] "[email protected]: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 Line 72 in eb15a1e
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 HoleHere I just run down where the pass through of Starting with
|
git = c("auto", "git2r", "external"), |
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
):
Line 54 in eb15a1e
install_remotes(remotes, |
install_remote
: Line 91 in eb15a1e
res[[i]] <- install_remote(remotes[[i]], ...), |
(Just a note, not an issue): This calls remote_sha
:
Line 33 in eb15a1e
remote_sha <- remote_sha(remote, local_sha) |
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
):
Line 73 in eb15a1e
install(source, |
install_deps
: Line 22 in eb15a1e
install_deps(pkgdir, dependencies = dependencies, quiet = quiet, |
dev_package_deps
: Line 191 in eb15a1e
packages <- dev_package_deps( |
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::[email protected]:USER/REPO
Remote.
In dev_package_deps
extra_deps
is called:
Line 145 in eb15a1e
res <- do.call(rbind, c(list(res), lapply(get_extra_deps(pkg, dependencies), extra_deps, pkg = pkg), stringsAsFactors = FALSE)) |
Now we have a repo, like
[email protected]:USER/REPO
that is private and only accessible from SSH creds (no PAT
):Line 589 in eb15a1e
extra <- lapply(dev_packages, parse_one_extra) |
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.
To summarize the behavior (with the Minimal change required: Then when you run: All scenarios below are assuming Package uses external and Deps uses externaloptions(remotes.git = "external")
install_git(..., git = "external") Package uses
|
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 |
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 |
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
<#679 (comment)>, 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
|
Just seeing where this stands as we're currently relying on my branch for production |
So should we implement this change or implement a |
May I ask will this be merged? Because I'm having the same problem here:
But when installing the dependency pkgB, |
@fenguoerbian You can probably use |
Thanks for the advice. Finally I asked the admin to install necessary dependencies and re-install |
This passes the
git
argument all the way through on theinstall_remotes
so that if you have any remote dependencies with a prefix ofgit::
and you setgit = "external"
on aninstall*
(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.
Created on 2021-12-14 by the reprex package (v2.0.1)
Session info