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

Projects
None yet
3 participants
@cderv
Copy link
Contributor

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

This comment has been minimized.

Copy link

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"

This comment has been minimized.

Copy link
@gaborcsardi

gaborcsardi Nov 27, 2016

Member

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

@gaborcsardi

This comment has been minimized.

Copy link
Member

commented Nov 27, 2016

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

@cderv

This comment has been minimized.

Copy link
Contributor Author

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

4 checks passed

codecov/patch 100% of diff hit (target 93.88%)
Details
codecov/project 93.90% (+0.02%) compared to faba996
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gaborcsardi

This comment has been minimized.

Copy link
Member

commented Dec 1, 2016

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

@cderv

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

commented Dec 6, 2016

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

commented Dec 6, 2016

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

This comment has been minimized.

Copy link
Contributor Author

commented Dec 6, 2016

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

@gaborcsardi

This comment has been minimized.

Copy link
Member

commented Nov 20, 2018

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

Download method respects option, libcurl if aval
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

Download method respects option, libcurl if aval
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

This comment has been minimized.

Copy link
Contributor Author

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
You can’t perform that action at this time.