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.