-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Swift3 #502
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
Swift3 #502
Conversation
@stephencelis it might make sense to skip #475, unless you want to keep a separate branch for 2.3 compatibility? Alternatively we could merge #475 first and then have a swift3 branch in parallel, but given the amount of changes this will become a maintenance nightmare. |
It's look great. How can I integrate it on a Pod file ? |
@iLandes try pod 'SQLite.swift', :git => 'https://github.com/stephencelis/SQLite.swift.git',
:branch => 'swift3-mariotaku' |
can I get a carthage line to integrate this branch into my project? thanks in advance. |
Many thanks to @mariotaku for picking up my branch and getting it across the finish line! And many more thanks to @jberkel for doing an incredible job coordinating this migration. It's a big one! |
It'd be nice to tag 2.3 compatibility, though I don't think maintaining both at once is feasible. |
start: UnsafePointer(bytes), count: length | ||
))) | ||
public init(bytes: UnsafeRawPointer, length: Int) { | ||
// TODO correct count |
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.
@mariotaku what is needed here?
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 works in tests and actual app, though I'm not sure whether my changes correct, maybe we could take a look at swift 2.3 branch.
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.
looks good to me, I added a bunch of tests to verify the behaviour
@@ -0,0 +1,4 @@ | |||
module CSQLite [system] { | |||
header "/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS10.0.sdk/usr/include/sqlite3.h" |
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.
This hard-coding of the Xcode path causes build errors (Carthage, CocoaPods or just building in Xcode) for all users who don't have their copy of Xcode 8.x at that path.
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.
What do you suggest to do instead?
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.
This hard-coding is required for CocoaPods. If you include SQLite.swift as a sub-project or via Carthage it uses an SDK-relative path instead.
I've raised this issue with CocoaPods in the past: CocoaPods/CocoaPods#3942
We're always open to alternative solutions for CocoaPods users, though, and would happily take contributions that solve the problem more elegantly for them!
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.
@stephencelis this also affects Carthage builds – without the modulemap, the CSQLite
imports cannot be resolved. Even worse, they're still required after the framework has been built (see #492).
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.
think this is solved now with db8f39b
@@ -0,0 +1,4 @@ | |||
module CSQLite [system] { | |||
header "/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator10.0.sdk/usr/include/sqlite3.h" |
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.
As above, this causes build errors for all users who don't have their copy of Xcode 8.x at that path.
@@ -0,0 +1,4 @@ | |||
module CSQLite [system] { | |||
header "/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.11.sdk/usr/include/sqlite3.h" |
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.
As above, this causes build errors for all users who don't have their copy of Xcode 8.x at that path.
@@ -0,0 +1,4 @@ | |||
module CSQLite [system] { | |||
header "/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/usr/include/sqlite3.h" |
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.
As above, this causes build errors for all users who don't have their copy of Xcode 8.x at that path.
@@ -1,4 +1,4 @@ | |||
module CSQLite [system] { | |||
header "/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.11.sdk/usr/include/sqlite3.h" | |||
header "/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/sqlite3.h" |
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.
As above, this causes build errors for all users who don't have their copy of Xcode 8.x at that path.
* Add modulemap for MacOSX 10.11 * Update modulemap * Use iPhone SE simulator to fix test failures (travis-ci/travis-ci#6422)
df86417
to
2c9fd0c
Compare
PR is ready for another round of review. To use in your application: CocoaPods: pod 'SQLite.swift', :git => 'https://github.com/stephencelis/SQLite.swift.git', :branch => 'swift3-mariotaku' Carthage:
|
I was just upgrading my app and something isn't working for watchOS. Wanted to leave a note about it. Will be digging in deeper to understand why. EDIT: Resolved. Sorry my fault. I have Xcode installed at |
Since I did not hear any more bug reports/complaints I just went ahead and merged the PR. The modulemap problems for non-CocoaPods users should hopefully be gone now. |
Supersedes #501, #475