MDEV-40608 MariaDB-devel is incomplete for plugins

[vuvova]

create and install mariadb-plugin-config.cmake

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ vaintroub
❌ vuvova
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[vuvova]

@vaintroub FYI, it's a draft, I'm not proposing it for a merge yet. But see the direction.
With that a plugin needs find_project(mariadb-plugin REQUIRED) and MARIADB_ADD_PLUGIN() to be buildable with cmake . && make, no server sources required.

[Copilot]

Pull request overview

Adds an installable CMake config intended to make the MariaDB development package usable for building plugins, and refactors plugin CMake plumbing to support that packaging.

Changes:

• Introduces support-files/mariadb-plugin-config.cmake.in and installs the configured result into ${INSTALL_SHAREDIR}/cmake/mariadb-plugin/.
• Refactors plugin discovery to live in the top-level CMakeLists.txt and introduces VERIFY_PLUGINS() in cmake/plugin.cmake.
• Renames the primary plugin macro to MARIADB_ADD_PLUGIN while keeping MYSQL_ADD_PLUGIN as a compatibility wrapper.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
support-files/mariadb-plugin-config.cmake.in	New installed CMake config template intended for plugin consumers.
support-files/CMakeLists.txt	Generates/installs the new plugin config CMake file.
CMakeLists.txt	Moves plugin subdirectory enumeration into the top-level build and calls VERIFY_PLUGINS().
cmake/plugin.cmake	Introduces MARIADB_ADD_PLUGIN, keeps MYSQL_ADD_PLUGIN wrapper, and adds VERIFY_PLUGINS().
.gitignore	Ignores the generated support-files/mariadb-plugin-config.cmake.

[vaintroub]

@vaintroub FYI, it's a draft, I'm not proposing it for a merge yet. But see the direction. With that a plugin needs find_project(mariadb-plugin REQUIRED) and MARIADB_ADD_PLUGIN() to be buildable with cmake . && make, no server sources required.

you mean "find_package(mariadb-plugin CONFIG REQUIRED)" . Yes, I thought about something like that

[Copilot]

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.

Suppressed comments (2)

support-files/mariadb-plugin-config.cmake.in:18

• should-fix: support-files/mariadb-plugin-config.cmake.in:17-18 computes basedir from the installed location, but install_layout.cmake derives *DIRABS from CMAKE_INSTALL_PREFIX (which defaults to /usr/local in a fresh external plugin build). This can cause external plugins to install into the wrong prefix unless the user manually sets -DCMAKE_INSTALL_PREFIX. Consider defaulting the install prefix to the detected MariaDB base dir when the prefix is still the CMake default.

SET(SERVER_VERSION @SERVER_VERSION@)
GET_FILENAME_COMPONENT(basedir "${CMAKE_CURRENT_LIST_DIR}/../../.." ABSOLUTE)

support-files/mariadb-plugin-config.cmake.in:53

• nit: support-files/mariadb-plugin-config.cmake.in:49-53 validates that the minor version fits in a byte, but PLUGIN_HEX_VERSION also encodes the major version as major*256+minor. If major exceeds 255, the encoding can overflow the intended 16-bit range / mismatch expectations.

  IF(NOT ARG_VERSION MATCHES "^([0-9]+)\\.([0-9]+)" OR CMAKE_MATCH_2 GREATER 255)
    MESSAGE(FATAL_ERROR "Plugin ${plugin} has no or invalid VERSION")
  ENDIF()
  SET(V_MAJOR ${CMAKE_MATCH_1})
  SET(V_MINOR ${CMAKE_MATCH_2})

[Copilot]

Pull request overview

Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.

[vaintroub]

I (together with Claude) have tried to use that new find_package and macro, end-to-end as a tester would do (create package - only ZIP, and verified on tar.gz - unpack into scratch directory, create dummy plugins, compile and install), and here are the things I bumped into in the process. I include the fixes via Claude, in bb-11.4-MDEV-40608-wlad , use them at your own convenience. Sometimes comments are very wordy, I did not care for that.

Point 1. Blocker: Windows is excluded, mariadb-plugin.cmake is not packaged. Fix: 0e5a2404e7e

Point 2. Blocker: mysqlservices is linked as a bare name with nothing telling the linker where it is; also headers were only added for STORAGE_ENGINE/RECOMPILE_FOR_EMBEDDED, so no ordinary plugin can even find mysql/plugin.h. Fix: 765e4f273f8

Point 3. Blocker: CMAKE_INSTALL_PREFIX is never pointed at the package location the plugin is being built against, so cmake --install drops it in some generic default location instead. Fix: b977b9d5216

Point 4. Blocker (Windows only, storage engines): same bare-name problem as Point 2, for server. Fix: 30de896576f

Point 5. Severe: No tests. I Added tests/mariadb-add-plugin-test/ and a GitHub Actions workflow so this doesn't regress silently - builds dummy auth/client-auth/storage-engine plugins against a freshly packaged server on both Windows and Linux, verified green on both. 13970d07361

Point 6. Severe : Not a blocker, but worth a hard look before this ships: find_package(mariadb-plugin) leaks a lot more than the two unnamespaced targets (mysqlservices, server) into the caller's global CMake state - it defines ~10 unprefixed macros/functions (MYSQL_ADD_PLUGIN, VERIFY_PLUGINS, etc.), a bare custom target called GenError, dozens of INSTALL_*DIR/CPACK_* variables, and - the one that actually bit us in testing - unconditionally prepends -DDBUG_OFF to CMAKE_C_FLAGS/CMAKE_CXX_FLAGS for the whole including project, and calls INCLUDE(CPack) a second time if a plugin author's own CMakeLists.txt calls MARIADB_ADD_PLUGIN more than once (our own three-plugin test project hits this every time: CPack.cmake has already been included!!). None of this is fatal, but it's a lot of ambient global state for a project to inherit just by calling find_package. Some of it is cheap to narrow: DBUG_OFF as an INTERFACE_COMPILE_DEFINITIONS on mysqlservices instead of the global flags, EXTERNAL_PLUGIN_PRE/POST as FUNCTIONs instead of MACROs (external-only, so none of the in-tree static-plugin-list PARENT_SCOPE mechanism applies there) to stop stray variables and the CMAKE_POLICY change from leaking by default, and a guard around the INCLUDE(CPack) call. Or no INCLUDE(CPack) at all, let people do that themselves, as it has some quirks. It needs to be at the very end of CMake to know all targets.

Point 7 (question, not a demand): the external path only really needs ADD_LIBRARY(MODULE), a link to mysqlservices (plus server for storage engines on MSVC/AIX), and one INSTALL(), provided those mysqlservices/server libraries carry their header dependencies as INTERFACE_INCLUDE_DIRECTORIES, as in 765e4f273f8. link flag -Wl,-no-undefined for "pure" plugins on Linux would also be in order, I'd even require that. Everything else currently shared with the in-tree macro - the GenError dependency, the PLUGIN_${plugin} cache variable, static/dynamic selection, the MYSQL_INSTALL_TARGETS stub - is either moot once MODULE_ONLY is forced, or a no-op standing in for something that simply doesn't apply externally. Given how little of the real machinery survives once you strip that out, wouldn't the external path be better off as its own small dedicated function, rather than reusing the in-tree macro and patching each leak (Point 6) one at a time? CPack packaging (letting a plugin ship its own .rpm/.deb) could stay, just opt-in rather than unconditional. Adding proper NAMESPACE to exported mysqlservices/server libraries would then also be trivial.
I have not attempted to look at what the deb/rpm packaging is doing here - I would not be the right reviewer for that.

[vuvova]

Thanks. Windows was excluded intentionally, I would not be the right implementor for that. But perhaps I'll be able to whip something up based on your commits.

The MariaDB-devel or libmariadb-dev was supposed to be installed, this puts libmariadbservices into the standard /usr/lib location and the linker can find it. Headers are also in the standard location /usr/include/mysql. Additional headers are added for plugins that use server internal headers, normal plugins don't need them. Your commit — likely — makes it easier to build plugins when a devel package in not installed in the standard location. It was possible before, I did it, but it required a cmake option and environment variable to be set manually. I wanted to improve it, but it wasn't a priority. Good, if your commit helps here, I'll take it, still, it's not a main use case, so I likely won't try too hard.

I don't even understand some other claude points. How did you use this cmake file, what plugin did you try to build and how exactly?

[vaintroub]

Well, if Windows is intentionally excluded, I'm definitely not the right reviewer for that :) Whatever I'm reviewing, will at the end work on Windows, if it can. This work can, if I stays the reviewer :)

CMake config modules do not guess. They know where things are pretty much exactly, even if it is not /usr/lib . If you find_package(<foobar> CONFIG REQUIRED), using a -config.cmake file from an unpacked ZIP, or from cmake --install . --prefix /tmp there is no guessing. Libraries are coming from your installation, and your -config.cmake contains cmake targets, with their transitive dependencies, and header dependencies. bare -l pointing whoknowswhere does not fare here.

It is best if targets that are exported from config are prefixed with namespace. mariadb:: namespace, or mariadb-plugin:: . This is what -config.cmake are for , for targets. Of course nobody can hinder you to export a function from a config file, like mariadb_add_plugin(), but if it is there to link with some libraries, those libraries must be targets with full path name, and they include their transive library and header dependencies, and compile flags and such, via target_include_directories, target_compile_options. this is why INSTALL(EXPORT) exist, so you stop guessing and your "make VERBOSE=1" does not include -lmysqlservices or mysqlservices.lib, but always full and correct library path name, requred -I<path/to/correct/include_dir>, and compile options you need, in this case -DDBUG_OFF.

Anyway, the testcase.

1.There is nothing installed on the box
2. I get a package, tar.gz or zip, and unpack the package in /tmp/bla
3. I switch to directory with my own cmake project. Its CMakeLists.txt builds 3 dummy plugins: auth, storage_engine and a client plugin. it uses find_package(mariadb-plugin) and the mariadb_add_plugin macro and nothing else.
5. I configure with cmake -DCMAKE_PREFIX_PATH=/tmp/bla . # prefix path set so that the cmake config file is found
6. I build with cmake --build . # or "make" of you prefer)
7. I install with cmake --install . # I expect installed shared libraries to be in /tmp/bla/lib/plugin
8. I test that mariadbd --plugin-load-add=my_dummy_plugin --help works

If you use RPM or DEB or whatever else that installs into "standard paths", the above is supposed to work the same, you just do not need DCMAKE_PREFIX_PATH in step5

This is the full technology demonstration:
Open https://github.com/mariadb/server/tree/bb-11.4-MDEV-40608-wlad . There is a green checkbox on this page, next to commit description, click on it. scroll down, you'd find 2 new github actions runners. This is CI integration for the test that this cmake config and macro work. Open one of them https://github.com/MariaDB/server/actions/runs/33004954010/job/98296157972 , find "Test external plugin build", and find what I described above, steps 1-8.
What plugins does it build? Test, dummy plugins, this is the directory incl. CMakeLists.txt .

What plugins did I additionally build? TidesDB, on Windows, and on Linux. TidesDB has non-standard dependencies, so I used vcpkg to build it. If I configure vcpkg to build "static libraries" (it needs compression libs, pthreads-win32), then tidesdb is just one DLL without external dependencies. But by default (vcpkg using shared libs), tidesdb.dll would depend on 4 non-standard DLL, and this is a problem, that needs to be solved some other day.

Anyway, I do understand what Claude says. I told what I did not like, and asked him/it to formulate. Maybe it is me, or maybe it is his fault that you do not understand. Ask if something is not clear, I will give my best try to answer.

[vuvova]

about tidesdb, it's already fixed in my fork, in this commit: vuvova/tidesql@6958d1e

[vuvova]

1, 2, 3 done. 4 — I don't know, I could try, but it's not something I could do blind.
5. tests will be in buildbot in the scope of MDBF-1237.
6. and 7. — I think there's a lot of plugin.cmake does that's needed for external plugins: install layout, paths; how to install tests; libraries, dependencies, cnf file, post-trans scriptlet. A completely separate file would have to duplicate a lot of that and will be inevitably going out of sync every now and then. What I can try to do is to extract the common part into a separate macro.

[vaintroub]

I think the tests need absolutely necessarily be in the tree that actually provides the functionality. We need to test on CI, ideally buildbot, on package builders for all types of packages, ideally. But even starting with Github Actions as smoke test is fine by me. It took me about day to get this compiling, and linking, and from my POV nothing even remotely worked here, even if it already was supposed to be working already in some environment, where all things were Debian-or-rpm preinstalled.

Relying on non-existing-yet, presumably Linux-only "foundry" ecosystem to test is a no-go. We test our C++ code as we compile it, there is MTR and there are package tests, and we need to also test user-facing CMake files, as we create them. Otherwise it is equal to "ship when it compiled" attitude, regressions are inevitable.

[vuvova]

Want me to try to reduce what's leaked into the plugin? install_layout.cmake is needed basically as is, completely, but plugin.cmake could be trimmed down and not included in full

[vaintroub]

Want me to try to reduce what's leaked into the plugin? install_layout.cmake is needed basically as is, completely, but plugin.cmake could be trimmed down and not included in full

https://jira.mariadb.org/secure/attachment/78275/mariadb-plugin-package-guide-v3.pdf has the layout variables with MARIADB_ prefix, in Chapter 11.

[vaintroub]

Some things regarding visibility are still not addressed, apparently injecting INCLUDE_DIRECTORIES with our include directories into 3rd party CMakeLists.txt. The core problem: INCLUDE_DIRECTORIES() doesn't attach to the plugin's target — it attaches to the directory the calling CMakeLists.txt lives in, and applies to every target declared in that directory (and, via inheritance, every subdirectory added afterward), whether or not that other target has anything to do with the plugin.

Concretely where it can go bad,
this inside support-files/mariadb-plugin-config.cmake.in
INCLUDE_DIRECTORIES(${basedir}/@INSTALL_INCLUDEDIR@/server/private)

injects include dir with an extremely common name config.h into 3rd party . A collision with 3rd parties config.h is very likely. But it is easily avoidable if you use TARGET_INCLUDE_DIRECTORIES, and never INCLUDE_DIRECTORIES.

So could you please change that macro does not inject INCLUDE_DIRECTORIES at all.
It is cleaner.

Another point- WITH_WSREP you removed has ABI effects on the fragile ABI of the "dirty" plugins- if you compile server with it, and plugin without it, THD are not compatible anymore, they include a bunch of WITH_WSREP stuff

#ifdef WITH_WSREP
  bool                      wsrep_applier;
  bool                      wsrep_applier_closing;
  bool                      wsrep_client_thread;
  query_id_t                wsrep_last_query_id;
  XID                       wsrep_xid;
  ...

Those could be "very dirty" plugins, that also define MYSQL_SERVER and access THD directly, but there is a bunch of such things in our repo.

Another points that still hold, I do not think 3rd party mariadb_add_plugin should automatically force installaton on 3rd party, and I still think it is best maybe to provide separate helper function for INSTALL , and a bunch of supporting MARIADB_ prefixed variables via config. Speaking of which, mariadb_add_plugin should be a function, not a macro, so it does not leak its internal variables into the parent scope, but I think already said that.

[vuvova]

TARGET_INCLUDE_DIRECTORIES ­— I'm not sure. Two thoughts

• A plugin can come with a second target, like, command-line tool, that might need same includes.
• A config.h conflict is quite possible, but it's no less possible for a plugin itself

That is, setting include directories only for one target doesn't solve the conflict, it only introduces inconsistency between targets that the plugin's CMakeLists.txt will need to work around

[vaintroub]

TARGET_INCLUDE_DIRECTORIES ­— I'm not sure. Two thoughts

• A plugin can come with a second target, like, command-line tool, that might need same includes.
• A config.h conflict is quite possible, but it's no less possible for a plugin itself

That is, setting include directories only for one target doesn't solve the conflict, it only introduces inconsistency between targets that the plugin's CMakeLists.txt will need to work around

Second target can use our exported libraries as well. Which , of course should carry correct header dependency for users.
Besides, this goal was not stated anywhere before.

Anyway, target_include_directories() for targets we produce, will pull our config.h, it has higher precedence than INCLUDE_DIRECTORIES, so user's config.h can't hurt us, if you use it. But it can, if you don't.

I do not understand a point "does not solve a problem". It does solve a problem. User builds a tool in a subdirectory, he wants to use his own config.h, not the one you inject in say top-level CMakeLists.txt. So please, address this point.

[vuvova]

right, so a workaround is trivial, I didn't think of that, thanks.
still I don't see that TARGET_INCLUDE_DIRECTORIES solves anything.

WSREP — what to do about it? wsrep_client_service.h is a server header installed into include/mysql/server/private/wsrep_client_service.h it's included from sql_class.h and it includes wsrep/client_service.hpp which isn't installed. So if WITH_WSREP is defined, storage engines simply don't compile.
I can install wsrep headers into server/private/wsrep/* I guess, but it starts getting out of hand.

[vaintroub]

WSREP — what to do about it? wsrep_client_service.h is a server header installed into include/mysql/server/private/wsrep_client_service.h it's included from sql_class.h and it includes wsrep/client_service.hpp which isn't installed.

I think we need to install that header? Or else, we can say "do not define MYSQL_SERVER", do not access THD's members. I do not know how often it is used, but we define it automatically for anything that is builtin. I have no clue which other structs are WSREP dependent, but it could be more than just THD

[vuvova]

ok, WSREP is back, together with headers. What to do with DBUG_OFF? It also affects ABI, it's not written in any header, so I need to figure out whether it was used and put this into mariadb-plugin-config.cmake. But it's config-dependent. How could it work in multi-config generators?

[vaintroub]

ok, WSREP is back, together with headers. What to do with DBUG_OFF? It also affects ABI, it's not written in any header, so I need to figure out whether it was used and put this into mariadb-plugin-config.cmake. But it's config-dependent. How could it work in multi-config generators?

TARGET_COMPILE_DEFINITIONS(mysqlservices INTERFACE "$<$<NOT:$CONFIG:Debug>:DBUG_OFF>")

Something like this should work, I think
put the genex on the real mysqlservices target, in libservices/CMakeLists.txt — not on the exported/consumer-facing config template — and let CMake's own INSTALL(EXPORT) machinery resolve it for you.

[vuvova]

Yes, I thought about something like this. Not exactly that, more, like

FOREACH(BUILD_TYPE RELEASE RELWITHDEBINFO MINSIZEREL)
  SET(CMAKE_C_FLAGS_${BUILD_TYPE}  "${CMAKE_C_FLAGS_${BUILD_TYPE}} -DDBUG_OFF")
  SET(CMAKE_CXX_FLAGS_${BUILD_TYPE}  "${CMAKE_CXX_FLAGS_${BUILD_TYPE}} -DDBUG_OFF")
ENDFOREACH()

which would almost always work, except when whoever builds the server manually adds DBUG_OFF to compiler flags. But I think we don't need to try to handle this case.

[vaintroub]

Yes, I thought about something like this. Not exactly that, more, like

FOREACH(BUILD_TYPE RELEASE RELWITHDEBINFO MINSIZEREL)
      SET(CMAKE_C_FLAGS_${BUILD_TYPE}  "${CMAKE_C_FLAGS_${BUILD_TYPE}} -DDBUG_OFF")
      SET(CMAKE_CXX_FLAGS_${BUILD_TYPE}  "${CMAKE_CXX_FLAGS_${BUILD_TYPE}} -DDBUG_OFF")
ENDFOREACH()

which would almost always work, except when whoever builds the server manually adds DBUG_OFF to compiler flags. But I think we don't need to try to handle this case.

I'd rather rely on INSTALL(EXPORT) doing the right thing, and target_compile_definitons, it works with multiconfig, and I hope the actually installed library will export the right flags. The consumer (3rd party using mariadb_add_plugin) can build his debug version, while consuming server's release version, the flag should still be DBUG_OFF.

[vaintroub]

Yes, I thought about something like this. Not exactly that, more, like

FOREACH(BUILD_TYPE RELEASE RELWITHDEBINFO MINSIZEREL)
  SET(CMAKE_C_FLAGS_${BUILD_TYPE}  "${CMAKE_C_FLAGS_${BUILD_TYPE}} -DDBUG_OFF")
  SET(CMAKE_CXX_FLAGS_${BUILD_TYPE}  "${CMAKE_CXX_FLAGS_${BUILD_TYPE}} -DDBUG_OFF")
ENDFOREACH()

which would almost always work, except when whoever builds the server manually adds DBUG_OFF to compiler flags. But I think we don't need to try to handle this case.

Let me actually experiment with it with multiconfig, I'm genuinely interested in how to handle this correctly. I'll come back with my findings.

[vuvova]

FOREACH(BUILD_TYPE RELEASE RELWITHDEBINFO MINSIZEREL)
  SET(CMAKE_C_FLAGS_${BUILD_TYPE}  "${CMAKE_C_FLAGS_${BUILD_TYPE}} -DDBUG_OFF")
  SET(CMAKE_CXX_FLAGS_${BUILD_TYPE}  "${CMAKE_CXX_FLAGS_${BUILD_TYPE}} -DDBUG_OFF")
ENDFOREACH()

Just for the record, it's off the table, it's completely wrong. DBUG_OFF setting should depend on how the server was built not on the CMAKE_BUILD_TYPE of the plugin.

[vaintroub]

FOREACH(BUILD_TYPE RELEASE RELWITHDEBINFO MINSIZEREL)
  SET(CMAKE_C_FLAGS_${BUILD_TYPE}  "${CMAKE_C_FLAGS_${BUILD_TYPE}} -DDBUG_OFF")
  SET(CMAKE_CXX_FLAGS_${BUILD_TYPE}  "${CMAKE_CXX_FLAGS_${BUILD_TYPE}} -DDBUG_OFF")
ENDFOREACH()

Just for the record, it's off the table, it's completely wrong. DBUG_OFF setting should depend on how the server was built not on the CMAKE_BUILD_TYPE of the plugin.

yes, this is what I was trying to tell.

[vaintroub]

e6629d1 is how to install config related flags. It uses INSTALL(CODE) , which can accept configuration type genex, to store the interesting flags like DBUG_OFF.

Claude told me that SAFE_MUTEX and ENABLE_DEBUG_SYNC affect fragile THD ABI, in the same way as DBUG_OFF. Tested with VS + cmake --install . --config {Debug,RelWithDebInfo} , and ninja with CMAKE_BUILD_TYPE={Debug, RelWithDebInfo} + cmake --install

Could you also please integrate 7ca5c78 ? This is MSVC only, for e.g building 3rd party plugin in Debug mode against installed RelWithDebInfo, linker flags CRT library mismatch, and this is easily avoidable in patch.

[vuvova]

CMP0087 is from 3.14, we still require only 3.12. I'll use FILE(GENERATE

[vuvova]

removed config.h completely, done DBUG_OFF at al, and your CRT commit

[vaintroub]

CMP0087 is from 3.14, we still require only 3.12. I'll use FILE(GENERATE

Maybe its the time to require something newer? FILE(GENERATE) will need generated file name to ne config-dependent, it works but a little more awkward

[vaintroub]

removed config.h completely, done DBUG_OFF at al, and your CRT commit

Good, but please still fix INCLUDE_DIRECTORIES, we still have a bunch of commonly looking decimal.h, errmsg.h and big_endian.h/little_endian.h .And there is no reason, in 2026, to use INCLUDE_DIRECTORIES, really nothing I can think of. If somebody wants to our headers (why only headers)?, it should be intentional, not automatic

[vuvova]

I did, didn't I? check again

[vaintroub]

I did, didn't I? check again

Yes, you did. Thanks.

[vaintroub]

CMP0087 is from 3.14, we still require only 3.12. I'll use FILE(GENERATE

This does not work, as it is written, with multiconfig generators.. Check Windows packaging builder, it fails like CMake Error in support-files/CMakeLists.txt: Evaluation file to be written multiple times with different content.

You can reproduce it on Linux by using "Ninja Multi-Config" generator.
So, either the files in FILE(GENERATE) need to generate files with different, config-dependent names . Or maybe we can bump CMake to 3.14, then use INSTALL(CODE..) as in e6629d1