Pass all authentication and build arguments to dependencies #145
Conversation
e080620 to
b228918
Compare
We now pass all authentication arguments and build arguments to remotes and dependencies. This means you can supply the authentication in in the top level `install_()` call and have that same authentication be used in downstream Remotes. This also should propagate upgrade to dependencies appropriately, so setting `upgrade = FALSE` should disable upgrading for all dependencies, even grandchildren. Fixes r-lib#53 Fixes r-lib#86 Fixes r-lib#87
b228918 to
ef92888
Compare
Codecov Report
@@ Coverage Diff @@
## master #145 +/- ##
==========================================
- Coverage 92.84% 86.19% -6.66%
==========================================
Files 29 29
Lines 1650 1666 +16
==========================================
- Hits 1532 1436 -96
- Misses 118 230 +112
Continue to review full report at Codecov.
|
5ccaa43 to
d197a70
Compare
d197a70 to
dd7018a
Compare
| build, build_opts) { | ||
| dependencies = FALSE, quiet = NULL) { | ||
|
|
||
| # We want to pass only args that exist in the downstream functions |
There was a problem hiding this comment.
I don't really love this, but I am not sure of a better way to do it, you could list even more arguments here to catch them, but that seems like it would be a pain to maintain as the possible number of arguments in the install_() functions increases.
There was a problem hiding this comment.
Yeah, it would be nice to have some "centralized" solution for this. But it will do here.
There was a problem hiding this comment.
Could we required that all install functions take ... and then use ellipsis to warn if an argument is never evaluated?
There was a problem hiding this comment.
remotes does not have dependencies, by design. ellipsis already has compiled code, and will trigger the DLL locking issue on Windows.
There was a problem hiding this comment.
Oh, oops, I forgot about that constraint.
Really the same issue as r-lib#140
They now get passed all arguments from the parent call, but some of them are not applicable to the remote constructor
dd7018a to
f7ce9a1
Compare
|
@gaborcsardi This could be reviewed now when you have time |
| build, build_opts) { | ||
| dependencies = FALSE, quiet = NULL) { | ||
|
|
||
| # We want to pass only args that exist in the downstream functions |
There was a problem hiding this comment.
Yeah, it would be nice to have some "centralized" solution for this. But it will do here.
|
Thanks for the review! |
| packages, | ||
| repos = repos, | ||
| type = type, | ||
| dependencies = dependencies, |
There was a problem hiding this comment.
Do we need to translate dependencies = TRUE to dependencies = NA to avoid installing all suggested dependencies of dependencies?
There was a problem hiding this comment.
Based on an empirical test, this works fine, at least install_github("r-lib/processx", dependencies = TRUE) only installs Suggests for processx.
Effectively reverts r-lib#145. This will break the install of private dependencies for people who used the auth_token (or equivalent) argument. It is however easy and more robust to pass credentials to the deps via environment variables.
We now pass all authentication arguments and build arguments to remotes
and dependencies. This means you can supply the authentication
in in the top level
install_()call and have that same authenticationbe used in downstream Remotes.
This also should propagate upgrade to dependencies appropriately, so
setting
upgrade = FALSEshould disable upgrading for all dependencies,even grandchildren.
Fixes #86
Fixes #53