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

Modified to support using framework #353

Closed
wants to merge 5 commits into from
Closed

Modified to support using framework #353

wants to merge 5 commits into from

Conversation

robertmryan
Copy link
Collaborator

To use FMDB in framework, one should eliminate all non-modular headers. The main culprit here is #import <sqlite3.h>. The thing is, FMDB uses SQLite types and constants in these headers, so they had to be removed.

  1. Removed #import <sqlite3.h> from all of the headers.
  2. Moved sqlite3 and sqlite3_stmt declarations in FMDatabase.h into FMDatabasePrivate.h. FMDB source that needs these will now `#import "FMDatabasePrivate.h" and will have access to them.
  3. Removed #if SQLITE_VERSION_NUMBER >= xxx logic from the headers.
  4. lastInsertRowId now returns long long int, not sqlite3_int64.
  5. sqliteHandle now returns void * not sqlite3 *.
  6. The savepoint and release savepoint and rollback now always compile regardless of SQLite version (but you'll get SQLite errors if you try those commands with earlier SQLite versions).
  7. makeFunctionNamed has changed the block parameter type.
  8. The FMDatabaseAdditions routines for applicationID, much like the savepoint routines, will be compiled regardless of SQLite version.
  9. Some random usage of deferencing ivars from another class have been replaced with accessor methods.

Note, with all of these changes, in order to conform to modular headers, this is not entirely backward compatible. They'll have to make changes if

  • If they referenced the _db variable themselves (which they shouldn't be doing anyway; this never should have been in the public interface to start with);
  • If they referenced _statement variable of FMStatement (which is even less likely); or
  • If they used makeFunctionNamed, the signature of the block has changed (now uses void instead of SQLite types).

To use FMDB in framework, one should eliminate all non-modular headers. The main culprit here is `#import <sqlite3.h>`. The thing is, FMDB uses SQLite types and constants in these headers, so they had to be removed.

1. Removed `#import <sqlite3.h>` from all of the headers.

2. Moved `sqlite3` and `sqlite3_stmt` declarations in `FMDatabase.h` into `FMDatabasePrivate.h`. FMDB source that needs these will now `#import "FMDatabasePrivate.h" and will have access to them.

3. Removed `#if SQLITE_VERSION_NUMBER >= xxx` logic from the headers.

4. `lastInsertRowId` now returns `long long int`, not `sqlite3_int64`.

5. `sqliteHandle` now returns `void *` not `sqlite3 *`.

6. The `savepoint` and `release savepoint` and `rollback` now always compile regardless of SQLite version (but you'll get SQLite errors if you try those commands with earlier SQLite versions).

7. `makeFunctionNamed` has changed the block parameter type.

8. The FMDatabaseAdditions routines for `applicationID`, much like the `savepoint` routines, will be compiled regardless of SQLite version.

9. Some random usage of deferencing ivars from another class have been replaced with accessor methods.

Note, with all of these changes, in order to conform to modular headers, this is not entirely backward compatible. They'll have to make changes if

 - If they referenced the `_db` variable themselves (which they shouldn't be doing anyway; this never should have been in the public interface to start with);

 - If they referenced `_statement` variable of `FMStatement` (which is even less likely); or

 - If they used `makeFunctionNamed`, the signature of the block has changed (now uses `void` instead of SQLite types).
@robertmryan
Copy link
Collaborator Author

Gus, this is not really a request to merge this into FMDB, but rather proof of concept of what one needs to do in order to put FMDB in its own framework. If we do changes like this, it then solves issue #309 because a framework can then use FMDB in its own framework.

I'm not entirely happy with this pull request though, because it violates the goal of 100% backward compatibility. The issues are modest (and enumerated in the last three bullet points above), but are there. But it was the only way that leapt out at me that allowed us to put FMDB in its own framework.

What do you think?

@robertmryan
Copy link
Collaborator Author

At the very least, this might be a candidate for FMDB 3.0 (removing dependency on non-modular sqlite3.h from the public FMDB headers).

@Kirow
Copy link

Kirow commented Mar 30, 2015

@robertmryan , thx for update. I still have 1 little problem when installing it from cocoapods.

use_frameworks!
pod 'FMDB', :git => 'https://github.com/robertmryan/fmdb.git'

Problem with FMDatabasePrivate.h. I need manually to remove it from from umbrella, and move it to private header in build phases settings. And this should be done each time when pods updated, pretty annoying. Do you know how to fix it?

@ccgus
Copy link
Owner

ccgus commented Mar 30, 2015

@robertmryan Yea, making FMDB work perfectly with swift should be a part of 3.0. I'm a bit annoyed it doesn't just work as it is.

@robertmryan
Copy link
Collaborator Author

@Kirow Good point. We should make sure we handle that more gracefully if this is ever merged with the master branch of FMDB. Frankly, I didn't alter the project to create the framework automatically, because I wasn't sure what Gus wanted to do with the existing static library targets. I assume we should retire those targets and create framework targets, but again that's a big enough change that we might want to make that part of 3.0. (And, as I suggested above, I suspect that the lack of backward compatibility, as minor as those changes are, means that this might better be deferred until of 3.0.)

But feel free to try it out and make sure it works for you. That might help Gus assess whether he thinks this sort of change should be incorporated in 3.0 or not.

@robertmryan
Copy link
Collaborator Author

@ccgus By the way, I noticed that the Swift extensions didn't work with Swift 1.2 (part of Xcode 6.3 beta 4. So I modified them so they work with both Swift 1.1 and Swift 1.2. Even if you don't run with the rest of this pull request, you might want to integrate these changes (once you've tested them to your satisfaction).

Separate topic, but I notice that I've neglected to add Swift test classes here. (That's very bad: I shouldn't be adding code that isn't part of a test suite.) Would you like me to add them? The only reason I hesitate is that I don't know if that has implications for earlier Xcode users (but I suspect we've already crossed the Rubicon on that one).

@robertmryan
Copy link
Collaborator Author

On the presumption that you are not inclined to merge this framework related stuff into the master branch quite yet, should we move it into a separate branch (perhaps "framework") so that you can merge it and we get it out of the way (keeping it for archival purposes as well as for anyone who might need it for doing Swift frameworks)?

@ccgus
Copy link
Owner

ccgus commented Mar 30, 2015

I haven't had a chance to look this over yet, sorry. So I'm not ignoring, just battling some other stuff right now.

Kirow and others added 2 commits April 1, 2015 14:08
fix for use_frameworks! usage. Exclude FMDatabasePrivate.h from public headers
@ccgus
Copy link
Owner

ccgus commented May 4, 2015

Alright, after a delay (sorry) and reviewing the code, I'm not going to merge this as it is for FMDB 2.x. The changes of not including sqlite.h, as well as hiding the handle will cause compile errors for devs. Here's my proposal though. I've started a 3.0 branch a little while back, and we can merge these into those. Then, add a line to the swift info in the readme saying "hey- if you want swift 1.2 support, grab this new branch which is in progress, but hey is still passes all tests so you should be good to go. And since you're using swift, you're OK with stuff breaking in the future, right?"

comments?

@robertmryan
Copy link
Collaborator Author

This pull request is tackling two entirely different issues: First is the changes necessary to put FMDB in a framework and I agree that this should be deferred to 3.0 for reasons we discuss above. I might suggest merging it into a separate FMDB branch, though, so people who need frameworks can refer to an official FMDB branch vs my own fork. (And I say this for tactical reasons, namely that I need to somehow get this out of the way so I can continue to contribute to FMDB. Right now, this unmerged pull request is "in the way". lol.)

The second issue is the Swift 1.2 support. This is tackled by https://github.com/robertmryan/fmdb/commit/f0479b7a638e8f9efe5ab23d5a62984a0eb3674e (and https://github.com/robertmryan/fmdb/commit/9ec2151cff143c20a82440bd8ac72c51f49d7609). This is entirely backward compatible and, in my opinion, should be merged in at our earliest convenience, allowing us to use FMDB with the current production versions of Xcode. I don't think we want to tell people that they can't use these Swift extensions. Your call.

@ccgus
Copy link
Owner

ccgus commented May 4, 2015

Alrighty- I couldn't figure out how to merge just that change set, so I copied them in. Looks good? I look forward to doing this again after WWDC, when swift gets more changes :)

@robertmryan
Copy link
Collaborator Author

Yep, looks great. My apologies for burying the Swift 1.2 issues in this broader pull request. I was fixing the framework issue and stumbled across the Swift 1.2 issue and just fixed it, not thinking about the fact that, in the end, the framework might have ended up being a separate branch for the time being.

Anyway, the Swift 1.2 commit looks good. Thanks!

Would you like me to do a separate pull request for the framework-related changes, this time putting that in a separate branch?

@ccgus
Copy link
Owner

ccgus commented May 4, 2015

Yep, that'd be great. Danke.

@ccgus
Copy link
Owner

ccgus commented May 4, 2015

Oh- one thing. I'd rename the "FMDatabasePrivate.h" file to, "FMDatabaeSqlite.h". It's not really private stuff, it's more things that swift can't seem to cope with. Maybe … well, what needs to happen so swift can cope with it?

@robertmryan
Copy link
Collaborator Author

OK, I'll do a separate pull request (probably later this week). And I'll do it as a separate branch.

BTW, my concept in "private" is a hint of my bias, that these are implementation details that should be hidden from client apps. There are actually a ton of properties and ivars that are (IMHO) implementation details that are being exposed to client apps, which I hope will become private in FMDB 3.

But I'll use FMDatabaseSQLite.h if you prefer.

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.

None yet

3 participants