-
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
Revamp CMake support #1118
base: master
Are you sure you want to change the base?
Revamp CMake support #1118
Conversation
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. Unrelated to this PR, I would appreciate if #992 made it into 4.1 though :) |
7815446
to
c8d5da6
Compare
0b83bb8
to
e1e4433
Compare
e4b2e0d
to
39b9037
Compare
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
7468818
to
77d1c95
Compare
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
77d1c95
to
49c2d12
Compare
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
49c2d12
to
6f6e888
Compare
6976799
to
1deca3e
Compare
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.
1deca3e
to
efe3a05
Compare
@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 :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
no_boost: true | ||
- backend: oracle | ||
no_boost: true | ||
runner: ubuntu-22.04 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems redundant as this is the default anyhow?
(also below)
test_release_package: [false] | ||
build_examples: [false] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just out of curiosity, why do we define these ones here but not no_boost
which looks like a similar boolean option?
- backend: oracle | ||
no_boost: true | ||
runner: ubuntu-22.04 | ||
- name: SQLite3 Cxx17 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strategy: | ||
fail-fast: false | ||
matrix: | ||
lib_type: [shared, static] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
|
||
// 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
# 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(). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would keep this comment, why remove it? It can be useful to people new to CMake.
# 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
# | ||
############################################################################### | ||
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'") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does AsAvailable
really make sense here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 backendFOO
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
.
|
||
add_library(soci_db2 | ||
${SOCI_LIB_TYPE} | ||
"blob.cpp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
Fixes #1115
Fixes #1152
Fixes #1122
Fixes #1094