Revamp CMake support #1118

Krzmbrzl · 2024-01-03T16:04:18Z

Fixes #1115
Fixes #1152
Fixes #1122
Fixes #1094

vadz · 2024-01-11T14:37:46Z

Do you think we should wait for this before making SOCI 4.1.0 release or can we leave this change until a later 4.1.x?

Krzmbrzl · 2024-01-13T19:31:37Z

Do you think we should wait for this before making SOCI 4.1.0 release or can we leave this change until a later 4.1.x?

Well, it would probably be nice if 4.1 had this already, but it's more of a nice-to-have. So I guess you don't have to wait for this PR to be done.
However, my planned timeline for this PR is to get this finished within the next two to three weeks. Depending on your plans for 4.1, waiting for such a time period would be okay? But as I said: it's optional.

Unrelated to this PR, I would appreciate if #992 made it into 4.1 though :)

This can lead to inconsistencies when another project embeds SOCI which especially on Windows are likely to lead to issues when SOCI is built as a DLL. Fixes SOCI#1159

It turned out that on Windows this is actually necessary in order for DLLs to be usable. In contrast to what has been removed in a2faea6, this commit adds a guard that will ensure that when CMAKE_RUNTIME_DIRECTORY (which is the crucial one in order for DLLs to work) is already set, we don't overwrite it. This means that we should stick to whatever has already been configured, avoiding any inconsistencies. Additionally, we now emit a warning if SOCI is embedded by a project that doesn't set these variables as that can also lead to inconsistencies as then the order of when exactly SOCI is embedded into the top-level project matters. Relates to SOCI#1159

The install paths can now be configured by setting the corresponding SOCI_INSTALL_* variables. Additionally, this commit fixes the header files being installed under .../soci/soci/bla.h (two "soci" subdirs) and now install properly under .../soci/bla.h

There seems to be a bug in Visual Studio 2015 that leads to a call tp logger_impl::set_stream instead of standard_logger_impl::set_stream in the "basic logging support" test case, which throws a "not supported" exception instead of actually setting the log stream, leading to a test case failure. This hasn't been observed for any other compiler, including more recent versions of Visual Studio. Thus, it was concluded that this was indeed a compiler bug in VS2015. Moving the standard_logger_impl implementation out of the anonymous namespace works around this bug without affecting the behavior of the library in any way.

Krzmbrzl · 2024-09-01T18:42:59Z

@vadz Now that I finally worked around the VS2015 bug, I think this PR is ready for review

@papoteur-mga I'd appreciate your continued feedback on this too :)

vadz

Thanks again for all your work on this!

Unfortunately I ran out of time for today at 77/104 files viewed, I'll try to finish this a.s.a.p. but I'd like to already publish the existing comments/questions to see what do you think about them.

I'd also like to mention that I had seen in some discussions that you were not sure which directory structure to use for the installation. IMO the answer to this is always "the same one as now for compatibility unless there are some really good reasons to change it", so I hope we're not going to change things if we can avoid it. In fact, if we could avoid all these renamings even internally, it would be nice too, but, of course, it's the externally visible file locations, targets, option values etc that are the most important.

vadz · 2024-09-03T12:27:31Z

.github/workflows/ci.yml

+            no_boost: true
+          - backend: oracle
+            no_boost: true
+            runner: ubuntu-22.04


Seems redundant as this is the default anyhow?

(also below)

vadz · 2024-09-03T12:28:06Z

.github/workflows/ci.yml

+        test_release_package: [false]
+        build_examples: [false]


Just out of curiosity, why do we define these ones here but not no_boost which looks like a similar boolean option?

vadz · 2024-09-03T12:35:50Z

.github/workflows/ci.yml

+          - backend: oracle
+            no_boost: true
+            runner: ubuntu-22.04
+          - name: SQLite3 Cxx17


This name is not used in the summary pane any longer, but the summary looked much nicer when it was, e.g. compare the current summary
with the new one .

Could we please use the names in the summary again?

vadz · 2024-09-03T12:36:30Z

.github/workflows/ci.yml

    strategy:
      fail-fast: false
      matrix:
+        lib_type: [shared, static]


This doubles the number of builds and I'm not sure if it's worth it... Maybe it would be enough to test just some static builds instead of testing all of them?

vadz · 2024-09-03T12:54:57Z

tests/common-tests.cpp

+
+// Important: we have to include catch before common.tests.h in order to
+// make the CATCH_CONFIG_RUNNER definition have an effect
+#define CATCH_CONFIG_RUNNER


Could we please have a separate main.cpp predefining this and containing just #include <catch.hpp> and main() definition?

This would improve rebuild performance a lot, as you wouldn't need to recompile 80% of CATCH when changing any common tests.

vadz · 2024-09-03T13:24:07Z

examples/connect/CMakeLists.txt

-# Note that depending on the SOCI library automatically pulls in the required
-# SOCI compilation options too, i.e. there is no need to explicitly use
-# target_include_directories().


I would keep this comment, why remove it? It can be useful to people new to CMake.

vadz · 2024-09-03T13:24:50Z

examples/connect/CMakeLists.txt

-# Linking with just soci_core is enough when using shared libraries, as the
-# required backends will be loaded dynamically during run-time, but when using
-# static libraries you would need to link with all the soci_<backend> libraries
-# needed too.


I would probably also keep this part of the comment too, especially if it's still correct (is it?) and just mention that SOCI::soci links with everything.

vadz · 2024-09-03T13:26:53Z

src/backends/CMakeLists.txt

-#
-###############################################################################
-colormsg(_HIBLUE_ "Configuring SOCI backend libraries:")
+set(SOCI_EMPTY ${PROJECT_IS_TOP_LEVEL} CACHE STRING "Include the 'empty' backend. Can be bool-valued or one of 'Enabled', 'Disabled' and 'AsAvailable'")


Does AsAvailable really make sense here?

vadz · 2024-09-03T13:32:03Z

src/backends/CMakeLists.txt

Sorry but I don't understand why should we support the non-standard Enabled and Disabled options in addition to the standard ON/OFF ones. Is it for compatibility with the existing cmakefiles (if so, I didn't even know that they supported this...)? If not, I'd strongly prefer to drop them and simplify both the build files and the documentation.

As for AsAvailable, I'd ideally like to avoid having it too and for things to work like this:

SOCI_FOO not defined/empty: use backend FOO if it's available, give a warning if it isn't.

SOCI_FOO=OFF: never use it.

SOCI_FOO=ON: give an error if the backend is not available.

If we really need to use some explicit value for the first case (why?) I'd use something like CHECK.

vadz · 2024-09-03T13:33:29Z

src/backends/db2/CMakeLists.txt

+
+add_library(soci_db2
+  ${SOCI_LIB_TYPE}
+    "blob.cpp"


Inconsistent indentation and why do we quote all these file names? This makes them less readable and doesn't bring anything, it's not like we're ever going to have spaces in the source file names (or at least only over my dead body).

vadz · 2024-09-20T14:56:02Z

I'd really like to refactor the tests to avoid having to recompile the entirety of common-tests.h several times (at least once for each backend and more for ODBC) when building them, but the changed needed to do it risk conflicting with this PR.

Not sure if I should wait for it to be merged or should make these changes and resolve the conflicts later.

Krzmbrzl · 2024-09-20T19:10:58Z

So effectively you want to do the refactoring that I have already done here? 👀

Personally, I would tend to not create the same change in a different PR while we are already working on getting this PR merged 🤷

vadz · 2024-09-20T21:50:54Z

Yes, I'd like to apply this part (which is, again, something I really wanted to do since a long time, so thanks for finally doing it), except

Without test-foo.cpp renaming to foo_tests.cpp which, IMO, doesn't have enough benefits to compensate for inconveniences.
Without waiting until all the other issues elsewhere can be resolved which risks taking some time.

Would it be possible to do this somehow?

Krzmbrzl · 2024-09-21T05:49:00Z

compensate for inconveniences.

Which inconveniences?

Would it be possible to do this somehow?

I guess, but it would be a bit fiddly. Not sure if I personally see the value in having this part of the change a bit earlier given that the current approach has been used for so long (so my question here is: why the sudden hurry?) 🤷

vadz · 2024-09-21T14:32:10Z

compensate for inconveniences.

Which inconveniences?

Renaming the file complicates its history (even if Git is much better at this than many other VCS). And the real question is anyhow not "why not do it" but "why do it", i.e. do we have any good reason for renaming these files? And I just don't see any, both test-foo and foo_tests are pretty much equivalent and the choice between them seems just a matter of personal preferences (and FWIW I do prefer using - to _).

Would it be possible to do this somehow?

I guess, but it would be a bit fiddly. Not sure if I personally see the value in having this part of the change a bit earlier given that the current approach has been used for so long (so my question here is: why the sudden hurry?) 🤷

No particular hurry, but I'm thinking about this every time I have to modify the tests and I plan on doing this again soon, so I thought it would be nice to finally get it done.

Krzmbrzl force-pushed the revamp-cmake branch from 7815446 to c8d5da6 Compare February 13, 2024 17:37

This was referenced Feb 17, 2024

Missing backend includes #1122

Open

fsanitize=undefined in parent project causes to link due to missing rtti infos in soci.so #1103

Closed

Krzmbrzl force-pushed the revamp-cmake branch 7 times, most recently from 0b83bb8 to e1e4433 Compare February 17, 2024 16:02

Krzmbrzl force-pushed the revamp-cmake branch 18 times, most recently from e4b2e0d to 39b9037 Compare February 27, 2024 19:04

Krzmbrzl added 2 commits August 31, 2024 16:38

Link core to DL libraries

08eb273

Remove debug message

21515f2

Krzmbrzl force-pushed the revamp-cmake branch from fce3cbb to 21515f2 Compare August 31, 2024 14:40

Don't set output directories

a2faea6

This can lead to inconsistencies when another project embeds SOCI which especially on Windows are likely to lead to issues when SOCI is built as a DLL. Fixes SOCI#1159

This was referenced Aug 31, 2024

fatal error: 'mysql.h' file not found #1152

Open

CMake installation issues with soci as a dependency #1094

Closed

Krzmbrzl force-pushed the revamp-cmake branch from 1830195 to 7468818 Compare August 31, 2024 15:50

Also use versioned .so install for MySQL

6a106e7

Krzmbrzl force-pushed the revamp-cmake branch from 7468818 to 77d1c95 Compare September 1, 2024 07:32

Krzmbrzl force-pushed the revamp-cmake branch from 77d1c95 to 49c2d12 Compare September 1, 2024 11:20

Rework install paths

8d14ee4

The install paths can now be configured by setting the corresponding SOCI_INSTALL_* variables. Additionally, this commit fixes the header files being installed under .../soci/soci/bla.h (two "soci" subdirs) and now install properly under .../soci/bla.h

Krzmbrzl force-pushed the revamp-cmake branch from 49c2d12 to 6f6e888 Compare September 1, 2024 11:21

Krzmbrzl added 2 commits September 1, 2024 18:43

Update example

a74341c

Fix not all headers getting installed

6b6db13

Krzmbrzl force-pushed the revamp-cmake branch 4 times, most recently from 6976799 to 1deca3e Compare September 1, 2024 17:31

Krzmbrzl force-pushed the revamp-cmake branch from 1deca3e to efe3a05 Compare September 1, 2024 17:46

Krzmbrzl marked this pull request as ready for review September 1, 2024 18:42

vadz reviewed Sep 3, 2024

View reviewed changes

vadz marked this pull request as draft November 17, 2024 19:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revamp CMake support #1118

Revamp CMake support #1118

Krzmbrzl commented Jan 3, 2024 •

edited

Loading

vadz commented Jan 11, 2024

Krzmbrzl commented Jan 13, 2024

Krzmbrzl commented Sep 1, 2024

vadz left a comment

vadz Sep 3, 2024

vadz Sep 3, 2024

vadz Sep 3, 2024

vadz Sep 3, 2024

vadz Sep 3, 2024

vadz Sep 3, 2024

vadz Sep 3, 2024

vadz Sep 3, 2024

vadz Sep 3, 2024

vadz Sep 3, 2024

vadz commented Sep 20, 2024

Krzmbrzl commented Sep 20, 2024

vadz commented Sep 20, 2024

Krzmbrzl commented Sep 21, 2024

vadz commented Sep 21, 2024

Revamp CMake support #1118

Are you sure you want to change the base?

Revamp CMake support #1118

Conversation

Krzmbrzl commented Jan 3, 2024 • edited Loading

vadz commented Jan 11, 2024

Krzmbrzl commented Jan 13, 2024

Krzmbrzl commented Sep 1, 2024

vadz 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vadz commented Sep 20, 2024

Krzmbrzl commented Sep 20, 2024

vadz commented Sep 20, 2024

Krzmbrzl commented Sep 21, 2024

vadz commented Sep 21, 2024

Krzmbrzl commented Jan 3, 2024 •

edited

Loading