Add glibmm (2.66.8, 2.88.0) to xrepo#9928
Conversation
Adds the latest versions of glibmm-2.4 and glibmm-2.68 respectively, to xrepo. glibmm cannot be built static for msvc-like compilers though, per [glib/meson.build](https://gitlab.gnome.org/GNOME/glibmm/-/blob/master/glib/meson.build).
There was a problem hiding this comment.
Code Review
This pull request introduces a new package definition for glibmm, supporting multiple versions and their corresponding dependencies. Feedback focuses on ensuring proper linking by adding add_links and C++ standard requirements, making the shared configuration mutable for non-Windows platforms, and explicitly passing the library type to the Meson build system. Additionally, it is recommended to enhance the package test by verifying actual code compilation and linking rather than just header presence.
| on_load(function (package) | ||
| -- glibmm doesn't allow static build for MSVC-like compilers. | ||
| if package:is_plat("windows") then | ||
| package:config_set("shared", true) | ||
| end | ||
|
|
||
| if package:version():lt("2.68") then | ||
| package:add("deps", "libsigcplusplus <3.0.0") | ||
| package:add("deps", "glib >=2.61.2") | ||
| else | ||
| package:add("deps", "libsigcplusplus >=3.0.0") | ||
| package:add("deps", "glib >=2.87.3") | ||
| end | ||
|
|
||
| local abi = package:version() and package:version():lt("2.68") and "2.4" or "2.68" | ||
| package:add("includedirs", | ||
| "include/glibmm-" .. abi, | ||
| "lib/glibmm-" .. abi .. "/include", | ||
| "include/giomm-" .. abi, | ||
| "lib/giomm-" .. abi .. "/include") | ||
| end) |
There was a problem hiding this comment.
The on_load block is missing add_links, which is critical for consumers to link against the library. Additionally, it's recommended to specify the required C++ standard (c++11 for ABI 2.4 and c++17 for ABI 2.68) so that it propagates to consumer projects. I've also refactored the logic to be cleaner and avoid duplicated version checks.
on_load(function (package)
if package:is_plat("windows") then
package:config_set("shared", true)
end
local version = package:version()
local is_legacy = version and version:lt("2.68")
local abi = is_legacy and "2.4" or "2.68"
if is_legacy then
package:add("deps", "libsigcplusplus <3.0.0")
package:add("deps", "glib >=2.61.2")
package:add("languages", "c++11")
else
package:add("deps", "libsigcplusplus >=3.0.0")
package:add("deps", "glib >=2.87.3")
package:add("languages", "c++17")
end
package:add("links", "glibmm-" .. abi, "giomm-" .. abi)
package:add("includedirs",
"include/glibmm-" .. abi,
"lib/glibmm-" .. abi .. "/include",
"include/giomm-" .. abi,
"lib/giomm-" .. abi .. "/include")
end)
| add_versions("2.66.8", "64f11d3b95a24e2a8d4166ecff518730f79ecc27222ef41faf7c7e0340fc9329") | ||
|
|
||
| add_configs("deprecated_api", {description = "Build deprecated API and include it in the library", default = false, type = "boolean"}) | ||
| add_configs("shared", {description = "Build shared library.", default = false, type = "boolean", readonly = true}) |
There was a problem hiding this comment.
The shared configuration should not be readonly = true globally. This prevents users on non-Windows platforms from choosing between static and shared builds. The restriction for Windows should be handled in on_load (which you are already doing), but the configuration definition itself should remain mutable for other platforms.
add_configs("shared", {description = "Build shared library.", default = false, type = "boolean"})
| local configs = {"-Dbuild-documentation=false", "-Dbuild-examples=false"} | ||
| table.insert(configs, "-Dbuild-deprecated-api=" .. (package:config("deprecated_api") and "true" or "false")) | ||
| import("package.tools.meson").install(package, configs) |
There was a problem hiding this comment.
It is better to explicitly pass -Ddefault_library to ensure the build system respects the shared configuration. Also, disabling tests with -Dbuild-tests=false can speed up the installation process.
local configs = {"-Dbuild-documentation=false", "-Dbuild-examples=false", "-Dbuild-tests=false"}
table.insert(configs, "-Ddefault_library=" .. (package:config("shared") and "shared" or "static"))
table.insert(configs, "-Dbuild-deprecated-api=" .. (package:config("deprecated_api") and "true" or "false"))
import("package.tools.meson").install(package, configs)
| end) | ||
|
|
||
| on_test(function (package) | ||
| assert(package:has_cxxincludes("glibmm.h", {configs = {languages = "c++17"}})) |
There was a problem hiding this comment.
The test only checks for the presence of the header. It's better to use check_cxxsnippets with a small code snippet to verify that the library can be linked correctly.
assert(package:check_cxxsnippets({test = [[\n void test() {\n Glib::ustring s = "hello";\n }\n ]]}, {configs = {languages = "c++17"}, includes = "glibmm.h"}))
- Removed the readonly = true for shared library config - Added c++ language versions for the 2 abi's. - Excluded android, iphoneos, wasm and bsd due to not having the dependencies (though glibmm IS buildable on freebsd).
- Remove extra double quotes.
|
Do dependencies get built as static for MinGW? In [MingW (MacOS)... shared...], I saw a lot of "multiple definition of..." from line 2854 to 3608. I'm not sure what I can do to fix this. Note that all MingW tests passed in the first commit for some reason... Or is this because of trying to build 2 versions together? |
|
I don't know why. you can ask ai. You are hitting a “multiple definition” linker error, caused by two different objects (from static and/or import libraries) both defining the same GLib/GObject symbols (e.g., g_signal_connect_object, g_signal_connect_data, etc.). This typically happens in MinGW cross-compilation (as is your case) when: Both a static library (like libgobject-2.0.a) and a DLL import library (like libglibmm_generate_extra_defs-2.68.dll.a) are linked together, but both provide object code for the same symbols. How to Fix
|
Adds the latest versions of glibmm-2.4 and glibmm-2.68 respectively, to xrepo. glibmm cannot be built static for msvc-like compilers though, per glib/meson.build.
This closes #9851 .