Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

win: Remove use of Windows 8+ API #212

Merged
merged 1 commit into from
Apr 1, 2020
Merged

Conversation

jblazquez
Copy link
Contributor

This PR removes the single use of the PathCchRemoveFileSpec because that's a Windows 8+ API and a reasonable alternative that doesn't require newer OS versions is available. Indeed, the newer API is not used in MinGW builds because it's not available there either.

With this change, one can now build a Windows 7 (and probably also Windows Vista too) compatible version of Sentry.

Without this change, applications linking against the sentry-native library will not run on Windows 7. If one tries to build using CMAKE_SYSTEM_VERSION=6.1 for Windows 7 compatibility, the build fails because CMake selects the Windows SDK 8.1 version and Crashpad won't build against that version of the SDK at all:

> cmake -DCMAKE_SYSTEM_VERSION=6.1 ..
-- Building for: Visual Studio 16 2019
-- Selecting Windows SDK version 8.1 to target Windows 6.1.
...
> cmake --build . --config Debug
minidump_misc_info_writer.cc
C:\Program Files (x86)\Windows Kits\8.1\Include\um\dbghelp.h(1544,2): error C2220: the following warning is treated as an error
C:\Program Files (x86)\Windows Kits\8.1\Include\um\dbghelp.h(1544,2): warning C4091: 'typedef ': ignored on left of '<unnamed-enum-hdBase>' when no variable is declared
C:\Program Files (x86)\Windows Kits\8.1\Include\um\dbghelp.h(3190,2): warning C4091: 'typedef ': ignored on left of '<unnamed-enum-sfImage>' when no variable is declared
external\crashpad\minidump/minidump_misc_info_writer.h(114,58): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int
external\crashpad\minidump/minidump_misc_info_writer.h(114,58): error C2143: syntax error: missing ',' before '&'
external\crashpad\minidump\minidump_misc_info_writer.cc(338,41): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int
external\crashpad\minidump\minidump_misc_info_writer.cc(338,41): error C2143: syntax error: missing ',' before '&'
external\crashpad\minidump\minidump_misc_info_writer.cc(341,25): error C2039: 'XStateData': is not a member of '_MINIDUMP_MISC_INFO_4'
C:\Program Files (x86)\Windows Kits\8.1\Include\um\dbghelp.h(4407): message : see declaration of '_MINIDUMP_MISC_INFO_4'
external\crashpad\minidump\minidump_misc_info_writer.cc(341,27): error C2065: 'xstate_data': undeclared identifier
external\crashpad\minidump\minidump_misc_info_writer.cc(348,28): error C2039: 'ProcessCookie': is not a member of '_MINIDUMP_MISC_INFO_4'
C:\Program Files (x86)\Windows Kits\8.1\Include\um\dbghelp.h(4407): message : see declaration of '_MINIDUMP_MISC_INFO_4'
external\crashpad\minidump\minidump_misc_info_writer.cc(349,24): error C2065: 'MINIDUMP_MISC5_PROCESS_COOKIE': undeclared identifier
external\crashpad\minidump\minidump_misc_info_writer.cc(387,48): error C2065: 'MINIDUMP_MISC5_PROCESS_COOKIE': undeclared identifier
external\crashpad\minidump\minidump_misc_info_writer.cc(388,19): error C2065: 'MINIDUMP_MISC_INFO_5': undeclared identifier

And if one builds with CMAKE_SYSTEM_VERSION=10 then PathCchRemoveFileSpec is used unconditionally in sentry__path_dir which breaks compatibility with Windows 7.

@jblazquez
Copy link
Contributor Author

Given that building against the Windows 8.1 SDK is broken with the Crashpad backend, and given that CMake uses that SDK when targeting anything prior to Windows 10, I think more needs to be done in order to enable CMAKE_SYSTEM_VERSION to be used safely. As it stands now, anything other than CMAKE_SYSTEM_VERSION=10 will fail to build.

@Swatinem Swatinem merged commit b28ce1b into getsentry:master Apr 1, 2020
@Mixaill
Copy link
Contributor

Mixaill commented Apr 1, 2020

Well, I do not have 8.1 SDK installed (only 7.1A (xp toolchain) and 10.0), so all the system versions build fine for me. =\

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants