SPM support #202

zmeyc · 2017-04-10T11:19:40Z

No description provided.

groue

Hello @zmeyc. Thanks a lot for this PR. And Linux support in also on the way, which is a pretty good news ;-) I've asked a few questions, I wonder if you can have a look at them, because you know SPM much better than I do. And I also have a request: if it is not too hard, that would be great if the Makefile had a new test_SPM_install target which would assert SPM can integrate GRDB with SPM. That would greatly help me granting constant SPM support even if I don't use SPM much often, you see? There's also the topic of unit tests, but I know this is a delicate question, and we'll see that later.

groue · 2017-04-10T12:20:52Z

Package.swift

+import PackageDescription
+
+let package = Package(
+    name: "GRDB.swift",


Is the name of the package also used when one imports it from Swift with import ? Wouldn't be GRDB a better name, then? (Honest question, I really don't know)

I don't know if it affects anything at all, it doesn't seem to affect filenames in build dir nor import name. Can be changed to GRDB probably.
Module name is derived from directory name (it's currently 'GRDB', so it's 'import GRDB').

OK, you made it clear that it's not quite clear ;-) In doubt let's rename to GRDB. This .swift suffix can only add confusion or nasty surprises ;-)

groue · 2017-04-10T12:23:41Z

Package.swift

+let package = Package(
+    name: "GRDB.swift",
+    dependencies: [
+        .Package(url: "https://github.com/groue/SQLiteSDK.git", majorVersion: 0)


Is the convention of prefixing C packages with the letter C dead? The SPM documentation still uses it: Clibgit, CJPEG...

Yes, I used SQLiteSDK because there were 'import SQLiteSDK' in most of the files and this allowed not patching source code at all. I had to restore these imports as they were removed in latest master. It's probably better to rename the repo to "CSQLite" and adjust imports accordingly.

All right! groue/SQLiteSDK has just been renamed https://github.com/groue/CSQLite :-)

groue · 2017-04-10T12:25:39Z

Package.swift

+    dependencies: [
+        .Package(url: "https://github.com/groue/SQLiteSDK.git", majorVersion: 0)
+    ],
+    exclude: ["GRDB.xcworkspace", "Playgrounds", "SQLCipher", "SQLiteCustom", "Support", "DemoApps", "Tests", "Documentation"]


This list looks like a maintenance burden. Not a big problem, but if gathering sources in a Sources directory would avoid it, then maybe we should follow the recommended convention.

Yes, we can move GRDB dir into Sources/

There are three possible layouts:

Dirs for each module in root (currently used). The downside is having to exclude everything which SPM doesn't know how to handle.

For single module repos, Sources/ can contain source files without directories.

If at least one directory is present in Sources/, dirs are treated as distinct modules and no other source files are allowed at top level in Sources/
We can use this layout. So, it will become:
Sources/GRDB/...other files...

OK, thanks for this detailed info. I don't feel like holding this PR just because we can't decide how to layout the files and directories: the status quo is good enough, even if we have to maintain an exclude list. 👍

groue · 2017-04-10T14:17:44Z

Answering your comment, @zmeyc:

Regarding test_SPM_install: I'm not sure how it should work. To test that it still builds with SPM, it should be enough to call
swift package clean
swift build

This checks that the package can build, and that's already great! OK, I'll add those lines to the Makefile myself. And this is well enough for a start. I'll want later to check that GRBD package can actually be integrated in a minimal binary package - but this PR doesn't need that.

Unit tests require a flat layout:
Tests/BlablaTests/File.swift
Every dir in Tests/ becomes a module. I don't know where the code shared between tests should go, probably we'll have to ask SPM devs.

All right. And unit tests will need a deep overhaul in the future Linux PR... So let's not lose time with unit tests right now. Just make me a little favor and tell me that you were actually able to use SQLite with GRDB through SPM :-)

If I sum up:

please rename the package to GRDB
please rename the C package to CSQLite
wait until I can update the Makefile, and build a minimal binary that loads GRDB through SPM: this will convince myself that this PR deserves to be merged for good, and that I'll be able to support it in the future 😄

groue · 2017-04-10T14:20:04Z

(BTW @zmeyc, my personal machine is currently being repaired - I may be quick to talk but slow to actually perform my part of the job - please be patient, and assured I have the deepest interest in this PR)

zmeyc · 2017-04-10T14:38:58Z

@groue One reason we might want to consider another name is because some other frameworks (notably Kitura and Perfect) have their own versions of CSQLite which will clash with GRDB's one. :(
I already had such problems with SwiftyJSON (everyone has their own fork). I don't know how to properly resolve these clashes or is there any automated mechanism in the plans.

zmeyc · 2017-04-10T14:41:53Z

@groue But I hope that unlike SwiftyJSON, CSQLite is not imported and used in these frameworks by default. So maybe it's not a problem.

groue · 2017-04-10T14:46:20Z

When this problem happens, we'll deal with it. Unless you feel like a trendsetter (which I'm not on the SPM topic), sticking with existing conventions is better than crafting an nth standard. Besides, server stacks already have their database packages. And last, GRDB is not ready at all (really not) for the server. I even think that a hypothetical server-enabled GRDB will live with another name in another repo. Did I miss something?

zmeyc · 2017-04-10T15:14:34Z

@groue I fully agree.

ekimia · 2017-04-10T23:25:45Z

@groue for what it's worth there is the application of GRDB for framework usage. SPM support fits in nicely with that use case.

groue · 2017-04-11T11:37:09Z

@zmeyc I took the liberty to complete the renaming tasks on your pr_spm1 branch.

swift build performs as expected (without any error).

I still can't check integration since GRDB is not yet published as an SPM package, and top-of-tree development doesn't look available yet with Xcode 8.3.1. I'll do that.

Thanks for this PR, and your patience.

zmeyc · 2017-04-11T20:24:29Z

@groue Thanks for finishing the PR, I suddenly felt ill yesterday. Hopefully I'll recover in a few days and will continue with Linux PR.
I've done a quick check:

 swift package init --type=executable
...
    dependencies: [
        .Package(url: "https://github.com/groue/GRDB.swift.git", majorVersion: 0)
    ]
...

and it seems everything is built and linked correctly.

Top of trunk development can be done like this:

swift package edit GRDB --revision master

After this SPM will use whatever is present in Packages/GRDB directory.
It can even be a symlink to another directory.

swift package generate-xcodeproj

also seems to work.

Btw, last tag seems to be misspelled (no 'v' prefix).

groue · 2017-04-12T12:27:54Z

I've done a quick check [...] and it seems everything is built and linked correctly.

Yes, thanks to you. There is a small executable in Tests/SPM which simply checks access to an in-memory database, and to the C API. Your tip for the top of trunk development made it suitable for an automated test (see 5804c04).

Btw, last tag seems to be misspelled (no 'v' prefix).

It was because of an old tag without v that prevented SPM from performing well. Now everything is back in place, with consistent v-prefixed tags!

Hopefully I'll recover in a few days and will continue with Linux PR.

Take care, and recover first and foremost :-)

SPM support

1751fb3

zmeyc mentioned this pull request Apr 10, 2017

SPM support #201

Closed

groue requested changes Apr 10, 2017

View reviewed changes

This was referenced Apr 10, 2017

Modulemap for system sqlite groue/CSQLite#1

Merged

SPM support #72

Closed

groue changed the title ~~SPM support (without Linux)~~ SPM support Apr 10, 2017

groue added 2 commits April 11, 2017 13:22

Rename package to GRDB, and rename SQLite module to CSQLite

63b3b42

Add test_framework_SPM target to Makefile

6c2b421

groue merged commit b06f765 into groue:master Apr 11, 2017

groue added the enhancement label Apr 12, 2017

groue mentioned this pull request Oct 23, 2018

Would like to setup Carthage Deploy artifacts on Travis #433

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SPM support #202

SPM support #202

zmeyc commented Apr 10, 2017

groue left a comment

groue Apr 10, 2017

zmeyc Apr 10, 2017

groue Apr 10, 2017

groue Apr 10, 2017

zmeyc Apr 10, 2017

groue Apr 10, 2017

groue Apr 10, 2017

zmeyc Apr 10, 2017

groue Apr 10, 2017 •

edited

Loading

groue commented Apr 10, 2017

groue commented Apr 10, 2017

zmeyc commented Apr 10, 2017

zmeyc commented Apr 10, 2017

groue commented Apr 10, 2017

zmeyc commented Apr 10, 2017

ekimia commented Apr 10, 2017

groue commented Apr 11, 2017

zmeyc commented Apr 11, 2017 •

edited

Loading

groue commented Apr 12, 2017 •

edited

Loading

SPM support #202

SPM support #202

Conversation

zmeyc commented Apr 10, 2017

groue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

groue Apr 10, 2017 • edited Loading

Choose a reason for hiding this comment

groue commented Apr 10, 2017

groue commented Apr 10, 2017

zmeyc commented Apr 10, 2017

zmeyc commented Apr 10, 2017

groue commented Apr 10, 2017

zmeyc commented Apr 10, 2017

ekimia commented Apr 10, 2017

groue commented Apr 11, 2017

zmeyc commented Apr 11, 2017 • edited Loading

groue commented Apr 12, 2017 • edited Loading

groue Apr 10, 2017 •

edited

Loading

zmeyc commented Apr 11, 2017 •

edited

Loading

groue commented Apr 12, 2017 •

edited

Loading