Add FindMariaDB.cmake module and use it if MySQL is not found
ClosedPublic

Authored by asturmlechner on Mar 25 2020, 3:43 PM.

Details

Reviewers
heikobecker
wbauer
Group Reviewers
Amarok
Summary
  • Previously, MariaDB lib was only found w/ distro provided compat links
  • In fact both MySQL and MariaDB client libs can be installed side by side

FindMySQL.cmake: Split out MySQL Embedded detection into FindMySQLe.cmake

  • MySQLe is pretty much dead now
  • If MySQLe is not found, we will not attempt to link MYSQLE_LIBRARIES anymore
  • Move ZLIB detection behind MySQLe conditional
  • WITH_MYSQL_EMBEDDED is now obsolete
  • Drop superfluous set_package_properties if defined in Find*.cmake module

Drop superfluous ZLIB linking

  • Amarok builds fine without, according to CMakeLists.txt only required for MySQLe anyway.
Test Plan

Built fine against mysql-connector-c as well as mariadb-connector-c.
Unfortunately, MySQLe is long gone in Gentoo so I rely on sb. else's test.

Diff Detail

Repository
R181 Amarok
Branch
mysqle-split-for-phab
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 24232
Build 24250: arc lint + arc unit
asturmlechner created this revision.Mar 25 2020, 3:43 PM
Restricted Application added a project: Amarok. · View Herald TranscriptMar 25 2020, 3:43 PM
Restricted Application added a subscriber: amarok-devel. · View Herald Transcript
asturmlechner requested review of this revision.Mar 25 2020, 3:43 PM

updated for the case when mysqle is shipped with mariadb

asturmlechner edited the summary of this revision. (Show Details)Mar 25 2020, 6:39 PM

On openSUSE, it doesn't enable MySQLe anymore:

 -- Found MySQL: -L/usr/lib64 -lmariadb  
/usr/bin/mysql_config: unrecognized option '--libmysqld-libs'
-- Performing Test HAVE_MYSQL_OPT_EMBEDDED_CONNECTION
-- Performing Test HAVE_MYSQL_OPT_EMBEDDED_CONNECTION - Failed
-- Could NOT find MySQLe (missing: HAVE_MYSQL_OPT_EMBEDDED_CONNECTION)

See inline comment for the reason.

cmake/modules/FindMySQLe.cmake
83

set(CMAKE_REQUIRED_INCLUDES ${MYSQL_INCLUDE_DIR}) is missing here (it was present in the old FindMySQL.cmake).
Adding it makes the test succeed again here.

This comment was removed by wbauer.

re-add accidentally removed required includes to FindMySQLe.cmake

asturmlechner marked an inline comment as done.Mar 25 2020, 8:07 PM
heikobecker accepted this revision as: heikobecker.Apr 11 2020, 10:31 AM
This revision is now accepted and ready to land.Apr 11 2020, 10:31 AM

Sorry if you were waiting for me.
It's good to go in from my side. I actually built mariadb-connector-c without mysql support here as a test, and it still worked fine.

Although, there has been a change to master meanwhile that you probably should integrate:
https://invent.kde.org/multimedia/amarok/-/commit/f0907b34ef9fa447f2fb11ce500cdf9f90022b5a

wbauer added a comment.EditedJun 12 2020, 10:08 PM

One thing I should note though: we do use a patch to fix things on openSUSE currently, but that's not a problem of this change.

I actuallly planned to propose our patch upstream anyway (I originally took it from Arch Linux and didn't really understand why it is needed), although it would actually not be needed anymore with this and a mariadb-connector-c without mysql support ... ;-)

In case you're interested right away, this is it:

diff --git a/cmake/modules/FindMySQL.cmake b/cmake/modules/FindMySQL.cmake
index 0bd7fdfd07..8973729f1f 100644
--- a/cmake/modules/FindMySQL.cmake
+++ b/cmake/modules/FindMySQL.cmake
@@ -38,14 +38,16 @@ if(MYSQLCONFIG_EXECUTABLE)
         OUTPUT_STRIP_TRAILING_WHITESPACE
     )
 
-    if(NOT MC_MYSQL_EMBEDDED_LIBRARIES OR NOT "${MC_return_embedded}" STREQUAL "0")
-        # At least on OpenSUSE and FreeBSD --libmysql-libs doesn't exist, so we just use
-        # MYSQL_LIBRARIES for that. We'll see if that's enough when testing
-        # below.
-        set(MYSQL_EMBEDDED_LIBRARIES ${MYSQL_LIBRARIES})
-    else()
-        set(MYSQL_EMBEDDED_LIBRARIES ${MC_MYSQL_EMBEDDED_LIBRARIES})
-    endif()
+    # mysql-config removed --libmysql-libs, but amarok need libmysqld other
+    # than libmysqlclient to run mysql embedded server.
+    find_library(MYSQL_EMBEDDED_LIBRARIES NAMES mysqld libmysqld
+        PATHS
+                $ENV{MYSQL_DIR}/libmysql_r/.libs
+                $ENV{MYSQL_DIR}/lib
+                $ENV{MYSQL_DIR}/lib/mysql
+        PATH_SUFFIXES
+                mysql
+    )
 endif()
 
 # Try searching manually via find_path/find_library,  possibly with hints

That of course will likely break things on other systems (in this form), so better don't incorporate it blindly... ;-)

malteveerman added inline comments.
cmake/modules/FindMySQLe.cmake
45

This breaks embedded for me. The embedded storage plugin silently links against libmysqlclient and then fails when initializing.
It's a problem that exists independently of this revision but I thought we could fix it here as well.
I'm on Arch where --libmysql-libs doesn't exist either.

wbauer added inline comments.Jun 17 2020, 10:06 AM
cmake/modules/FindMySQLe.cmake
45

Right, that's exactly what I was referring to in my last comment, and what the openSUSE patch I mentioned (which originally was taken from Arch) is supposed to fix.

I.e. instead of set(MYSQLE_LIBRARIES ${MYSQL_LIBRARIES}) here, it should do something like this:

+    find_library(MYSQL_EMBEDDED_LIBRARIES NAMES mysqld libmysqld
+        PATHS
+                $ENV{MYSQL_DIR}/libmysql_r/.libs
+                $ENV{MYSQL_DIR}/lib
+                $ENV{MYSQL_DIR}/lib/mysql
+        PATH_SUFFIXES
+                mysql
+    )

I'm undecided whether this should be done in this patch or rather separately though, as the problem did exist before (so it's actually an unrelated change).

Although, I just noticed that it actually does that below (in line#73ff) if (NOT MYSQLE_LIBRARIES), so maybe it would just be enough to remove the set(MYSQLE_LIBRARIES ${MYSQL_LIBRARIES}) here completely?
I'll give that a try...

wbauer added inline comments.Jun 17 2020, 12:25 PM
cmake/modules/FindMySQLe.cmake
45

Although, I just noticed that it actually does that below (in line#73ff) if (NOT MYSQLE_LIBRARIES), so maybe it would just be enough to remove the set(MYSQLE_LIBRARIES ${MYSQL_LIBRARIES}) here completely?

Indeed, it works fine for me (without any additional patch) if I remove that set (MYSQLE_LIBRARIES ${MYSQL_LIBRARIES}) line.

As this if branch is empty then (except for the comments, it would probably be more readable to remove the whole branch and invert the if condition.
Actually I think just if ("${MC_return_embedded}" STREQUAL "0") should be enough then, at least that works here on openSUSE too. And probably is more correct as well.

malteveerman added inline comments.Jun 17 2020, 12:32 PM
cmake/modules/FindMySQLe.cmake
45

Works for me as well.

whats the common sense of using different names for the same product? MariaDB is the same as MySql, just different names; core and everything else stays the same.

whats the common sense of using different names for the same product? MariaDB is the same as MySql, just different names; core and everything else stays the same.

I don't understand the question. The fork was done before the license change and of course they diverge. I'm not saying they should (API-wise) but unfortunately they do.

asturmlechner closed this revision.Sep 29 2020, 8:05 PM

Thanks for testing! This was pushed with final changes addressing @wbauer's comments in commits 4337b3ef, 03ef605c and 822eddec.