sql-client: add support for mariadb-client #13318

InuSasha · 2018-01-06T00:20:44Z

make it possible to link against mariadb-client instead of mysql

Description

Search explicit for libmariadb and mysql-header under include/mariadb/.
Additional added a test, to protect against using of mysql & mariadb at the same time

Motivation and Context

most distributions switch to mariadb, this follows up.

How Has This Been Tested?

tested on top of LibreELEC master with MariaDB 10.2 and MySQL 5.7 as sql-server.
test-cases:

database migration from Video108 -> Video109
database migration from Music66 -> Music69
list movies
mark movie as seen
builds against mysql-client and mariadb-client and against both (as negative test)

reallife test on top of KRYTON (LE 8.2.2), is started.

Screenshots (if appropriate):

Types of change

Bug fix (non-breaking change which fixes an issue)
Improvement (non-breaking change which improves existing functionality)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the Code guidelines of this project
My change requires a change to the documentation, either Doxygen or wiki
I have updated the documentation accordingly
I have read the CONTRIBUTING document
I have added tests to cover my change
All new and existing tests passed

Rechi · 2018-01-06T08:51:20Z

@wsnipex do we want to split MySqlClient and MariaDBClient in two different cmake modules or should they just be handled by a single one?

xbmc/dbwrappers/mysqldataset.cpp

 #include "mysql/errmsg.h"
+#endif
+#ifdef HAS_MARIADB


xbmc/dbwrappers/mysqldataset.h

 #include "mysql/mysql.h"
+#endif
+#ifdef HAS_MARIADB


xbmc/dbwrappers/mysqldataset.cpp

 #include "mysql/errmsg.h"
+#endif
+#ifdef HAS_MARIADB
+#include "mariadb/errmsg.h"


xbmc/dbwrappers/mysqldataset.h

 #include "mysql/mysql.h"
+#endif
+#ifdef HAS_MARIADB
+#include "mariadb/mysql.h"


Paxxi · 2018-01-06T09:09:45Z

Is it worth keeping mysql at all? The more they diverge the more issues we'll start seeing so might as well drop mysql if the platforms we support have mariadb

wsnipex · 2018-01-06T09:16:57Z

We should probably prefer mariadb over mysql and use maria if both are found in auto mode(ENABLE_MYSQL != ON).
Otherwise looks good.

@Paxxi: supporting both shouldn't really cause any overhead for now, so I'd postpone dropping mysql to v19.

Paxxi · 2018-01-06T09:19:02Z

Sounds good to me

InuSasha · 2018-01-06T12:18:15Z

ok, will prefer mariadb over mysql, when auto detect.
abort when both are explicit enabled.
when one is explicit enabled, ignore the other.
right?

InuSasha · 2018-01-06T15:33:59Z

implement the suggestions

wsnipex

thanks

CMakeLists.txt

@@ -179,6 +178,16 @@ core_optional_dep(${optional_deps})
 core_require_dyload_dep(${required_dyload})
 core_optional_dyload_dep(${dyload_optional})

+if(ENABLE_MYSQLCLIENT AND ENABLE_MARIADBCLIENT)
+    MESSAGE(FATAL_ERROR "You can not use MySql and MariaDB at the same time. Disable one by adding -DENABLE_MYSQLCLIENT=OFF or -DENABLE_MARIADBCLIENT=OFF")
+endif()


InuSasha · 2018-01-08T18:55:05Z

@Rechi switch to check the MYSQL/MARIADBCLIENT_FOUND. With the ENABLE it is $&%$!
But when you change the client, you have to clean your build path (drop the cached variables).

the-dreamer · 2018-01-10T08:25:52Z

Will this commit solve the issue that streamdetails is not saved to sql db anymore? it is working when sqlite (local db) is used so there is something wrong on the mysql site. The debug log showed that it is not saved when using mysql by not calling streamdetails function.

I use myself a mariadb server and only streamdetails is not saved. Watchstates, resume points etc. are saved.

InuSasha · 2018-01-10T09:27:28Z

@the-dreamer: simple answer, no

InuSasha · 2018-01-12T10:54:12Z

after one week as backport on Krypton (LE 8.2.2) all is working fine.
Good to go.

The failed jenkins-build, was a jenkins problem, not a code problem... (maybe i pushed a second rebase, to fast)

Rechi · 2018-01-12T12:59:08Z

With current code mysql is the preferred one in autodetection mode instead of mariadb.
Commits need squashing.
The jenkins failure isn't caused by code changes.

InuSasha · 2018-01-12T13:25:12Z

@Rechi sure, about the preferred?
I tested it, and maria was selected.
When there was an earlier run of cmake (with mysql used), this was cached. In this case, you must clean your build path. Sorry i do not have any solution for it, now.

Rechi · 2018-01-13T09:49:13Z

Sorry only read the code changes and thought of a case which isn't handled correctly, but tested it now and it works as expected.
Commits still should be squashed.

@wsnipex any comment?

Rechi · 2018-01-13T10:26:19Z

@InuSasha I found an easier way to handle mariadb and mysq in CMakeLists.txt which also allows changing the client without deleting the cmake build files.

+++ b/CMakeLists.txt
@@ -149,7 +149,7 @@
                   DBus
                   LCMS2
+                  MariaDBClient
                   MDNS
                   MicroHttpd
-                  MySqlClient
                   PulseAudio
                   Python
@@ -180,4 +180,8 @@
 core_optional_dyload_dep(${dyload_optional})
 
+if(NOT MARIADBCLIENT_FOUND)
+  core_optional_dep(MySqlClient)
+endif()
+
 if(NOT UDEV_FOUND)
   core_optional_dep(LibUSB)

edit: had placed the if at a wrong place

InuSasha · 2018-01-13T12:01:26Z

@Rechi your solution does not work in case of selecting mysql (only -DENABLE_MYSQLCLIENT=ON is set). In this case mariadb is selected, automatically.

Rechi · 2018-01-13T14:56:18Z

Actually IMO maridb should always be the preferred one, unless you explicitly disable it.

InuSasha · 2018-01-13T14:59:40Z

or the mysql is enabled explicit. Other way, it is very confusing, for builders.
"Why is mariadb used? i had activate mysql..."

InuSasha · 2018-01-28T17:53:44Z

@Rechi: switched to your suggestion. I am no fully agreed with it, but implemented is better the perfect :).

wsnipex · 2018-01-30T08:25:30Z

Sadly, it's not as simple. You have to consider the user force enabling either mariadb or mysql, which makes it much more complicated.
I'm wondering if having only one mariadb find module, which also checks for mysql libs/includes, might be better. Thoughts?

Rechi · 2018-01-30T08:28:13Z

@wsnipex you don't like my version where you have to explicitly disable mariadb (if it is installed) to get mysql?

wsnipex · 2018-01-30T08:54:29Z

not especially, because it's unintuitive and inconsistent with other options.
Also, what about distros that ship mariadb, but with only the mysql compatibility libs (redhat/centos does that for example)

Rechi · 2018-01-30T08:57:35Z

If mariadb is not found it will check for mysql, so redhat/centos will still work if they did before.

wsnipex · 2018-01-30T09:26:32Z

imho it is confusing and unintuitive if you have mariadb installed but -DENABLE_MARIADBCLIENT=ON fails. Granted that will only happen for distros that have dubious packaging..
It would be nice if we'd fall back to mysql in this scenario as well.

Rechi · 2018-01-30T16:11:02Z

@InuSasha @wsnipex and I agreed on the following solution

+++ b/CMakeLists.txt
@@ -152,5 +152,4 @@
                   MDNS
                   MicroHttpd
-                  MySqlClient
                   PulseAudio
                   Python
@@ -180,4 +179,15 @@
 core_optional_dyload_dep(${dyload_optional})
 
+if(ENABLE_MARIADBCLIENT AND NOT ENABLE_MARIADBCLIENT STREQUAL AUTO AND ENABLE_MYSQLCLIENT AND NOT ENABLE_MYSQLCLIENT STREQUAL AUTO)
+  MESSAGE(FATAL_ERROR "You can not use MySql and MariaDB at the same time. Disable one by adding -DENABLE_MYSQLCLIENT=OFF or -DENABLE_MARIADBCLIENT=OFF.")
+elseif(ENABLE_MYSQLCLIENT AND NOT ENABLE_MYSQLCLIENT STREQUAL AUTO)
+  set(ENABLE_MARIADBCLIENT OFF)
+endif()
+
+core_optional_dep(MariaDBClient)
+if(NOT MARIADBCLIENT_FOUND)
+  core_optional_dep(MySqlClient)
+endif()
+
 if(NOT UDEV_FOUND)
   core_optional_dep(LibUSB)

Please update the PR with this and squash it into a single commit.

InuSasha · 2018-01-30T18:46:46Z

@Rechi @wsnipex i believe, we have it, now.
i had to make a little change on your suggestion.

+if(NOT ENABLE_MARIADBCLIENT STREQUAL OFF)
   core_optional_dep(MariaDBClient)
+endif()

commits squashed

thanks for your help :)

Rechi · 2018-01-30T18:48:57Z

core_optional_dep should have handled that already

InuSasha · 2018-01-30T18:55:48Z

not in my test. i do not know why...
here the shorted log, with your suggestion.

root@kodi:/home/inu/kodi/mariadb.build# rm -rf *; cmake ../mariadb -DENABLE_MYSQLCLIENT=ON
-- The CXX compiler identification is GNU 7.2.0
...
-- Found MariaDBClient: /usr/lib/x86_64-linux-gnu/libmariadbclient.a (found version "10.2.7")
...
-- MARIADBCLIENT enabled: Yes
...

CMakeLists.txt

+  set(ENABLE_MARIADBCLIENT OFF)
+endif()
+
+if(NOT ENABLE_MARIADBCLIENT STREQUAL OFF)


CMakeLists.txt

+if(ENABLE_MARIADBCLIENT AND NOT ENABLE_MARIADBCLIENT STREQUAL AUTO AND ENABLE_MYSQLCLIENT AND NOT ENABLE_MYSQLCLIENT STREQUAL AUTO)
+  MESSAGE(FATAL_ERROR "You can not use MySql and MariaDB at the same time. Disable one by adding -DENABLE_MYSQLCLIENT=OFF or -DENABLE_MARIADBCLIENT=OFF.")
+elseif(ENABLE_MYSQLCLIENT AND NOT ENABLE_MYSQLCLIENT STREQUAL AUTO)
+  set(ENABLE_MARIADBCLIENT OFF)


mariadb is a place-in for mysql. only, the search pathes and the include path are changed.

InuSasha · 2018-01-31T18:10:19Z

@Rechi @wsnipex found it :), we have to cache it

-  set(ENABLE_MARIADBCLIENT OFF)
+  set(ENABLE_MARIADBCLIENT OFF CACHE BOOL "")

dropped my extra check, too

wsnipex · 2018-02-05T08:53:31Z

thanks for the patience!

HeresJonny · 2018-02-15T21:31:24Z

Since Milhouse #205 and adamg 8.90.4, this patch was added. Unfortunately, the change now prevents Kodi from starting up (on both builds). The log error is:

21:31:09.745 T:139727820420864 ERROR: SQL: [MyMusic70] An unknown error occurred
Query: SELECT idVersion FROM version
21:31:09.795 T:139727820420864 ERROR: Process error processing job

MilhouseVH · 2018-02-15T21:53:57Z

Unfortunately, the change now prevents Kodi from starting up (on both builds)

Are you sure that this "prevents Kodi from starting up" - missing databases don't usually prevent Kodi from starting. Is Kodi now crashing?

The reason your MyMusic70 database has no version is because the migration from your previous MyMusicXX failed - can you enable debug logging, drop MyMusic70 and restart Kodi so that the MyMusicXX migration is attempted once more, then upload the resulting Kodi log to a pastebin site and paste the link.

HeresJonny · 2018-02-15T22:33:38Z

I dropped MyMusic69. MyMusic70 was created from scratch by scraping.
https://pastebin.com/y6MUKZhu

MilhouseVH · 2018-02-16T05:13:42Z

I dropped MyMusic69. MyMusic70 was created from scratch by scraping.
https://pastebin.com/y6MUKZhu

Did you post the wrong log? Your log shows s a version of Kodi from 25 Dec 2017, so presumably LibreELEC is of the same vintage in which case it doesn't include MariaDB support. The log also doesn't create a MyMusic70 database, in fact it doesn't create any database - it does however find and use an existing MyMusic69 database.

Let's move the conversation to the forum - until we know what is wrong this is just noise.

MilhouseVH · 2018-02-16T09:45:09Z

Update from forum: https://forum.kodi.tv/showthread.php?tid=298462&pid=2704269#pid2704269

This MyMusic70 problem may be caused by the mariadb-connector-c having some sort of incompatibility with MySQL when query cache is enabled (query cache is disabled by default in more recent versions of MySQL Server so maybe not a long-term issue).

InuSasha · 2018-02-16T10:18:55Z

Solution is to set SET SESSION query_cache_type = OFF; per connection for build against MariaDB client and used MySQL-DB

InuSasha mentioned this pull request Jan 6, 2018

mariadb: switch from mysql to mariadb LibreELEC/LibreELEC.tv#2387

Merged

5 tasks

Rechi reviewed Jan 6, 2018

View reviewed changes

wsnipex approved these changes Jan 7, 2018

View reviewed changes

Rechi requested changes Jan 7, 2018

View reviewed changes

jenkins4kodi added Rebase needed PR that does not apply/merge cleanly to current base branch and removed Rebase needed PR that does not apply/merge cleanly to current base branch labels Jan 10, 2018

wsnipex reviewed Jan 31, 2018

View reviewed changes

CMakeLists.txt Outdated

set(ENABLE_MARIADBCLIENT OFF)

endif()

if(NOT ENABLE_MARIADBCLIENT STREQUAL OFF)

This comment was marked as spam.

Sign in to view

Rechi reviewed Jan 31, 2018

View reviewed changes

sql-client: add support for mariadb-client

35c2995

mariadb is a place-in for mysql. only, the search pathes and the include path are changed.

wsnipex merged commit 3316cd1 into xbmc:master Feb 5, 2018

MartijnKaijser added Component: Depends v18 Leia labels Feb 5, 2018

MartijnKaijser added this to the L 18.0-alpha1 milestone Feb 5, 2018

Rechi added the CMake label Feb 5, 2018

sql-client: add support for mariadb-client #13318

sql-client: add support for mariadb-client #13318

Conversation

InuSasha commented Jan 6, 2018 • edited

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of change

Checklist:

Rechi commented Jan 6, 2018

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

Paxxi commented Jan 6, 2018

wsnipex commented Jan 6, 2018

Paxxi commented Jan 6, 2018

InuSasha commented Jan 6, 2018

InuSasha commented Jan 6, 2018

wsnipex left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

InuSasha commented Jan 8, 2018

the-dreamer commented Jan 10, 2018

InuSasha commented Jan 10, 2018

InuSasha commented Jan 12, 2018

Rechi commented Jan 12, 2018

InuSasha commented Jan 12, 2018

Rechi commented Jan 13, 2018

Rechi commented Jan 13, 2018 • edited

InuSasha commented Jan 13, 2018

Rechi commented Jan 13, 2018

InuSasha commented Jan 13, 2018 • edited

InuSasha commented Jan 28, 2018

wsnipex commented Jan 30, 2018

Rechi commented Jan 30, 2018

wsnipex commented Jan 30, 2018

Rechi commented Jan 30, 2018

wsnipex commented Jan 30, 2018

Rechi commented Jan 30, 2018

InuSasha commented Jan 30, 2018

Rechi commented Jan 30, 2018

InuSasha commented Jan 30, 2018 • edited

This comment was marked as spam.

This comment was marked as spam.

InuSasha commented Jan 31, 2018

wsnipex commented Feb 5, 2018

HeresJonny commented Feb 15, 2018

MilhouseVH commented Feb 15, 2018

HeresJonny commented Feb 15, 2018 • edited

MilhouseVH commented Feb 16, 2018

MilhouseVH commented Feb 16, 2018

InuSasha commented Feb 16, 2018 • edited

InuSasha commented Jan 6, 2018 •

edited

Rechi commented Jan 13, 2018 •

edited

InuSasha commented Jan 13, 2018 •

edited

InuSasha commented Jan 30, 2018 •

edited

HeresJonny commented Feb 15, 2018 •

edited

InuSasha commented Feb 16, 2018 •

edited