-
-
Notifications
You must be signed in to change notification settings - Fork 76
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
Full Unicode support + MariaDB compatibility #27
Conversation
@@ -18,7 +22,7 @@ public final class Bind { | |||
public typealias CBind = MYSQL_BIND | |||
|
|||
/// MySQL represents Characters using signed integers. |
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.
can we get this comment updated too
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.
Sure, I can fix that. But I'm also curious why it was written in the first place, because I want to make sure I'm not overlooking something. Maybe @tannernelson will know.
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.
I'm trying to figure that out as well to make sure we're not setting something off.
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.
Agreed. After further inspection and testing, I think we should be safe. Char
is only used while populating the buffer, and MySQL has no concept of "signed" vs "unsigned" - it's just raw bytes in a buffer. So in reality UInt8
accounts for everything Int8
was handling, but it also accounts for the characters that were causing overflow.
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.
@collinhundley sounds perfect, I know we generally prefer [UInt8]
type. @tannernelson can you give final merge word on this one! You have much more MySQL context.
👍 from me.
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.
It would be great to get this merged. I need to make some changes to my fork but they'll affect this merge if I push them right now...
Hey @collinhundley, awesome contribution here! I have a couple questions about MariaDB, and particular the compiler flag / imports you're using. Can you tell me where the CMariaDBLinux etc. live and how they're being integrated? |
@loganwright Check out this PR for the C module here: vapor-community/cmysql#3 Once that PR has been accepted, a new module named -Xswiftc -DMARIADB which will cause this package to import that module instead of |
@collinhundley Does MariaDB not work with macOS? |
@tannernelson It does work on macOS. For reasons I don't understand, the C library is inconsistent between macOS and Linux. On both platforms it is a drop-in replacement for MySQL (with identical headers, etc) but for some reason the files/directories are named differently on Linux. On macOS the library/headers are found in the same location as MySQL, which is why there is no need for a separate It's possible that this could change depending on the user's operating system and/or MariaDB version, but I've tested it to work on Ubuntu 15.10/16.04 and macOS 10.11/10.12. |
Cool, sounds good. :) Thanks for the contribution! |
This PR fixes the Unicode issue discussed in #25, and adds the ability to connect to MariaDB instead of MySQL.
In order to support MariaDB without breaking the existing MySQL support, a new compiler flag was added:
-DMARIADB
. If this flag is passed to the compiler, it will link against the MariaDB C moduleCMariaDBLinux
. If omitted, it will continue to link against the MySQL module as usual.See PR #3 in vapor/cmysql, which is also required for these changes to take effect.