-
Notifications
You must be signed in to change notification settings - Fork 476
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
Streamline cmake usage #1115
Comments
/CC
|
I can hardly argue that SOCI CMake build system is too complex, as I don't understand half of it myself (and I have doubts about the other half), so it would be definitely great to simplify it. Concerning the dependencies, I think ideally we'd have 3 state options for all of the backends:
I think this should satisfy all the use cases. |
Yes this is in line with what I was thinking 👍 |
@Krzmbrzl Hello, I am not related to the soci package on the AUR. I once helped someone out who had compilation issues but I do not have said conversation at hand. The same issue had to be addressed in the PKGBUILD. I just referenced it. I would really appreciate an improved CMake file. |
@Spixmaster thanks for chiming in anyway :) What are things that you would say an improved cmake support should provide or improve upon? Anything I haven't touched on in my initial post? |
@Krzmbrzl Honestly, I am not that proficient with CMake. All features that I use are including source code and headers, link libraries, set compile options, testing, documentation generation and setting variables and custom variables which turn on some options. I only link the needed soci libraries. target_link_libraries(${PROJECT_NAME} PUBLIC "soci_core")
target_link_libraries(${PROJECT_NAME} PUBLIC "soci_sqlite3") Therefore, I do not experience such issues like you. My two main concerns are that there is this bug that libsoci_empty.so is not at the right place. When investigating the issue I found the CMakeLists.txt very complex and confusing which is my second point but this regard is of course subjective. I do not know about your problem that the transitive dependency "boost" is not declared. I link whatever is missing. I assumed that transitive dependencies need to be stated explicity but this does not seem to be the case. One thing I do not understand is, why you include the headers of soci. I do not need to that as they are at the corrent place. Your mentioned file is at |
FWIW, the section
isn't needed anymore (thanks to #982). I haven't really touched the CMake part of SOCI in a long time but I do remember the amount of initial effort for getting it to work, so I would greatly appreciate a rewrite because I don't understand most of the CMake magic we're doing here either. |
@vadz while we're in the process of rewriting the buildsystem, what to do with the Makefiles in this repo? I would argue that having them in addition to cmake only doubles the effort of maintaining a consistent build process. |
I've never touched these makefiles and I'd just leave them be, they do no harm and it hasn't been a problem to maintain them (because there have been no new source files in SOCI since a long time...). |
From the quick peeks I took they will end up broken by my changes. That's why I'd rather remove them than have them around in a broken (and thus unusable) state. |
@msobczak Do you still use the non-CMake makefiles or can they be removed (because AFAIK nobody else has ever used them)? |
@vadz do you have an opinion on whether it is mandatory for the new cmake setup to be able to compile shared and static libraries in a single run (as the current setup can)? That's very much nonstandard for cmake projects and is also not really supported by cmake. I thought I had found a nice workaround but Windows being Windows wrecked that idea. The current setup works by compiling everything twice anyway so I'd argue that folks that actually need both libraries (which probably aren't that many) could also just invoke building two times 🤷 |
I've actually always wondered why did SOCI do it like this. I guess it's a throwback to the projects using libtool, which also builds both by default, but I agree that it's not useful and I'd be perfectly fine with compiling just one kind of libraries at a time. I'd prefer the default to be "shared", but I don't know what is the CMake standard/convention/lore here? |
Fine by me - I'm not sure there really is a standard per se. There's the |
Preamble
User's perspective
I think there have been a couple of mentions in the issues here and also elsewhere that SOCI's current cmake usage is somewhat non-standard in the sense that SOCI is doing things rather differently than most other (modern) cmake-based projects.
This makes it quite hard to get SOCI to work as part of the regular cmake workflow of a superproject as it involves constantly checking SOCI's cmake sources to see what kind of variables one has to access and what library depends on what and whether or not that dependency is declared on the exported target in order to transitively apply to targets that themselves depend on one of SOCI's target.
That is to say that a cmake user would expect to be able to do something like this
and that would be it. Instead, it has to look more like this
That is to say that what could be a very smooth integration ends up being a set of workarounds in order to get things going. The above example may not look like much but figuring the workarounds out has taken a non-trivial amount of time and some of the issues only appeared on certain platforms. Someone with less experience in cmake might not have the experience to figure this kind of stuff out.
Developer's perspective
SOCI's cmake setup uses a lot of functions calling each other instead of defining targets inline. That is a bit non-standard in my experience but not necessarily a bad thing. However, all these functions make following what is actually going on somewhat hard, especially since SOCI seems to follow what I would call "implicit strategies" in a couple of places.
An example is the selection of what targets to build. SOCI makes this conditional on whether or not the required dependencies are found or not. That can be a nice default, but unfortunately this can also make things a lot worse, if for example I configure SOCI as a submodule in my project and explicitly enable e.g. the MySQL backend, because I want to use that. However, SOCI does some checking and determines that a dependency is not met and simply disables the MySQL backend again. This leads to the corresponding target to not be defined and thus I get an error of an undefined target somewhere in my own cmake code. This is very surprising given that I seemingly have instructed this specific target to be built.
Finally, the implicit target definition by function calls also makes it a bit hard to use optimizations such as not compiling every source twice if building static and dynamic libraries.
Solution
I believe that it would be beneficial to rewrite the cmake support for SOCI from scratch using modern cmake (this implies dropping support for very old cmake versions - I think we should upgrade to at least 3.15-ish - current version is 3.28). This should improve the overall experience of SOCI in cmake contexts.
I am currently thinking of providing such a rewrite as I have struggled multiple times with the current implementation and therefore I believe that long-term this might be worth the time investment.
However, before I start with this undertaking I would like to make sure that the respective changes would actually be accepted. Or under what conditions such a change will be accepted.
The text was updated successfully, but these errors were encountered: