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.

Sign up for GitHub

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

Jump to bottom

enable consistent DSO symbol export across POSIX/Windows + SOCI_DLL usage #1192

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

enable consistent DSO symbol export across POSIX/Windows + SOCI_DLL usage #1192

wants to merge 15 commits into from

Conversation

phetdam
Copy link

@phetdam phetdam commented Jan 16, 2025

Summary

This PR addresses the very old #560 by ensuring that the SOCI_DECL macro and related symbol import/export macros in soci-platform.h are consistently defined with the SOCI_DLL macro that indicates that SOCI is being used as a shared library. Furthermore, SOCI_DLL is set as a PUBLIC CMake compile definition for the shared SOCI library target so that downstream consumers of SOCI::soci_core can directly use target_link_libraries with their own CMake project targets.

Furthermore, the soci_backend CMake macro used to add a new SOCI backend library is enhanced to use DEFINE_SYMBOL appropriately, e.g. defining SOCI_<backend>_SOURCE when compiling a SOCI backend library, obviating the need for the manual #define SOCI_<backend>_SOURCE in each backend's source files. Similarly, the SOCI core target uses DEFINE_SYMBOL to define SOCI_SOURCE so #define SOCI_SOURCE can be removed from the SOCI core target sources.

The changes in this PR have been made with the GCC visibility wiki as an additional reference. Some miscellaneous fixes are also included in the PR, namely ensuring failover_callback in callbacks.h does not have a SOCI_DECL annotation (as the class is wholly defined inline; SOCI_DECL just confuses the linker), and building a separate soci_tests_common_static static library since the shared SOCI library tests linking against soci_tests_common require SOCI_DLL to be defined.

phetdam added 12 commits January 4, 2025 05:18
@phetdam
enable consistent SOCI_DLL definition and symbol visibility
d9f30a1 
modified:   cmake/SociBackend.cmake
    Ensure shared targets define SOCI_DLL, clean up soci_backend target
    property setting, and ensure SOCI_${NAMEU}_SOURCE is defined during build

modified:   cmake/modules/FindMySQL.cmake
    Do not set CMAKE_REQUIRED_INCLUDES unless MYSQL_INCLUDE_DIR is found (fix)

modified:   include/soci/soci-platform.h
    Ensure that SOCI_DLL needs to be defined before any sort of symbol export
    visibility can be exposed (even if SOCI_HAVE_VISIBILITY_SUPPORT is defined)

modified:   src/core/CMakeLists.txt
    Ensure SOCI_DLL is defined by SOCI core library, clean up target property
    setting, and ensure SOCI_SOURCE is defined during build
@phetdam
remove DEFINE_SYMBOL for static libraries (no effect)
3cd7a37 
modified:   cmake/SociBackend.cmake
modified:   src/core/CMakeLists.txt
@phetdam
remove manual SOCI_SOURCE definition from each core source file
7c3f6a2 
modified:   src/core/backend-loader.cpp
modified:   src/core/blob.cpp
modified:   src/core/common.cpp
modified:   src/core/connection-parameters.cpp
modified:   src/core/connection-pool.cpp
modified:   src/core/error.cpp
modified:   src/core/into-type.cpp
modified:   src/core/logger.cpp
modified:   src/core/once-temp-type.cpp
modified:   src/core/prepare-temp-type.cpp
modified:   src/core/procedure.cpp
modified:   src/core/ref-counted-prepare-info.cpp
modified:   src/core/ref-counted-statement.cpp
modified:   src/core/row.cpp
modified:   src/core/rowid.cpp
modified:   src/core/session.cpp
modified:   src/core/soci-simple.cpp
modified:   src/core/statement.cpp
modified:   src/core/transaction.cpp
modified:   src/core/use-type.cpp
modified:   src/core/values.cpp
@phetdam
remove manual SOCI_EMPTY_SOURCE definition from source files
2ca670f 
modified:   src/backends/empty/blob.cpp
modified:   src/backends/empty/factory.cpp
modified:   src/backends/empty/row-id.cpp
modified:   src/backends/empty/session.cpp
modified:   src/backends/empty/standard-into-type.cpp
modified:   src/backends/empty/standard-use-type.cpp
modified:   src/backends/empty/statement.cpp
modified:   src/backends/empty/vector-into-type.cpp
modified:   src/backends/empty/vector-use-type.cpp
@phetdam
remove manual SOCI_ODBC_SOURCE definition from source files
de9ca0e 
modified:   src/backends/odbc/blob.cpp
modified:   src/backends/odbc/factory.cpp
modified:   src/backends/odbc/row-id.cpp
modified:   src/backends/odbc/session.cpp
modified:   src/backends/odbc/standard-into-type.cpp
modified:   src/backends/odbc/standard-use-type.cpp
modified:   src/backends/odbc/statement.cpp
modified:   src/backends/odbc/vector-into-type.cpp
modified:   src/backends/odbc/vector-use-type.cpp
@phetdam
remova manual SOCI_DB2_SOURCE definition from source files
09af039 
modified:   src/backends/db2/blob.cpp
modified:   src/backends/db2/factory.cpp
modified:   src/backends/db2/row-id.cpp
modified:   src/backends/db2/session.cpp
modified:   src/backends/db2/standard-into-type.cpp
modified:   src/backends/db2/standard-use-type.cpp
modified:   src/backends/db2/statement.cpp
modified:   src/backends/db2/vector-into-type.cpp
modified:   src/backends/db2/vector-use-type.cpp
@phetdam
fix linker errors on Windows due to mismatched symbol import expectat…
4b5009d 
…ions

modified:   cmake/SociBackend.cmake
    Use soci_tests_common_static for tests linking against static SOCI libs.
    These pbject files have not been compiled with SOCI_DLL

modified:   include/soci/callbacks.h
    Remove SOCI_DECL to prevent linker unresolved symbol error as impl of
    failover_callback is 100% inline. Add comment + add <string> include for
    completeness (should not forward declare std::string)

modified:   tests/CMakeLists.txt
    Add separate soci_tests_common and soci_tests_common_static static common
    test libraries; former compiled with SOCI_DLL for use with shared libs
@phetdam
remove manual SOCI_FIREBIRD_SOURCE definition from source files
d18c5fc 
modified:   src/backends/firebird/blob.cpp
modified:   src/backends/firebird/common.cpp
modified:   src/backends/firebird/error-firebird.cpp
modified:   src/backends/firebird/factory.cpp
modified:   src/backends/firebird/session.cpp
modified:   src/backends/firebird/standard-into-type.cpp
modified:   src/backends/firebird/standard-use-type.cpp
modified:   src/backends/firebird/statement.cpp
modified:   src/backends/firebird/vector-into-type.cpp
modified:   src/backends/firebird/vector-use-type.cpp
@phetdam
remove manual SOCI_MYSQL_SOURCE definition from source files
159efb8 
modified:   src/backends/mysql/blob.cpp
modified:   src/backends/mysql/common.cpp
modified:   src/backends/mysql/factory.cpp
modified:   src/backends/mysql/row-id.cpp
modified:   src/backends/mysql/session.cpp
modified:   src/backends/mysql/standard-into-type.cpp
modified:   src/backends/mysql/standard-use-type.cpp
modified:   src/backends/mysql/statement.cpp
modified:   src/backends/mysql/vector-into-type.cpp
modified:   src/backends/mysql/vector-use-type.cpp
@phetdam
remove manual SOCI_POSTGRESQL_SOURCE definition from source files
cf97cf5 
modified:   src/backends/postgresql/blob.cpp
modified:   src/backends/postgresql/error.cpp
modified:   src/backends/postgresql/factory.cpp
modified:   src/backends/postgresql/row-id.cpp
modified:   src/backends/postgresql/session.cpp
modified:   src/backends/postgresql/standard-into-type.cpp
modified:   src/backends/postgresql/standard-use-type.cpp
modified:   src/backends/postgresql/statement.cpp
modified:   src/backends/postgresql/vector-into-type.cpp
modified:   src/backends/postgresql/vector-use-type.cpp
@phetdam
remove manual SOCI_ORACLE_SOURCE definition from source files
a5d3d14 
modified:   src/backends/oracle/blob.cpp
modified:   src/backends/oracle/error.cpp
modified:   src/backends/oracle/factory.cpp
modified:   src/backends/oracle/row-id.cpp
modified:   src/backends/oracle/session.cpp
modified:   src/backends/oracle/standard-into-type.cpp
modified:   src/backends/oracle/standard-use-type.cpp
modified:   src/backends/oracle/statement.cpp
modified:   src/backends/oracle/vector-into-type.cpp
modified:   src/backends/oracle/vector-use-type.cpp
@phetdam
remove manual SOCI_SQLITE3_SOURCE definition from source files
13e4184 
modified:   src/backends/sqlite3/blob.cpp
modified:   src/backends/sqlite3/error.cpp
modified:   src/backends/sqlite3/factory.cpp
modified:   src/backends/sqlite3/row-id.cpp
modified:   src/backends/sqlite3/session.cpp
modified:   src/backends/sqlite3/standard-into-type.cpp
modified:   src/backends/sqlite3/standard-use-type.cpp
modified:   src/backends/sqlite3/statement.cpp
modified:   src/backends/sqlite3/vector-into-type.cpp
modified:   src/backends/sqlite3/vector-use-type.cpp
@phetdam
Copy link
Author

phetdam commented Jan 16, 2025

Note that despite the huge (100+) number of changed files, the vast majority of the files are sources that have simply had their #define SOCI_<backend>_SOURCE statements removed. CMake's DEFINE_SYMBOL takes care of this macro definition.

@vadz
Copy link
Member

vadz commented Jan 16, 2025

Thanks, but I wonder if it's not going to create more conflicts with #1118.

@Krzmbrzl Any comments?

@Krzmbrzl
Copy link
Contributor

I would very much appreciate if no further CMake changes are merged into master. This PR could probably be based on top of #1118 rather easily though. Either we can include it in that PR or have the rebased PR ready for when the CMake PR gets merged.

@phetdam phetdam mentioned this pull request Jan 19, 2025
Open
@phetdam
Copy link
Author

phetdam commented Jan 19, 2025

Hey, sorry for the late response everyone.

As I mentioned above the majority of the files touched are .cpp files that have had their manual #define SOCI_<backend>_SOURCE lines removed. This also includes the core .cpp sources that have #define SOCI_SOURCE lines in them.

Material changes are fortunately pretty limited in scope and diff size, namely the following:

  1. Updating soci_backend and SOCI core target to use DEFINE_SYMBOL appropriately
  2. Define SOCI_DLL for all shared target definitions
  3. Adding a separate soci_tests_common_static since soci_tests_common has SOCI_DLL as a compile definition
  4. Require consistent SOCI_DLL usage by ensuring decl macros are nonempty for import/export when SOCI_DLL defined
  5. Removing the SOCI_DECL annotation from failover_callback since the class is fully defined inline
  6. Minor tweak to FindMySQL.cmake because MYSQL_INCLUDE_DIR wasn't always defined/nonempty

Since #1118 is PR of massive scope I can see that merging against master to ingest some of these changes would certainly result in a large set of merge conflicts (just the nature of the large refactoring). In general, I would prefer having these changes merged to master (just personal philosophy of merge improvements ASAP when possible) rather than wait. But in either case, I'd be happy to work with @Krzmbrzl to get these in #1118 as the changes are quite targeted.

@Krzmbrzl
Copy link
Contributor

Another thing that might be worth considering: By moving the SOCI_SOURCE definitions from being in the actual source files to letting CMake define them as appropriate would (could) mean that building SOCI with non-CMake buildsystems. Personally, I don't need that but I think vadz is typically using something else.

@phetdam
Copy link
Author

phetdam commented Jan 27, 2025

Ah, it's pretty straightforward to work around this. When building the SOCI core or backend libraries, the SOCI_SOURCE or SOCI_<backend>_SOURCE definition just needs to be added to the command line, e.g. with -DSOCI_SOURCE.

This is the non-CMake way to do it when using Make/MSBuild to build projects.

@Krzmbrzl
Copy link
Contributor

Yep. It's just something to keep in mind and I wanted to point out the need to do it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@phetdam @vadz @Krzmbrzl