bump cpp-httplib version to v0.7.6 latest by breznak · Pull Request #883 · htm-community/htm.core

breznak · 2020-09-07T13:22:24Z

hoping it might resolve issue in #783

breznak · 2020-09-07T13:25:00Z

Locally, I'm getting sign-compare errors in the lib, which we don't want to fix directly.

[ 57%] Building CXX object src/test/CMakeFiles/unit_tests.dir/unit/regions/SPRegionTest.cpp.o
In file included from /mnt/store/devel/HTM/htm-community/nupic.cpp/src/test/unit/engine/RESTapiTest.cpp:34:
/mnt/store/devel/HTM/htm-community/nupic.cpp/build/ThirdParty/cpp-httplib/cpp-httplib-src/httplib.h: In function ‘bool httplib::detail::contains_header(const string&, const string&)’:
/mnt/store/devel/HTM/htm-community/nupic.cpp/build/ThirdParty/cpp-httplib/cpp-httplib-src/httplib.h:2840:23: error: comparison of integer expressions of different signedness: ‘int’ and ‘std::__cxx11::basic_string::size_type’ {aka ‘long unsigned int’} [-Werror=sign-compare]
2840 | for (int i = 0; i < name.length(); ++i) {

Is the -isystem include broken again?

dkeeney

Yes, this looks good.

breznak · 2020-09-07T13:28:22Z

https://stackoverflow.com/questions/3371127/use-isystem-instead-of-i-with-cmake

but I'm not sure where we should apply it (ideally all externals use SYSTEM include)

Zbysekz · 2020-09-07T13:29:02Z

Update on 0.7.6 didn't solve the issue on my PC, test keeps failing.

Note: I have to correct the signess in the for loop in the library header & comment client->set_timeout_sec(30); everywhere on our side since they changed this function

breznak · 2020-09-07T13:34:16Z

Note: I have to correct the signess in the for loop in the library header & comment client->set_timeout_sec(30); everywhere on our side since they changed this function

thanks for reporting. sad it didn't resolve the issue. Do we want to go through updating our codebase to this anyway?

dkeeney · 2020-09-07T22:30:30Z

Oh, I think I see it...
This error for the sign compare occurs where we compile the client and the server for the RESTapi. It is not part of the htm.core library. On line 450 and 464 of src/CMakeLists.txt, I did not specify SYSTEM for EXTERNAL_INCLUDES.

Also line 432 for the MNIST and other places.

We could catch all of them in one place by including SYSTEM at the beginning of the list at the point where the EXTERNAL_INCLUDES list is built which is line 93 of extern/bootstrap.cmake. If we do that, the SYSTEM at line 249 of src/CMakeLists.txt becomes redundant so that could be removed.

I have not tried this but it should work.

in target_include_dirs(xx SYSTEM yy) to avoid unrelated warnings in thirdparty projects.

somehow, did not suffice to use SYSTEM for external_includes; so using pragma ignored individually when including httplib.h

in the 3rd party lib, so avoid using it.

breznak

It now seems to be working with the new lib. Please see comments bellow and possibly approve.

breznak · 2020-09-08T08:32:41Z

bindings/py/cpp_src/CMakeLists.txt

-        ${EXTERNAL_INCLUDES}
+        ${CORE_LIB_INCLUDES}
        ${PROJECT_SOURCE_DIR}
+	SYSTEM ${PYTHON_INCLUDE_DIRS}


Thanks @dkeeney for the advice!,

We could catch all of them in one place by including SYSTEM at the beginning of the list at the point where the EXTERNAL_INCLUDES list is built which is line 93 of extern/bootstrap.cmake.

I tried this, by adding "SYSTEM" to the set(EXTERNAL_INCLUDES ...) but that didn't work as intended.

On line 450 and 464 of src/CMakeLists.txt, I did not specify SYSTEM for EXTERNAL_INCLUDES.

..so I fixed all ocurences of EXTERNAL_INCLUDES in target_include_dirs() with SYSTEM, that works.
But not for httplib, unfortunately. Is it included in the EXTERNAL_INCLUDES, or does it use a different means of including?

breznak · 2020-09-08T08:34:28Z

src/examples/rest/client.cpp

+#pragma GCC diagnostic push
+// turn off the specific warning. Can also use "-Wall"
+#pragma GCC diagnostic ignored "-Wsign-compare"
+#include <httplib.h>


the SYSTEM include did not fix the warnings generated in httplib (I wonder why?), so I had to hack with #pragma here and in server_core.hpp.

the SYSTEM include did not fix the warnings

I don't know of a way to get around the warnings other than with the #pragma.

breznak · 2020-09-08T08:35:22Z

src/examples/rest/client.cpp

  VERBOSE << "Connecting to server: " + serverHost + " port: " << port << std::endl;
  httplib::Client client(serverHost.c_str(), port);
-  client.set_timeout_sec(30);  // The time it waits for a network connection.
+  //client.set_timeout_sec(30);  // The time it waits for a network connection.


@Zbysekz did you find out if there's a replacement for the httplib.set_timeout_sec(), or if we just stop using that function?

Asked question yhirose/cpp-httplib#642

We have an answer: set_connection_timeout() is the substitute.
@breznak this PR is closed maybe starting another? I am not sure about consequences, or what timeout is by default

nice! sorry for merging early. Yes, we can start a new PR where you can revert commit 606cb75d00cb376 and then change it to the new signature.

I am not sure about consequences, or what timeout is by default

nor do i. our tests passed even without it. maybe in the server it makes sense, in the unit tests 30sec is imho too much.

dkeeney

Looks good.

dkeeney · 2020-09-08T20:17:00Z

maybe in the server it makes sense, in the unit tests 30sec is imho too much.

The default web client timeout is 2 min (same as almost all web browsers). The call to set_timeout_sec( ) was an attempt to shorten the time the client takes to report that it cannot find the server. If the connection happens right away the timeout setting has no effect. I think we can safely remove the call to set the timeout.

breznak · 2020-09-08T20:43:59Z

I think we can safely remove the call to set the timeout.

thank you for explanation David. So then I think no further action is needed and this can stay as is.

bump cpp-httplib version to v0.7.6 latest

0d90649

breznak added ready dependencies labels Sep 7, 2020

breznak requested review from Zbysekz and dkeeney September 7, 2020 13:22

dkeeney previously approved these changes Sep 7, 2020

View reviewed changes

breznak mentioned this pull request Sep 7, 2020

cpp-httplib segfault in Debug as RESTapiTest.example get() returns null_ptr #884

Open

breznak added 4 commits September 8, 2020 09:17

Merge branch 'master_community' into bump_httplib

01a1c0f

cmake: apply SYSTEM includes where applicable

7b557f7

in target_include_dirs(xx SYSTEM yy) to avoid unrelated warnings in thirdparty projects.

httplib: special treatment to fix includes, use pragma ignored

8d3f815

somehow, did not suffice to use SYSTEM for external_includes; so using pragma ignored individually when including httplib.h

httplib: set_timeout_sec() deprecated

606cb75

in the 3rd party lib, so avoid using it.

breznak dismissed dkeeney’s stale review via 606cb75 September 8, 2020 08:28

breznak commented Sep 8, 2020

View reviewed changes

breznak requested a review from dkeeney September 8, 2020 08:36

dkeeney approved these changes Sep 8, 2020

View reviewed changes

breznak merged commit aaa6e37 into master Sep 8, 2020

breznak deleted the bump_httplib branch September 8, 2020 12:33

Conversation

breznak commented Sep 7, 2020

Uh oh!

breznak commented Sep 7, 2020

Uh oh!

dkeeney left a comment

Choose a reason for hiding this comment

Uh oh!

breznak commented Sep 7, 2020

Uh oh!

Zbysekz commented Sep 7, 2020

Uh oh!

breznak commented Sep 7, 2020

Uh oh!

dkeeney commented Sep 7, 2020

Uh oh!

breznak left a comment

Choose a reason for hiding this comment

Uh oh!

breznak Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

breznak Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

dkeeney Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

breznak Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

Zbysekz Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

Zbysekz Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

breznak Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

dkeeney left a comment

Choose a reason for hiding this comment

Uh oh!

dkeeney commented Sep 8, 2020

Uh oh!

breznak commented Sep 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants