mingw compilation fixes #78
Conversation
On both mingw and MSVC (when targeting < Windows 10)
past-due
changed the title
past-due
force-pushed
the
mingw-patches-1
branch
from
2e1a848
to
74d6785
Compare
past-due
force-pushed
the
mingw-patches-1
branch
from
74d6785
to
59b37de
Compare
snapshot/CMakeLists.txt
Outdated
|
|
||
| # Define NTDDI_VERSION >= NTDDI_WIN10_RS5 - so InitializeContext2 definition is always available in process_reader_win.cc | ||
| # NTDDI_WIN10_RS5 = 0x0A000006 /* ABRACADABRA_WIN10_RS5 */ | ||
| set_property( | ||
| SOURCE "win/process_reader_win.cc" | ||
| APPEND | ||
| PROPERTY COMPILE_DEFINITIONS | ||
| WINVER=0x0A00 _WIN32_WINNT=0x0A00 NTDDI_VERSION=0x0A000006 | ||
| ) |
There was a problem hiding this comment.
Can you please set these definitions only for MinGW?
There was a problem hiding this comment.
IIRC (it’s been a bit since I tested) these definitions were also needed - even if using a modern Windows 10 / 11 SDK that has the appropriate header entries - when compiling with MSVC if CMAKE_SYSTEM_VERSION (for the embedding sentry-native project) was set to < 10 (targeting a Windows 7 minimum, for example).
There was a problem hiding this comment.
I would prefer that we make this as conditional as possible. The crashpad build scripts are littered with unconditional settings that only make sense in a subset of the configurations affected. Since no CI setup would test (at least the build of) MSVC projects targeting Windows 7 (which we also don't officially support), these settings should be isolated as much as possible.
How about wrapping it inside something along the lines of
if (MINGW OR ("${CMAKE_VS_WINDOWS_TARGET_PLATFORM_VERSION}" STREQUAL ""))
endif()
this should hit all MSVC target configurations below 10 (since it either isn't defined or empty), and it will work for MinGW.
There was a problem hiding this comment.
Unfortunately, a check on CMAKE_VS_WINDOWS_TARGET_PLATFORM_VERSION doesn't cover at least one common MSVC scenario.
Testing this with latest MSVC & Windows SDK, embedding sentry-native + these patches into a project with set(CMAKE_SYSTEM_VERSION "6.1"):
CMAKE_VS_WINDOWS_TARGET_PLATFORM_VERSION is set in this case (10.0.20348.0, as it is picking it up from the installed recent Windows SDK), but with actual targeting set at Windows 7+, this CMake check is insufficient and compilation ultimately fails on MSVC.
[...] external\crashpad\snapshot\win\process_reader_win.cc(332,7): error C2039: 'InitializeContext2': is not a member of '`global namespace''
(etc)
So the NTDDI_VERSION (etc) overrides are still required to expose the definitions.
I've adjusted this check to:
if (MINGW OR ("${CMAKE_VS_WINDOWS_TARGET_PLATFORM_VERSION}" STREQUAL "") OR (CMAKE_SYSTEM_VERSION LESS 10))
which does work in the several scenarios I've tested.
I have opened a PR in the parent sentry-native repo to add CI for llvm-mingw builds (including testing this crashpad integration): getsentry/sentry-native#797 |
|
Thanks for fixing this @past-due! |
- Switch Crashpad transport on Linux to use libcurl ([#803](#803), [crashpad#75](getsentry/crashpad#75), [crashpad#79](getsentry/crashpad#79)) - Avoid accidentally mutating CONTEXT when client-side stack walking in Crashpad ([#803](#803), [crashpad#77](getsentry/crashpad#77)) - Fix various mingw compilation issues ([#794](#794), [crashpad#78](getsentry/crashpad#78)) - Updated Crashpad backend to 2023-02-07. ([#803](#803), [crashpad#80](getsentry/crashpad#80))
Fixing mingw support: getsentry/sentry-native#744