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 option to override the cloud CRAN mirror for installing dependencies #67

Merged
merged 6 commits into from
Jan 31, 2019

Conversation

daroczig
Copy link
Contributor

This adds a new CLI option for overriding the CRAN mirror to be used to download dependencies. Let me know what you think.

@eddelbuettel
Copy link
Owner

My first gut instinct was ... doesn't this come by default via getOption("repos")[["CRAN"]] ? But then I probably don't know remotes::install_github() well enough...

@daroczig
Copy link
Contributor Author

Oh, sure, @CRAN@ is the default in getOption("repos")[["CRAN"]], but I'd like to override that to eg http://eddelbuettel.github.io/drat -- and I have not found an easy way to do that on the command line without temp setting .Rprofile or similar. NOTE: it definitely can be due to my limited knowledge.

@eddelbuettel
Copy link
Owner

Would this help? Default for the last few Debian releases.

But I guess we can add the option.

@eddelbuettel
Copy link
Owner

But I think your current suggested default of @CRAN@ is not a great choice. Why not just use https://cloud.r-project.org ?

@daroczig
Copy link
Contributor Author

Thanks, I've just updated the default value. But if you don't feel comfortable merging this, no worries, I'll do the .Rprofile update to set my own preferred CRAN mirror before calling installGithub.r.

@@ -13,8 +13,9 @@ suppressMessages({
})

## configuration for docopt
doc <- "Usage: installGithub.r [-h] [-x] [-d DEPS] [-u UPDATE] [REPOS...]
doc <- "Usage: installGithub.r [-r DEPREPO...] [-h] [-x] [-d DEPS] [-u UPDATE] [REPOS...]
Copy link
Owner

Choose a reason for hiding this comment

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

And I think the three dots here have special meaning ... which we do not want.

Also, as I have been picking so much, twothree more changes:

  • can we renamed it to CRAN rather than DEPREPO ? Shorter, simpler, obvious default
  • can you place it behind [-u UPDATE] here and below it below?
  • can you add an entry to ChangeLog?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And I think the three dots here have special meaning ... which we do not want.

Yeah, that looked strange to me as well, but that's how it's being used in install2.r and I thought that's part of your coding style :P So I decided to do this for the sake of consistency. Changing it in a sec.

@eddelbuettel
Copy link
Owner

Well if locally setting works then we should do that.

What happens now for you with @CRAN@? Does it fail? Does it stop and ask (which hardly works for littler) ? I am not that opposed to merging. Just want to understand, and keep it simple...

@daroczig
Copy link
Contributor Author

What happens now for you with @cran@? Does it fail? Does it stop and ask (which hardly works for littler) ?

Sure, I've tested before submitting the PR both with @CRAN@ and the CDN mirror and both worked on the command line without trying that interactive menu.

Will do the above changes later today, thanks! I'm not sure regarding the deprepo -> CRAN rename thought, as it could be something else too, eg a drat repo and it's called repos in install.packages and install2.r as well.

@eddelbuettel
Copy link
Owner

Yes, but we use REPOS here for the git repo ... Hm.

@eddelbuettel
Copy link
Owner

So maybe the simpler fix is to have a new variable cranrepo, that by is by getOption() with a default / fallback of the CDN ? The it should be set by default, is overrideable and we don;t clutter things with a new argument. Or I just merge -- let's not overcomplicate...

@daroczig
Copy link
Contributor Author

I'm happy to update the PR as per your suggestion to have options as a default so that folks could rely on that as well -- actually, I was originally thinking about copying over https://github.com/eddelbuettel/littler/blob/master/inst/examples/install2.r#L52 to installGithub.r as well, but then decided that it might overcomplicate things :)

For the sake of consistency, I'd rather keep the option name as repos (as the very same folks are using install2.r and installGithub.r so they do not have to set two separate options for the same stuff), same for the -r CLI option.

The REPOS that stands for the GH repos can be actually renamed to anything else ... without affecting users in any way, so I think that's fine. Actually, I was thinking originally renaming that to GHREPOS :) But did not want to introduce a lot of change here.

Let me push a commit on what I have in mind based on the above and then pls let me know what you think.

@daroczig
Copy link
Contributor Author

Okay, this is definitely more updates than I originally planned, sorry for that, but I hope looks good.

@eddelbuettel
Copy link
Owner

Changing install2.r was not part of the plan.

@eddelbuettel
Copy link
Owner

But I guess on the upside we can now set multiple repos on the command-line....

@eddelbuettel eddelbuettel merged commit 68b58e0 into eddelbuettel:master Jan 31, 2019
@daroczig
Copy link
Contributor Author

Changing install2.r was not part of the plan.

Oh, sorry again, I thought it's better to do these small and non-breaking changes there for the sake of consistency.

But I guess on the upside we can now set multiple repos on the command-line....

I think that was already the case -- it was just not documented and it threw a warning (on comparing multiple values with "NULL") when someone set multiple repos (but worked).

@eddelbuettel
Copy link
Owner

All good -- should be a useful extension. Especially being able to add something like drat on the fly.

Thanks for working on this!

eddelbuettel added a commit that referenced this pull request Feb 1, 2019
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