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

ggml-vulkan: remove unused find_program(glslc) #12416

Merged
merged 1 commit into from
Mar 17, 2025

Conversation

guusw
Copy link
Contributor

@guusw guusw commented Mar 17, 2025

It's unused and already found by FindVulkan.cmake in the parent CMakeLists.
Also having this find_program here directly prevents usage of hit variables like VULKAN_ROOT which are used by FindVulkan (find_package(Vulkan))

It's already found by FindVulkan.cmake in the parent CMakeLists
@github-actions github-actions bot added Vulkan Issues specific to the Vulkan backend ggml changes relating to the ggml tensor library for machine learning labels Mar 17, 2025
@0cc4m 0cc4m requested a review from bandoti March 17, 2025 10:19
@bandoti
Copy link
Collaborator

bandoti commented Mar 17, 2025

This change would break cross-compiling. The reason the glslc program is found in isolation here is because that executable will be binary compatible with the build machine, which might be a different architecture than what is found with Vulkan library.

I am working on adding a CI build for this and I will put a comment here so it doesn't cause confusion moving forward.

@bandoti
Copy link
Collaborator

bandoti commented Mar 17, 2025

That being said, we can investigate adding a separate find_package(Vulkan ...) explicitly for the glslc package and ignoring the cross-compile paths.

Is there a specific issue this is causing?

@bandoti
Copy link
Collaborator

bandoti commented Mar 17, 2025

After I started adding the CI change #12428 to put regression tests on cross-compiling, I am seeing this application actually doesn't need glslc at all (as this PR suggests), because it is passed as a command-line argument. We can likely merge this in just checking to make sure. 😅

Copy link
Collaborator

@bandoti bandoti left a comment

Choose a reason for hiding this comment

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

Looks good to me. I am following up with #12428 to add regression tests to CI for cross-compiling.

@bandoti bandoti merged commit 01e8f21 into ggml-org:master Mar 17, 2025
47 checks passed
arthw pushed a commit to arthw/llama.cpp that referenced this pull request Mar 19, 2025
It's already found by FindVulkan.cmake in the parent CMakeLists
@guusw guusw deleted the guus/remove-glslc-find-program branch March 19, 2025 05:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ggml changes relating to the ggml tensor library for machine learning Vulkan Issues specific to the Vulkan backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants