Improve FindSoci.cmake

[Krzmbrzl]

The new version uses consistent spelling that matches the name of then module file (Soci instead of SOCI). In addition it delegates the handling of individual components to the standard cmake function and adds support for querying the SOCI version (from the version.h header file).

[Krzmbrzl]

I think it might be worth it to let the module define the soci_* cmake targets as well. So if e.g. the mysql plugin is found, then we should probably also define the soci_mysql cmake target. That makes it easier for downstream code to link against soci...

[vadz]

Sorry, but doesn't this break backwards compatibility? I.e. won't all the existing CMakefiles using SOCI_XXX stop working now? Because if so, we wouldn't want to do this...

[Krzmbrzl]

Does SOCI install this file as a cmake module during the install step? 'cause at least on Ubuntu 20.04 this does not seem to be shipped with the SOCI package and thus any project thst wants to use this file has to copy it into their project anyway. Thus, there is no backwards compatibility to consider as explicit action is required on the user's side to get this updated version. And if they do that, they can also change the variable names.

But to answer the question: Yes this would break compatibility. But keep in mind that the current behavior is against common practices already (even triggering a cmake warning), so I would consider this a bug that gets fixed.

[vadz]

I'd expect packagers to include this file in their installation, but I'm not really sure. In any case, even with manual copying, the thing is that all applications using SOCI with CMake would need to update their own existing CMakefiles, which seems like a completely unnecessary burden.

Which warning exactly does this trigger? Could we work around/suppress it instead? Or, perhaps, we could apply your changes but also still define the old variables for compatibility?

[Krzmbrzl]

Which warning exactly does this trigger?

The one complaining that the casing used in find_package is different than the one used in the find-module.

Or, perhaps, we could apply your changes but also still define the old variables for compatibility?

Yeah, I guess that should be possible :thinking: I'll adapt the PR accordingly :+1:

[zann1x]

The one complaining that the casing used in find_package is different than the one used in the find-module.

To simply fix this warning, all you need to change is the name in the find_package_handle_standard_args call to SOCI. A similar kind of fix was applied to some of the backend's find modules in this commit already: https://github.com/SOCI/soci/commit/175c716f2b0b78ca3c907aae60605d0a65127beb

Aside from that, the library is called SOCI. Why shouldn't we call it like that in the find module then? Even official CMake modules do it this way: https://github.com/Kitware/CMake/blob/cf6235719b7d5889ead0676e598b56bf677bd0ba/Modules/FindSDL.cmake

[Krzmbrzl]

Why shouldn't we call it like that in the find module then?

If I had created the file from scratch, then I would have done exactly that. However, existing cmake files are using this module as find_package(Soci) and not find_package(SOCI) (in my tests, the latter version wouldn't use the FindSoci.cmake file at all), so changing that seems like a more dramatic change in terms of compatibility.

[vadz]

Why shouldn't we call it like that in the find module then?

If I had created the file from scratch, then I would have done exactly that. However, existing cmake files are using this module as find_package(Soci) and not find_package(SOCI) (in my tests, the latter version wouldn't use the FindSoci.cmake file at all)

This is bad because our own documentation recommends using the latter:

https://github.com/SOCI/soci/blob/e6b1852168a78cc3aa7c014386adfed829993552/docs/installation.md?plain=1#L298

OTOH I see also packages using find_package(soci) and presumably it somehow works for them.

Just what exactly are the CMake case-sensitivity rules? Or is it dependent on the file system case-sensitivity, in fact?

If we all agree that using FindSOCI.cmake would be preferable, perhaps we could just rename the existing file to this name and provide FindSoci.cmake simply including it for compatibility?

[Krzmbrzl]

Or is it dependent on the file system case-sensitivity, in fact?

That could certainly be the case, indeed :thinking:

Let me re-check my findings before we make any decision. After all things were a bit messy when I was playing around with this stuff (I had to figure out how exactly to go about things).

[zann1x]

AFAIK CMake variables are case-sensitive, no matter the file system. The name you pass to find_package on the other hand, seemingly is not (https://cmake.org/cmake/help/latest/command/find_package.html#config-mode-search-procedure):

In all cases the is treated as case-insensitive and corresponds to any of the names specified ( or names given by NAMES).

So my guess is that renaming the file to FindSOCI.cmake should be okay.

[Krzmbrzl]

Okay my test consists of the following minimal CMakeLists.txt:

cmake_minimum_required(VERSION 3.15)

project(dummy)

list(APPEND CMAKE_MODULE_PATH
    "${CMAKE_SOURCE_DIR}/cmake/"
)

find_package(Soci REQUIRED)

where I have a sub-directory called cmake in which I have copied the current FindSoci.cmake file (the one that is currently in master).

If executed as shown above, everything works fine:

-- Soci found: Looking for plugins
--     * Plugin mysql found /usr/lib/x86_64-linux-gnu/libsoci_mysql.so.
--     * Plugin odbc found /usr/lib/x86_64-linux-gnu/libsoci_odbc.so.
--     * Plugin postgresql found /usr/lib/x86_64-linux-gnu/libsoci_postgresql.so.
--     * Plugin sqlite3 found /usr/lib/x86_64-linux-gnu/libsoci_sqlite3.so.
-- Found Soci: /usr/include/soci  
-- Configuring done
-- Generating done
-- Build files have been written to: /tmp/test-project/build

However, if I change the find_package call to find_package(SOCI) instead, I get

CMake Error at CMakeLists.txt:9 (find_package):
  By not providing "FindSOCI.cmake" in CMAKE_MODULE_PATH this project has
  asked CMake to find a package configuration file provided by "SOCI", but
  CMake did not find one.

  Could not find a package configuration file provided by "SOCI" with any of
  the following names:

    SOCIConfig.cmake
    soci-config.cmake

  Add the installation prefix of "SOCI" to CMAKE_PREFIX_PATH or set
  "SOCI_DIR" to a directory containing one of the above files.  If "SOCI"
  provides a separate development package or SDK, be sure it has been
  installed.

-- Configuring incomplete, errors occurred!
See also "/tmp/test-project/build/CMakeFiles/CMakeOutput.log".

I get the corresponding error, when using all-lowercase soci as well.

So this validates what I found in my original testing: The casing of what is passed to find_package does matter. However, I am reasonably sure that if this experiment was repeated on e.g. Windows (I did it on Linux), the casing wouldn't matter as even if cmake was going to look for a file called FindSOCI.cmake, it would still find FindSoci.cmake as Window's filesystem is case-insensitive.

If I switch back to using find_package(SOCI REQUIRED) and add a new findSOCI.cmake in my cmake sub-dir with the following contents

include(FindSoci)

I get the cmake warning that I have mentioned earlier already, but Soci will be found

--     * Plugin mysql found /usr/lib/x86_64-linux-gnu/libsoci_mysql.so.
--     * Plugin odbc found /usr/lib/x86_64-linux-gnu/libsoci_odbc.so.
--     * Plugin postgresql found /usr/lib/x86_64-linux-gnu/libsoci_postgresql.so.
--     * Plugin sqlite3 found /usr/lib/x86_64-linux-gnu/libsoci_sqlite3.so.
CMake Warning (dev) at /usr/share/cmake-3.23/Modules/FindPackageHandleStandardArgs.cmake:438 (message):
  The package name passed to `find_package_handle_standard_args` (Soci) does
  not match the name of the calling package (SOCI).  This can lead to
  problems in calling code that expects `find_package` result variables
  (e.g., `_FOUND`) to follow a certain pattern.
Call Stack (most recent call first):
  cmake/FindSoci.cmake:97 (FIND_PACKAGE_HANDLE_STANDARD_ARGS)
  cmake/FindSOCI.cmake:1 (include)
  CMakeLists.txt:9 (find_package)
This warning is for project developers.  Use -Wno-dev to suppress it.

-- Found Soci: /usr/include/soci  
-- Configuring done
-- Generating done
-- Build files have been written to: /tmp/test-project/build

---

So to summarize: Renaming FindSoci.cmake to findSOCI.cmake will break backwards compatibility. This can be somewhat mitigated by adding a dummy FindSoci.cmake that just includes FindSOCI.cmake but since this requires users to explicitly copy an additional file into their source tree, I think the easier mitigation strategy for them would be to simply adapt their find_package call.

---

EDIT:

The name you pass to find_package on the other hand, seemingly is not (cmake.org/cmake/help/latest/command/find_package.html#config-mode-search-procedure):

This documentation refers to config mode, which is not the context in which Find*.cmake files are being used. For config mode, we'd need a SociConfig.cmake or Soci-config.cmake file. This is actually what a distributor would normally bundle as installing this on a system allows users to find SOCI, without having to manually copy over any Find-file. The latter is essentially only a workaround for when no config file is available.

[vadz]

I'm still not sure what to do here as I don't use SOCI with CMake myself at all (we integrate it into our own build system), but it still seems like a bad idea to force people changing their CMakefiles because of such an inconsequential change.

Could you perhaps please split the uncontroversial parts (version addition, refactoring) of this PR into separate commits that could be merged already before we do anything (or not...) about the package name case?

[Krzmbrzl]

but it still seems like a bad idea to force people changing their CMakefiles because of such an inconsequential change.

It's an effort of < 1 minute and I still think it is unlikely that anyone will be affected by the changes without explicitly copying in the new FindSoci file into their source tree.

Could you perhaps please split the uncontroversial parts (version addition, refactoring) of this PR into separate commits that could be merged already before we do anything (or not...) about the package name case?

Yeah, I can do that. But please let us rename the file to get rid of this inconsistency. As someone who does use cmake a lot, I can tell you that things just get so much easier if packages stick to cmake best practices as that avoids folks to constantly look up how project XY happens to do things differently again :pray:

[vadz]

but it still seems like a bad idea to force people changing their CMakefiles because of such an inconsequential change.

It's an effort of < 1 minute

This is only the case if you already know what to do. Believe me, it's easy to spend many hours on this if you don't. And now multiply this by the number of SOCI users.

and I still think it is unlikely that anyone will be affected by the changes without explicitly copying in the new FindSoci file into their source tree.

IMO the most reasonable way of using dependencies in C++ is via Git submodules. So I don't think any copying needs to be involved: it would be enough to just update the submodule (maybe even sub-submodule because sometimes it's not even a direct dependency) and the build would break.

Again, I'm sorry but I don't think the extra inconvenience of looking up something once, especially if it just involves checking the project docs, outweighs the extra aggravation of breaking the already working builds for people who did look it up before.

[Krzmbrzl]

But it's not like we're breaking anything. If we add the fallback FindSoci file the users would merely get a warning that even tells them what is going on. If someone then takes hours figuring this out, then sorry that's their problem. You don't have to be a genius to follow the instructions of a warning you are seeing :shrug:

[Krzmbrzl]

Oh and if you're using SOCI as a submodule, you're not using the FindSoci file anyway. In this case you would just use the cmake targets directly. The FindSoci file is only used to find a version of SOCI installed on the system.

[vadz]

Maybe I'm still misunderstanding the problem then. My impression was that renaming the file would break the existing CMakefiles on case-sensitive file systems because the file wouldn't be found any more, i.e. find_package(Soci) will stop working. Isn't this correct?

[Krzmbrzl]

My impression was that renaming the file would break the existing CMakefiles on case-sensitive file systems because the file wouldn't be found any more, i.e. find_package(Soci) will stop working. Isn't this correct?

That is indeed correct. However, my argument was that by renaming the file to FindSOCI.cmake and then create an additional file FindSoci.cmake that just includes FindSOCI.cmake, we would remain backwards compatible (users using find_package(Soci) would then only get a warning, but everything should still work).

However, thinking about this some more, I came to believe that this is not viable after all, since we can't have two files whose names differ only in casing on case-insensitive filesystems. That's just not possible.

So from that POV, I now have to agree that renaming the file can't be done ina backwards compatible way.

But if we don't rename the file, the documentation has to be adapted as find_package(SOCI) is wrong and only happens to work on case-insensitive filesystems. Furthermore, I don't understand what you mean with "controversial changes" in this PR. If we sti with users using find_package(Soci), then the correct names for the variables that are to be set is Soci_* and not SOCI_*. But the latter are also set explicitly for backwards compatibility, so I don't see the issue here 👀

Ideally, we'd mark the old SOCI_* variables as deprecated but I can't find a way to do that in cmake. So I guess going forward, we'll just have those sticking around as-is.

[Krzmbrzl]

@vadz what's the status here? It seems like our discussion has just been discontinued close to coming to a conclusion :eyes:

[vadz]

@vadz what's the status here? It seems like our discussion has just been discontinued close to coming to a conclusion

Yes, sorry, I am just instinctively shying away from anything CMake-related, but I shouldn't, of course, so let me drag myself back to this...

So from that POV, I now have to agree that renaming the file can't be done ina backwards compatible way.

My preference would be to not do it at all then. Yes, SOCI would arguably make more sense in a vacuum, but the benefit, compared to Soci, is just not big enough to justify breaking anything. Moreover, it looks like CMake convention is actually to use the latter case, e.g. FindRuby() macro has switched from using RUBY_Found etc to Ruby_Found in the latest CMake versions, so using Soci is more consistent from CMake point of view.

But if we don't rename the file, the documentation has to be adapted as find_package(SOCI) is wrong and only happens to work on case-insensitive filesystems.

Yes, we should do this.

Furthermore, I don't understand what you mean with "controversial changes" in this PR.

Maybe "controversial" was too strong a term, but I meant the changes I wasn't sure about, i.e. the renaming -- as opposed to the version-related part of the change.

Ideally, we'd mark the old SOCI_* variables as deprecated but I can't find a way to do that in cmake. So I guess going forward, we'll just have those sticking around as-is.

My CMake lore knowledge remains woefully insufficient, so I can't help with this, i.e. I don't know about how to do it neither and hence I think we should indeed just provide both versions (again, see FindRuby() example above) and just document that Soci_ ones should be used while SOCI_ ones exist only for compatibility.

If you could please update this PR in this way, I'd be glad to merge it, TIA!

[Krzmbrzl]

If we no longer want the official name (at least in cmake context) to be SOCI, but Soci, we should probably also change the library alias targets (introduced via most recently merged PR) from SOCI::* to Soci::*.

[vadz]

If we no longer want the official name (at least in cmake context) to be SOCI, but Soci, we should probably also change the library alias targets (introduced via most recently merged PR) from SOCI::* to Soci::*.

Yes, you're right, thanks for noticing about it. I'm going to do it, before anybody starts using them (ping #982).

[Krzmbrzl]

it looks like CMake convention is actually to use the latter case, e.g. FindRuby() macro has switched from using RUBY_Found etc to Ruby_Found in the latest CMake versions, so using Soci is more consistent from CMake point of view.

This is actually not quite correct. They switched the casing, because the module is called Ruby and not RUBY- Thus, the CMake philosophy is to use the same casing that is also used for the library's name, which in our case here, would be SOCI and not Soci.

I meant the changes I wasn't sure about, i.e. the renaming -- as opposed to the version-related part of the change.

The renaming has never been done - it was only proposed and discussed.

If you could please update this PR in this way, I'd be glad to merge it, TIA!

Given the above, I don't think there is anything to update here :eyes:

[vadz]

Given the above, I don't think there is anything to update here

The docs maybe, to better explain SOCI vs Soci? I'd also prefer extracting Soci_Version stuff into a separate commit, but otherwise, yes, after looking at this again, I think this PR already does the right thing.

Please let me know if you plan updating the docs or anything else or not, TIA.

[Krzmbrzl]

Ah yes right - forgot about the docs. Will do.

I'd also prefer extracting Soci_Version stuff into a separate commit

You mean the ability to read the version number?

[vadz]

You mean the ability to read the version number?

Yes.

In a (unlikely, but not completely impossible) case we need to revert the Soci_Variables commit later, it would make it easier to preserve this change. It also would make both commits more readable than the combined commit right now.

[Krzmbrzl]

Note: I still have to test the new code

[Krzmbrzl]

Okay, now everything is split into different commits and I confirmed everything to be working as expected. From my point of view this can be merged now.

Before that, I want to make one final argument for renaming the file: https://github.com/SOCI/soci/blob/549874055e66dc22a2a96652238b9f4c4fed0fd9/cmake/modules/FindSoci.cmake#L4 The file clearly states that it is experimental and WIP, so essentially nobody can expect things to remain fixed. Thus, from my POV changing the API (in the sense of now requiring find_package(SOCI) instead of find_package(Soci) would be fine :shrug:

If you don't agree, feel free to merge as-is though.

[vadz]

It was experimental ever since its creation in 125c06bef in 2011, so it really doesn't mean much.

I'll merge this soon, if there are no objections from anybody else, thanks for your work on this and for your patience!

[Krzmbrzl]

I rebased my branch against current master to remove the merge conflict.

[vadz]

Thanks again!