Linking with `wx::base` still defines `-D__WX${TOOLKIT}__` (Issue #26043)
VZ
vadz created an issue (wxWidgets/wxWidgets#26043)I've just realized that having a console application using target_link_libraries(myapp PRIVATE wx::base) results in this application being compiled with the toolkit symbol, e.g. __WXGTK__, defined.
This is a problem because it means using GUI functions is possible at compile-time and any errors are only flagged at link-time and also because when using -fsanitize=undefined, UBSAN adds dependencies on the type info objects for GUI classes that are not satisfied when linking, i.e. this results in link errors unless you additionally use -fno-sanitize=vptr (which disables a useful check).
I'm not sure how to fix this, my first attempt was:
diff --git a/build/cmake/functions.cmake b/build/cmake/functions.cmake index f8d77b0ff8..ae556d4476 100644 --- a/build/cmake/functions.cmake +++ b/build/cmake/functions.cmake @@ -388,8 +388,13 @@ function(wx_set_target_properties target_name) target_link_libraries(${target_name} PUBLIC ${wxTOOLKIT_LIBRARIES}) endif() + if(wxTARGET_IS_BASE) + target_compile_definitions(${target_name} + PUBLIC __WXBASE__ wxUSE_GUI=0) + else() target_compile_definitions(${target_name} PUBLIC ${wxTOOLKIT_DEFINITIONS}) + endif() if(wxBUILD_SHARED) string(TOUPPER ${target_name_short} target_name_upper)
but this doesn't work because linking with wx::base and wx::core now results in wxUSE_GUI redefinition: I hoped that having -DwxUSE_GUI=0 -DwxUSE_GUI=1 on the command line would work as expected, with the latter overriding the former, but it doesn't and results in a warning (which becomes an error with -Werror).
Does anybody see a way to somehow define wxUSE_GUI=0 only if only wx::base is being used but not any other target? I don't think this is possible, as all dependencies are independent at CMake level, but I could be missing something.
If not, I guess we need some new symbol wxUSE_BASE_BUT_NOT_IF_OVERRIDDEN_BY_USE_GUI_LATER (maybe with a slightly shorter name) that would make wx/defs.h define wxUSE_GUI=0 only if it's not already defined. Please let me know if anybody sees a better solution — or why this wouldn't work.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
MaartenBent left a comment (wxWidgets/wxWidgets#26043)So is the problem that base libraries (base, net, xml) define __WXMSW__ (or other toolkit), but should define __WXBASE__ (or no toolkit?)?
Or is there an additional problem with the wxUSE_GUI/wxUSE_BASE defines?
These are private, so are not inherited when linking with wx::base or wx::core. It uses the value from setup.h or defs.h.
I see the first issue in the console sample, when creating a VS solution with CMake. Can I reproduce the 2nd issue with VS?
A fix for the first issue could be:
build/cmake/functions.cmake | 9 +++++++-- build/cmake/toolkit.cmake | 7 ------- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/build/cmake/functions.cmake b/build/cmake/functions.cmake index 8cfd21b414f..4cd223f5a83 100644 --- a/build/cmake/functions.cmake +++ b/build/cmake/functions.cmake @@ -439,8 +439,13 @@ function(wx_set_target_properties target_name) target_link_libraries(${target_name} PUBLIC ${wxTOOLKIT_LIBRARIES}) endif() - target_compile_definitions(${target_name} - PUBLIC ${wxTOOLKIT_DEFINITIONS}) + if(wxTOOLKIT_DEFINITIONS AND wxUSE_GUI AND NOT wxTARGET_IS_BASE) + target_compile_definitions(${target_name} + PUBLIC ${wxTOOLKIT_DEFINITIONS}) + else() + target_compile_definitions(${target_name} + PUBLIC __WXBASE__)
+ endif()
if(wxBUILD_SHARED)
string(TOUPPER ${target_name_short} target_name_upper)
diff --git a/build/cmake/toolkit.cmake b/build/cmake/toolkit.cmake index 093a4af029a..409f5f3735d 100644 --- a/build/cmake/toolkit.cmake +++ b/build/cmake/toolkit.cmake @@ -56,13 +56,6 @@ endif() set(wxTOOLKIT_DEFINITIONS __WX${wxBUILD_TOOLKIT_UPPER}__) -if(NOT wxUSE_GUI) - set(wxBUILD_TOOLKIT "base") - string(TOUPPER ${wxBUILD_TOOLKIT} wxBUILD_TOOLKIT_UPPER) - set(WX${wxBUILD_TOOLKIT_UPPER} ON) - set(wxTOOLKIT_DEFINITIONS __WX${wxBUILD_TOOLKIT_UPPER}__) -endif() - # Initialize toolkit variables if(wxUSE_GUI) set(wxTOOLKIT_INCLUDE_DIRS)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26043)So is the problem that base libraries (base, net, xml) define
__WXMSW__(or other toolkit), but should define__WXBASE__(or no toolkit?)?
Sorry for being unclear. The problem is that base libraries do not define wxUSE_GUI=0 which is required to avoid getting GUI headers included, e.g. wx/app.h includes wx/gtk/app.h when wxUSE_GUI=1 (which is the default if it's not defined as 0).
Also, currently __WXBASE__ is defined by wx/defs.h if wxUSE_GUI==0, but not vice versa. I think your proposal here is to use the already existing __WXBASE__ instead of my wxUSE_BASE_BUT_NOT_IF_OVERRIDDEN_BY_USE_GUI_LATER and force wxUSE_GUI to be 0 if it's defined and no toolkit symbol is, right? If so, I think that it would work, but it may confuse the existing code which checks for __WXBASE__ to determine if it's being built without GUI, as now it would suddenly become defined for the GUI programs too.
Or is there an additional problem with the
wxUSE_GUI/wxUSE_BASEdefines? These are private, so are not inherited when linking withwx::baseorwx::core. It uses the value fromsetup.hordefs.h.
But the problem is that if wxUSE_GUI=0 is not defined on the command line/in compiler options, then it is 1 by default, even when it shouldn't be.
I see the first issue in the console sample, when creating a VS solution with CMake. Can I reproduce the 2nd issue with VS?
I am not sure, I don't use MSVS for this project yet, but normally it should be as simple as doing
cmake_minimum_required(VERSION 3.23) project(whatever) add_subdirectory(3rdparty/wx) add_executable(console) target_sources(console main.cpp) target_link_libraries(console PRIVATE wx::base)
and checking if you see -D__WXMSW__ in the compiler options.
A fix for the first issue could be:
EDIT: with this patch, the gui libraries will inherit
__WXBASE__and define both__WXBASE__and__WXtoolkit__.
I don't think defining __WXBASE__ for GUI programs is a good idea. It's not used much, I think, but I am afraid that all the code using it would be broken by this.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
MaartenBent left a comment (wxWidgets/wxWidgets#26043)I am not sure, I don't use MSVS for this project yet, but normally it should be as simple as doing
and checking if you see -D__WXMSW__ in the compiler options.
That is indeed what I see in with the console sample, and that is easily fixed by not adding the toolkit defines to the base libraries.
- target_compile_definitions(${target_name} - PUBLIC ${wxTOOLKIT_DEFINITIONS}) + if(wxTOOLKIT_DEFINITIONS AND NOT wxTARGET_IS_BASE) + target_compile_definitions(${target_name} + PUBLIC ${wxTOOLKIT_DEFINITIONS}) + endif()
I'll create a PR with this (but slightly modified) patch.
I don't know if this fixes the wxUSE_GUI thing, maybe it needs your solution as well.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26043)Returning this, I misdiagnosed the problem initially: __WX${TOOLKIT}__ is indeed defined even when using only wxBase when not using CMake too, and while this still feels wrong, it doesn't seem to result in any actual problems, so we can leave it aside now. The real problem is that wxUSE_GUI is not defined as 0 with CMake.
I think we could make my initial patch work by adding -UwxUSE_GUI to the non-base libraries INTERFACE compile options before doing -DwxUSE_GUI=1, but this looks ugly.
The most logical solution would be to default to -DwxUSE_GUI=0 and define it as 1 when any GUI libraries are used, but this risks being too incompatible. The simplest compatible (AFAICS) solution I see is the following:
- Define
wxUSE_BASE_LIBS=1for CMake base libraries. - Define
wxUSE_GUI_LIBS=1for CMake GUI libraries. - Define
wxUSE_GUI=0if it is not defined andwxUSE_BASE_LIBS && !wxUSE_GUI_LIBSinbuild/cmake/setup.h.in.
The idea is that wxUSE_BASE_LIBS would only be defined for CMake builds and in this case wxUSE_GUI_LIBS would be defined too if GUI libraries are used, so this should work correctly for CMake. And for all of the other builds nothing should change because neither of them will be defined.
Does anybody see any simpler solution or any reason why this wouldn't work?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
MaartenBent left a comment (wxWidgets/wxWidgets#26043)I tried with wxUSE_GUI=0 in the base interface and wxUSE_GUI=1 in the core interface, but couldn't get it to work. The will both be defined for projects linking with core, and the base one would always be the last (so active) one.
I think your solution should work. I can test different build configurations if you create a PR with it.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26043)I had another idea, please see #26074 — do you see any problems with doing it like this? AFAICS this is simpler while still being backwards-compatible (and, in particular, not affecting any other build systems at all).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.