Skip to content
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

Minimal required changes to make it work for mariadb 10.2 #857

Merged
merged 3 commits into from
Jun 21, 2017
Merged

Minimal required changes to make it work for mariadb 10.2 #857

merged 3 commits into from
Jun 21, 2017

Conversation

damm
Copy link
Contributor

@damm damm commented Jun 21, 2017

Updated extconf.rb to check if we have the header
Update mysql2_ext.h to load it if the def is set

Signed-off-by: Scott M. Likens [email protected]

Refs #851

Updated extconf.rb to check if we have the header
Update mysql2_ext.h to load it if the def is set

Signed-off-by: Scott M. Likens <[email protected]>
# MariaDB 10.2+ Needs mysql/mysql_version.h loaded to get the constants we need
have_header('mysql/mysql_version.h')


Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - nit: please move to line 114, and use the prefix variable from the path test at line 100.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this as if we are just using the original ifdef we don't need to check against this?

@@ -20,6 +20,9 @@ void Init_mysql2(void);
#include <mysql/errmsg.h>
#include <mysql/mysqld_error.h>
#endif
#ifdef HAVE_MYSQL_MYSQL_VERSION_H
#include <mysql/mysql_version.h>
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the risk of a too many combinations, should this also be conditional on the mysql/ prefix as above?

Copy link
Contributor Author

@damm damm Jun 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this configuration file available always in other conditions? it looks like yes

I updated it so it matches both possibilities.

@sodabrew
Copy link
Collaborator

I also checked that mysql_version.h is always available in the MySQL Connector/C and MariaDB Connector/C distributions.

@sodabrew sodabrew merged commit 8ad9293 into brianmario:master Jun 21, 2017
@sodabrew sodabrew added this to the 0.4.7 milestone Jun 21, 2017
@AdamWill
Copy link

AdamWill commented Jul 22, 2017

Here's some background on the issue: https://jira.mariadb.org/browse/MDEV-13370

I think upstream's intent is that you shouldn't have to do explicit includes like this (and note that having that include will prevent the project from building against mariadb-connector-c 3.x; the header does not exist in that version). If I'm reading what they're telling me right, I think they must either add MYSQL_SERVER_VERSION (and other things from mysql_version.h) to mariadb_version.h, or document that we're not supposed to use it any more.

I haven't yet been able to find any clear documentation of what interfaces / definitions / etc. are intended to be lost in this rather poorly-explained switch to including mariadb-connector-c as libmariadb in the main mariadb codebase, and what things have only been lost unintentionally as part of the change :/

This was referenced Nov 11, 2017
stephanos added a commit to stephanos/mysql2 that referenced this pull request Apr 22, 2018
wiquan pushed a commit to AylaNetworks/mysql2 that referenced this pull request Mar 13, 2019
…#857)

Always include mysql_version.h

Signed-off-by: Scott M. Likens <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants