MDEV-37996 CMake: INTERFACE targets for bundled-or-system libraries#5311
MDEV-37996 CMake: INTERFACE targets for bundled-or-system libraries#5311vaintroub wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the MariaDB build system to use modern CMake target-based dependency management (such as MariaDB::zlib, MariaDB::OpenSSL, and MariaDB::pcre2) instead of global directory-level include variables. While this is a great improvement, several issues were identified where removing the old include variables will break compilation. Specifically, the mariadb_ssl target must be defined even when SSL is disabled to prevent a fatal CMake error, client tools and performance schema unit tests will fail to compile due to missing transitive zlib and SSL headers, and the sql target needs to explicitly link to MariaDB::zlib to inherit its include directories.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| ${CMAKE_SOURCE_DIR}/mysys_ssl | ||
| ${ZLIB_INCLUDE_DIRS} | ||
| ${SSL_INCLUDE_DIRS} | ||
| $<TARGET_PROPERTY:mariadb_ssl,INTERFACE_INCLUDE_DIRECTORIES> |
There was a problem hiding this comment.
The ${ZLIB_INCLUDE_DIRS} variable was removed from the directory-level INCLUDE_DIRECTORIES in client/CMakeLists.txt, but it was not replaced with $<TARGET_PROPERTY:mariadb_zlib,INTERFACE_INCLUDE_DIRECTORIES>. Since client tools (such as mysqldump or mysqlbinlog) directly or transitively include zlib.h, and they do not link directly to MariaDB::zlib (they link to ${CLIENT_LIB} which is libmariadb), this will cause compilation failures when using a bundled or non-standard zlib. Please add the mariadb_zlib interface include directories here.
$<TARGET_PROPERTY:mariadb_zlib,INTERFACE_INCLUDE_DIRECTORIES>
$<TARGET_PROPERTY:mariadb_ssl,INTERFACE_INCLUDE_DIRECTORIES>
c9b660d to
4bd757b
Compare
622a584 to
104843f
Compare
There was a problem hiding this comment.
Looks great!
There is however a build regression on the fulltest builders in ci. mariadb-test-embedded + pcre linking
/usr/bin/ld: ../../extra/pcre2/src/pcre2-build/libpcre2-posix.a(pcre2posix.c.o): in function `pcre2_regcomp':
/home/buildbot/amd64-ubuntu-2204-fulltest/build/extra/pcre2/src/pcre2/src/pcre2posix.c:322: undefined reference to `pcre2_pattern_info_8'
And on the connect.bson_udf and connect.json_udf tests.
Thanks for reporting! Should now be fixed . The connect failures are results of recently added overzealous checks in connector/c, which I now removed. |
|
note @drrtuy - some patterns here apply to DuckDB on merge to 11.4 |
Stop overwriting the standard find_package() result variables (ZLIB_FOUND,
ZLIB_LIBRARIES, ZLIB_INCLUDE_DIR(S), ...), this breaks vcpkg.
Provide namespaced INTERFACE targets that point at either the bundled or
the system library and carry their include directories (and, for SSL, the
compile definitions)
MariaDB::zlib, MariaDB::OpenSSL, MariaDB::pcre2-8, MariaDB::pcre2-posix,
MariaDB::fmt, MariaDB::readline
Link these consistently instead of the scattered ${*_LIBRARIES} and
${*_INCLUDE_DIR(S)} variables sprinkled across the tree.
Bundled pcre2-posix depends on pcre2-8 so the static link order is correct
for consumers that link only posix.
wolfssl carries its own usage requirements (HAVE_WOLFSSL,
WOLFSSL_USER_SETTINGS); MariaDB::OpenSSL keeps only HAVE_OPENSSL.
Update libmariadb to 3.3 (47a31a98): handles MariaDB::zlib as a submodule.
There was a problem hiding this comment.
Pull request overview
This PR modernizes the CMake dependency wiring to avoid clobbering standard find_package() result variables (notably for zlib) and instead expose consistent, namespaced INTERFACE targets (MariaDB::zlib, MariaDB::OpenSSL, MariaDB::pcre2-*, MariaDB::fmt, MariaDB::readline) that select bundled vs system implementations and propagate usage requirements.
Changes:
- Introduces namespaced INTERFACE targets for zlib/SSL/pcre2/fmt/readline in
cmake/*.cmakeand updates consumers to link them instead of${*_LIBRARIES}/${*_INCLUDE_DIR(S)}variables. - Removes a number of directory-scope include/definition injections (e.g. SSL/ZLIB include dirs) and relies on transitive target usage requirements.
- Adjusts bundled wolfssl to export required compile definitions to consumers via target usage requirements.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| vio/CMakeLists.txt | Switches VIO to link MariaDB::OpenSSL instead of raw SSL vars. |
| unittest/mysys/CMakeLists.txt | Removes SSL/ZLIB directory-scope flags from unit tests. |
| unittest/embedded/CMakeLists.txt | Drops explicit PCRE/readline include dirs from embedded unit tests. |
| storage/rocksdb/CMakeLists.txt | Links RocksDB aux lib against MariaDB::zlib. |
| storage/rocksdb/build_rocksdb.cmake | Uses MariaDB::zlib in RocksDB third-party link list. |
| storage/perfschema/unittest/CMakeLists.txt | Removes PCRE/SSL include dirs from perfschema unit tests. |
| storage/perfschema/CMakeLists.txt | Removes PCRE/SSL include dirs from perfschema build includes. |
| storage/maria/unittest/CMakeLists.txt | Links maria unit tests to MariaDB::zlib. |
| storage/maria/CMakeLists.txt | Removes SSL include dirs and switches S3 linkage to MariaDB::zlib. |
| storage/innobase/CMakeLists.txt | Links InnoDB against MariaDB::zlib. |
| storage/connect/CMakeLists.txt | Links CONNECT engine against MariaDB::zlib. |
| storage/archive/CMakeLists.txt | Links ARCHIVE engine against MariaDB::zlib. |
| sql/CMakeLists.txt | Links SQL library against MariaDB::pcre2-8, MariaDB::OpenSSL, MariaDB::fmt. |
| plugin/qc_info/CMakeLists.txt | Removes PCRE include dirs from qc_info plugin build. |
| plugin/feedback/CMakeLists.txt | Switches feedback plugin to link MariaDB::OpenSSL. |
| mysys/CMakeLists.txt | Removes zlib include dirs and links mysys against MariaDB::zlib. |
| mysys_ssl/CMakeLists.txt | Switches mysys_ssl to link MariaDB::OpenSSL and removes SSL include/defines. |
| libmysqld/examples/CMakeLists.txt | Uses MariaDB::readline and MariaDB::pcre2-* for embedded examples. |
| libmysqld/CMakeLists.txt | Rewires embedded server linkage to MariaDB::* interface targets. |
| extra/wolfssl/CMakeLists.txt | Exports wolfssl configuration macros as PUBLIC compile definitions. |
| extra/mariabackup/CMakeLists.txt | Links mariabackup against MariaDB::pcre2-posix when needed. |
| extra/CMakeLists.txt | Removes zlib include dirs from extra directory scope. |
| cmake/zlib.cmake | Defines MariaDB::zlib interface target for bundled/system zlib. |
| cmake/ssl.cmake | Defines MariaDB::OpenSSL interface target for bundled/system SSL and propagates compile defs. |
| cmake/readline.cmake | Defines MariaDB::readline interface target (empty on platforms without readline). |
| cmake/plugin.cmake | Uses target-derived zlib/ssl include dirs for plugin builds. |
| cmake/pcre.cmake | Defines MariaDB::pcre2-8 / MariaDB::pcre2-posix interface targets. |
| cmake/libfmt.cmake | Defines MariaDB::fmt interface target for header-only fmt includes. |
| client/CMakeLists.txt | Uses target-derived SSL include dirs and links readline/pcre via MariaDB::* targets. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ADD_LIBRARY(mariadb_zlib INTERFACE) | ||
| TARGET_LINK_LIBRARIES(mariadb_zlib INTERFACE ZLIB::ZLIB) | ||
| ELSE() |
| ADD_LIBRARY(mariadb_pcre2_posix INTERFACE) | ||
| TARGET_LINK_LIBRARIES(mariadb_pcre2_posix INTERFACE pcre2-posix) | ||
| TARGET_INCLUDE_DIRECTORIES(mariadb_pcre2_posix INTERFACE ${PCRE_INCLUDE_DIRS}) |
| INCLUDE_DIRECTORIES(${CMAKE_SOURCE_DIR}/include ${CMAKE_SOURCE_DIR}/zlib | ||
| ${CMAKE_SOURCE_DIR}/unittest/mytap) |
Stop overwriting the standard find_package() result variables (ZLIB_FOUND, ZLIB_LIBRARIES, ZLIB_INCLUDE_DIR(S), ...), this breaks vcpkg.
Provide namespaced INTERFACE targets that point at either the bundled or the system library and carry their include directories (and, for SSL, the compile definitions):
MariaDB::zlib, MariaDB::OpenSSL, MariaDB::pcre2-8, MariaDB::pcre2-posix,
MariaDB::fmt, MariaDB::readline
Link these consistently instead of the scattered ${*_LIBRARIES} and ${*_INCLUDE_DIR(S)} variables sprinkled across the tree.