ubuntu 18.04

bertrandop can you please review the patch 889e14fbcc12bc4.

I cannot because having no more Linux machine home where I am confined.
However, next versions of Connect (I hope distributed with new MariaDB releases) will be additionally able to use Curl for REST queries. When using Curl the GetRest.so will not be required.
On Windows, Curl was available on Windows 10, on Linux if it is not, just install it (and cpprest won't be required)

Cool, looking forward to see the curl usage in the new Connect version.
So what do you suggest to do with the current patch?
I have tried and it works .
To close the JIRA task and not to push the current patch?

The big question is how can we synchronize that you do and what I do?

[Anel Husakovic]: I looked into your patch 889e14fbcc12bc4 but did not include it yet in the new version of CMakeLists.txt because I think it needs to be edited. There are double if's (IF (UNIX) just following another IF (UNIX) or lines such as:

IF (cpprestsdk_FOUND OR ("${LSB_RELEASE_ID_SHORT}" STREQUAL "Ubuntu"))

inside an IF (cpprestsdk FOUND), meaning that it is always true.
Just tell me what should be done when UNIX is ubuntu and I'll add it.

Hi bertrandop, thanks for looking into my patch.
First IF(UNIX) in my patch is checking release and setting LSB_RELEASE_ID_SHORT variable. It can be done after or before cpprestsdk package is checked. Since only problem is with ubuntu release, as stated in description (note in that situation cpprestsdk_found will not be true) and I'm checking that in condition IF (cpprestsdk_FOUND OR ("${LSB_RELEASE_ID_SHORT}" STREQUAL "Ubuntu")). Everything after (second IF(UNIX) is from your patch, I just set indentation).

@Anel Husakovic: I tried to include your patch but I was obliged to remove it.
Indeed under ubuntu, saying:

  IF (cpprestsdk_FOUND OR ("${LSB_RELEASE_ID_SHORT}" STREQUAL "Ubuntu"))

executes the code inside the IF even if cpprestsdk is not found and causes a compile error.

bertrandop yes it fails and it should we expect that to happen, right?
cpprestsdk is not found and is saying that:

-- Build files have been written to: /home/anel/mariadb/builds/10.2

[1234/1686] Linking CXX shared module storage/connect/ha_connect.so

FAILED: storage/connect/ha_connect.so 

.

.

.

/usr/bin/x86_64-linux-gnu-ld: cannot find -lcpprest

collect2: error: ld returned 1 exit status

[1239/1686] Building CXX object sql/CM...eFiles/partition.dir/ha_partition.cc.o

ninja: build stopped: subcommand failed.

Previously compilation worked but ha_connect.so was built without statically linked libcpprest.so what wasn't correct and without any hints what was wrong.
This patch is solving that and If you want me to change something in logic let me know.
Do you want to do it other way around?
Note also that there is another part of the patch which is also tested

IF(CONNECT_WITH_REST AND NOT WITH_SSL MATCHES "bundled")

By default, when running from source WITH_SSL is bundled which is not working (you have in description error that are raised) so one need to change that also, I used system instead.

Hi bertrandop I changed a bit the patch.
So the new patch is b767ac869855148.
Note that here I have done other way around, when ubuntu is found, I changed cpprestsdk_dir in order to find the right package and that variable cpprest_found be true in this case:

  IF ("${LSB_RELEASE_ID_SHORT}" STREQUAL "Ubuntu")

      set(cpprestsdk_DIR /usr/lib/${CMAKE_LIBRARY_ARCHITECTURE}/cmake/)

    ENDIF()

Note 1: Without libcpprest.so found, we got the following compile error (and we should):

== Configuring MariaDB Connector/C

-- Ubuntu

CMake Error at /usr/lib/x86_64-linux-gnu/cmake/cpprestsdk-targets.cmake:114 (message):

  The imported target "cpprestsdk::cpprest" references the file

     "/usr/lib/x86_64-linux-gnu/libcpprest.so.2.10"

  but this file does not exist.  Possible reasons include:

  * The file was deleted, renamed, or moved to another location.

  * An install or uninstall procedure did not complete successfully.

  * The installation package was faulty and contained

     "/usr/lib/x86_64-linux-gnu/cmake/cpprestsdk-targets.cmake"

  but not all the files it references.

Call Stack (most recent call first):

  /usr/lib/x86_64-linux-gnu/cmake/cpprestsdk-config.cmake:17 (include)

  storage/connect/CMakeLists.txt:335 (FIND_PACKAGE)

-- Configuring incomplete, errors occurred!

With libcpprest.so found we got statically linked library:

$ ldd ./storage/connect/ha_connect.so |grep libcpp

	libcpprest.so.2.10 => /usr/lib/x86_64-linux-gnu/libcpprest.so.2.10 (0x00007fea22c81000)

Let me know if you are ok with this patch.
Regards.

Because compiling ha_connect.so (or dll) must be ok whether or not cpprestsdk is available, I choose to dynamically link it. However, to simplify the way it is done, CONNECT does not dynamically links to the cpprestsdk lib but to the intermediate lib GetRest, as explained in Appendix B of the CONNECT documentation. This reduces to only one Function address to be set when linking to GetRest (which is itself statically linked to cpprestsdk)

As a matter of fact, almost all the part of the CMakeLists.txt file we are discussing is just that when compiling MariaDB from source, it also permits to include GetRest into CONNECT, which then becomes statically linked to cpprestsdk. But of course this works only if the package is available.

However, this is not required and the CMakeLists.txt part of REST could be just reduced to:

#

# REST

#

OPTION(CONNECT_WITH_REST "Compile CONNECT storage engine with REST support" ON)

IF(CONNECT_WITH_REST)

# MESSAGE(STATUS "=====> REST support is ON")

  SET(CONNECT_SOURCES ${CONNECT_SOURCES} tabrest.cpp tabrest.h)

  add_definitions(-DREST_SUPPORT)

ENDIF(CONNECT_WITH_REST)

This maybe is what should just be done, in particular on Ubuntu, if it causes problems.

Then, CONNECT compiled from source would work like binary distributions, meaning that separately making the GetRest.so would have to be still done, and perhaps in a way specific to Ubuntu.

Note that all this will be just another option in future versions because I have tested on buildbot that REST works without cpprestsdk providing cURL is available, which seems to be the case on almost all systems.

Dear bertrandop,
with the latest patch that I provided here:
https://github.com/MariaDB/server/commit/b767ac869855148d1a9e8de7d3085ef5658b382f
everything you said works correctly.

Let me go through examples to show you:
Scenario 1 - see what my patch is doing

• Result: If we have the cpprestsdk package installed ha_storage.so will statically linked libcpprest.so:
• The proof:

  # Here we see it is installed:

  $ apt list libcpprest-dev

  Listing... Done

  libcpprest-dev/bionic,now 2.10.2-6 amd64 [installed]

  # Here we see it has libcpprest.so library

  $ dpkg -L libcpprest-dev|grep libcpprest.so

  /usr/lib/x86_64-linux-gnu/libcpprest.so

  # And here we see that with my patch on ubuntu, as an problematic release, libcpprest.so is correctly statically linked with ha_connect.so

  $ ldd ./storage/connect/ha_connect.so |grep libcpprest

  libcpprest.so.2.10 => /usr/lib/x86_64-linux-gnu/libcpprest.so.2.10 (0x00007f64fea42000)

Scenario 2 - uninstall libcpprest-dev package and try dynamically call Getrest.so

• The result: When libcpprest-dev is uninstalled, there is no compilation error, since we allow to use dynamically Getrest.so library.
• The proof:

#Uninstall libcpprest-dev

$ sudo apt remove libcpprest-dev

# Configure and build binaries again without libcpprest-dev. Call ninja, make, cmake --build . --target connect or whatever you used.

# The result should be as below:

-- Configuring done

-- Generating done

-- Build files have been written to: /home/anel/mariadb/builds/10.2

[63/63] Linking CXX shared module storage/connect/ha_connect.so

# Verify that ha_connect.so doesn't have libcpprest.so included - result is empty

$ ldd ./storage/connect/ha_connect.so |grep libcpprest  # emtpy 

• Now start the server and invoke the query - Error raised - no GetRest.so loaded :

  MariaDB [test]> CREATE TABLE webusers ENGINE=CONNECT TABLE_TYPE=JSON HTTP='http://jsonplaceholder.typicode.com/users';

  ERROR 1105 (HY000): Error loading shared library GetRest.so: /usr/lib/GetRest.so: undefined symbol: _ZTVN3web4http7details13http_msg_baseE

• Place GetRest.so in the right place

  $ sudo cp ./mariadb/GetRest.so /usr/lib/x86_64-linux-gnu/

• Without again starting the server execute query one more time:

  MariaDB [test]> CREATE TABLE webusers ENGINE=CONNECT TABLE_TYPE=JSON HTTP='http://jsonplaceholder.typicode.com/users';

  Query OK, 0 rows affected (0.24 sec)

• Above proof is enough to say that without cpprestsdk there is NO COMPILATION error and the patch is doing the same as was doing before.

Conclusions

• The patch solved problem of finding the cpprestsdk package for cmake for ubuntu release, what is important when statically linking the binaries.
• If you run cmake -DWITH_SSL=bundled there will be compilation error and it is not related to above problem, so I just make sure that user will not provide that option WITH_SSL=bundled since in that case WolfSSL as bundled option to the server will be used and CONNECT will fail.

Looking forward to your reply and review.
Thanks

Thank you for working on this. I am sure it works like you describe.

Nevertheless, I will take the opposite decision and completely remove the possibility of compiling a statically linked to cpprestsdk version of Connect. The new version of CMakeLists.txt will just contain:

#

# REST

#

OPTION(CONNECT_WITH_REST "Compile CONNECT storage engine with REST support" ON)

IF(CONNECT_WITH_REST)

# MESSAGE(STATUS "=====> REST support is ON")

  SET(CONNECT_SOURCES ${CONNECT_SOURCES} tabrest.cpp tabrest.h)

  add_definitions(-DREST_SUPPORT)

ENDIF(CONNECT_WITH_REST)

The reason why?

Allowing to compile a version of Connect statically linked to cpprestsdk was actually a temporary tool allowing to test REST on early versions of connect having no other mean to do it and should have been already removed.

Indeed, an engine module, such as Connect, must be loadable by all means and therefore, never be dependent on an external product. This was already true for Java and JBDC and must be also true for REST.

Don't forget that compiling a version is not only to use it locally but more generally to make a distribution (see Creating a Debian Repository) Even your module will work locally, not only it won't if you export it to another machine without cpprestsdk, but it will not load at all, making it useless even for those not using REST.

Furthermore, cpprestsdk is not mandatory anymore, most future versions will use curl instead and curl seems to be available on almost all systems.

So what you should do now (the version you have is not able to use curl) is to use REST the way it is described in appendix 2 and see if you have any trouble when compiling the external GetRest.so lib on Ubuntu.

Thanks for your understanding.

Ok, I see what you mean, I just lost a lot of time and found out this problem related to my environment. I don't know what do you mean by appendix 2 - I gues this link?
https://mariadb.com/kb/en/connect-making-the-getrest-library/
I already have used GetRest.so for dynamically linking and it works, showed in example above also.

Please take a look other part of the patch and let me know is it acceptable, not related to static linkage, but rather to WITH_SSL option:

IF(CONNECT_WITH_REST AND NOT WITH_SSL MATCHES "bundled")

Do you want me to help regarding cURL development?
I will be working more on CONNECT in next period .
Thanks

I don't anything about that SSL stuff. Do you think it is important? Perhaps it could be wrong by preventing Connect to generate the _tabrest _ table type that could work anyway with CURL, and is harmless when not using REST.

I have attached the new _tabrest _files. Maybe you can look at the way I call CURL in the Xcurl function. I did not know how to execute an external program and found this looking in the NET but there maybe a more appropriate way to do it. Perhaps also, replacing manually them in the source, you can test using REST via CURL.

When we use with_ssl=bundled we got compilation error. We have to disable this.
Will take a look this week to see cURL implementation and let you know . Thanks.
So regarding this MDEV, just let me know what you decided regarding WITH_SSL patch so we can close it and start working on different MDEV (like cURL for example).

There is nothing specific in the four files added when REST is ON that can explain a compilation error related to SSL. Unless something is found showing the contrary, and in this case that is the file itself that must be fixed to avoid it, no patch should be applied here.
If people have problem with SSL they just have to compile WITH REST OFF.
So this MDEV is closed.