Skip to content

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