Added support for external sqlite3 by emmenlau · Pull Request #234 · SRombauts/SQLiteCpp (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation16 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
This PR adds support for an externally compiled SQLite3. There are two main changes:
- Detect and use SQLite3 via cmake if requested by the user, and
- Always link SQLiteCpp against cmake detected
Threads::Threadsand DL-libs.
The second change is a simplification and cleanup. SQLite3 always requires threads and DL-libraries. Therefore it seems not sensible to specify them individually for each target.
This PR also uses cmake standard functionality to detect the actual threads-implementation and DL-libraries for the current platform. This should improve portability to other (less common) platforms that may not use the name pthreads but another threading library name.
Hello @emmenlau
Thank you for all your pull requests!
I am don't know if you have noticed, but all CI tests failed on this one?
In the meantime, I believe that I have merged another pull request similar to this one.
Cheers!
Sébastien
Hello @emmenlau, looking back at this one, I see that there are still very valuable bits here!
Would you have the time to rebase it onto the new master branch?
If not, I will hopefully find some time to do it at a later time.
Cheers!
Dear @SRombauts , I've just reviewed the upstream changes and I think that some relevant parts have not been answered by the previous MR. I've therefore updated my PR accordingly.
This PR changes/improves multiple things:
- It transitively links SQLiteCpp against either the included
sqlite3or the systemsqlite3 - It generalizes the linking against
dlandthreadlibraries on Linux/Unix/Mac, so that this works for all targets and also is passed transitively to downstream projects - Due to the transitive linking of sqlite3, dl and threads, the CMakeLists.txt can be simplified and cleaned a bit, and downstream projects work more comfortably
But one question: SQLiteCpp can be built against an internal sqlite3. However this internal sqlite3 is never installed. Is this intentional? It seems not sensible to allow building required dependencies but then leaving it to the user to resolve them for downstream projects?
Would it not be better if the internal sqlite3, if built, is installed alongside SQLiteCpp?
Oh yes, for sure, the thing is I never use this kind of "install" workflow
Please ignore this comment, it is deprecated with the latest version.
Coverage remained the same at 100.0% when pulling a166062 on BioDataAnalysis:emmenlau_support_external_sqlite into 8485bb7 on SRombauts:master.
Dear @SRombauts , I've continued to improve the cmake instructions in this PR. In order to support internal or external sqlite3 very cleanly, I've removed the generic line
include_directories("${PROJECT_SOURCE_DIR}/include")
and instead used specific includes with target_include_directories(PRIVATE for every target. This does not add much more code, but its the recommended clean code for "modern cmake". It can help in the future to discover targets dependencies and wrong/incorrect includes, because every target only has their minimal includes set.
Is this ok with you?
Hello, fun fact, I have already removed the line "include_directories("${PROJECT_SOURCE_DIR}/include")" yesterday evening on master ;)
…thread and dl libs on Unix/Linux/Mac
Haha ok! I think all builds should now show green lights. If nothing else comes up, can this be merged?
Yes, it looks good too me... even though we don't actually test the installation itself, nor the use of it by a test project.
Adding an installation step and checking the result should be fairly easy I guess.
Testing the use of the installed library might be a bit more involving: I do have a separate SQLiteCpp_Example project, but perhaps could we even do that directly in here
For what its worth, I've checked the installation in our CI on the three major platforms Win, Lin and Mac, and could use the installed sqlite3 in downstream projects. And this PR also makes SQLiteCpp usable with a completely external sqlite3 for me, which previously caused some issues...
Very good, thank you for your contribution!
I'll see if I can automate some tests to avoid any regressions around these kind of sensitive features.
Cheers!
