Fix compatibility with modern MySQL
ClosedPublic

Authored by vmatare on Mar 20 2020, 5:40 PM.

Details

Summary

MySQL 8.0 onward changed a few details in the client API, and embedded support has apparently been removed completely. This PR makes minimally invasive changes that fix the related compile errors in a (hopefully) backward compatible manner.

Test Plan

compile against MySQL <= 5, MySQL >= 8, and various versions of MariaDB (though it seems they didn't touch that part of the API yet)

Diff Detail

Repository
R181 Amarok
Lint
Lint Skipped
Unit
Unit Tests Skipped
vmatare created this revision.Mar 20 2020, 5:40 PM
Restricted Application added a project: Amarok. · View Herald TranscriptMar 20 2020, 5:40 PM
Restricted Application added a subscriber: amarok-devel. · View Herald Transcript
vmatare requested review of this revision.Mar 20 2020, 5:40 PM
vmatare updated this revision to Diff 78114.

storage: Fix for MySQL 8.0 onward

The external API was slightly redesigned.

wbauer added a subscriber: wbauer.EditedMar 23 2020, 9:49 PM

This breaks compilation here on openSUSE with MariaDB:

In file included from /home/abuild/rpmbuild/BUILD/amarok-2.9.70git.20200314T055024~d790424e56/src/core-impl/storage/sql/mysql-shared/MySqlStorage.cpp:31:
/usr/include/mysql/mysql.h:39:14: error: conflicting declaration 'typedef char my_bool'
   39 | typedef char my_bool;
      |              ^~~~~~~
In file included from /home/abuild/rpmbuild/BUILD/amarok-2.9.70git.20200314T055024~d790424e56/src/core-impl/storage/sql/mysql-shared/MySqlStorage.cpp:20:
/home/abuild/rpmbuild/BUILD/amarok-2.9.70git.20200314T055024~d790424e56/src/core-impl/storage/sql/mysql-shared/MySqlStorage.h:37:7: note: previous declaration as 'using my_bool = bool'
   37 | using my_bool = bool;
      |       ^~~~~~~
In file included from /home/abuild/rpmbuild/BUILD/amarok-2.9.70git.20200314T055024~d790424e56/src/core-impl/storage/sql/mysql-shared/MySqlStorage.cpp:31:
/usr/include/mysql/mysql.h:362:3: error: conflicting declaration 'typedef struct st_mysql MYSQL'
  362 | } MYSQL;
      |   ^~~~~
In file included from /home/abuild/rpmbuild/BUILD/amarok-2.9.70git.20200314T055024~d790424e56/src/core-impl/storage/sql/mysql-shared/MySqlStorage.cpp:20:
/home/abuild/rpmbuild/BUILD/amarok-2.9.70git.20200314T055024~d790424e56/src/core-impl/storage/sql/mysql-shared/MySqlStorage.h:36:8: note: previous declaration as 'struct MYSQL'
   36 | struct MYSQL;
      |        ^~~~~

The content of my /usr/include/mysql/mysql_version.h (which is just a symlink to mariadb_version.h):

/* Copyright Abandoned 1996, 1999, 2001 MySQL AB                                                                                                                                    
   This file is public domain and comes with NO WARRANTY of any kind */                                                                                                             
                                                                                                                                                                                    
/* Version numbers for protocol & mysqld */                                                                                                                                         
                                                                                                                                                                                    
#ifndef _mariadb_version_h_                                                                                                                                                         
#define _mariadb_version_h_                                                                                                                                                         
                                                                                                                                                                                    
#ifdef _CUSTOMCONFIG_                                                                                                                                                               
#include <custom_conf.h>                                                                                                                                                            
#else                                                                                                                                                                               
#define PROTOCOL_VERSION                10                                                                                                                                          
#define MARIADB_CLIENT_VERSION_STR      "10.4.3"                                                                                                                                    
#define MARIADB_BASE_VERSION            "mariadb-10.4"                                                                                                                              
#define MARIADB_VERSION_ID              100403                                                                                                                                      
#define MARIADB_PORT                    3306                                                                                                                                        
#define MARIADB_UNIX_ADDR               "/run/mysql/mysql.sock"                                                                                                                     
                                                                                                                                                                                    
#define MYSQL_CONFIG_NAME               "my"                                                                                                                                        
#define MYSQL_VERSION_ID                100403                                                                                                                                      
#define MYSQL_SERVER_VERSION            "10.4.3-MariaDB"                                                                                                                            
                                                                                                                                                                                    
#define MARIADB_PACKAGE_VERSION "3.1.7"                                                                                                                                             
#define MARIADB_PACKAGE_VERSION_ID 30107                                                                                                                                            
#define MARIADB_SYSTEM_TYPE "Linux"                                                                                                                                                 
#define MARIADB_MACHINE_TYPE "x86_64"                                                                                                                                               
#define MARIADB_PLUGINDIR "/usr//usr/lib64/mysql/plugin/"

/* mysqld compile time options */
#ifndef MYSQL_CHARSET
#define MYSQL_CHARSET                   ""
#endif
#endif

/* Source information */
#define CC_SOURCE_REVISION ""

#endif /* _mariadb_version_h_ */

Maybe you should also check if e.g. MARIADB_VERSION_ID is undefined or something.

vmatare updated this revision to Diff 78327.Mar 24 2020, 12:17 AM
vmatare edited the summary of this revision. (Show Details)
vmatare edited the test plan for this revision. (Show Details)

storage: distinguish MariaDB and MySQL for API quirks

MariaDB doesn't seem to be updating the API the way MySQL does, so exclude it from the version hackery.

This breaks compilation here on openSUSE with MariaDB:

Great, thanks for testing! I think I updated the diff, but I'm confused by this whole Phabricator thingy. Not sure if I amended or replaced the patch now. This whole GUI makes absolutely no sense to me :-/ For me these are 3 separate commits, because they really deal with 3 separate issues (removed embedded MySQL, MySQL API change, and special treatment for MariaDB). Let me know if I should resubmit the whole thing as one patch.

That diff now looks like a diff in top of your previous diff.

I know, the question was whether that supersedes or amends the previous diff. But seems like it's the former. I'll update, sorry.

vmatare updated this revision to Diff 78331.Mar 24 2020, 12:34 AM

The whole thing as one combined patch

Thanks for the very nice patch! Compiles and works for me on Gentoo (MySQL Connector/C 8.0.19).

Confirming successful build here as well.

The content of my /usr/include/mysql/mysql_version.h (which is just a symlink to mariadb_version.h):

I just went back and installed mariadb-connector-c in Gentoo in mysqlcompat-mode (a soon to be removed option) and interestingly, it does not provide that symlink. Just something to take into account when using that header, I don't know how consistent other distros are doing this.

Locally though I have already been toying around with separate MySQL/MariaDB support so I could then probably amend that with another ifdef.

tests/core-impl/collections/CMakeLists.txt
3 ↗(On Diff #78331)

amarok cmake files seem to be using a tab size of 4 spaces fairly consistently, please keep it that way

vmatare updated this revision to Diff 78424.Mar 25 2020, 2:19 AM

indent with 4 spaces in cmake

vmatare updated this revision to Diff 78425.Mar 25 2020, 2:49 AM
vmatare marked an inline comment as done.

Another attempt at a simple solution that works with all versions of MariaDB and MySQL. This one comes with the added cost of including the full mysql.h because that seems to be the only consistently supplied drop-in header by MariaDB. Other solutions would involve additional CMake hackery to distinguish between MariaDB and MySQL, but for the sake of sanity, let's sacrifice compilation time for simplicity here.

Please test again, and thanks a lot for the review so far.

asturmlechner accepted this revision.Mar 25 2020, 11:52 AM

Please test again, and thanks a lot for the review so far.

Successfully tested with:
mariadb-connector-c-3.0.6 (mysqlcompat mode)
mysql-connector-c-8.0.19
mariadb-connector-c-3.1.7 (with my local modifications)

This revision is now accepted and ready to land.Mar 25 2020, 11:52 AM

Waiting for @wbauer to confirm it works as well - btw, does openSUSE still provide mysqle to test -DWITH_MYSQL_EMBEDDED=on?

heikobecker accepted this revision.Mar 25 2020, 3:36 PM
heikobecker added a subscriber: heikobecker.

Builds fine here with mariadb and WITH_MYSQL_EMBEDDED=TRUE.

I assume you don't have commit access and you'll need somebody to push this patch for you, right?

Waiting for @wbauer to confirm it works as well

Yes, the latest version builds fine here (the previous did as well), and the embedded collection still works as well.

btw, does openSUSE still provide mysqle to test -DWITH_MYSQL_EMBEDDED=on?

Yes, as part of mariadb. (the "real" mysql is not included in the distribution anymore since a while)

I assume you don't have commit access and you'll need somebody to push this patch for you, right?

Yes, please go ahead if you think this has been tested well enough.

The content of my /usr/include/mysql/mysql_version.h (which is just a symlink to mariadb_version.h):

I just went back and installed mariadb-connector-c in Gentoo in mysqlcompat-mode (a soon to be removed option) and interestingly, it does not provide that symlink.

Indeed, the symlink is added in openSUSE's packaging. From mariadb-connector-c.spec:

# add a compatibility symlink
ln -s mariadb_config %{buildroot}%{_bindir}/mysql_config
ln -s mariadb_version.h %{buildroot}%{_includedir}/mysql/mysql_version.h
This revision was automatically updated to reflect the committed changes.