change calls from RCurl to curl package #2786

moki1202 · 2021-03-14T07:52:38Z

No description provided.

don't know what to enter in the "type" argument and I am still not sure that form_file is the ideal function to replace the POSTFORM function.

moki1202 · 2021-03-14T07:54:13Z

@infotroph tried my best to update convert.input file.although I am not at all sure that form_file would be an ideal function to replace POSTFORM function. hhtr::POST might be the ideal choice as you said.

infotroph

I've only looked at convert_input.R so far, but posting this to save my comments and will come back to it as soon as I have time. Thanks for your patience.

infotroph · 2021-03-19T09:08:30Z

base/utils/R/convert.input.R

-    out.html <- RCurl::getURL(paste0("http://dap-dev.ncsa.illinois.edu:8184/inputs/",
-                              browndog$inputtype), .opts = curloptions)
+    out.html <- curl::curl_download(paste0("http://dap-dev.ncsa.illinois.edu:8184/inputs/",
+                              browndog$inputtype), curloptions)


I have not tested this yet, but it looks correct 👍

On testing: curl_download writes to disk rather than returning a string. Probably want readLines(curl::curl(..., handle = hdl)), with curl options set beforehand via handle_setopt(hdl, ...)

infotroph · 2021-03-19T09:16:30Z

base/utils/R/convert.input.R


    # post zipped file to Brown Dog
-    html <- RCurl::postForm(url, fileData = RCurl::fileUpload(zipfile), .opts = curloptions)
+    html <- curl::form_file(url, type = "___")


It looks like the recommended pattern for form posting is to build up the multipart request inside the handle, then pass the whole handle as a fetch. See this example in the curl vignette.

@infotroph thanks for the review ,I'll start working on the other files .

infotroph · 2021-03-20T08:28:41Z

base/utils/R/download.url.R

  dir.create(basename(file), recursive = TRUE)
  count <- 0
-  while (!RCurl::url.exists(url, .opts = .opts) && count < timeout) {
+  while (!curl::curl_fetch_memory(url, handle = curl::new_handle(nobody = 1)) && count < timeout) {


I see two subtleties here:

When the url doesn't exist, curl_fetch memory will still return a value that evaluates to TRUE, so we need to explicitly check the status code (e.g. (curl_fetch_memory(...)$status_code %/% 200) == 1)

More trickily, when the requested hostname can't be resolved curl_fetch_memory will throw an error whereas RCurl::url.exists just returns FALSE. I'm not sure whether we need to handle this case or not but will ask around.

@infotroph should we be using httr:HEAD then?

Looks like httr::HEAD has the same behavior both with respect to status codes and with respect to erroring on unresolvable hostnames, so we don't need to switch because of those. However HEAD definitely shows the intention much clearly than the nobody = 1 incantation does, so it'd be worth using at least in packages that already import httr.

infotroph · 2021-03-20T08:29:56Z

base/utils/R/download.url.R

+  f <- open(file, mode = "wb")
+  curl::curl_download(url, file, handle = curl::new_handle(.list = .opts))
+  close(f)


Suggested change

f <- open(file, mode = "wb")

curl::curl_download(url, file, handle = curl::new_handle(.list = .opts))

close(f)

curl::curl_download(url, file, handle = curl::new_handle(.list = .opts))

curl_download handles the opening and closing for us

infotroph · 2021-03-20T08:34:56Z

modules/data.atmosphere/R/met.process.R


  userpass <- paste(browndog$username, browndog$password, sep = ":")
  curloptions <- list(userpwd = userpass, httpauth = 1L, followlocation = TRUE)
  result <- RCurl::postForm(paste0(browndog$url, formatname, "/"),


I'm guessing this is intentionally incomplete for now and will use the same pattern you develop in convert.input, but here's a placeholder to remind us to finish it :)

infotroph · 2021-03-20T08:51:10Z

Input requested: This PR will alter PEcAn.utils::download.url by replacing the call to RCurl::url.exists with a header request performed via curl::curl_fetch_memory, but these behave differently in cases where the host isn't resolvable: url.exists returns FALSE, curl throws an error.

paging @robkooper as architect and @serbinsh as user with an ongoing troublesome retry-the-URL use case: Does download.url need to handle temporary DNS failures, or is it strictly intended to wait for an otherwise-accessible host who is taking a while to prepare a particular file?

[comment edited: Tagged the wrong Kooper.]

moki1202 · 2021-03-21T16:27:55Z

out<-RCurl::curlPerform(url = "https://SDMDataAccess.nrcs.usda.gov/Tabular/SDMTabularService.asmx", httpheader = headerFields, postfields = body, writefunction = reader$update
@infotroph cant really figure out the transformation for this part. can you once explain these arguments (httpheader, writefunction, postfields)?

mdietze · 2022-03-24T19:35:33Z

Similar to comments on other PRs, was curious what's left to do on this one @moki1202 @infotroph

infotroph · 2022-03-25T09:49:20Z

@mdietze @moki1202

I tested this locally and discovered I gave some bad advice in the comments above, so I'm fixing in a local branch and will push ASAP.
Branch moki1202/patch-8 has been deleted and getting it back will be a hassle, so I'll open a replacement PR and close this one once it's open.

moki1202 · 2022-03-25T10:12:19Z

@mdietze @moki1202

I tested this locally and discovered I gave some bad advice in the comments above, so I'm fixing in a local branch and will push ASAP.

Branch moki1202/patch-8 has been deleted and getting it back will be a hassle, so I'll open a replacement PR and close this one once it's open.

@infotroph Apologies from my side. Don't know why I deleted this branch. Might have been early days. Let me know how I can help to complete this.

infotroph · 2022-08-25T04:43:24Z

Continuing work on this in PR #2944; closing this one

infotroph

Apparently LAST YEAR I left a draft review unfinished, including some comments that would have been useful in later conversation. Posting now so they're visible to everyone else, with my apologies.

infotroph · 2021-03-22T22:52:10Z

base/utils/R/convert.input.R

-    html <- RCurl::postForm(url, fileData = RCurl::fileUpload(zipfile), .opts = curloptions)
+    h <- new_handle()
+    handle_setform(h,
+    fileData = curl::form_file(curl::curl_upload(zipfile, url))


I'm still getting up to speed on this, but think this line will just be

Suggested change

fileData = curl::form_file(curl::curl_upload(zipfile, url))

fileData = curl::form_file(zipfile)

(I was wrong about this... see #2944)

infotroph · 2022-02-15T23:45:45Z

base/utils/R/convert.input.R

-    out.html <- RCurl::getURL(paste0("http://dap-dev.ncsa.illinois.edu:8184/inputs/",
-                              browndog$inputtype), .opts = curloptions)
+    out.html <- curl::curl_download(paste0("http://dap-dev.ncsa.illinois.edu:8184/inputs/",
+                              browndog$inputtype), curloptions)


On testing: curl_download writes to disk rather than returning a string. Probably want readLines(curl::curl(..., handle = hdl)), with curl options set beforehand via handle_setopt(hdl, ...)

Update convert.input.R

9a30dcc

don't know what to enter in the "type" argument and I am still not sure that form_file is the ideal function to replace the POSTFORM function.

moki1202 added 2 commits March 14, 2021 13:58

Update download.url.R

57290cd

Update met.process.R

9c86b3f

infotroph requested changes Mar 19, 2021

View reviewed changes

infotroph reviewed Mar 20, 2021

View reviewed changes

moki1202 added 10 commits March 21, 2021 00:37

Update download.NLDAS.R

976a155

Update download.GLDAS.R

675bcfa

Update extract_soil_nc.R

6dea9ad

Update LandTrendr.AGB.R

eb977af

Update land.utils.R

b172a68

Update salix_benchmarks.Rmd

15185f1

Update convert.input.R

55dccd8

Update convert.input.R

4c3ccdf

Update browndog.R

95b8dc7

Update met.process.R

ab9cbca

moki1202 added 10 commits March 21, 2021 22:00

Update DESCRIPTION

4a9f382

Update abbreviated_workflow_SIPNET.R

fbcb2e5

Update workflow.bm.R

eeb787c

Update workflow.pda.R

cc00f84

Update workflow.wcr.assim.R

48c31f9

Update workflow.R

dd18db2

Update DESCRIPTION

53dcf86

Update DESCRIPTION

2b50e5f

Update pecan.depends.R

8fc0f5e

Update running_maat_in_pecan.Rmd

40ea64d

moki1202 and others added 9 commits March 21, 2021 22:11

Update graph_fluxtowers.R

7864b50

Update graph_SDA_fluxtowers.R

7789db7

Update workflow.template.R

6c3df94

Update DESCRIPTION

6110737

Update DESCRIPTION

a0b835c

Update LoadFLUXNETsites.R

8c64719

Update LoadPalEONsites.R

4bc6721

Merge branch 'develop' into patch-8

f689ce5

Merge branch 'develop' into patch-8

8795d84

infotroph marked this pull request as draft October 19, 2021 09:51

infotroph mentioned this pull request Jun 23, 2022

Replace {RCurl} with {curl} #2944

Merged

13 tasks

infotroph closed this Aug 25, 2022

infotroph reviewed Aug 25, 2022

View reviewed changes

	fileData = curl::form_file(curl::curl_upload(zipfile, url))
	fileData = curl::form_file(zipfile)

change calls from RCurl to curl package #2786

change calls from RCurl to curl package #2786

Uh oh!

Conversation

moki1202 commented Mar 14, 2021

Uh oh!

moki1202 commented Mar 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

infotroph left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

infotroph commented Mar 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

moki1202 commented Mar 21, 2021

Uh oh!

mdietze commented Mar 24, 2022

Uh oh!

infotroph commented Mar 25, 2022

Uh oh!

moki1202 commented Mar 25, 2022

Uh oh!

infotroph commented Aug 25, 2022

Uh oh!

infotroph left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

moki1202 commented Mar 14, 2021 •

edited

Loading

infotroph commented Mar 20, 2021 •

edited

Loading