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

ci: Add find_package testing #3870

Merged
merged 1 commit into from
Feb 20, 2025
Merged

Conversation

jpr42
Copy link
Contributor

@jpr42 jpr42 commented Feb 7, 2025

Fix find_package breaking on Windows when BUILD_SHARED_LIBS=ON due to SPIRV stub library.

Changed exit code for spirv-remap to 0 when requesting help, to allow for better integration testing.

closes #3462

# Ensure these executables run without issue
add_custom_target(test-executables ALL
COMMAND glslang::glslang-standalone --version
COMMAND glslang::spirv-remap --help
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is why I changed the error code to return 0 instead. Otherwise the build will fail.

I believe this is more correct regardless.

Copy link
Contributor

Choose a reason for hiding this comment

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

While I don't entirely like this, spirv-remap is in the process of getting moved out of the glslang repo so this will all eventually change anyway.

Comment on lines +58 to +60
if (NOT TARGET glslang::SPVRemapper)
message(FATAL_ERROR "glslang::SPVRemapper does not exist!")
endif()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's likely a good idea to write better integration code for this library. I'm not that well versed enough to do it though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just took the code in the README.md for the C Functional Interface

@@ -128,7 +128,9 @@ if(GLSLANG_ENABLE_INSTALL)
install(TARGETS SPVRemapper EXPORT glslang-targets)
endif()

install(TARGETS SPIRV EXPORT glslang-targets)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

find_package support was broken on Windows when BUILD_SHARED_LIBS was ON.

CMake Error at D:/a/glslang/glslang/build/install/lib/cmake/glslang/glslang-targets.cmake:117 (message):
  The imported target "glslang::SPIRV" references the file

     "D:/a/glslang/glslang/build/install/lib/SPIRV.lib"

  but this file does not exist.  Possible reasons include:

  * The file was deleted, renamed, or moved to another location.

  * An install or uninstall procedure did not complete successfully.

  * The installation package was faulty and contained

     "D:/a/glslang/glslang/build/install/lib/cmake/glslang/glslang-targets.cmake"

  but not all the files it references.

Call Stack (most recent call first):
  D:/a/glslang/glslang/build/install/lib/cmake/glslang/glslang-config.cmake:31 (include)
  CMakeLists.txt:42 (find_package)

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 another solution could be to just export 1 function in stub.cpp so that the .lib is generated when BUILD_SHARED_LIBS is ON. This would also keep backcompat. But makes SPIRV inconsistent with the other stub libraries.

Let me know what approach you prefer.

Fix find_package breaking on Windows when BUILD_SHARED_LIBS=ON
due to SPIRV stub library.

Changed exit code for spirv-remap to 0 when requesting help,
to allow for better integration testing.

closes KhronosGroup#3462
@arcady-lunarg arcady-lunarg added the kokoro:run Trigger Google bot runs label Feb 18, 2025
@kokoro-team kokoro-team removed the kokoro:run Trigger Google bot runs label Feb 18, 2025
Copy link
Contributor

@jeremy-lunarg jeremy-lunarg left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@jeremy-lunarg jeremy-lunarg merged commit 104bd85 into KhronosGroup:main Feb 20, 2025
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add github action for testing cmake installation
4 participants