Revamp CMake support #1118

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

Fixes #1115
Fixes #1152
Fixes #1122
Fixes #1094
Fixes #1159
Fixes #644

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

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.

.github/workflows/ci.yml

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?

Compared to the Windows CI these are really cheap to do. Besides, previously SOCI did this duplication anyway due to building shared and static libraries in a single build. So effectively, this is reproducing the original behavior (plus some extra overhead of these things now actually being different build jobs).

Finally, it is a bit of a hassle to specify that only some combinations shall be built as a static library in GitHub Action syntax. Therefore, I would tend to just keep all tests 👀

tests/common-tests.cpp

examples/connect/CMakeLists.txt

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.

I don't want to encourage people to use different linking strategies depending on whether they want to use SOCI as a shared or static library when the way that works for static lib works just as well for the dynamic case.
This will only cause confusion if people set up their project to use SOCI as a shared library and then at some point switch to static SOCI, just to run into a bunch of errors even though for the dynamic library everything worked as expected.

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?

I'm open for better suggestions. This is effectively the current behavior where it would just check whether the dependencies are available and if not, disable the backend. This is annoying if you explicitly enabled that backend because you need it (as the cmake invocation will work fine, even though required dependencies are met).

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.

I like the general idea, but the this would only be possible, if we don't declare these options as existing in CMake. Because as soon as we do that, they can't be undefined anymore.

Defining options before using them is something that I generally consider good practice and also helps in cases where cmake is used via a GUI that lists all options (cache variables) so that the user sees what can be edited.

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

I agree, this is a better choice. Maybe AUTO would be even better? 👀

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

I consider it good practice to quote filenames as they can (in general) contain spaces and I dislike having only selected ones being quoted. But if you have a strong opinion on this, I can remove the quotes.

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 force-pushed the revamp-cmake branch 2 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

Krzmbrzl mentioned this pull request Jan 1, 2025

CMake double installing steps. #644

Open

Krzmbrzl added 5 commits January 3, 2025 16:05

Merge remote-tracking branch 'upstream/master' into revamp-cmake

c88da3b

Don't include test-main.cpp in unity builds

c2d0d15

Enable unity builds on AppVeyor

a780c39

Actually set compiler flags for UBSAN

858d585

Use standard checkout action

1d7b086

Krzmbrzl force-pushed the revamp-cmake branch 2 times, most recently from 227e55b to be77273 Compare January 3, 2025 16:33

Give acion runs better names

0709bb2

Krzmbrzl force-pushed the revamp-cmake branch from be77273 to 0709bb2 Compare January 3, 2025 16:35

Krzmbrzl added 4 commits January 3, 2025 17:47

Simplify build matrix

17c0884

Default destructor

815b9b7

Remove redundant space

fac29fe

Add comment back in

0db29f6

Krzmbrzl force-pushed the revamp-cmake branch from 906db12 to 0db29f6 Compare January 3, 2025 17:21

Krzmbrzl added 2 commits January 3, 2025 19:22

Fix bug in soci_verify_parsed_arguments

9244c88

Properly integrate new type check with new cmake layout

af92da7

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

Krzmbrzl Jan 3, 2025

vadz Sep 3, 2024

Krzmbrzl Jan 3, 2025

vadz Sep 3, 2024

Krzmbrzl Dec 31, 2024

vadz Sep 3, 2024

Krzmbrzl Jan 3, 2025

vadz Sep 3, 2024

Krzmbrzl Dec 31, 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