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

(#4530) Include the mysqld_error.h header in mariadb-connector-c #4532

Merged
merged 1 commit into from
Feb 13, 2021
Merged

(#4530) Include the mysqld_error.h header in mariadb-connector-c #4532

merged 1 commit into from
Feb 13, 2021

Conversation

floriansimon1
Copy link
Contributor

Specify library name and version: lib/1.0

  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

@CLAassistant
Copy link

CLAassistant commented Feb 10, 2021

CLA assistant check
All committers have signed the CLA.

@conan-center-bot
Copy link
Collaborator

All green in build 1 (ac0d2f76bec914fda9f396b1c72f59f422c9dfc2)! 😊

  • mariadb-connector-c/3.1.11: Generated 88 packages. All logs here

@floriansimon1
Copy link
Contributor Author

I had to reupload my commit with my correct email in order to sign the CLA.

@floriansimon1
Copy link
Contributor Author

@SpaceIm I would appreciate it if you could review this, given you originally wrote that recipe 😃

@conan-center-bot
Copy link
Collaborator

All green in build 2 (e2eddd52bb55eec1e693719713a329f5ff790e0d)! 😊

  • mariadb-connector-c/3.1.11: Generated 88 packages. All logs here

@prince-chrismc
Copy link
Contributor

Why do you need this file? Trying to see if it meets #3903

Looking through the code it's completely private and I do not see why it's required.
https://github.com/mariadb-corporation/mariadb-connector-c/search?q=ma_server_error
This is absolutely a discrepancy because of our cmake_wrapper I am just curious before I approve 👍

It was also explicitly excluded mariadb-corporation/mariadb-connector-c@11321f1 (see commit message) though I am not sure the meaning of "server"

@floriansimon1
Copy link
Contributor Author

Hi,

First of all, thanks for your interest in my pull request! I understand your concerns, and I will try to address them.

So, our source code uses these error codes to interpret what the errors returned by unsigned int STDCALL mysql_stmt_errno(MYSQL_STMT * stmt); (mariadb_stmt.h) mean. Without that header, it's difficult to understand what the error codes mean. They're basically just random numbers.

I have tried looking for another way to source the definition of these errors, but I haven't managed to find anything saying that you need a second library of headers to get a definition of error codes. I guess that makes sense, since MariaDB is supposed to be a drop-in replacement for MySQL. It wouldn't make sense to mix the two. And they don't really provide a set of headers, as far as I can tell.

If you download their distribution there, the file is in there, so I wouldn't say it's private. I think they would have removed it from the release otherwise.

Moreover, the IS_SUBPROJECT condition seems to suggest that this project can be integrated in other projects (such as the Maria DB server), in which case this file is undesirable. It's not the case here: we're not a subproject of a bigger whole, we're a standalone library.

I could try setting IS_SUBPROJECT to false, but from what I gathered it would be kind of pointless. Doing so would mean pulling in some Windows specific code setting variables such as build types, compile flags (can't remember the exact details) which I think would interfere with Conan. My thinking is this would lead to some more patching. I'd rather just pull in the header I need than make the code too complex.

Worst case scenario, if we determine we do in fact need to set IS_SUBPROJECT to false, we can remove my very simple patch and go for the more complex option. Doing the reverse is a bit more complex, given how unintuitive that would be.

Now, take what I say about IS_SUBPROJECT with a grain of salt. I haven't confirmed that with any official source. All I know is that, before our move to Conan, we had that file and we've always included it. When we moved to the Conan recipe forked off something we got from a random GitHub repo, we still had that header. When I tried using the official recipe rather than the hack we had working, it stopped compiling.

Unfortunately, that issue prevents us from moving to the official recipe.

Thanks a lot for your time!

@floriansimon1
Copy link
Contributor Author

PS: I think the Maria DB server is the actual database daemon, as opposed to the client which talks to it or the connector which is meant to be used as a library in external programs.

@floriansimon1
Copy link
Contributor Author

See mariadb-corporation/mariadb-connector-c@d3644be for a commit message explaining what ma_server_error is.

The file itself is here.

@prince-chrismc
Copy link
Contributor

Seems like it's a common problem... https://github.com/search?q=mysqld_error.h&type=commits
MySQL problems 🤷

@SSE4 SSE4 requested a review from uilianries February 12, 2021 15:00
@conan-center-bot conan-center-bot merged commit 3a84410 into conan-io:master Feb 13, 2021
@floriansimon1 floriansimon1 deleted the bugfix/mariadb branch February 16, 2021 18:44
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.

6 participants