mysqlclient-sys
mysqlclient-sys copied to clipboard
sgrif
Wrong calling convention for 32-bit Windows version
All extern functions are marked as extern "C", but in mysqlclient.lib some functions have __stdcall convention.
Is there a reason this hasn't been merged?
The lack of review is slightly concerning, is there a lack of reviewers? I could offer to do so but I'm not knowledgeable about specifics of mysql, and I am not a user of this crate (Or any dependant ones) so I'm not in any particular hurry. Would avoiding as much duplication in the two versions of the bindings for windows be desireable? (Through a macro or something similar, if possible)
cc @sgrif
Can I assume support for 32-bit Windows will never be a thing? I know 32-bit Windows is not really commonplace in new installations, but it's still required in some legacy projects because of hardware limitations. Upgrading those to the Rust / Diesel ecosystem is impossible because of this.
The Windows support of this crate is already lacking, building for 64 bit Windows is already painful. I can't believe there's still no bundled support (like SQLite has). It has been requested a long time ago ( https://github.com/sgrif/mysqlclient-sys/issues/30 ), without any response for more than a year...
If this PR won't be merged, is it possible the Bundling support would solve 32-bit support as well?
Can I assume support for 32-bit Windows will never be a thing?
That really depends on if someone is willing to implement/fix that. I personally cannot do that as I do not have access to a 32-bit windows.
As for the linked PR above. That one has a number of problem that prevents it from being merged:
- It is really large. Reviewing and merging a +2700 line PR is out of scope for me. I'm not willing to review such a large PR in my free time.
- There are merge conflicts that needs to be resolved
- The PR does not provide any evidence that it resolves the underlying issue. I would expect there at least some text that something has been tested and it works. Even better would be some passing CI run, for example using diesel CI (by for example opening a PR there that uses the patched version).
I can't believe there's still no bundled support (like SQLite has). It has been requested a long time ago ( #30 ), without any response for more than a year...
Please remember that this crate is maintained by people in their free time. So feature requests are just feature request. They don't need to be implemented or even answered. That does not imply that we don't want to have this feature, just that we do not have the time to work on that, because we consider other things more important. At least for me better windows support is something that would be nice to have, but it nothing I'm willing to invest my personal time to develop that. I do not even have the necessary equipment for that as I do not own a windows machine. If you consider that important for your use case consider one of the following options:
- Contribute a working fix yourself
- Pay someone to develop the fix for you
If this PR won't be merged, is it possible the Bundling support would solve 32-bit support as well?
We are interested in seeing that implemented and we happy to accept a well written PR there. At least I personally do not have the time to implement that by myself so do not expect that to just happen. Remember: OpenSource is not about that other people just implement stuff for free for you. The points mentioned above about the open PR also apply for a potential bundling PR.
Sorry if my comment sounded harsh, I did not mean it as a personal attack, it was just the frustration caused by days of debugging. By the part "can't believe there's still no bundled support" I meant I can't believe there were not enough demand and drive so far in the community to achieve this, definitely wasn't trying to belittle your personal work.
The only criticism I have is the lack of communication on the PRs, which seems a bit discourageing for potential contributors. I understand that "they are just feature requests", and you don't always have the time to review a PR or solve the conflicts or whatever, which is totally fine. But it would be nice to state this in a comment, so people stumbling on this issue can see what the blocking factor is. And commenting that a feature would be welcome (with proper testing and such) is a must have in my opinion, how else would the contributors know that something is worthwile to work on or not, because it won't be accepted anyway. Your current comment was really helpful from this point of view though, made it perfectly clear what the current state and limitations are, thanks for the clarification. Especially thank you for the quick reply!
I'm using this crate in a professional project by the way, there might be enough demand to validate my work on contributing a PR, but we'll see how the project goes. They'll probably go with the path of least resistance, which unfortunately might be changing the hardware / OS to 64 bit, if it's planned anyway...
Theoretically speaking, if I can get a budget for some work on the crate, which one would be the better investment in time, in your opinion? 32bit support or bundled support? Would the bundled support automatically solve the 32 bit support as well? Unfortunately I have no idea how bundling actually works, so I can't tell if these are totally unrelated issues or not.
Thanks for your work so far!
The only criticism I have is the lack of communication on the PRs, which seems a bit discourageing for potential contributors. I understand that "they are just feature requests", and you don't always have the time to review a PR or solve the conflicts or whatever, which is totally fine. But it would be nice to state this in a comment, so people stumbling on this issue can see what the blocking factor is. And commenting that a feature would be welcome (with proper testing and such) is a must have in my opinion, how else would the contributors know that something is worthwile to work on or not, because it won't be accepted anyway.
That's certainly right. The problem is that I only have the corresponding access rights to do that since a few months. Everything that happened before that is not on my radar, as I do not have the time to comment on all old potential stale PR's.
Would the bundled support automatically solve the 32 bit support as well?
At least from my point of view these are two unrelated points. Bundling changes the search for a linked library from looking for a system provided library to statically linking a self build version. It should not change the generated bindings at all. (It's mostly a change to the build.rs file).
Adding 32bit windows support would involve changing the bindings. I'm not sure if that could be done in a more minimal way as the linked PR. So that would likely require some research first what needs to be changed at all. (Not in terms of change, but in terms of the size of the change.)