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

Move sqlcipher.xcodeproj targets into GRDBCipher.xcodeproj #481

Closed
wants to merge 8 commits into from

Conversation

darrenclark
Copy link
Contributor

Proof of concept related to: #480

Essentially just brings the sqlcipher.xcodeproj targets into GRDBCipher.xcodeproj

screen shot 2019-02-17 at 10 59 35 am

Not working

  • Doesn't work with new Xcode build system. Since the sqlcipher-amalgamation target builds sqlite3.c, I don't think the new build system knows about this, so it fails the build immediately saying "sqlite3.c is missing"

    Might need to use a custom run script build phase that explicitly specifies sqlite3.c as an output

  • Various failing tests with getting back SQLITE_ERROR instead of SQLITE_MISUSE. Not sure about this.

    screen shot 2019-02-17 at 10 59 10 am
  • Doing a "clean" in xcode doesn't clean sqlite3.c yet

Proof of concept related to:
groue#480

Not working:

- Various failing tests with getting back `SQLITE_ERROR` instead of
  `SQLITE_MISUSE`.  Not sure about this..

- Doesn't work with new Xcode build system.  Since the
  `sqlcipher-amalgamation` target builds `sqlite3.c`, I don't think the
  new build system knows about this, so it fails the build of
  the `sqlcipher-ios` target immediately say that "sqlite3.c is missing"

  Might need to use a custom run script build phase that explicitly sets
  the outputs.
@groue
Copy link
Owner

groue commented Feb 18, 2019

Thank you @darrenclark for this exploration! Your help is very welcome.

It would be great to turn this proof of concept into a shippable product:

  • The ideal setup would support the same deployment targets as GRDB 4: Xcode 10.0+, Swift 4.2+, iOS 9+, macOS 10.9+

  • Support for CLI builds for iOS and macOS

  • We would be able to document a reliable installation technique that does not involve CocoaPods or Carthage (aka "manual installation")

  • And the cherry on the cake is that brave users don't have too much work to setup their Carthage builds.

Phew! Let's see how far we can go ;-)

Let's start from your questions.

Doesn't work with new Xcode build system. Since the sqlcipher-amalgamation target builds sqlite3.c, I don't think the new build system knows about this, so it fails the build immediately saying "sqlite3.c is missing". Might need to use a custom run script build phase that explicitly specifies sqlite3.c as an output

Yes, I think target outputs are a good way to instruct the new build system about dependencies.

Doing a "clean" in xcode doesn't clean sqlite3.c yet

Maybe the explicit target output will clean it?

Various failing tests with getting back SQLITE_ERROR instead of SQLITE_MISUSE. Not sure about this.

The screenshot tests that iterating a GRDB cursor after its first error keeps on outputting an error. This is important because if it were not the case, this would be a big breaking change. The canonical cursor iteration code relies on the fact that the first cursor error is fatal:

// The canonical cursor iteration code
do {
    while let value = try cursor.next() {
        // Handle value
    }
} catch {
    // Cursor is dead
}

This GRDB contract currently relies on the raw SQLite behavior. The failing test shows that SQLite has changed: sqlite3_next() now returns SQLITE_MISUSE after sqlite3_next() has previously returned an error. This is still OK: we'll just have to change the test (any error after the first error should be enough for the test to pass).

@groue groue added this to the GRDB 4.0.0 milestone Feb 18, 2019
@groue
Copy link
Owner

groue commented Mar 1, 2019

Hello again @darrenclark, do you feel like pushing this proof of concept up to the mergeable pull request?

@darrenclark
Copy link
Contributor Author

Hey @groue, sure, it’s been a hectic couple weeks for me so I haven’t had much time.

I’ll try to take a look at it this weekend

@groue
Copy link
Owner

groue commented Mar 1, 2019

That would be great!

@darrenclark
Copy link
Contributor Author

darrenclark commented Mar 6, 2019

This is still on my radar, things have been a little chaotic over the last couple days and haven't had a chance to look into it yet

@groue
Copy link
Owner

groue commented Mar 6, 2019

Don't worry :-) The GRDB-4.0 backlog #479 is slowly progressing on my side too, and we don't have to rush.

1. Now uses a shell script build phase (rather than external build
system target)

2. Does the `configure` + `make` in a different (.gitignore-ed)
directory so it doesn't show up as changes in git
@darrenclark darrenclark changed the title [Proof of Concept] Move sqlcipher.xcodeproj targets into GRDBCipher.xcodeproj Move sqlcipher.xcodeproj targets into GRDBCipher.xcodeproj Mar 9, 2019
@darrenclark
Copy link
Contributor Author

Got a little further on this. Now working with the new build system & building for Mac now too.

TODO:

  • Loop back over all the build settings to make sure they are correct / clean up commented out build settings in xcconfig
  • Update the tests for SQLITE_ERROR vs. SQLITE_MISUSE
  • Test Cocoapods, Carthage, and xcodebuild builds

@groue
Copy link
Owner

groue commented Mar 9, 2019

Thanks @darrenclark!

Update the tests for SQLITE_ERROR vs. SQLITE_MISUSE

Yes. As I said above: any error after the first error should be enough for the test to pass. This is enough to validate the Cursor intent.

Test Cocoapods, Carthage, and xcodebuild builds

The Makefile already contains tests. You may not have to update them:

  • CocoaPods: test_install_GRDBCipher_CocoaPods and test_CocoaPodsLint_GRDBCipher.
  • Carthage: test_CarthageBuild
  • xcodebuild: test_framework_GRDBCipher

@groue
Copy link
Owner

groue commented Mar 9, 2019

About tests:

You can run them locally, on your mac, with make test.

You can activate more tests with Travis: just click on the "Ready for review" button.

Now compiling with SQLITE_OMIT_AUTORESET defined.  Enabling this ensures
error return values consistent with Apple's sqlite3 for sqlite3_step
@darrenclark
Copy link
Contributor Author

@groue I think this is ready.

I wasn't able to run the full suite locally, have some Xcode / Cocoapods configuration issues at the moment. Will keep an eye on TravisCI.

After digging into the failing tests a little more, I noticed it was a compiler flag causing the (https://www.sqlite.org/compile.html#omit_autoreset). It seems SQLiteCustom is also compiled with this SQLITE_OMIT_AUTORESET (and based on the tests, I think Apple's sqlite is too), so I added it here.

  • I updated the sqlcipher submodule to the v4.0.1 tag, I think https://github.com/groue/sqlcipher needs those commits from upstream

  • Do you have the button to set this to "Ready for Review"? I don't have it on my screen:

Screen Shot 2019-03-09 at 2 48 41 PM

@groue groue marked this pull request as ready for review March 9, 2019 22:09
@groue
Copy link
Owner

groue commented Mar 9, 2019

Travis support for Github is not extended yet to the new "draft PR": hitting the "Ready for review" button (I see it), and turning this draft PR into a real one did not trigger Travis checks. OK, I'll check that in the few next hours.

@darrenclark
Copy link
Contributor Author

@groue I can just re-open the PR if you want?

@groue
Copy link
Owner

groue commented Mar 10, 2019

Hello @darrenclark, I've just pushed an empty commit, and this was enough to trigger the Travis hook :-)

Meanwhile, here is some news on the GRDB 4 front:

The GRDB-4.0 branch has been updated with the recent changes brought by 3.7.0: you may want to merge this branch into yours.

In the way, you'll see that the make test_install_GRDBCipher_CocoaPods test has been commented out in .travis.yml, because it started to fail a few days ago (I'm 100% sure these failures are not due to a change in GRDB). Anyway: I don't know if this test is actually useful. Travis already checks make test_CocoaPodsLint_GRDBCipher, and I don't know if it is worth providing our own pod integration test.

@groue
Copy link
Owner

groue commented Apr 1, 2019

@darrenclark, I haven't heard from you for a while now. This is not a problem. But I now assume you will not complete this PR.

I'd be happy if you keep your https://github.com/darrenclark/GRDB.swift/tree/GRDB-4.0-sqlcipher branch available, should anyone use it as a base for a tight integration of SQLCipher in GRDB 4.

Thanks for pushing this topic forward!

cc @Marus, @michaelkirk-signal, for information.

@groue groue closed this Apr 1, 2019
@darrenclark
Copy link
Contributor Author

Hey @groue, apologies, things have been quite busy the last few weeks.

Will leave the branch up!

I'm not a user of SQLCipher, but will leave it up in case anyone wants to push this PR forward

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants