Conversation
|
|
|
@vaintroub FYI, it's a draft, I'm not proposing it for a merge yet. But see the direction. |
There was a problem hiding this comment.
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.inand installs the configured result into${INSTALL_SHAREDIR}/cmake/mariadb-plugin/. - Refactors plugin discovery to live in the top-level
CMakeLists.txtand introducesVERIFY_PLUGINS()incmake/plugin.cmake. - Renames the primary plugin macro to
MARIADB_ADD_PLUGINwhile keepingMYSQL_ADD_PLUGINas 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. |
you mean "find_package(mariadb-plugin CONFIG REQUIRED)" . Yes, I thought about something like that |
667da4a to
0c00074
Compare
There was a problem hiding this comment.
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-18computesbasedirfrom the installed location, butinstall_layout.cmakederives*DIRABSfromCMAKE_INSTALL_PREFIX(which defaults to/usr/localin 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-53validates that the minor version fits in a byte, butPLUGIN_HEX_VERSIONalso encodes the major version asmajor*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})
0c00074 to
2cb8979
Compare
41be97a to
ca49c88
Compare
There was a problem hiding this comment.
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.
|
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? |
|
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 It is best if targets that are exported from config are prefixed with namespace. Anyway, the testcase. 1.There is nothing installed on the box 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: 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. |
|
about tidesdb, it's already fixed in my fork, in this commit: https://github.com/vuvova/tidesql/commit/6958d1ed6f2a14ba08f1f0ec90eab1bb7980b180 |
c06b0ab to
6279961
Compare
|
1, 2, 3 done. 4 — I don't know, I could try, but it's not something I could do blind. |
|
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. |
TARGET_COMPILE_DEFINITIONS(mysqlservices INTERFACE "$<$<NOT:$CONFIG:Debug>:DBUG_OFF>") Something like this should work, I think |
|
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 |
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. |
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. |
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 |
yes, this is what I was trying to tell. |
|
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. |
|
CMP0087 is from 3.14, we still require only 3.12. I'll use |
|
removed |
0157edf to
26ac30b
Compare
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 |
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 |
|
I did, didn't I? check again |
Yes, you did. Thanks. |
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. |
|
I'll use config-dependent names. Prefer to bump the requirements over something that doesn't have a trivial workaround. |
|
Ok, sill outstanding, and an important thing to add, that would actually help find all of the bugs fixes previously: I do not believe in MDBFs coverage, or into sufficient coverage once it gets into foundation's responsibility. It did not work before, and I do not see that improving. See "ecosystem" tests not working or flagging real regressions, since years. Since we produce user-facing CMake functionality, in this server repo, we'll need to test it in this repo . We do mtr, installation tests, unit test etc, so there is no reason to not test this one as well. Still unaddressed, of lesser priority, but still adding some ugliness, below:
|
|
Testing — ok, but let's do it in buildbot. It would be much cheaper. I'll create a separate issue for it, as it won't be in the server repo.
|
30856c2 to
3dadbdb
Compare
As you wish. I do not think it is bad to have test code in the server repo, but whatever.
Well, it is just a common convention to add namespace for things, if you export them for 3rd party use. Maybe you do not want to have it as mysqlservices but as MariaDB::server_services and the other part, for "dirty" plugins as MariaDB::server_internals.
It turned out to be worse than I thought, after looking closely at what PRE/POST actually do. Here is my assessment. This is currently only good for a CMake project that builds exactly one plugin, and is happy to live with MariaDB's own packaging and naming conventions. It is not good as a general macro in the shape of add_library(), because it does too many additional things on global scope: overwrite of the user's own PROJECT(), and its own ideas about install and packaging. Maybe this is fine for internal stuff, for "foundry". But it is not good for general use. And the only way out of it, for a 3rd party, right now, is to use mysqlservices and mariadb_private directly — currently unscoped, but at least they don't drag PROJECT()/CPack along with them. Here is what I found when I checked in detail: EXTERNAL_PLUGIN_PRE calls project(${target} VERSION ... DESCRIPTION ...). I checked what this actually overwrites, and it's not just PROJECT_NAME/PROJECT_VERSION — CMAKE_PROJECT_NAME too: after plugin_a: PROJECT_NAME=plugin_a CMAKE_PROJECT_NAME=plugin_a Any code the plugin author has that reads PROJECT_VERSION or CMAKE_PROJECT_NAME — for a generated header, a log message, whatever — now silently shows whichever plugin was declared last, not their own project. EXTERNAL_PLUGIN_POST sets CPACK_PACKAGE_NAME/CPACK_RPM_*/CPACK_PACKAGE_FILE_NAME and calls INCLUDE(CPack). I tried two plugins for real, and the resulting CPackConfig.cmake has only the second plugin's name. First one's package is just gone. No error, nothing. In my view, both of these are the wrong design: things that belong to the project (its name, version, package identity) get overwritten by a macro that's only supposed to build one thing. It doesn't ask, doesn't check if there's already something meaningful there, and there's no way to opt out. And this isn't only about several plugins in the same file. Even with one plugin, if the developer already has their own project() with their own version, or their own CPACK_PACKAGE_NAME because they package other things too, it gets clobbered the same way. So "reduce boilerplate for many MARIADB_ADD_PLUGIN()" isn't really the right framing — the actual problem is that we force our own idea of packaging onto someone who just wanted their plugin built. Now, criticism alone is not useful, we need an alternative. I would keep the plugin-building part of MARIADB_ADD_PLUGIN() as it is now, but take the install/packaging part out of it. A separate macro, something like MARIADB_PACKAGE_PLUGINS(), that a user calls once, explicitly, at the end of their top-level CMakeLists.txt, for whoever actually wants a package built. It would set up CPACK_PACKAGE_NAME/CPACK_RPM_*/etc, for one or several plugins/components, however many were declared. INCLUDE(CPack) itself I would still leave to the user. It already has a well-known, conventional place — the end of the top-level CMakeLists.txt — same as it works for every other CMake project. Our macro has no business calling it on the user's behalf. |
|
yes, multiple plugins in one package was intentionally not implemented. it's a missing functionality, not a bug. I see a multi-plugin cmake_minimum_required(VERSION 3.12)
project(bundle VERSION 1.2.3 DESCRIPTION "foobar bundle")
find_package(mariadb-plugin REQUIRED)
mariadb_add_plugin(foo foo.cc AUTHOR aufoo VERSION 1.1 DESCRIPTION "foo plugin")
mariadb_add_plugin(bar bar.cc AUTHOR aubar VERSION 2.2 DESCRIPTION "bar plugin")
include(CPack)which would create one package, say, cmake_minimum_required(VERSION 3.12)
project(bundle)
find_package(mariadb-plugin REQUIRED)
mariadb_add_plugin(foo foo.cc AUTHOR aufoo VERSION 1.1 DESCRIPTION "foo plugin" COMPONENT foo)
mariadb_add_plugin(bar bar.cc AUTHOR aubar VERSION 2.2 DESCRIPTION "bar plugin" COMPONENT bar)
include(CPack)which would create in that case a single-plugin cmake_minimum_required(VERSION 3.12)
project(foo)
find_package(mariadb-plugin REQUIRED)
mariadb_add_plugin(foo foo.cc AUTHOR aufoo VERSION 1.1 DESCRIPTION "foo plugin")
include(CPack)I suggest to implement all that later and only do those changes now that we know will eventually be needed and they change how users use this file. That is, |
GenError's custom command must specify headers as OUTPUT, otherwise ninja cannot deduce that mysqld.cc depends on errmsg-utf8.txt As a bonus, BYPRODUCTS lists generated files for `ninja clean`
3dadbdb to
7f3f1ed
Compare
|
Another alternative would be to do always component install in multi-plugin case with the component name equal to plugin name when no |
This works on Linux and on Windows, with rpm/deb/tar.gz/zip installations. For rpm/deb it just works, for tar.gz/zip there is no standard location, so one needs to configure plugin with -DCMAKE_PREFIX_PATH=/pah/to/mariadb/basedir after that, `cmake --install .` works too, installing in the same basedir. `cmake --build . --target package` works, creating rpm/deb/targz/zip depending on whether it's Linux or Windows and whether -DRPM or -DDEB was specified. * create and install mariadb-plugin-config.cmake * for now it only supports one plugin per project, error out if there are many * deb: move all headers that plugins need to libmariadb-dev, together with libmysqlservices.a. At least until we'll create mariadb-plugin-dev. Nobody should need huge libmariadbd-dev to develop a plugin * rpm: all in MariaDB-devel already, no changes here * install wsrep headers too, THD layout depends on WITH_WSREP * show DBUG_OFF, ENABLED_DEBUG_SYNC, and SAFE_MUTEX to plugins, same reason (it doesn't happen automatically as they're not in my_config.h) * but don't install config.h - high chance of name conflict with other projects and it's an exact copy of my_config.h anyway. * adjust plugin.cmake to work for external plugins * move server-internal part to top-level CMakeLists.txt * remove double-defined macros from unireg.h (the guard doesn't help if unireg.h is included first) ColumnStore, until fixed, needs a backward-compatibility workaround
mysqlservices only exposes a thin C API, no CRT state crosses it, so don't force whatever CRT/config built the server onto a plugin linking it. Without /Zl, a plugin built in a config with no matching installed mysqlservices variant (CMake silently substitutes one - verified with a toy project) gets an ignorable but noisy LNK4098 warning. Assisted-by: Claude:claude-5-sonnet
7ca2c29 to
c5f7a2a
Compare
c5f7a2a to
68fcba2
Compare
create and install mariadb-plugin-config.cmake