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

Fix/deps host #345

Merged
merged 5 commits into from Apr 10, 2019
Merged

Fix/deps host #345

merged 5 commits into from Apr 10, 2019

Conversation

antoine-sachet
Copy link
Contributor

@antoine-sachet antoine-sachet commented Apr 10, 2019

This fixes #337 which currently prevents the installation of a package with dependencies hosted on a different service (e.g. a bitbucket package with github dependencies). It is effectively a revert of #145.

Important note: this will break the installation of private dependencies for people passing auth_token to install_github and expecting it to be passed to the dependencies as well. Which is why I do not expect this PR to be merged (but see next steps below)

Even if we just passed credentials (and not the host) to the dependencies, the install would still break when there is a collision in argument name. Typically, the wrong auth_token would be passed to the github dependency of a gitlab package.

There is however a very simple way to pass credentials to dependencies via environment variables.

Next steps: There is a more elaborate fix. Pass credentials (not host) to the dependency IF AND ONLY IF the dependency host matches the package host. This would not break existing installs so is really a better way to go.

antoine-sachet and others added 4 commits Apr 10, 2019
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.
@jimhester
Copy link
Member

jimhester commented Apr 10, 2019

Thank you for the diagnosis and fix. I actually think we should just merge this as-is for now, as you said the environment variables give people a more robust fallback and the current behavior is just broken with remotes from different sources than the top level install_*() function.

@jimhester jimhester merged commit 06caf54 into r-lib:master Apr 10, 2019
0 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants