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

Support LLVM MinGW with CMake. #265

Merged
merged 8 commits into from
Jan 26, 2024
Merged

Conversation

tmiw
Copy link
Contributor

@tmiw tmiw commented Dec 27, 2023

This PR updates the CMake files as follows:

  1. Updates the list of wxWidgets files to reflect updates to it since the CMake files were last touched.
  2. Includes CMake logic from wxWidgets to pull in WebView2 and include its .lib/.dll files.
  3. For LLVM MinGW only, uses a slightly modified wx/setup.h to require WebView2Loader.dll due to MinGW missing Microsoft's buffercheck code.
  4. Minor fixes to allow WinSparkle to be built on Linux instead of Windows via cross-compiling.
  5. Fixes a minor compiler error in the main WinSparkle code due to a missing #include.

Context: I'm one of the maintainers of the FreeDV project and am looking to include an auto-update mechanism into our codebase (which uses CMake for configuration and LLVM MinGW to build the Windows binaries).

Let me know if you have any questions or need me to modify anything here.

Comment on lines 109 to 113
# LLVM MinGW uses slightly different naming for UxTheme and Shlwapi.
target_link_libraries(${PROJECT_NAME} wininet version rpcrt4 comctl32 crypt32 wsock32 ws2_32 uxtheme shlwapi "${WEBVIEW2_LOADER_LIB}")
else(CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
target_link_libraries(${PROJECT_NAME} wininet version rpcrt4 comctl32 crypt32 wsock32 ws2_32 UxTheme Shlwapi)
endif(CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
Copy link
Owner

Choose a reason for hiding this comment

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

Doesn't look like something that should matter on case-insensitive Windows filesystems?

Copy link
Contributor

Choose a reason for hiding this comment

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

It matters when cross-compiling using MinGW (or similar) from Linux host.

I don't remember what project was that, but the simplest solution we've used is to use the MinGW syntax (all lower-case) which works in all cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went ahead and simplified so we use lower-case for both Clang and not-Clang.

Comment on lines 255 to 257
set(WEBVIEW2_VERSION "1.0.705.50")
set(WEBVIEW2_URL "https://www.nuget.org/api/v2/package/Microsoft.Web.WebView2/${WEBVIEW2_VERSION}")
set(WEBVIEW2_SHA256 "6a34bb553e18cfac7297b4031f3eac2558e439f8d16a45945c22945ac404105d")
Copy link
Owner

Choose a reason for hiding this comment

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

This is going to be a PITA to maintain... I'd much prefer if it used the nuget tool or at least information from packages.config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some simple regex-based parsing logic to grab the version from packages.config.

Copy link
Owner

@vslavik vslavik left a comment

Choose a reason for hiding this comment

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

I'd appreciate if this could follow the golden git rule of one commit per one self-contained change - right now some changes are split across two commits while the primary 79c5af5 commit does more than it says. In particular, wx/cmake and other compilation fixes are unrelated to Clang.

For LLVM MinGW only, uses a slightly modified wx/setup.h to require WebView2Loader.dll due to MinGW missing Microsoft's buffercheck code.

This should be an #ifdef in the setup.h file instead of duplicated all of it.

@tmiw
Copy link
Contributor Author

tmiw commented Jan 3, 2024

I'd appreciate if this could follow the golden git rule of one commit per one self-contained change - right now some changes are split across two commits while the primary 79c5af5 commit does more than it says. In particular, wx/cmake and other compilation fixes are unrelated to Clang.

Understood, have tried to do so for commits after this message.

This should be an #ifdef in the setup.h file instead of duplicated all of it.

Added #if defined(__clang__) in the existing setup.h instead of creating a new one.

@vslavik vslavik merged commit f2e0b4c into vslavik:master Jan 26, 2024
6 checks passed
@vslavik
Copy link
Owner

vslavik commented Jan 26, 2024

Thanks!

I'd like to add this to CI to avoid accidentally breaking it. Do I need to do anything special or will using MSYS2's CLANG64 environment do?

@tmiw
Copy link
Contributor Author

tmiw commented Jan 26, 2024

Thanks!

I'd like to add this to CI to avoid accidentally breaking it. Do I need to do anything special or will using MSYS2's CLANG64 environment do?

I've never tried that environment, but I've basically been testing by doing something like the following:

$ cd ~
$ wget https://github.com/mstorsjo/llvm-mingw/releases/download/20231128/llvm-mingw-20231128-ucrt-ubuntu-20.04-x86_64.tar.xz
$ tar xf llvm-mingw-20231128-ucrt-ubuntu-20.04-x86_64.tar.xz
$ export PATH=~/llvm-mingw-20231128-ucrt-ubuntu-20.04-x86_64/bin:$PATH
$ cd winsparkle/cmake
$ mkdir build
$ cd build
$ cmake -DCMAKE_TOOLCHAIN_FILE=/path/to/llvm-mingw-toolchain.cmake ..
$ make

For llvm-mingw-toolchain.cmake, one of the files over at https://github.com/drowe67/freedv-gui/tree/master/cross-compile will work (depending on platform). Hope this helps!

@vslavik
Copy link
Owner

vslavik commented Jan 27, 2024

Thanks for the pointers; I'll add this together with other CMake CI integration later, now that this is merged.

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