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

Use lowercase windows.h for building on Linux with MinGW #44

Merged
merged 1 commit into from
Nov 14, 2023
Merged

Use lowercase windows.h for building on Linux with MinGW #44

merged 1 commit into from
Nov 14, 2023

Conversation

aixxe
Copy link
Contributor

@aixxe aixxe commented Nov 13, 2023

A few very minor changes to make life a bit easier for anyone cross-compiling under Linux.

Here's a minimal example to confirm the build error with Docker:

docker run --rm -it archlinux:base-devel /bin/bash
pacman --sync --refresh --noconfirm --needed git ninja cmake mingw-w64 python
git clone https://github.com/cursey/safetyhook.git
cmake -S safetyhook/ -G Ninja -B safetyhook_build/ \
  -DCMAKE_SYSTEM_NAME=Windows \
  -DCMAKE_SYSTEM_PROCESSOR=x86_64 \
  -DCMAKE_C_COMPILER=/usr/bin/x86_64-w64-mingw32-gcc \
  -DCMAKE_CXX_COMPILER=/usr/bin/x86_64-w64-mingw32-g++ \
  -DCMAKE_FIND_ROOT_PATH=/usr/x86_64-w64-mingw32 \
  -DCMAKE_BUILD_TYPE=Release \
  -DSAFETYHOOK_FETCH_ZYDIS=ON \
  -DSAFETYHOOK_BUILD_TESTS=OFF \
  -DSAFETYHOOK_AMALGAMATE=ON
cmake --build safetyhook_build/

The PR changes can be tested by running these additional two commands:

cd safetyhook
curl https://patch-diff.githubusercontent.com/raw/cursey/safetyhook/pull/44.patch | git apply
cmake --build ../safetyhook_build/

I ran into some other errors with xbyak when building with tests, so these changes will only cover building the library itself.

Copy link
Owner

@cursey cursey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this fixes cross compilation in the case of mingw, it creates issues for cross compilation using something like clang (or clang-cl) utilizing the real Windows SDK.

I think I'd prefer if the #include <Windows.h> were wrapped in a __has_include(...) similar to how Zydis.h is included. See:

#if __has_include(<Zydis/Zydis.h>)
#include <Zydis/Zydis.h>
#elif __has_include(<Zydis.h>)
#include <Zydis.h>
#else
#error "Zydis not found"
#endif

Something like:

#if __has_include(<Windows.h>)
#include <Windows.h>
#elif __has_include(<windows.h>)
#include <windows.h>
#else
#error "Windows.h not found"
#endif

@aixxe
Copy link
Contributor Author

aixxe commented Nov 14, 2023

Thanks for reviewing! I've made those changes just now.

Copy link
Owner

@cursey cursey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cursey cursey merged commit 6d87465 into cursey:main Nov 14, 2023
5 checks passed
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.

2 participants