ci: experiment with new mingw clang64#808
ci: experiment with new mingw clang64#808SchrodingerZhu wants to merge 12 commits intomicrosoft:mainfrom
Conversation
|
It seems that However, there is a cmake issue that I do not understand: By using cc @mjp41 |
could be this in cmakelists: target_link_options(${name} PRIVATE |
|
I think it is working now. |
|
If you mean to use If you want to use clang that has a MinGW target by default, you should use MSYS2 CLANG64 shell. |
|
@lhmouse ah, nice catch. I just found that I deleted the shell command mistakenly. |
7a8b2e1 to
fa486a2
Compare
|
I currently have a full time table. I may need more time to work on this. |
|
@lhmouse also feel free to patch on top of this if you are interested and available. Really a nice catch ... It turns out I was running clang-cl outside MSYS...... No wonder the |
|
If you really mean to call the msys2 clang, you should set |
|
There was a problem hiding this comment.
Pull request overview
This PR adds experimental MinGW Clang64 CI support following the refactored CI structure from PR #806. The changes enable building and testing snmalloc on Windows using MSYS2's Clang64 environment, which is a different toolchain from the existing MSVC-based Windows builds.
Changes:
- Added a new MinGW Clang64 GitHub Actions workflow with Debug and RelWithDebInfo build profiles
- Updated CMake to add
-fms-extensionsflag for Clang on MinGW to enable Microsoft pragma support - Changed linker flag syntax from
/DEBUGtoLINKER:/DEBUGfor better cross-linker compatibility - Excluded MS-specific pragmas (
pragma warningandpragma init_seg) from MinGW builds - Disabled the
protect_forktest on MinGW since fork functionality doesn't work on this platform
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| .github/workflows/mingw.yml | New workflow for MinGW Clang64 CI with matrix builds for Debug and RelWithDebInfo profiles |
| CMakeLists.txt | Added -fms-extensions for MinGW Clang and fixed linker flag syntax for cross-platform compatibility |
| src/snmalloc/pal/pal_windows.h | Excluded MSVC-specific pragmas from MinGW builds to avoid compiler errors |
| src/test/func/protect_fork/protect_fork.cc | Skip test on MinGW where fork() is not supported |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cmake | ||
| --build "$(cygpath -u "$GITHUB_WORKSPACE")/build" | ||
| --parallel | ||
|
|
There was a problem hiding this comment.
There is trailing whitespace at the end of this line. Removing it would improve consistency.
| run: | | ||
| cd "$(cygpath -u "$GITHUB_WORKSPACE")/build" | ||
| ctest | ||
|
|
There was a problem hiding this comment.
There is trailing whitespace at the end of this line. Removing it would improve consistency.
| -B"$(cygpath -u "$GITHUB_WORKSPACE")/build" | ||
| -DCMAKE_BUILD_TYPE=${{ matrix.profile }} | ||
| -DCMAKE_CXX_COMPILER=clang++ | ||
| -DCMAKE_C_COMPILER=clang |
There was a problem hiding this comment.
The configure step is missing the -DSNMALLOC_CI_BUILD=ON flag that is consistently used in other CI workflows. This flag enables additional debug information and backtrace support in CI builds, which would be beneficial for debugging failures in this workflow as well.
| -DCMAKE_C_COMPILER=clang | |
| -DCMAKE_C_COMPILER=clang | |
| -DSNMALLOC_CI_BUILD=ON |
| shell: msys2 {0} | ||
| run: | | ||
| cd "$(cygpath -u "$GITHUB_WORKSPACE")/build" | ||
| ctest |
There was a problem hiding this comment.
The ctest invocation is missing common flags used in other workflows. It should include --output-on-failure to provide better debugging information when tests fail, -j 4 for parallel execution, and --timeout 400 to prevent hanging tests. For consistency with other workflows (see .github/workflows/reusable-cmake-build.yml:115), consider using: ctest --output-on-failure -j 4 -C ${{ matrix.profile }} --timeout 400
| ctest | |
| ctest --output-on-failure -j 4 -C ${{ matrix.profile }} --timeout 400 |
| profile: [RelWithDebInfo, Debug] | ||
|
|
||
| name: MinGW Clang64 Build (${{ matrix.profile }}) | ||
| runs-on: windows-2025 |
There was a problem hiding this comment.
The job is missing a timeout-minutes configuration. Other workflows in this repository consistently set timeout-minutes (e.g., 30 minutes for similar build jobs in reusable-cmake-build.yml). This is important to prevent jobs from hanging indefinitely and consuming runner resources.
| runs-on: windows-2025 | |
| runs-on: windows-2025 | |
| timeout-minutes: 30 |
|
|
||
| target_link_options(${name} PRIVATE |
There was a problem hiding this comment.
There is trailing whitespace at the end of this line. While this doesn't affect functionality, removing it would improve code consistency.
| target_link_options(${name} PRIVATE | |
| target_link_options(${name} PRIVATE |
|
|
||
| - name: Setup MSYS2 | ||
| id: msys2 | ||
| uses: msys2/setup-msys2@v2 |
There was a problem hiding this comment.
There is an extra blank line here. For consistency with other workflow files in the repository, there should be a single blank line between steps.
| - name: Setup MSYS2 | |
| id: msys2 | |
| uses: msys2/setup-msys2@v2 | |
| - name: Setup MSYS2 | |
| id: msys2 | |
| uses: msys2/setup-msys2@v2 | |
| uses: msys2/setup-msys2@v2 |
|
|
||
| name: MinGW Clang64 Build (${{ matrix.profile }}) | ||
| runs-on: windows-2025 | ||
|
|
There was a problem hiding this comment.
There is trailing whitespace at the end of this line. Removing it would improve consistency.
Add a clang64 based mingw CI.
This should follow #806