-
Notifications
You must be signed in to change notification settings - Fork 550
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix for issue #878 #888
fix for issue #878 #888
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Belated review.
#ifdef LIBMYSQL_VERSION | ||
#define MYSQL_LINK_VERSION LIBMYSQL_VERSION | ||
#ifdef MARIADB_CLIENT_VERSION_STR | ||
#define MYSQL_LINK_VERSION MARIADB_CLIENT_VERSION_STR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a suggested fix I found as well, works for me.
#ifdef MARIADB_CLIENT_VERSION_STR | ||
#define MYSQL_LINK_VERSION MARIADB_CLIENT_VERSION_STR | ||
#elif LIBMYSQL_VERSION | ||
#define MYSQL_LINK_VERSION MYSQL_VERSION_ID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MYSQL_VERSION_ID is numeric and will fail the major.minor dot tests later in this file.
#include <errmsg.h> | ||
#include <mysqld_error.h> | ||
#include <mysql_version.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was added in #857 to get MariaDB 10.2 working :)
Also remove the second path below.
@@ -11,10 +11,8 @@ void Init_mysql2(void); | |||
|
|||
#ifdef HAVE_MYSQL_H | |||
#include <mysql.h> | |||
#include <mysql_com.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mysql_com.h
has been included since the very beginning, and has probably always been superfluous it looks - but I am loathe to remove it at a micro release.
I spelunked some old MySQL mailing lists, and the problem here is MYSQL_VERSION_ID is purely numeric, and will fail the tests that were written for testing against major.minor (but not .micro) in the version string. The documentation for these constants suggests they should always be available: https://dev.mysql.com/doc/refman/5.7/en/c-api-server-client-versions.html A better approach that parsing the major.minor might be dividing the MYSQL_VERSION_ID by 100, but again I would queue up this change for the next major release of the gem. |
Thank you for the PR! I worked this out a bit differently in #900, please take a look to make sure it works for you! |
Alternative fix that addresses @grknight concern on coupling to the file locations.