It adds a method at TRestTools to do http POST requests #172

jgalan · 2022-03-31T06:57:30Z

It adds dependency with libcurl libraries
It adds a static method TRestTools::POSTRequest to do POST requests to a website.
TRestTools::ReadASCIITable added an optional argument to skip header lines.
TRestMetadata::GetSearchPath now adds also REST_USER_PATH as a default.
TRestTools::ExportASCIITable method added to write a file with the contents of std::vector<std::vector>>.

@rest-for-physics/core_dev

jgalan · 2022-03-31T07:03:03Z

@lobis there is no libcurl installed at the docker? It was a library ready at sultan. Could you check in your docker image if it is available? tx

lobis · 2022-03-31T07:20:32Z

@lobis there is no libcurl installed at the docker? It was a library ready at sultan. Could you check in your docker image if it is available? tx

curl is installed, you can check via

docker run -it ghcr.io/lobis/root-geant4-garfield:cpp17_ROOT-v6-26-00_Geant4-v10.4.3_Garfield-af4a1451 curl --help

Perhaps the problem is elsewhere or I have to install some dev dependency @jgalan ?

Also I propose to replace the docker image to ghcr.io/lobis/root-geant4-garfield:rest-for-physics so we don't have to modify it anymore if we updated Garfield, Geant4 or root. This tag will point to a fixed image.

jgalan · 2022-03-31T07:24:38Z

Did you have a look to the pipeline error? It does not find #include <curl/curl.h>

lobis · 2022-03-31T07:27:11Z

This is the correct output, its printing the help menu for curl, so it is installed

lobis · 2022-03-31T07:28:05Z

Did you have a look to the pipeline error? It does not find #include <curl/curl.h>

How can I Installed this? curl is already installed, I cannot find an apt package called libcurl-dev if that's whats missing.

jgalan · 2022-03-31T07:29:34Z

Maybe curl is installed, but not the development libraries. Headers are not found.

sudo apt-get install libcurl-dev

jgalan · 2022-03-31T07:31:10Z

Perhaps curl-devel?

https://stackoverflow.com/questions/11471690/curl-h-no-such-file-or-directory

lobis · 2022-03-31T07:36:02Z

#include <curl/curl.h>

I think it is libcurl4-openssl-dev. It should be installed now.

…can be externally updated

lobis · 2022-03-31T08:37:53Z

Perhaps we could use ROOT built in libraries to perform this request and thus avoid having to link this library to the framework, as not everybody will have it installed. (as we saw its not on Ubuntu 20.04 by default).

https://root-forum.cern.ch/t/http-get-request/16891/3

ROOT can download files via http so I am sure this is possible.

If this is not the case or its too hard, perhaps we could add this header to the external dependencies via cmake?

Also why don't we use http GET with query parameters to retrieve the data instead of POST?

jgalan · 2022-03-31T09:01:32Z

Perhaps we could use ROOT built in libraries to perform this request and thus avoid having to link this library to the framework, as not everybody will have it installed. (as we saw its not on Ubuntu 20.04 by default).

https://root-forum.cern.ch/t/http-get-request/16891/3

Yes, I looked to that root forum thread previously and I tried the w3.C macro they provide without success. After a non-negligible amount of time trying out. Then I realised that this post was written 8 years ago, then I thought maybe the protocols have changed, etc.

ROOT can download files via http so I am sure this is possible.

Yes, likely, and we have our own method TRestTools::DownloadRemoteFile which I find quite convenient, as I find convenient to have TRestTools::POSTRequest.

If this is not the case or its too hard, perhaps we could add this header to the external dependencies via cmake?

I added it to the CMakeLists.txt a bit manually. We could perhaps have a REQUIRED curl in cmake so that the cmake execution fails and the system administrator is aware this library needs to be installed. It is a very popular library. In sultan it was already installed.

Also why don't we use http GET with query parameters to retrieve the data instead of POST?

I dont know, this was suggested by @Vindaar I am just following. Of course, we could have another method TRestTools::GETRequest but I am not sure about the benefits or issues of using one or the other method.

Plz, will you add curl-dev to the docker? :)

lobis · 2022-03-31T09:02:43Z

Plz, will you add curl-dev to the docker? :)

It is added #172 (comment). It isn't working?

lobis · 2022-03-31T09:06:48Z

I dont know, this was suggested by @Vindaar I am just following. Of course, we could have another method TRestTools::GETRequest but I am not sure about the benefits or issues of using one or the other method.

I this case we should use whatever is easier I think. The already existing method uses wget, we should have a single dependency to perform this type of requests, so either move TRestTools::DownloadRemoteFile to use curl, or use wget to perform the post request in this new method. Since it looks like we're already using wget (even though its not an official dependency, we just call it from system but...) I think we shoult not include curl and just use wget

…work into jgalan_tools_post

lobis · 2022-03-31T10:09:56Z

I added this PR #173 which adds cpr as a CMake FetchContent dependency (so user does not have to install anything). I think the library provides a much better syntax for requests. However there is a problem with linking the library... I think its due to the COMPILELIB macro.

Vindaar · 2022-04-01T09:03:44Z

Also why don't we use http GET with query parameters to retrieve the data instead of POST?

My solution is simply reverse engineered by looking at the network communication the browser does when using the tool. Initially I tried a GET with parameters in my code as well, but always got an error from the server. Feel free to try it yourself (i.e. just using curl in the command line), would love to see a simpler solution.

nkx111 · 2022-04-01T09:38:39Z

Why not calling curl from system() as TRestTools::DownloadRemoteFile() does? Then no extra dependency needs to be added.

Vindaar · 2022-04-01T09:52:04Z

Why not calling curl from system() as TRestTools::DownloadRemoteFile() does? Then no extra dependency needs to be added.

While this doesn't add a dependency to REST, it makes the whole functionality more brittle. Makes the code more platform dependent, but more importantly we now have an implicit dependency. While curl is extremely ubiquitous, I don't think it's a good idea to depend on it being installed.

nkx111 · 2022-04-01T10:14:36Z

While curl is extremely ubiquitous, I don't think it's a good idea to depend on it being installed.

I don't quite understand. Isn't that the current implementation including curl header also dependent on curl being installed?

jgalan · 2022-04-01T10:16:33Z

While curl is extremely ubiquitous, I don't think it's a good idea to depend on it being installed.

I don't quite understand. Isn't that the current implementation including curl header also dependent on curl being installed?

No, curl command might be installed/compiled as a binary, but you need the development, usually a separate package curl-dev or curl-devel ...

Strange, no? This didnt happen on Gentoo where you compile everything :)

jgalan · 2022-04-01T10:18:35Z

@Vindaar How it would look like the system call?

Vindaar · 2022-04-01T10:20:20Z

@Vindaar How it would look like the system call?

Sorry, what are you asking?

jgalan · 2022-04-01T10:22:35Z

@Vindaar How it would look like the system call?

Sorry, what are you asking?

Actually, what would be the full curl command that takes in the argument the url and a POST request with key:value arguments, and writes the result to a file just as DownloadRemoteFile does with wget.

… list

Vindaar · 2022-04-01T10:44:27Z

@Vindaar How it would look like the system call?

Sorry, what are you asking?

Actually, what would be the full curl command that takes in the argument the url and a POST request with key:value arguments, and writes the result to a file just as DownloadRemoteFile does with wget.

Ahh, I see.

The POST request:

curl -d "Layer=Au&Ldensity=-1&Thick=250.0&Sigma1=0.0&Substrate=SiO2&Sdensity=-1&Sigma2=0.0&Pol=0&Scan=Energy&Min=7495.0&Max=15000.0&Npts=499&temp=Angle+%28deg%29&Fixed=0.95&Plot=LinLog&Output=Plot" \
-X POST "https://henke.lbl.gov/cgi-bin/laymir.pl"

That returns the HTML. Use your favorite way to extract the HREF from the resulting HTML (if done on the command line xmllint is probably the best choice; probably better done in code using custom parsing. As the HTML is so simple and doesn't change, a find the <a HREF=", then parse until "> is fine).

Then just GET:

curl "https://henke.lbl.gov/cgi-bin/<extracted_link>"

jgalan · 2022-04-01T10:46:06Z

Ok, everyone happy with the system option? @lobis also?

Vindaar · 2022-04-01T10:55:55Z

Ok, everyone happy with the system option? @lobis also?

As I alluded to above, I don't think relying on an installed binary is a good idea. Rather deal with the dependency once at compilation stage of REST and be certain that a compiled REST is able to fetch the files than pray that the dependency is there at runtime and deal with the fallout of that.

But feel free to go with it.

jgalan · 2022-04-01T10:58:35Z

Ok, everyone happy with the system option? @lobis also?

As I alluded to above, I don't think relying on an installed binary is a good idea. Rather deal with the dependency once at compilation stage of REST and be certain that a compiled REST is able to fetch the files than pray that the dependency is there at runtime and deal with the fallout of that.

But feel free to go with it.

Right, I also prefer to deal with the installation of the library if needed at compilation time, for me just a cmake REQUIRED curl would suffice.

It is also true that some time ago I tried to compile GDL the GNU version of IDL, and I gave up because I had to disable 100 dependencies. It is true that a 100 dependencies is not good.

juanangp · 2022-04-01T11:02:16Z

Ok, everyone happy with the system option? @lobis also?

I am personally against any system call. I think we should actually remove any system call from the code. See for instance http://www.cplusplus.com/articles/j3wTURfi/

jgalan · 2022-04-01T11:04:53Z

Ok, so thats the new commit I added, it does not use a system call.

lobis · 2022-04-01T12:22:41Z

I also don't like system calls but I think it's better than relying on adding new user requirements. In my opinion the best would be to use the CPR library installed via cmake fetch content, but there are some bugs I couldn't fix.

Vindaar · 2022-04-01T12:54:19Z

Why not use something like cpp-httplib?

…es with connection

jgalan · 2022-04-01T13:41:28Z

I need the updates at this PR at master, the new methods added are operative and documented, could you please approve and create an issue about upgrading DownloadRemoteFile and POSTRequest methods to avoid using system and curl/curl.h?

From my point of view we will survive till the previous issue is solved.

lobis · 2022-04-01T13:49:09Z

Why not use something like cpp-httplib?

I was suggesting https://github.com/libcpr/cpr but https://github.com/yhirose/cpp-httplib seems also fine.

lobis

TODO: remove libcurl requirement for the user

source/framework/tools/inc/TRestTools.h

source/framework/tools/src/TRestTools.cxx

jgalan · 2022-04-01T16:07:21Z

TODO: remove libcurl requirement for the user

Probably this TODO is lost if not in an issue

jgalan added 2 commits March 31, 2022 01:31

TRestTools::POSTRequest method added

540f0f5

CMakeLists.txt now links to -lcurl

10651a5

jgalan requested review from nkx111 and Vindaar March 31, 2022 06:57

jgalan marked this pull request as draft March 31, 2022 06:58

.gitlab-ci.yml - updated docker image to fixed tag for pipeline that …

433477d

…can be externally updated

jgalan added 2 commits March 31, 2022 11:54

TRestTools::POSTRequest simplified

9215f32

Merge branch 'jgalan_tools_post' of github.com:rest-for-physics/frame…

9cd0713

…work into jgalan_tools_post

TRestTools::POSTRequest adding url and making it c++14 compatible

cd42ce4

jgalan requested review from lobis and a team March 31, 2022 14:33

jgalan marked this pull request as ready for review March 31, 2022 14:33

jgalan added 3 commits April 1, 2022 07:34

TRestTools::ReadASCIITable added an argument to skip header lines

6f7e3f8

Merge branch 'jgalan_tools_minor' into jgalan_tools_post

757731b

TRestTools. Fixing compilation error

c7b7dd5

jgalan added 2 commits April 1, 2022 12:33

TRestMetadata::GetSearchPath. User path has been added to the default…

407dd8a

… list

TRestTools::ExportASCIITable method added

0f5c998

TRestTools::DownloadRemoteFile. Retrying few attempts in case of issu…

5d077d7

…es with connection

lobis approved these changes Apr 1, 2022

View reviewed changes

jgalan requested review from a team and juanangp April 1, 2022 15:01

juanangp reviewed Apr 1, 2022

View reviewed changes

source/framework/tools/inc/TRestTools.h Outdated Show resolved Hide resolved

juanangp reviewed Apr 1, 2022

View reviewed changes

source/framework/tools/src/TRestTools.cxx Outdated Show resolved Hide resolved

jgalan added 2 commits April 1, 2022 17:36

TRestTools::ExportASCIITable table by reference

c7e6e4d

TRestTools::DownloadRemoteFile replacing sleep function

5f53737

juanangp approved these changes Apr 1, 2022

View reviewed changes

jgalan merged commit 1d60c10 into master Apr 1, 2022

jgalan deleted the jgalan_tools_post branch April 1, 2022 16:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

It adds a method at TRestTools to do http POST requests #172

It adds a method at TRestTools to do http POST requests #172

jgalan commented Mar 31, 2022 •

edited by pull-request-badge bot

Loading

jgalan commented Mar 31, 2022

lobis commented Mar 31, 2022

jgalan commented Mar 31, 2022

lobis commented Mar 31, 2022

lobis commented Mar 31, 2022

jgalan commented Mar 31, 2022

jgalan commented Mar 31, 2022

lobis commented Mar 31, 2022 •

edited

Loading

lobis commented Mar 31, 2022 •

edited

Loading

jgalan commented Mar 31, 2022

lobis commented Mar 31, 2022 •

edited

Loading

lobis commented Mar 31, 2022

lobis commented Mar 31, 2022

Vindaar commented Apr 1, 2022 •

edited

Loading

nkx111 commented Apr 1, 2022

Vindaar commented Apr 1, 2022

nkx111 commented Apr 1, 2022

jgalan commented Apr 1, 2022 •

edited

Loading

jgalan commented Apr 1, 2022 •

edited

Loading

Vindaar commented Apr 1, 2022

jgalan commented Apr 1, 2022 •

edited

Loading

Vindaar commented Apr 1, 2022

jgalan commented Apr 1, 2022 •

edited

Loading

Vindaar commented Apr 1, 2022

jgalan commented Apr 1, 2022 •

edited

Loading

juanangp commented Apr 1, 2022

jgalan commented Apr 1, 2022 •

edited

Loading

lobis commented Apr 1, 2022

Vindaar commented Apr 1, 2022

jgalan commented Apr 1, 2022

lobis commented Apr 1, 2022

lobis left a comment

jgalan commented Apr 1, 2022

It adds a method at TRestTools to do http POST requests #172

It adds a method at TRestTools to do http POST requests #172

Conversation

jgalan commented Mar 31, 2022 • edited by pull-request-badge bot Loading

jgalan commented Mar 31, 2022

lobis commented Mar 31, 2022

jgalan commented Mar 31, 2022

lobis commented Mar 31, 2022

lobis commented Mar 31, 2022

jgalan commented Mar 31, 2022

jgalan commented Mar 31, 2022

lobis commented Mar 31, 2022 • edited Loading

lobis commented Mar 31, 2022 • edited Loading

jgalan commented Mar 31, 2022

lobis commented Mar 31, 2022 • edited Loading

lobis commented Mar 31, 2022

lobis commented Mar 31, 2022

Vindaar commented Apr 1, 2022 • edited Loading

nkx111 commented Apr 1, 2022

Vindaar commented Apr 1, 2022

nkx111 commented Apr 1, 2022

jgalan commented Apr 1, 2022 • edited Loading

jgalan commented Apr 1, 2022 • edited Loading

Vindaar commented Apr 1, 2022

jgalan commented Apr 1, 2022 • edited Loading

Vindaar commented Apr 1, 2022

jgalan commented Apr 1, 2022 • edited Loading

Vindaar commented Apr 1, 2022

jgalan commented Apr 1, 2022 • edited Loading

juanangp commented Apr 1, 2022

jgalan commented Apr 1, 2022 • edited Loading

lobis commented Apr 1, 2022

Vindaar commented Apr 1, 2022

jgalan commented Apr 1, 2022

lobis commented Apr 1, 2022

lobis left a comment

Choose a reason for hiding this comment

jgalan commented Apr 1, 2022

jgalan commented Mar 31, 2022 •

edited by pull-request-badge bot

Loading

lobis commented Mar 31, 2022 •

edited

Loading

lobis commented Mar 31, 2022 •

edited

Loading

lobis commented Mar 31, 2022 •

edited

Loading

Vindaar commented Apr 1, 2022 •

edited

Loading

jgalan commented Apr 1, 2022 •

edited

Loading

jgalan commented Apr 1, 2022 •

edited

Loading

jgalan commented Apr 1, 2022 •

edited

Loading

jgalan commented Apr 1, 2022 •

edited

Loading

jgalan commented Apr 1, 2022 •

edited

Loading

jgalan commented Apr 1, 2022 •

edited

Loading