Skip to content

patch for issue #878 #880

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

Closed
wants to merge 1 commit into from
Closed

Conversation

actsasflinn
Copy link

Newer version of mariadb seem to have moved or renamed a few header files.

#include <server/mysql_com.h>
#include <errmsg.h>
#include <mysqld_error.h>
#include <server/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.

I wonder why these headers are spread out across two paths like this. What did they change?

Copy link
Contributor

@jaredbeck jaredbeck Aug 23, 2017

Choose a reason for hiding this comment

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

Looks like MariaDB/server@3ec96c1 is the culprit?

MDEV-13370 Ambiguous behaviour regarding installation of header files
install all server includes under /usr/include/mysql/server/ and C/C includes under /usr/include/mysql/

https://jira.mariadb.org/browse/MDEV-13370

@jaredbeck
Copy link
Contributor

jaredbeck commented Aug 23, 2017

I have tested this PR with the following:

mysql --version
mysql  Ver 15.1 Distrib 10.2.8-MariaDB, for osx10.12 (x86_64) using readline 5.1

gem uninstall mysql2
# Uninstall all versions

# Edit my Gemfile
gem "mysql2",
  git: "https://github.com/actsasflinn/mysql2.git",
  ref: "76ee6acb09077503a579f4ba5aae9b4137144957"

bundle
Using mysql2 0.4.9 from https://github.com/actsasflinn/mysql2.git (at 76ee6ac@76ee6ac)

Apparently when you can use git: to download a gem it still compiles native extensions.

file /Users/jared/.rbenv/..snip../gems/mysql2-76ee6acb0907/ext/mysql2/mysql2.bundle 
Mach-O 64-bit bundle x86_64

So, I think this is a reasonable test of this PR. I guess I'd recommend merging.

@sodabrew
Copy link
Collaborator

Thanks for the link to the upstream bug! What I took from it is that if we make this suggested change, it will allow mysql2 to draw from the MariaDB server package headers. I actually want to be sure that the mysql2 gem can compile and run with only the MariaDB Connector/C client package installed.

@jaredbeck
Copy link
Contributor

jaredbeck commented Aug 24, 2017

Thanks for the link to the upstream bug!

You're welcome! It definitely took some research (and some git log) to track it down.

What I took from it is that if we make this suggested change, it will allow mysql2 to draw from the MariaDB server package headers. I actually want to be sure that the mysql2 gem can compile and run with only the MariaDB Connector/C client package installed.

Oh, good point! But, please be aware that in homebrew the mariadb and mariadb-connector-c packages are marked as "conflicting", ie. mutually exclusive. We must install mariadb for local development, so we cannot install the mariadb-connector-c package. So, mysql2 must be able to compile without the mariadb-connector-c package.

@sodabrew
Copy link
Collaborator

Oh, that's interesting that the packages conflict currently. If it's the case that this PR allows mysql2 to operate with the MariaDB-server-client package, and it already works with the MariaDB-connector-c package, then it's sort of a no-brainer to just merge this as-is and let the have_header checks sort things out.

Having this new Connector/C on the scene really makes the testing matrix complicated... :|

@jaredbeck
Copy link
Contributor

If it's the case that this PR allows mysql2 to operate with the MariaDB-server-client package, and it already works with the MariaDB-connector-c package, then it's sort of a no-brainer to just merge this as-is and let the have_header checks sort things out.

Fine by me.

It's no reason to hold up the PR, but just FYI I'm going to try to upgrade our staging server to mariadb 10.2.8 so I can test this PR against the ubuntu packages. I'm not sure how valuable that test is, because we install both the mariadb-server and the libmariadb-dev packages on our servers, but .. can't hurt.

@jaredbeck
Copy link
Contributor

jaredbeck commented Aug 28, 2017

.. I'm going to try to upgrade our staging server to mariadb 10.2.8 so I can test this PR against the ubuntu packages. I'm not sure how valuable that test is, because we install both the mariadb-server and the libmariadb-dev packages on our servers, but .. can't hurt.

I've performed this test and am now running mariadb 10.2.8 on my staging server and using the following in my Gemfile:

gem "mysql2",
  git: "https://github.com/actsasflinn/mysql2.git",
  ref: "76ee6acb09077503a579f4ba5aae9b4137144957"

@sodabrew anything else we need to do before merging this? Just a reminder this is a priority because homebrew users are completely unable to use the (official) mysql2 gem right now.

@jaredbeck
Copy link
Contributor

@sodabrew two week ping. Anything else you need from us?

@ghost
Copy link

ghost commented Sep 8, 2017

I think this is the wrong approach.

The real fix is to not include mysql_com.h (renamed to mariadb_com.h) nor mysql_version.h (renamed to mariadb_version.h) when they don't exist as the #include <mysql.h> will pull in their mariadb counterparts.

@actsasflinn
Copy link
Author

@grknight I agree, the gem should be less coupled to file locations. There's some additional changes to make in order to handle differences in what is defined in mysql_version.h and mariadb_version.h - they do not appear to be equal. Using server/mysql_version.h retains compatibility.

At a bare minimum referencing LIBMYSQL_VERSION doesn't seem to be supported by mariadb_version.h
See: https://github.com/brianmario/mysql2/blob/master/ext/mysql2/client.c#L56-L60

In the short term does it make sense to accept the patch and then circle back on refactoring to a better long term solution?

@actsasflinn
Copy link
Author

I've got a second fix implementation addressing @grknight concerns... not sure if it holistically addresses the coupling issues.

actsasflinn@f60600d

@sodabrew sodabrew mentioned this pull request Nov 11, 2017
@sodabrew
Copy link
Collaborator

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!

@sodabrew sodabrew closed this Nov 11, 2017
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.

3 participants