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

theblackunknown · 2024-01-03T21:24:12Z

This PR proposes to have a single CMake Config package to centralize all CMake targets instead of multiple ones.

The idea would be to consume SPIRV-Cross in an another project like this.

find_package(SPIRV-Cross CONFIG REQUIRED)

# ...

target_link_libraries(mytarget
    PRIVATE
        SPIRV-Cross::spirv-cross-core
        SPIRV-Cross::spirv-cross-msl
        SPIRV-Cross::spirv-cross-glsl
        # ...
)

CLAassistant · 2024-01-03T21:24:17Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

theblackunknown · 2024-01-03T21:26:36Z

CMakeLists.txt

+	)
+
+	write_basic_package_version_file("${CMAKE_CURRENT_BINARY_DIR}/spirv-cross-config-version.cmake"
+		VERSION ${SPIRV_CROSS_VERSION}


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 ?

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.

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)

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?

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

HansKristian-Work · 2024-01-05T11:24:29Z

CMakeLists.txt

@@ -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


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

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.

HansKristian-Work · 2024-01-05T11:26:30Z

CMakeLists.txt

+	    include("${CMAKE_CURRENT_LIST_DIR}/SPIRV-CrossTargets.cmake")
+	]=])
+
+	configure_package_config_file(


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

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:

I submitted similar changes on KhronosGroup/glslang in Add unified glslang CMake config collecting glslang-targets targets glslang#2989

I often do personal contribution on microsoft/vcpkg which is a package manager for C++. Having a CMake Config package does really helps to have a consistent experience and to consume C++ package. Based on changes from this PR I would like to add a vcpkg package which would help to consume SPIRV-Cross for any other projects using it.

theblackunknown

Thanks for the review @HansKristian-Work !
Let's continue iterating on this as finish the CLA administrative stuff

theblackunknown · 2024-01-05T13:12:29Z

CMakeLists.txt

@@ -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


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.

theblackunknown · 2024-01-05T14:27:49Z

CMakeLists.txt

+	)
+
+	write_basic_package_version_file("${CMAKE_CURRENT_BINARY_DIR}/spirv-cross-config-version.cmake"
+		VERSION ${SPIRV_CROSS_VERSION}


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)

theblackunknown commented Jan 3, 2024

View reviewed changes

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

088f5f5

theblackunknown force-pushed the cmake-config branch from 0d9f87a to 088f5f5 Compare January 3, 2024 21:30

HansKristian-Work reviewed Jan 5, 2024

View reviewed changes

theblackunknown commented Jan 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

theblackunknown commented Jan 3, 2024 •

edited

Loading

CLAassistant commented Jan 3, 2024

theblackunknown Jan 3, 2024 •

edited

Loading

HansKristian-Work Jan 5, 2024 •

edited

Loading

theblackunknown Jan 5, 2024

HansKristian-Work Jan 8, 2024

theblackunknown Jan 8, 2024

HansKristian-Work Jan 5, 2024

theblackunknown Jan 5, 2024

HansKristian-Work Jan 5, 2024

theblackunknown Jan 8, 2024

theblackunknown left a comment

theblackunknown Jan 5, 2024

theblackunknown Jan 5, 2024

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

Are you sure you want to change the base?

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

Conversation

theblackunknown commented Jan 3, 2024 • edited Loading

CLAassistant commented Jan 3, 2024

theblackunknown Jan 3, 2024 • edited Loading

Choose a reason for hiding this comment

HansKristian-Work Jan 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theblackunknown left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theblackunknown commented Jan 3, 2024 •

edited

Loading

theblackunknown Jan 3, 2024 •

edited

Loading

HansKristian-Work Jan 5, 2024 •

edited

Loading