search

UnwashedMeme / clsql

Several branches containing different patches, see the wiki for branch descriptions
Other
20 stars 12 forks source link

Remove unnecessary C libraries for db-mysql. #11

Closed tdrhq closed 4 years ago

tdrhq commented 4 years ago

In particular, this fixes db-mysql for Windows, for which the dlls don't work. (32 bit doesn't work, and 64 bit doesn't exist. The Makefiles are literally decades old)

The C library isn't really required. The history of the file goes back to the initial commit, so it's hard to figure out the exact context in which it was required. It seems to have been used for two things:

  1. Returning 64 bit values: Perhaps at the time some Lisp implementation didn't support 64 bit in the FFI. I'm pretty confident all the CLs have 64-bit support in FFIs these days, so we don't need the C library for this purpose, and just use the direct mysql functions while returning :unsigned-long-long type.

  2. Accesssing structure of MYSQL_FIELD. The comments say this is so that we don't have to depend on mysql version (the C libary, if it's built against the right mysql.h, should automatically get this struct.) This makes sense, but this struct hasn't changed since Mysql 4.1, so we can assume that's it's more or less stabilized.

Finally, there were a bunch of unused functions that could easily be removed.

With this change, I'm finally able to use CLSQL+MySQL under Windows! So it fixes, for instance https://github.com/UnwashedMeme/clsql/issues/9

I ran tests on Lispworks 7.1.2 and SBCL running under Debian 10 64-bit. I didn't run tests under Windows, but I'm using it in my app and it's working find on a Windows laptop. (PS. CLSQL needs some other changes to work under LW7, I'll send a separate PR for that.)

tdrhq commented 4 years ago

@bobbysmith007 ping!

bobbysmith007 commented 4 years ago

Howdy and thanks for this patch. I have been away on vacation so am just now getting to this. Additionally I don't use lisp much (at all) anymore so it is definitely a back-burnered issue. That said, I am really grateful that you have invested in this community and project and I will try to get the patch merged in as it looks good.

tdrhq commented 4 years ago

@bobbysmith007 Thanks for the merge! Two questions:

  1. Quicklisp pulls clsql from git.kpe.io, but the README says to send PRs here. Is there a cronjob (and is that still active after all these years) that will push this change to git.kpe.io?

  2. There are a bunch of smaller changes I want to fix in CLSQL (some of these are already in PRs, some I planned to send after these were accepted). If you're looking for a maintainer, I'll be happy to take over, whether temporary or permanent. (https://www.linkedin.com/in/arnoldnoronha/)

Thanks! --Arnold

bobbysmith007 commented 4 years ago

I had push privileges at some point, but I'm pretty removed from the whole situation at this point. I probably still have push permissions, but I would have to look at whats what and get my head on straight again before I would feel comfortable pushing to him right now. Kevin is a good guy though, and is generally available to talk to and get stuff merged in.

If you are working on it and making improvements I would be happy to add you as a maintainer. I have software that still relies on this, but its been in long-term-maintenance mode for years. I don't anticipate ever really getting around to working extensively in lisp again so a newer more active maintainer is definitely appreciated.

tdrhq commented 4 years ago

@bobbysmith007 I'll absolutely love to be maintainer. I don't plan any API breaking changes, just small cleanups here and there to make sure CLSQL is playing along with the latest versions of lisps and libraries. I'll be able to review PRs too.

bobbysmith007 commented 4 years ago

@UnwashedMeme , Please add @tdrhq to the maintainers options for the repo when you have a sec. I dont seem to have access to do this. Thanks!

tdrhq commented 4 years ago

@UnwashedMeme ping!