[wx/platform.h]: stop leaking the UNICODE/_UNICODE symbols to the downstream project (Issue #25184)
jamarr
jamarr81 created an issue (wxWidgets/wxWidgets#25184)Build System Used
I build wxWidgets and/or my application using:
- msvs -> vcpkg -> cmake -> msbuild
vcpkg install wxWidgets
Description
The wx/platform.h header should not be leaking the UNICODE/_UNICODE predefines that dictate how the Windows API/CRT functions get compiled/linked because this can result in Windows API/CRT function mistranslations in a downstream project/application that is already expecting to use the translated UTF8 versions.
Background
Given an application context utilizing pugixml (utf8), sqlite (utf8), luajit (utf8), curl (utf8), ws2 (utf8), winsdk (utf8) I have run into a winsdk mistranslation issue because wxwidgets (utf8) is unnecessarily leaking the UNICODE/_UNICODE preprocessor symbols in one of its headers. The result is that winsdk/ucrt calls like MessageBox, GetAddrInfo, PrintDlgEx, etc., and various other string functions are mistranslated to their W (Wide) versions, preventing the application from compiling.
Given that the application and all other libraries are working fine with utf8 strings, even with the winsdk (and ucrt) calls, adding wxwidgets (utf8) as a dependency (imo) should not prevent the application from compiling.
Context
As noted in the legacy unicode docs for Win32:
While Unicode-enabled functions in Windows use UTF-16, it is also possible to work with data encoded in UTF-8... which are supported in Windows as multibyte character set code pages... Windows also provides extensive support for code pages, and mixed-use in the same application is possible... converting only as part of a "thin layer" over the interface that must use another format... The code pages for UTF-8... support the complete Unicode character set, and conversion between these encodings and UTF-16 is lossless.
Concerns
From my understanding, wxWidgets always defines UNICODE/_UNICODE in every use case. This likely prevents (or at least deters) many Windows developers with UTF8 Application requirements from adopting wxWidgets on Windows, but it should not have to.
Solutions
From my limited understanding of wxWidgets' internal requirements, it would probably(?) be best not to define the UNICODE/_UNICODE symbols when wxUSE_UNICODE_UTF8 and wxUSE_UTF8_LOCALE_ONLY are defined. That said, even when these options are not specified, I'm not sure the "most correct" approach is to define these winsdk/ucrt symbols because that seems to make a bold assumption about which variation of winsdk/ucrt calls the downstream project/application has intentions of calling. I don't know if wxWidgets should be making such assumptions.
It seems that wxWidgets already has robust support for cross-platform handling of UTF-8 strings. Some contexts may need the raw performance of UTF16/wchar_t strings on Windows, but many others do not. Even in those use cases, it still seems(?) presumptuous to leak these symbols. If the downstream project uses UTF16/wchar_t on Windows, it can and should define these symbols in its configuration; it should not rely on wxWidgets to do so.
Platform and version information
- wxWidgets: 3.2.6
- wxWidgets port: wxMSW
- OS: Windows 11
- Compiler: MSVS 2022
- Non-default compiler options: /utf-8
As an aside, for this project, I am using static linking for all dependencies, including winsdk/ucrt and wxwidgets. I'm unsure how dynamic-linking may factor in, but I assume it should not matter in this use-case.
ā
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
jamarr
jamarr81 left a comment (wxWidgets/wxWidgets#25184)As a momentary workaround in my local build, I'm using this patch for vcpkg/wxwidgets:
--- vcpkg\overlay-ports\wxwidgets\portfile.cmake @@ -56,10 +56,18 @@ if(NOT DEFINED WXWIDGETS_USE_STD_CONTAINERS) set(WXWIDGETS_USE_STD_CONTAINERS OFF) endif() +# Should the default string mode be utf8everywhere.org? +if(NOT DEFINED WXWIDGETS_USE_UNICODE_UTF8) + set(WXWIDGETS_USE_UNICODE_UTF8 ON) +endif() vcpkg_cmake_configure( SOURCE_PATH "${SOURCE_PATH}" OPTIONS ${FEATURE_OPTIONS} + -DwxUSE_UNICODE=ON + -DwxUSE_UNICODE_UTF8=${WXWIDGETS_USE_UNICODE_UTF8} + -DwxUSE_UTF8_LOCALE_ONLY=${WXWIDGETS_USE_UNICODE_UTF8} -DwxUSE_REGEX=sys -DwxUSE_ZLIB=sys -DwxUSE_EXPAT=sys @@ -129,6 +137,9 @@ foreach(INC IN LISTS INCLUDES) endif() + # Remove winsdk/ucrt predefines from <wx/platform.h> to prevent downstream corruption of winsdk/ucrt headers. + string(REPLACE "define UNICODE" "" _contents "${_contents}") + string(REPLACE "define _UNICODE" "" _contents "${_contents}") # Remove install prefix from setup.h to ensure package is relocatable string(REGEX REPLACE "\n#define wxINSTALL_PREFIX [^\n]*" "\n#define wxINSTALL_PREFIX \"\"" _contents "${_contents}") file(WRITE "${INC}" "${_contents}") endforeach()
However, simply removing or commenting out the UNICODE/_UNICODE defined in <wx/platform.h> from a local wxWidgets copy will also allow a UTF8 application with wxwidgets (utf8) to compile successfully on Windows.
ā
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#25184)Using wxUSE_UNICODE_UTF8 is almost orthogonal to using _UNICODE: the former only defines how wxString stores its contents, while the latter defines whether we use Windows A or W functions. Of course, if you use wxUSE_UNICODE_UTF8 you probably want to use A functions to avoid conversions and vice versa, but in principle any combination could work.
Anyhow, to address this issue we'd need some way to opt out from predefining [_]UNICODE because just stopping to do it would almost certainly break huge amounts of existing code. So this can't be done by default and so we'd need to do something like this:
diff --git a/include/wx/platform.h b/include/wx/platform.h index aed10665c3..e6398b313b 100644 --- a/include/wx/platform.h +++ b/include/wx/platform.h @@ -198,12 +198,14 @@ # define wxUSE_UNICODE 1 #endif +#if !defined(wxNO_UNICODE_DEFINE) || defined(WXBUILDING) #ifndef _UNICODE # define _UNICODE #endif #ifndef UNICODE # define UNICODE #endif +#endif
(with the accompanying documentation changes).
ā
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
jamarr
jamarr81 left a comment (wxWidgets/wxWidgets#25184)@vadz, true. For a minor update version, this being made the "default" behavior could be disruptive (and non-BC), so an opt-out option would be more fitting in the short term.
It seems WXBUILDING is for static builds, which would satisfy my particular use case. However, I am curious if a dynamically linked wxwidgets (utf8) based library and associated application should also benefit from this change?
As a pure curiosity, would making the non-leaking behavior default (perhaps replacing the opt-out with an opt-in) only be considered for the next major (4.0) release? Does wxWidgets have a Milestone or similar medium for tracking bc-breaking changes that might be worth considering? Apologies if it is already somewhere obvious; I was not able to find anything myself.
ā
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#25184)It seems WXBUILDING is for static builds, which would satisfy my particular use case.
No, not at all, why? I thought we had an error in our docs but the linked page clearly says that it's defined for both static and shared builds. It is defined only when building the library itself and is necessary to keep wx itself compiling as it definitely needs these symbols to be defined.
As a pure curiosity, would making the non-leaking behavior default (perhaps replacing the opt-out with an opt-in) only be considered for the next major (4.0) release?
I don't think it's worth changing this. This is something that can be worked around outside the library relatively easily (just #undef them after including wx headers) and not worth breaking the existing code for.
ā
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
jamarr
jamarr81 left a comment (wxWidgets/wxWidgets#25184)It seems WXBUILDING is for static builds, which would satisfy my particular use case.
No, not at all, why? I thought we had an error in our docs but the linked page clearly says that it's defined for both static and shared builds. It is defined only when building the library itself and is necessary to keep wx itself compiling as it definitely needs these symbols to be defined.
Yup, my mistake. I misread the description. I probably skimmed over it too quickly.
ā
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
jamarr
jamarr81 left a comment (wxWidgets/wxWidgets#25184)As a pure curiosity, would making the non-leaking behavior default (perhaps replacing the opt-out with an opt-in) only be considered for the next major (4.0) release?
I don't think it's worth changing this. This is something that can be worked around outside the library relatively easily (just #undef them after including wx headers)...
Sure, that's true, but it's not very intuitive and (imo) seems more harmful than beneficial (as evidenced by this ticket). I don't know of any other library that leaks these symbols. Even qtbase/.../qplatformdefs.h only defines _UNICODE if UNICODE is already defined, but it's not leaking UNICODE/_UNICODE to downstream projects by default. š¤·
... and not worth breaking the existing code for.
Perhaps, but I have difficulty imagining that "a lot of" existing code specifically depends on wxWidgets defining these symbols. Projects that need these symbols should be and are likely already defining these for themselves, and it's already "breaking" projects that don't want to define them. š¤
In any case, as long as I have a convenient option to build wxwidgets (utf8) into an existing UTF8-Application, I am appreciative. š¤
ā
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
jamarr
jamarr81 left a comment (wxWidgets/wxWidgets#25184)@vadz, maybe pluralize wxNO_UNICODE_DEFINES? I do wonder if this symbol would be confusing/ambiguous in relation to wxUSE_UNICODE... but I'm not sure of alternatives, maybe wxNO_MSW_UNICODE_SYMBOLS.
ā
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#25184)I'm really fine with any spelling of this (but I agree that it's better to use a plural form), it just needs to be done and documented.
ā
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
jamarr
jamarr81 left a comment (wxWidgets/wxWidgets#25184)@vadz ... WXBUILDING ... is necessary to keep wx itself compiling as it definitely needs these symbols to be defined.
@vadz, how sure are you of this? As noted here, when using vcpkg, I'm patching the source files (wx/platform.h) and stripping these statements before compiling and have not had any compiler issues so far (wxWidgets is compiling fine with MSVS2022 on Windows). Aside from some random components having Windows/UTF8 bugs, some of which we've already patched, it has also been running fine.
I'm assuming that when UNICODE/_UNICODE symbols are not defined, and wxUSE_UNICDE/wxUSE_UNICODE_UTF8/wxUSE_UNICODE_UTF8_LOCALE_ONLY are all defined, then any Windows API/CRT calls revert to the Narrow (A) version and for any "string conversions" from UTF8 to UTF16 that need to happen, the conversion is entirely bypassed in wxWidgets and instead wholly handed off to Windows/UCRT. Would you not expect that to be the case?
From the last time I searched the wxWidgets codebase (it's been a month or so), I recall most of the code using wxUSE_UNICODE/wxUSE_UNICODE_UTF8 anywhere relevant as well as wxT(), as opposed to being directly dependent upon UNICODE/_UNICODE. I also thought the code was referencing the "default" Windows API/CRT function names and not explicitly referencing any Wide (W) versions of those functions.
But I'm just starting to get back to this, so maybe I'm forgetting/overlooking some things...
ā
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#25184)[_]UNICODE are not necessary when using "A" API with UTF-8 locale, but this is not the only or even main supported case.
ā
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
jamarr
jamarr81 left a comment (wxWidgets/wxWidgets#25184)@vadz, right. I suppose the || defined(WXBUILDING) doesn't automatically "feel right" to me since it's not valid/relevant in all cases, notably the wxMSW/wxUSE_UTF8_LOCALE_ONLY use case. That said, since UTF8 is not currently the default (and I accept it would not even be considered until at least v4.0 š ), asking those users also to define wxNO_UNICODE_DEFINES does seem like the simplest compromise. š¤·
It could be helpful to add a comment to this snippet and/or a link to this ticket to give more context/awareness to the situation.
And related to this, should page_wxusedef.tml and overview_unicode.html be updated for awareness? I'm not familiar with how wxWidgets documentation is maintained...
ā
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#25184)These pages are generated by Doxygen from docs/doxygen/mainpages/const_wxusedef.h and docs/doxygen/mainpages/const_wxusedef.h respectively and these files would indeed need to be updated.
ā
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
jamarr
jamarr81 left a comment (wxWidgets/wxWidgets#25184)@vadz, would we be able to fit this new option into the 3.3.0 milestone alongside these issues? Thanks!
ā
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#25184)This is not a real priority for me, but please consider making a PR adding the proposed symbol and updating the docs as indicated, I'd be glad to apply it. Thanks!
ā
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
jamarr
jamarr81 left a comment (wxWidgets/wxWidgets#25184)@MaartenBent, since you resolved #25803 (again, tysm!), I thought you might have some valuable insight into this related issue (see description/comments)?
This started as mainly needing to wrap the UNICODE/_UNICODE defines with WXBUILDING, to prevent leaking these symbols when the wx/platform.h header is included in the downstream project. However, I have already been building wxWidgets (v3.2.8.1 - hopefully upgrading to your patched winundef.h version soon) while stripping these two symbols when building (from vcpkg) - meaning, largely, wxWidgets already supports "Full UTF8 Builds" on Windows; making it "more official" feels long overdue.
Eventually, it would be really great if wxWidgets had an easier mechanism to enable "Full UTF8 Builds" on Windows:
#ifdef wxMSVC_USE_UTF8 #define wxUSE_UNICODE_UTF8 1 #define wxUSE_UTF8_LOCALE_ONLY 1 #define wxNO_UNICODE_DEFINES 1 #define wxNO_WIN32_W 1 #endif
You know what I mean?
ā
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#25184)@jamarr81 I understand, you want 1 define to enable all the utf-8 specific defines. Feel free to create a Pull Request adding this, and the suggested wxNO_UNICODE_DEFINES patch.
Instead of doing it in the source code, you could also add a cmake/configure option to enable the specific options and/or add the build defines. I'm fine with either.
ā
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.