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

Introduce MSVC CI support #80

Merged
merged 17 commits into from
Dec 10, 2024
68 changes: 52 additions & 16 deletions .github/workflows/ci_tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,15 @@ jobs:
strategy:
fail-fast: false
matrix:
platform: [ubuntu-latest]
compiler:
- cpp: g++
platform:
- description: "ubuntu gcc"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- description: "ubuntu gcc"
- description: "Ubuntu GCC"

if it's in description?

Copy link
Member Author

Choose a reason for hiding this comment

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

Adopted.

cpp: g++
c: gcc
neatudarius marked this conversation as resolved.
Show resolved Hide resolved
- cpp: clang++
os: ubuntu-latest
- description: "ubuntu clang"
cpp: clang++
c: clang
os: ubuntu-latest
cpp_version: [17, 20, 23, 26]
cmake_args:
- description: "Default"
Expand All @@ -46,59 +49,92 @@ jobs:
- description: "ASan"
args: "-DCMAKE_CXX_FLAGS='-fsanitize=address -fsanitize=undefined'"
include:
- platform: ubuntu-latest
compiler:
- platform:
description: "ubuntu gcc"
cpp: g++
c: gcc
os: ubuntu-latest
cpp_version: 17
cmake_args:
description: "Werror"
args: "-DCMAKE_CXX_FLAGS='-Werror=all -Werror=extra'"
- platform: ubuntu-latest
compiler:
- platform:
description: "ubuntu gcc"
cpp: g++
c: gcc
os: ubuntu-latest
cpp_version: 17
cmake_args:
description: "Dynamic"
args: "-DBUILD_SHARED_LIBS=on"
# MSVC has limited support for sanitizers, so we use include here for MSVC as an exception.
# This should be simplified after beman-project/exemplar#76 is merged.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# MSVC has limited support for sanitizers, so we use include here for MSVC as an exception.
# This should be simplified after beman-project/exemplar#76 is merged.

It isn't clear what is meant by "use include here". Also, it is unclear if #76 in its current form will be merged.

Copy link
Member Author

@wusatosi wusatosi Dec 4, 2024

Choose a reason for hiding this comment

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

yeah sorry I should check my comment

- platform:
description: "windows MSVC"
cpp: cl
c: cl
os: windows-latest
cpp_version: 17
cmake_args:
description: "Default"
args: ""
- platform:
description: "windows MSVC"
cpp: cl
c: cl
os: windows-latest
cpp_version: 17
cmake_args:
description: "ASan"
# Debug infomation needed to avoid cl: C5072
# https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-c5072?view=msvc-170
args: "-DCMAKE_CXX_FLAGS='/fsanitize=address /Zi'"


name: "Unit: ${{ matrix.compiler.c }} ${{ matrix.cpp_version }} ${{ matrix.cmake_args.description }}"
runs-on: ${{ matrix.platform }}
name: "Unit: ${{ matrix.platform.description }} ${{ matrix.cpp_version }} ${{ matrix.cmake_args.description }}"
runs-on: ${{ matrix.platform.os }}
steps:
- uses: actions/checkout@v4
- name: Install Ninja
uses: lukka/get-cmake@latest
with:
cmakeVersion: "~3.25.0"
ninjaVersion: "^1.11.1"
- name: Setup MSVC
if: startsWith(matrix.platform.os, 'windows')
uses: TheMrMilchmann/setup-msvc-dev@v3
with:
arch: x64
- name: Print installed softwares
run: |
clang++ --version
g++ --version
echo "Compiler:"
${{ matrix.platform.cpp }} --version
${{ matrix.platform.c }} --version
Copy link
Member

Choose a reason for hiding this comment

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

Note that --version isn't a cl.exe flag. Maybe something like this? See this reference page.

Suggested change
${{ matrix.platform.cpp }} --version
${{ matrix.platform.c }} --version
${{ matrix.platform.cpp }} ${{ matrix.platform.cpp == 'cl' && '' || '--version' }}
${{ matrix.platform.c }} ${{ matrix.platform.cpp == 'cl' && '' || '--version' }}

This comment was marked as outdated.

Copy link
Member Author

Choose a reason for hiding this comment

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

done.


echo "Build system:"
cmake --version
ninja --version
- name: Configure CMake
run: |
cmake -B build -S . -DCMAKE_CXX_STANDARD=${{ matrix.cpp_version }} ${{ matrix.cmake_args.args }}
env:
CC: ${{ matrix.compiler.c }}
CXX: ${{ matrix.compiler.cpp }}
CC: ${{ matrix.platform.c }}
CXX: ${{ matrix.platform.cpp }}
CMAKE_GENERATOR: "Ninja Multi-Config"
- name: Build Release
run: |
cmake --build build --config Release --verbose
cmake --build build --config Release --target all_verify_interface_header_sets
cmake --install build --config Release --prefix /opt/beman.exemplar
find /opt/beman.exemplar -type f
tree /opt/beman.exemplar
wusatosi marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Maybe something like this?

Suggested change
tree /opt/beman.exemplar
cd /opt/beman.exemplar
tree
cd -

Copy link
Member Author

Choose a reason for hiding this comment

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

that's a great idea!

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think this works?
image

This comment was marked as outdated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

switch out to ls -R, and it is working. It's just very ugly...

image
image

Copy link
Member

Choose a reason for hiding this comment

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

You can use the same trick I suggested in this comment to use the /F flag on windows only

- name: Test Release
run: ctest --test-dir build --build-config Release
- name: Build Debug
run: |
cmake --build build --config Debug --verbose
cmake --build build --config Debug --target all_verify_interface_header_sets
cmake --install build --config Debug --prefix /opt/beman.exemplar
Copy link
Member

Choose a reason for hiding this comment

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

This install isn't going to do anything because it's installing to the same location as the Release install. Unless that's the intent?

Copy link
Member Author

Choose a reason for hiding this comment

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

Intention here is to test the install command.

Copy link
Member

Choose a reason for hiding this comment

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

It ends up skipping installation because it's already installed, which is a sort of test. Not critical for header only, but if there's any generated config or binaries, they shouldn't clobber each other.

find /opt/beman.exemplar -type f
tree /opt/beman.exemplar
- name: Test Debug
run: ctest --test-dir build --build-config Debug

Expand Down