-
Notifications
You must be signed in to change notification settings - Fork 12.2k
cmake: clean up external project logic for vulkan-shaders-gen #14179
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
Conversation
This is unrelated to this PR, but since we went with the external project logic, cmake doesn't recognize file changes in shaders properly anymore. Sometimes shaders aren't rebuilt, which can be annoying to figure out. You can resolve using "--clean-first", but previously it was working fine. Is this something we can fix? |
Oh no! That's not good. At first glance I notice that _ggml_vk_shader_deps globs the shader files and then the set command is used afterward, appending the vulkan-shaders-gen targets. My first guess is to separate those dependencies because it might be messing with the list. Another option is that we could regenerate the list of globbed files at build time instead of generate-time using the glob flag:
|
I am happy to put the fix in with this one as it is related—and small enough. Thanks for letting me know. 😊 |
@0cc4m I added you as a reviewer primarily to check that the shader re-compile works. I spot tested on my system and it seems to rebuild properly when the shaders get modified. Thanks! |
Thank you, I couldn't reproduce the issue currently so I can't say for certain if this fixes it, but I'll watch it. |
I tested this by running This compiles without error, unlike the current master branch. For comparison, running |
@AsbjornOlling Thanks for the contribution/help fixing the issue. 😊 |
I'm not sure if it's from this change, but I'm seeing weird behavior on Windows where touching vulkan-shaders-gen.cpp does not cause the vulkan-shaders-gen exe to be rebuilt. |
Hmm. It might be because I removed the "build" step-target (not in this PR); might have to add that back. That way the change to vulkan-shaders-gen.cpp will cause its target to rebuild, and that will cascade to regenerate the shaders. This should be fine so long as the "install" step-target does not get included. I'll give this a try locally and submit a PR to fix if I can reproduce it. |
This PR cleans up the the ExternalProject_Add logic for vulkan-shaders-gen when multi-configuration generators are used.
These changes expand on #14047, which is to fix an issue causing the llama-cpp-rs rust bindings to no longer compile for vulkan, see: utilityai/llama-cpp-rs#747