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

Streamline cmake usage #1115

Open
Krzmbrzl opened this issue Dec 29, 2023 · 14 comments · May be fixed by #1118
Open

Streamline cmake usage #1115

Krzmbrzl opened this issue Dec 29, 2023 · 14 comments · May be fixed by #1118
Labels

Comments

@Krzmbrzl
Copy link
Contributor

Krzmbrzl commented Dec 29, 2023

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

find_package(Soci REQUIRED)
target_link_libraries(my_awesome_target PRIVATE soci::soci)

and that would be it. Instead, it has to look more like this

# SOCI's built include/ directory has to be added to the project explicitly.
# This is needed so that SOCI can find the file soci/soci-config.h that gets generated by CMake.
target_include_directories(my_awesome_target
	PUBLIC
		${SOCI_BINARY_DIR}/include
)
target_link_libraries(my_awesome_target soci_core_static soci_postgresql_static)
# Note that it is SOCI that requires the public Boost dependency, but unfortunately seems to fail to properly
# declare so and thus linking against SOCI does not reveal this Boost dependency to upstream dependents.
find_pkg("Boost" REQUIRED)
target_link_libraries(my_awesome_target PUBLIC Boost::boost)

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.

@Krzmbrzl
Copy link
Contributor Author

/CC

  • @vadz
  • @zann1x (you have been involved in discussions with other cmake changes suggested by me)
  • @Spixmaster (you seem to be involved in the Arch Linux Soci package?)

@vadz vadz added the CMake label Dec 29, 2023
@vadz
Copy link
Member

vadz commented Dec 29, 2023

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:

  • Off: self-explanatory.
  • Auto: default value, use it if dependencies are available.
  • On: use it if dependencies are available and give an error if they are not.

I think this should satisfy all the use cases.

@Krzmbrzl
Copy link
Contributor Author

Yes this is in line with what I was thinking 👍

@Spixmaster
Copy link
Contributor

Spixmaster commented Dec 30, 2023

@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.

@Krzmbrzl
Copy link
Contributor Author

@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?

@Spixmaster
Copy link
Contributor

Spixmaster commented Dec 30, 2023

@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 /usr/include/soci/soci-config.h for me. Cmake is run with option -DCMAKE_INSTALL_PREFIX="/usr".

@zann1x
Copy link
Contributor

zann1x commented Jan 3, 2024

FWIW, the section

# SOCI's built include/ directory has to be added to the project explicitly.
# This is needed so that SOCI can find the file soci/soci-config.h that gets generated by CMake.
target_include_directories(my_awesome_target
	PUBLIC
		${SOCI_BINARY_DIR}/include
)

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.

@Krzmbrzl
Copy link
Contributor Author

Krzmbrzl commented Jan 3, 2024

@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, for one, won't touch them which most likely means that the Makefiles will end up doing something different than the cmake path. If it was up to me, I'd delete the Makefiles...

@vadz
Copy link
Member

vadz commented Jan 3, 2024

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...).

@Krzmbrzl Krzmbrzl linked a pull request Jan 3, 2024 that will close this issue
@Krzmbrzl
Copy link
Contributor Author

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.

@vadz
Copy link
Member

vadz commented Feb 17, 2024

@msobczak Do you still use the non-CMake makefiles or can they be removed (because AFAIK nobody else has ever used them)?

@Krzmbrzl
Copy link
Contributor Author

@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 🤷

@vadz
Copy link
Member

vadz commented Feb 22, 2024

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?

@Krzmbrzl
Copy link
Contributor Author

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 BUILD_SHARED_LIBS variable that can be used to derive a default library type. However, as far as my experience there is no canonical way of choosing a lib type. So we might as well default to shared :)

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

Successfully merging a pull request may close this issue.

4 participants