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.

Sign up for GitHub

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

Create a single CMake config package for all SPIRV-Cross targets #2253

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Create a single CMake config package for all SPIRV-Cross targets #2253

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 34 additions & 2 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ project(SPIRV-Cross LANGUAGES CXX C)
enable_testing()

include(GNUInstallDirs)
include(CMakePackageConfigHelpers)

option(SPIRV_CROSS_EXCEPTIONS_TO_ASSERTIONS "Instead of throwing exceptions assert" OFF)
option(SPIRV_CROSS_SHARED "Build the C API as a single shared library." OFF)
Expand Down Expand Up @@ -187,13 +188,12 @@ macro(spirv_cross_add_library name config_name library_type)

if (NOT SPIRV_CROSS_SKIP_INSTALL)
install(TARGETS ${name}
EXPORT ${config_name}Config
EXPORT SPIRV-CrossTargets
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this break existing consumers of SPIRV-Cross?

Copy link
Author

Choose a reason for hiding this comment

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

I think you are right, I am not familiar how existing users are consuming this export set today.
I would be better to just generate a new one instead of replacing the existing one.

Copy link

Choose a reason for hiding this comment

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

Or you can let the new SPIRV-CrossConfig.cmake make use components:

@PACKAGE_INIT@
find_package(spirv_cross_core QUIET)
set(SPIRV-Cross_core_FOUND ${spirv_cross_core_FOUND})

find_package(spirv_cross_glsl QUIET)
set(SPIRV-Cross_core_FOUND ${spirv_cross_glsl_FOUND})

find_package(spirv_cross_hlsl QUIET)
set(SPIRV-Cross_core_FOUND ${spirv_cross_hlsl_FOUND})

find_package(spirv_cross_msl QUIET)
set(SPIRV-Cross_core_FOUND ${spirv_cross_msl_FOUND})

find_package(spirv_cross_cpp QUIET)
set(SPIRV-Cross_core_FOUND ${spirv_cross_cpp_FOUND})

find_package(spirv_cross_reflect QUIET)
set(SPIRV-Cross_core_FOUND ${spirv_cross_reflect_FOUND})

find_package(spirv_cross_c QUIET)
set(SPIRV-Cross_core_FOUND ${spirv_cross_c_FOUND})

find_package(spirv_cross_c_shared QUIET)
set(SPIRV-Cross_core_FOUND ${spirv_cross_c_found_FOUND})

check_required_components(SPIRV-Cross)

Then you can do things like:

find_package(SPIRV-Cross REQUIRED COMPONENTS spirv_cross_c_shared)

while still remaining compatible with

find_package(spirv_cross_c_shared REQUIRED)

(I think CMake does not allow exporting targets to more then one export set)

RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}
PUBLIC_HEADER DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/spirv_cross)
install(FILES ${hdrs} DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/spirv_cross)
install(EXPORT ${config_name}Config DESTINATION ${CMAKE_INSTALL_DATAROOTDIR}/${config_name}/cmake)
export(TARGETS ${name} FILE ${config_name}Config.cmake)
endif()
endmacro()
Expand Down Expand Up @@ -621,3 +621,35 @@ if (SPIRV_CROSS_CLI)
endif()
endif()
endif()

if (NOT SPIRV_CROSS_SKIP_INSTALL)
file(WRITE "${CMAKE_CURRENT_BINARY_DIR}/SPIRV-CrossConfig.cmake.in" [=[
@PACKAGE_INIT@
include("${CMAKE_CURRENT_LIST_DIR}/SPIRV-CrossTargets.cmake")
]=])

configure_package_config_file(
Copy link
Contributor

Choose a reason for hiding this comment

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

All of this is new to me. Is there any place I can read up on CMake best practices regarding this code?

Copy link
Author

Choose a reason for hiding this comment

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

I do not have a specific tutorial but I can share a few links and tips.

Let's start with some CMake documentation link:

  • find_package: in particular the Search Modes section describes what the "Config Mode" is.
  • CMakePackageConfigHelpers: this is a CMake utility module to assist you in writing CMake Config package. You do not have to do it and write one manually, but I have found that it is usually enough, at least to start with a first CMake Config package.

In the existing code install(TARGETS <target>... [...]) with its EXPORT argument and install(EXPORT <export-name> [...]) to generate this CMake Config package instead.

I have found that it is better to use the EXPORT argument to define a CMake target export set like it was designed for and includes it in the generated CMake Config package file like it is done in this PR.
For example, it gives you flexibility to specify indirect dependencies using CMakeFindDependencyMacro or expose package variable for user to consume.

Also, write_basic_package_version_file helps you to create a CMake Config Version package file which helps to resolve version compatibility when using find_package with its VERSION argument.

I hope this helps !

Last but not least, here are two more maybe useful links:

"${CMAKE_CURRENT_BINARY_DIR}/SPIRV-CrossConfig.cmake.in"
"${CMAKE_CURRENT_BINARY_DIR}/SPIRV-CrossConfig.cmake"
INSTALL_DESTINATION ${CMAKE_INSTALL_DATADIR}/SPIRV-Cross
)

write_basic_package_version_file("${CMAKE_CURRENT_BINARY_DIR}/SPIRV-CrossConfigVersion.cmake"
VERSION ${SPIRV_CROSS_VERSION}
Copy link
Author

@theblackunknown theblackunknown Jan 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

SPIRV-Cross project uses a specifc ABI version but this does not match any published tags (e.g. 0.58.0 vs sdk-1.3.250).
Any suggestion of what we should use to properly represent a SPIRV-Cross user consumable version ?

Copy link
Contributor

@HansKristian-Work HansKristian-Work Jan 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There is no SPIRV-Cross "version" per-se and I think the .so version is bumped when the C API is updated. Whatever shipped in an SDK shipped in an SDK, and I think someone tags those commits.

Maybe it would be possible for SDK builds to explicitly override a version string or something. Using the git commit as a version is probably the only reasonable way if there is no tag for that particular commit.

Copy link
Author

Choose a reason for hiding this comment

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

Are you suggesting that we allow user to choose what would be the SPIRV-Cross version when configuring the project ? (e.g. cmake .. -DSPIRV_CROSS_VERSION:STRING=1.3.250)

Using the git commit as a version is probably the only reasonable way if there is no tag for that particular commit.

Sounds reasonable, just to double check we would then have version which are not human readable as they would simply be git (short) SHA right ?
Ideally I would prefer that if we ever choose a version scheme a human readable one is picked, or better a semantic versioning (especially since you already chose to specify an ABI version this should be straightforward)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be fine with an option override for version strings. What is the intention for that version string? Package maintainers in distros and LunarG when packaging it in an SDK release?

Copy link
Author

Choose a reason for hiding this comment

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

From my perspective I am interested for it as a package maintainer for vcpkg yes.

Copy link

Choose a reason for hiding this comment

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

Can we remove SPIRV-CrossConfigVersion.cmake from this pr and discuss this in an issue?
That'd unblock this pr, and solve the original intent of this pr.

Filtering on versions can be added in a later pr.

COMPATIBILITY SameMajorVersion
)

install(
EXPORT SPIRV-CrossTargets
NAMESPACE "SPIRV-Cross::"
DESTINATION "${CMAKE_INSTALL_DATADIR}/SPIRV-Cross"
)

install(
FILES
"${CMAKE_CURRENT_BINARY_DIR}/SPIRV-CrossConfig.cmake"
"${CMAKE_CURRENT_BINARY_DIR}/SPIRV-CrossConfigVersion.cmake"
DESTINATION
"${CMAKE_INSTALL_DATADIR}/SPIRV-Cross"
)
endif()