Skip to content

Commit

Permalink
cmake: Workaround MSVC module support compiler bug
Browse files Browse the repository at this point in the history 
Adds the VULKAN_HEADERS_ENABLE_MODULE option to control whether to build the Vulkan-Hpp module.
This is necessary to allow CI to pass while waiting for the MSVC version 17.11, which fixes an
internal compiler bug, to be added to github actions runners.
  • Loading branch information
@charles-lunarg
charles-lunarg committed Jun 18, 2024
1 parent cde27c9 commit e3c37e6
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 7 deletions.
27 changes: 22 additions & 5 deletions .github/workflows/ci.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@ permissions:
contents: read

jobs:
cmake:
cmake-unix:
runs-on: ${{ matrix.os }}
strategy:
matrix:
os: [ ubuntu-latest, windows-latest, macos-latest ]
os: [ ubuntu-latest, macos-latest ]
cmake-version: [ '3.15', 'latest']
steps:
- uses: actions/checkout@v4
Expand All @@ -30,9 +30,26 @@ jobs:
cmakeVersion: ${{ matrix.cmake-version }}
- uses: ilammy/msvc-dev-cmd@v1
- run: cmake -S . -B build -D VULKAN_HEADERS_ENABLE_TESTS=ON -D VULKAN_HEADERS_ENABLE_INSTALL=ON -G Ninja
- run: cmake --build ./build --verbose
- run: cmake --install build/ --prefix build/install --verbose
- run: ctest --output-on-failure --verbose
- run: cmake --build ./build
- run: cmake --install build/ --prefix build/install
- run: ctest --output-on-failure
working-directory: build

cmake-windows:
runs-on: windows-latest
strategy:
matrix:
cmake-version: [ '3.15', 'latest']
steps:
- uses: actions/checkout@v4
- uses: lukka/get-cmake@latest
with:
cmakeVersion: ${{ matrix.cmake-version }}
- uses: ilammy/msvc-dev-cmd@v1
- run: cmake -S . -B build -D VULKAN_HEADERS_ENABLE_TESTS=ON -D VULKAN_HEADERS_ENABLE_INSTALL=ON -G Ninja -DVULKAN_HEADERS_ENABLE_MODULE=OFF # workaround for compiler bug in 17.10 and before
- run: cmake --build ./build
- run: cmake --install build/ --prefix build/install
- run: ctest --output-on-failure
working-directory: build

reuse:
Expand Down
3 changes: 2 additions & 1 deletion CMakeLists.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,9 @@ if (MSVC AND (MSVC_VERSION GREATER_EQUAL "1934") OR
set(COMPILER_SUPPORTS_CXX_MODULES TRUE)
endif()

option(VULKAN_HEADERS_ENABLE_MODULE "Enables building of the Vulkan C++ module. Default is true if supported by the CMake version and compilers" ${COMPILER_SUPPORTS_CXX_MODULES})

if (CMAKE_VERSION VERSION_GREATER_EQUAL "3.28" AND COMPILER_SUPPORTS_CXX_MODULES)
if (CMAKE_VERSION VERSION_GREATER_EQUAL "3.28" AND VULKAN_HEADERS_ENABLE_MODULE)
add_library(Vulkan-Module)
add_library(Vulkan::VulkanHppModule ALIAS Vulkan-Module)
target_sources(Vulkan-Module
Expand Down
2 changes: 1 addition & 1 deletion tests/CMakeLists.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ add_test(NAME integration.add_subdirectory
--build-and-test ${CMAKE_CURRENT_LIST_DIR}/integration
${CMAKE_CURRENT_BINARY_DIR}/add_subdirectory
--build-generator ${CMAKE_GENERATOR}
--build-options -DFIND_PACKAGE_TESTING=OFF
--build-options -DFIND_PACKAGE_TESTING=OFF -DVULKAN_HEADERS_ENABLE_MODULE=OFF
)

set(test_install_dir "${CMAKE_CURRENT_BINARY_DIR}/install")
Expand Down

6 comments on commit e3c37e6

@dj2
Copy link

@dj2 dj2 commented on e3c37e6 Jun 20, 2024

Choose a reason for hiding this comment

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

Note, this is also needed if you're building with clang-16 as otherwise you get:

FAILED: third_party/vulkan-headers/CMakeFiles/Vulkan-Module.dir/include/vulkan/vulkan.cppm.o.ddi 
"CMAKE_CXX_COMPILER_CLANG_SCAN_DEPS-NOTFOUND" -format=p1689 -- /usr/bin/clang++ ... third_party/vulkan-headers/CMakeFiles/Vulkan-Module.dir/include/vulkan/vulkan.cppm.o.ddi
/bin/sh: line 1: CMAKE_CXX_COMPILER_CLANG_SCAN_DEPS-NOTFOUND: command not found

@charles-lunarg
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note, this is also needed if you're building with clang-16 as otherwise you get:

FAILED: third_party/vulkan-headers/CMakeFiles/Vulkan-Module.dir/include/vulkan/vulkan.cppm.o.ddi 
"CMAKE_CXX_COMPILER_CLANG_SCAN_DEPS-NOTFOUND" -format=p1689 -- /usr/bin/clang++ ... third_party/vulkan-headers/CMakeFiles/Vulkan-Module.dir/include/vulkan/vulkan.cppm.o.ddi
/bin/sh: line 1: CMAKE_CXX_COMPILER_CLANG_SCAN_DEPS-NOTFOUND: command not found

Should the module code explicitly disable itself for clang-16? I don't want to cause widespread disruption due to the inclusion of the module code.

@dj2
Copy link

@dj2 dj2 commented on e3c37e6 Jun 20, 2024

Choose a reason for hiding this comment

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

Might be worth it, although it was easy enough to fix https://dawn-review.googlesource.com/c/dawn/+/194861, I also don't know what version of clang you'd need to target in order for it to work, I just happen to have clang-16 on this machine.

@charles-lunarg
Copy link
Contributor Author

@charles-lunarg charles-lunarg commented on e3c37e6 Jun 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@dj2 So I did a little digging and it turns out clang-16 works just fine as long as the clang-scan-deps tool is installed. On ubuntu its located in clang-tools-16 package (or whatever your compiler version is). Different distro's call it different things.

Tested it locally and having the package installed makes it compile just fine. Not sure if Vulkan-Headers should check for the clang tool but it would have been nice if the tools required to build modules were included in the clang-16 package (which cmake claims can handle module scanning!)

CMake discourse with the relevant info:
https://discourse.cmake.org/t/cmake-3-28-cmake-cxx-compiler-clang-scan-deps-notfound-not-found/9244

@dj2
Copy link

@dj2 dj2 commented on e3c37e6 Jun 20, 2024

Choose a reason for hiding this comment

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

Ah, good find on the clang-16 packages, that's probably exactly what's wrong with my machine.

Could you check that the CMAKE_CXX_COMPILER_CLANG_SCAN_DEPS is not CMAKE_CXX_COMPILER_CLANG_SCAN_DEPS-NOTFOUND ?

@charles-lunarg
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at the CMakeCache.txt, I see:

//`clang-scan-deps` dependency scanner
CMAKE_CXX_COMPILER_CLANG_SCAN_DEPS:FILEPATH=/usr/bin/clang-scan-deps-16

Please sign in to comment.