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

Revamp CMake support #1118

Open
wants to merge 79 commits into
base: master
Choose a base branch
from
Open

Revamp CMake support #1118

wants to merge 79 commits into from

Conversation

Krzmbrzl
Copy link
Contributor

@Krzmbrzl Krzmbrzl commented Jan 3, 2024

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

@vadz
Copy link
Member

vadz commented Jan 11, 2024

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
Copy link
Contributor Author

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
@Krzmbrzl Krzmbrzl force-pushed the revamp-cmake branch 4 times, most recently from 6976799 to 1deca3e Compare September 1, 2024 17:31
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 Krzmbrzl marked this pull request as ready for review September 1, 2024 18:42
@Krzmbrzl
Copy link
Contributor Author

Krzmbrzl commented Sep 1, 2024

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

Copy link
Member

@vadz vadz left a 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
Copy link
Member

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)

Comment on lines +33 to +34
test_release_package: [false]
build_examples: [false]
Copy link
Member

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
image with the new one image.

Could we please use the names in the summary again?

strategy:
fail-fast: false
matrix:
lib_type: [shared, static]
Copy link
Member

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
Copy link
Member

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.

Comment on lines -18 to -20
# 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().
Copy link
Member

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.

Comment on lines -22 to -25
# 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.
Copy link
Member

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'")
Copy link
Member

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?

Copy link
Member

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:

  1. SOCI_FOO not defined/empty: use backend FOO if it's available, give a warning if it isn't.
  2. SOCI_FOO=OFF: never use it.
  3. 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"
Copy link
Member

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

@vadz
Copy link
Member

vadz commented Sep 20, 2024

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
Copy link
Contributor Author

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
Copy link
Member

vadz commented Sep 20, 2024

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

  1. Without test-foo.cpp renaming to foo_tests.cpp which, IMO, doesn't have enough benefits to compensate for inconveniences.
  2. 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
Copy link
Contributor Author

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
Copy link
Member

vadz commented Sep 21, 2024

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.

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