From 6710b67cde2f56074e5757426c65eaba70078939 Mon Sep 17 00:00:00 2001 From: Juan Ramos Date: Fri, 8 Sep 2023 10:57:24 -0600 Subject: [PATCH] Ensure all header files shipped have the vk_ prefix CI will fail now if we ship a .h / .hpp file that doesn't use the correct prefix. I also documented the precedent for this prefix and the existing integration testing. --- .github/workflows/ci.yml | 2 +- BUILD.gn | 2 +- docs/generated_code.md | 2 +- include/CMakeLists.txt | 2 +- ...l_dispatch_table.h => vk_dispatch_table.h} | 0 scripts/generate_source.py | 2 +- tests/CMakeLists.txt | 2 +- tests/README.md | 41 +++++++++++++ tests/add_subdirectory/CMakeLists.txt | 59 ++++++++++++++----- ...l_dispatch_table.c => vk_dispatch_table.c} | 8 +-- .../add_subdirectory/vk_enum_string_helper.c | 2 +- .../{client.c => vk_layer_settings.c} | 2 +- tests/find_package/CMakeLists.txt | 4 +- tests/vk_dispatch_table/CMakeLists.txt | 24 ++++++++ .../test_interface.cpp | 4 +- tests/vul_dispatch_table/CMakeLists.txt | 24 -------- 16 files changed, 124 insertions(+), 56 deletions(-) rename include/vulkan/utility/{vul_dispatch_table.h => vk_dispatch_table.h} (100%) create mode 100644 tests/README.md rename tests/add_subdirectory/{vul_dispatch_table.c => vk_dispatch_table.c} (70%) rename tests/add_subdirectory/{client.c => vk_layer_settings.c} (94%) create mode 100644 tests/vk_dispatch_table/CMakeLists.txt rename tests/{vul_dispatch_table => vk_dispatch_table}/test_interface.cpp (94%) delete mode 100644 tests/vul_dispatch_table/CMakeLists.txt diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f9e7d0c..924348a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -44,7 +44,7 @@ jobs: cmake --build tests/find_package/build --config ${{matrix.config}} --verbose - name: Test add_subdirectory support run: | - cmake -S tests/add_subdirectory -B tests/add_subdirectory/build -D CMAKE_BUILD_TYPE=${{matrix.config}} -D GITHUB_VULKAN_HEADER_SOURCE_DIR=${{ github.workspace }}/external/${{matrix.config}}/Vulkan-Headers/ + cmake -S tests/add_subdirectory -B tests/add_subdirectory/build -D CMAKE_BUILD_TYPE=${{matrix.config}} -D VULKAN_HEADER_SOURCE_DIR=${{ github.workspace }}/external/${{matrix.config}}/Vulkan-Headers/ cmake --build tests/add_subdirectory/build --config ${{matrix.config}} --verbose android: diff --git a/BUILD.gn b/BUILD.gn index d02eb11..e8e01ca 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -15,7 +15,7 @@ static_library("vulkan_layer_settings") { "include/vulkan/layer/vk_layer_settings.h", "include/vulkan/layer/vk_layer_settings.hpp", "include/vulkan/layer/vk_layer_settings_ext.h", - "include/vulkan/utility/vul_dispatch_table.h", + "include/vulkan/utility/vk_dispatch_table.h", "include/vulkan/vk_enum_string_helper.h", "src/layer/layer_settings_manager.cpp", "src/layer/layer_settings_manager.hpp", diff --git a/docs/generated_code.md b/docs/generated_code.md index eda7434..6599e5b 100644 --- a/docs/generated_code.md +++ b/docs/generated_code.md @@ -27,7 +27,7 @@ If only dealing with a single file, run `scripts/generate_source.py` with `--ta ```bash # Example - only generates chassis.h -scripts/generate_source.py external/Vulkan-Headers/registry/ --target vul_dispatch_table.h +scripts/generate_source.py external/Vulkan-Headers/registry/ --target vk_dispatch_table.h ``` When making change to the `scripts/` folder, make sure to run `generate_source.py` diff --git a/include/CMakeLists.txt b/include/CMakeLists.txt index 6e1f26e..1138185 100644 --- a/include/CMakeLists.txt +++ b/include/CMakeLists.txt @@ -21,7 +21,7 @@ lunarg_target_compiler_configurations(VulkanUtilityHeaders VUL_WERROR) # https://cmake.org/cmake/help/latest/release/3.19.html#other if (CMAKE_VERSION VERSION_GREATER_EQUAL "3.19") target_sources(VulkanUtilityHeaders PRIVATE - vulkan/utility/vul_dispatch_table.h + vulkan/utility/vk_dispatch_table.h vulkan/vk_enum_string_helper.h ) endif() diff --git a/include/vulkan/utility/vul_dispatch_table.h b/include/vulkan/utility/vk_dispatch_table.h similarity index 100% rename from include/vulkan/utility/vul_dispatch_table.h rename to include/vulkan/utility/vk_dispatch_table.h diff --git a/scripts/generate_source.py b/scripts/generate_source.py index 7ad65fa..873af13 100644 --- a/scripts/generate_source.py +++ b/scripts/generate_source.py @@ -27,7 +27,7 @@ def RunGenerators(api: str, registry: str, targetFilter: str) -> None: # Build up a list of all generators and custom options generators = { - 'vul_dispatch_table.h' : { + 'vk_dispatch_table.h' : { 'generator' : DispatchTableOutputGenerator, 'directory' : 'include/vulkan/utility', }, diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index f0aaa43..1c8dd9d 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -5,4 +5,4 @@ # SPDX-License-Identifier: Apache-2.0 add_subdirectory(layer) add_subdirectory(generated) -add_subdirectory(vul_dispatch_table) +add_subdirectory(vk_dispatch_table) diff --git a/tests/README.md b/tests/README.md new file mode 100644 index 0000000..263d917 --- /dev/null +++ b/tests/README.md @@ -0,0 +1,41 @@ + + +# Library integration testing + +In order to avoid disruption of downstream users. It's important to test how this +repository is consumed. + +1. Self contained headers + +It's easy to write header files that aren't self contained. By compiling +a single source file that includes a single header we ensure a smooth experience for +downstream users. + +2. Ensure C compatibility of C header files + +It's VERY easy to write invalid C code. Especially for experience C++ programmers. + +## tests/find_package + +Test find_package support. The intent is to ensure we properly install files. + +Used by system/language package managers and the Vulkan SDK packaging. + +## tests/add_subdirectory + +1. Test add_subdirectory support + +While we don't have to support add_subdirectory it is a common feature request for CMake projects. + +2. Ensure file name consistency of header files we install + +All header files we ship will have the `vk_` prefix + +This convention was originally established in VulkanHeaders for files created by LunarG. +- EX: `vk_icd.h`, `vk_layer.h`, `vk_platform.h` diff --git a/tests/add_subdirectory/CMakeLists.txt b/tests/add_subdirectory/CMakeLists.txt index 5cfac7c..65e94e6 100644 --- a/tests/add_subdirectory/CMakeLists.txt +++ b/tests/add_subdirectory/CMakeLists.txt @@ -7,28 +7,55 @@ cmake_minimum_required(VERSION 3.17) project(TEST_ADD_SUBDIRECTORY LANGUAGES C) +if (NOT DEFINED VULKAN_HEADER_SOURCE_DIR) + message(FATAL_ERROR "VULKAN_HEADER_SOURCE_DIR not defined!") +endif() + +add_subdirectory(${VULKAN_HEADER_SOURCE_DIR} ${PROJECT_BINARY_DIR}/headers) + +add_subdirectory(${PROJECT_SOURCE_DIR}/../../ ${PROJECT_BINARY_DIR}/vul) + +get_filename_component(VUL_INCLUDE_DIR "${PROJECT_SOURCE_DIR}/../../include/" ABSOLUTE) + +# Get all the header files we SHIP +file(GLOB_RECURSE public_headers + CONFIGURE_DEPENDS + "${VUL_INCLUDE_DIR}/*.h" "${VUL_INCLUDE_DIR}/*.hpp" +) + +# Ensure each file has the same `vk_` prefix naming convention. +foreach(header IN LISTS public_headers) + get_filename_component(header ${header} "NAME") + message(DEBUG "Checking ${header}") + if (${header} MATCHES "^vk_") + message(DEBUG "Correct naming convention!") + else() + # EX: "Invalid file name! vk_dispatch_table.h" + message(FATAL_ERROR "Invalid file name! ${header}") + endif() +endforeach() + +# The intent is ensuring we don't accidentally change the names of these +# targets. Since it would break downstream users. +if (NOT TARGET Vulkan::LayerSettings) + message(FATAL_ERROR "Vulkan::LayerSettings target not defined!") +endif() +if (NOT TARGET Vulkan::UtilityHeaders) + message(FATAL_ERROR "Vulkan::UtilityHeaders target not defined!") +endif() + add_library(add_subdirectory_example STATIC) # Test c99 support target_compile_features(add_subdirectory_example PRIVATE c_std_99) target_sources(add_subdirectory_example PRIVATE - client.c + vk_dispatch_table.c vk_enum_string_helper.c - vul_dispatch_table.c + vk_layer_settings.c ) -# NOTE: Because VulkanHeaders is a PUBLIC dependency it needs to be found prior to VulkanUtilityLibraries -add_subdirectory(${GITHUB_VULKAN_HEADER_SOURCE_DIR} ${PROJECT_BINARY_DIR}/headers) - -add_subdirectory(${PROJECT_SOURCE_DIR}/../../ ${PROJECT_BINARY_DIR}/vul) - -if (NOT TARGET Vulkan::LayerSettings) - message(FATAL_ERROR "Vulkan::LayerSettings target not defined!") -endif() - -if (NOT TARGET Vulkan::UtilityHeaders) - message(FATAL_ERROR "Vulkan::UtilityHeaders target not defined!") -endif() - -target_link_libraries(add_subdirectory_example PRIVATE Vulkan::LayerSettings Vulkan::UtilityHeaders) +target_link_libraries(add_subdirectory_example PRIVATE + Vulkan::LayerSettings + Vulkan::UtilityHeaders +) diff --git a/tests/add_subdirectory/vul_dispatch_table.c b/tests/add_subdirectory/vk_dispatch_table.c similarity index 70% rename from tests/add_subdirectory/vul_dispatch_table.c rename to tests/add_subdirectory/vk_dispatch_table.c index 2185005..4145340 100644 --- a/tests/add_subdirectory/vul_dispatch_table.c +++ b/tests/add_subdirectory/vk_dispatch_table.c @@ -3,21 +3,21 @@ // Copyright 2023 LunarG, Inc. // // SPDX-License-Identifier: Apache-2.0 -#include +#include -PFN_vkVoidFunction local_vkGetInstanceProcAddr(VkInstance instance, const char *pName) { +PFN_vkVoidFunction VKAPI_PTR local_vkGetInstanceProcAddr(VkInstance instance, const char *pName) { (void)instance; (void)pName; return NULL; } -PFN_vkVoidFunction local_vkGetDeviceProcAddr(VkDevice device, const char *pName) { +PFN_vkVoidFunction VKAPI_PTR local_vkGetDeviceProcAddr(VkDevice device, const char *pName) { (void)device; (void)pName; return NULL; } -void foobar() { +void vk_dispatch_table() { VulDeviceDispatchTable device_dispatch_table; VulInstanceDispatchTable instance_dispatch_table; diff --git a/tests/add_subdirectory/vk_enum_string_helper.c b/tests/add_subdirectory/vk_enum_string_helper.c index c122273..59bde36 100644 --- a/tests/add_subdirectory/vk_enum_string_helper.c +++ b/tests/add_subdirectory/vk_enum_string_helper.c @@ -6,7 +6,7 @@ #include // Ensure vk_enum_string_helper.h can be compiled with a C compiler -const char* foobar() { return string_VkResult(VK_SUCCESS); } +const char* string_VkResult_compiles() { return string_VkResult(VK_SUCCESS); } // Ensure string_VkPipelineStageFlagBits2 is callable by C users const char* vk_format_feature_2_sampled_image_bit() { diff --git a/tests/add_subdirectory/client.c b/tests/add_subdirectory/vk_layer_settings.c similarity index 94% rename from tests/add_subdirectory/client.c rename to tests/add_subdirectory/vk_layer_settings.c index eaddeb4..b15b041 100644 --- a/tests/add_subdirectory/client.c +++ b/tests/add_subdirectory/vk_layer_settings.c @@ -5,7 +5,7 @@ // SPDX-License-Identifier: Apache-2.0 #include -VkBool32 foobar() { +VkBool32 vk_layer_settings() { VlLayerSettingSet layerSettingSet = VK_NULL_HANDLE; vlCreateLayerSettingSet("VK_LAYER_LUNARG_test", NULL, NULL, NULL, &layerSettingSet); diff --git a/tests/find_package/CMakeLists.txt b/tests/find_package/CMakeLists.txt index 8ec7938..3ccc6c0 100644 --- a/tests/find_package/CMakeLists.txt +++ b/tests/find_package/CMakeLists.txt @@ -13,9 +13,9 @@ add_library(find_package_example STATIC) target_compile_features(find_package_example PRIVATE c_std_99) target_sources(find_package_example PRIVATE - ${CMAKE_CURRENT_LIST_DIR}/../add_subdirectory/client.c + ${CMAKE_CURRENT_LIST_DIR}/../add_subdirectory/vk_layer_settings.c ${CMAKE_CURRENT_LIST_DIR}/../add_subdirectory/vk_enum_string_helper.c - ${CMAKE_CURRENT_LIST_DIR}/../add_subdirectory/vul_dispatch_table.c + ${CMAKE_CURRENT_LIST_DIR}/../add_subdirectory/vk_dispatch_table.c ) # NOTE: Because VulkanHeaders is a PUBLIC dependency it needs to be found prior to VulkanUtilityLibraries diff --git a/tests/vk_dispatch_table/CMakeLists.txt b/tests/vk_dispatch_table/CMakeLists.txt new file mode 100644 index 0000000..2ef713b --- /dev/null +++ b/tests/vk_dispatch_table/CMakeLists.txt @@ -0,0 +1,24 @@ +# Copyright 2023 The Khronos Group Inc. +# Copyright 2023 Valve Corporation +# Copyright 2023 LunarG, Inc. +# +# SPDX-License-Identifier: Apache-2.0 +set(CMAKE_FOLDER "${CMAKE_FOLDER}/VulkanUtilityHeaders/tests") + +find_package(GTest REQUIRED CONFIG) + +include(GoogleTest) + +add_executable(test_vk_dispatch_table test_interface.cpp) + +lunarg_target_compiler_configurations(test_vk_dispatch_table VUL_WERROR) + +target_link_libraries(test_vk_dispatch_table PRIVATE + GTest::gtest + GTest::gtest_main + Vulkan::UtilityHeaders +) + +target_include_directories(test_vk_dispatch_table PRIVATE $) + +gtest_discover_tests(test_vk_dispatch_table) diff --git a/tests/vul_dispatch_table/test_interface.cpp b/tests/vk_dispatch_table/test_interface.cpp similarity index 94% rename from tests/vul_dispatch_table/test_interface.cpp rename to tests/vk_dispatch_table/test_interface.cpp index 53ce4f8..8bafa7e 100644 --- a/tests/vul_dispatch_table/test_interface.cpp +++ b/tests/vk_dispatch_table/test_interface.cpp @@ -7,7 +7,7 @@ #include -#include +#include // Only exists so that local_vkGetDeviceProcAddr can return a 'real' function pointer inline void empty_func() {} @@ -27,7 +27,7 @@ inline PFN_vkVoidFunction local_vkGetDeviceProcAddr(VkDevice device, const char return reinterpret_cast(&empty_func); } -TEST(test_vul_dispatch_table, cpp_interface) { +TEST(test_vk_dispatch_table, cpp_interface) { VulDeviceDispatchTable device_dispatch_table{}; VulInstanceDispatchTable instance_dispatch_table{}; diff --git a/tests/vul_dispatch_table/CMakeLists.txt b/tests/vul_dispatch_table/CMakeLists.txt deleted file mode 100644 index bbc1456..0000000 --- a/tests/vul_dispatch_table/CMakeLists.txt +++ /dev/null @@ -1,24 +0,0 @@ -# Copyright 2023 The Khronos Group Inc. -# Copyright 2023 Valve Corporation -# Copyright 2023 LunarG, Inc. -# -# SPDX-License-Identifier: Apache-2.0 -set(CMAKE_FOLDER "${CMAKE_FOLDER}/VulkanUtilityHeaders/tests") - -find_package(GTest REQUIRED CONFIG) - -include(GoogleTest) - -add_executable(test_vul_dispatch_table test_interface.cpp) - -lunarg_target_compiler_configurations(test_vul_dispatch_table VUL_WERROR) - -target_link_libraries(test_vul_dispatch_table PRIVATE - GTest::gtest - GTest::gtest_main - Vulkan::UtilityHeaders -) - -target_include_directories(test_vul_dispatch_table PRIVATE $) - -gtest_discover_tests(test_vul_dispatch_table)