-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
fix: ggml: fix vulkan-shaders-gen build #10448
base: master
Are you sure you want to change the base?
Conversation
@jeffbolznv , @0cc4m |
@sparkleholic @jeffbolznv I understand, I'm sorry that a few PRs are getting delayed because of me. I'm currently ill, but getting better and I should be able to catch up with all of them later this week. Maybe you can resolve the merge conflict already? |
No rush, get well! I'm not an expert at cmake or cross compilation, so I don't think I should review the change. |
e9b3777
to
bf43351
Compare
@0cc4m |
I can confirm this still works as usual on my system, but I'm also not a CMake expert. Maybe @bandoti can take a look? |
I am happy to take a look. The one thing that comes to mind here after initial look through is being able to customize the compiler in this case. In particular, It would be worthwhile to add a default toolchain file for the build and allow one to customize by passing This would allow one to use a desire toolchain—maybe clang, etc. And a lot of those switches passed to ExternalProject_Add() can of course be moved to the toolchain file instead. |
@bandoti I have updated the code to handle the Please review the updated code once again. |
@bandoti Can you take another look? |
@sparkleholic Sorry for the delay! I had these review comments in but forgot to submit them. 🤦♂️ |
@sparkleholic Just following up on this change. If you need further assistance please let me know I am happy to help. |
@bandoti < Sorry for the late reply. |
The vulkan-shaders-gen target was not being built correctly in case of cross-compilation. Other outputs need to be built for the cross compile target, but vulkan-shaders-gen needs to be built for the host.
- Add GGML_SHADERS_GEN_TOOLCHAIN CMake option. - Auto-detect host toolchain if not set.
5cb143f
to
49c225f
Compare
Rebased branch to latest master. |
Use configure_file to generate host_toolchain.cmake from template
49c225f
to
7c1a685
Compare
@sparkleholic No worries on the delay! Your contribution is greatly appreciated :) It appears as though the test failure is unrelated to this. I'm wondering if merging the latest and rerunning the tests will clear the status. I have only one minor question in the review. |
@@ -3,7 +3,11 @@ find_package(Vulkan COMPONENTS glslc REQUIRED) | |||
|
|||
set(TARGET vulkan-shaders-gen) | |||
add_executable(${TARGET} vulkan-shaders-gen.cpp) | |||
install(TARGETS ${TARGET} RUNTIME) | |||
if (${CMAKE_RUNTIME_OUTPUT_DIRECTORY}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the intention here to be the variable value or the name?
if (CMAKE_RUNTIME_OUTPUT_DIRECTORY)
The vulkan-shaders-gen target was not being built correctly in case of cross-compilation.
Other outputs need to be built for the cross compile target, but vulkan-shaders-gen needs to be built for the host.