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

Change default download method if needed #46

Merged
merged 3 commits into from
Dec 1, 2016

Conversation

cderv
Copy link
Contributor

@cderv cderv commented Nov 27, 2016

This PR follows a discussion to resolve #45 . See issue post.

Based on some research about downloads method in R, I decided to modify download_method function in the following way

if R is older than 3.3, 
    if OS is windows
        set method to "wininet" 
    else on other OS if libcurl is avalaible, 
        set method to "libcurl"

For R 3.3 or newer, set method to "auto" the default
  • Comparing with the previous function, I just reversed the checking (Windows before libcurl) and I rely on default R for newer than 3.3

  • I noticed that for R older than 3.2.0, curl_download is used and dowload_method is not called. So I did not take care of this case.

  • If libcurl is not available on non windows O, we rely on default method

  • I have modified the test to be compliant with the new function, and I have added some infos in Readme about download methods to point users toward download.file function help file.

  • I did not change intall-github.R file as it seems you generated it by brew.

  • I did not add a bullet point in NEWS.md as it seems you have not done it before yet.

We could have done differently

  • Modify default only for https url like in downloader package
  • Mixed base_download and curl_download and deal with all that in download function as it is done in downloader function

Hope it is not too bad and I did not forgot anything
Tell me if something is not clear in what I have done

@codecov-io
Copy link

codecov-io commented Nov 27, 2016

Current coverage is 93.90% (diff: 100%)

Merging #46 into master will increase coverage by 0.02%

@@             master        #46   diff @@
==========================================
  Files            23         23          
  Lines          1030       1034     +4   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            967        971     +4   
  Misses           63         63          
  Partials          0          0          

Powered by Codecov. Last update faba996...227c440

}

"auto"
Copy link
Member

Choose a reason for hiding this comment

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

Can you please remove the return() calls, and put in an else for the "auto"?

@gaborcsardi
Copy link
Member

THanks! Left a small comment, otherwise it looks good.

@cderv
Copy link
Contributor Author

cderv commented Nov 27, 2016

Done. Just a precision : If "libcurl" is not available on non windows platform, method is set to "auto".

Now, not sure what I should do. If you have some advices...
Do I need to squash the three commits into one ?
Will you do this when merging?
Are we staying with three commits ?

I look forward to collaborate more to learn more on the way to go with PR and projects collaborations

@gaborcsardi gaborcsardi merged commit 7435733 into r-lib:master Dec 1, 2016
@gaborcsardi
Copy link
Member

Thanks, and sorry for the delay! You don't need to squash, GH can squash-and-merge now. Thanks again!

@cderv
Copy link
Contributor Author

cderv commented Dec 6, 2016

Hello,

I just noticed that I did not modified install-github.r file that you use for installing the dev version from github without any other package (ie devtools::install_github). I know your working on a new service "install-me" but should we update download_method function in install-github.r until it is available ?
I am not sure how this file is generated.

So, just for you to know, PR is merged and it is working now on windows but

source("https://raw.githubusercontent.com/MangoTheCat/remotes/master/install-github.R")$value("mangothecat/remotes")

is not working without proxy configuration on windows as it has not been updated with the new version of download_method.

@gaborcsardi
Copy link
Member

gaborcsardi commented Dec 6, 2016

Good point. I'll update the script, and the install-github.me service as well.

I should probably decide if I want

source("https://raw.githubusercontent.com/MangoTheCat/remotes/master/install-github.R")$value("mangothecat/remotes")

or

source("https://install-github.me/mangothecat/remotes")

@gaborcsardi
Copy link
Member

I updated the script and the script and the service as well. Can you please try that both commands in the previous comment work? Thanks!

@cderv
Copy link
Contributor Author

cderv commented Dec 6, 2016

I tried and it is working now - both method.

I would have done it but I was not sure if your install-github.r script is generated automatically or if I could update it manually in the different repo.

@gaborcsardi
Copy link
Member

No worries, it is updated like this:

Rscript -e 'brew::brew("install-github.Rin", "install-github.R")'

or the equivalent code from R, with the working directory set to the project root.

And you cannot update the service, because of permissions.

@cderv
Copy link
Contributor Author

cderv commented Dec 6, 2016

OK. good to know how it is working, I was wondering ! Thanks !

@gaborcsardi
Copy link
Member

So, just a note, that I'll change the default method to libcurl, whenever it is available, because it is not possible to send custom headers, and thus do a basic HTTP auth, with wininet.

You'll be still able to use wininet, if you set

options(download.file.method  = "wininet")

gaborcsardi added a commit that referenced this pull request Nov 20, 2018
The default download method now respects the
`download.file.method` user option, if set.

Otherwise, if libcurl is supported, it will be
selected. (Before, wininet was preferred on Windows,
because it sets the proxies automatically, see #46.)
gaborcsardi added a commit that referenced this pull request Nov 20, 2018
The default download method now respects the
`download.file.method` user option, if set.

Otherwise, if libcurl is supported, it will be
selected. (Before, wininet was preferred on Windows,
because it sets the proxies automatically, see #46.)
@cderv
Copy link
Contributor Author

cderv commented Nov 20, 2018

Thank you for the change notice @gaborcsardi ! I appreciate it.

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.

download package on windows behind proxy does not work (easily)
3 participants