Skip to content

Conversation

@0cc4m
Copy link
Collaborator

@0cc4m 0cc4m commented Nov 12, 2025

This removes the shell invocation from the vulkan-shaders-gen tool, and instead splits the string and feeds separate arguments into execvp. This way, no shell is needed. I also removed the file check on glslc.

@MaggotHATE Please check if this works for you.

@0cc4m 0cc4m requested review from Acly and jeffbolznv November 12, 2025 18:58
@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 Nov 12, 2025
@MaggotHATE
Copy link
Contributor

MaggotHATE commented Nov 12, 2025

It compiles completely with glslc passed as the argument on that portable setup, generations works, tested with Magistral 2509. Thank you!

Edit: whoops, looks like I've tested before the latest changes.

@0cc4m
Copy link
Collaborator Author

0cc4m commented Nov 12, 2025

@MaggotHATE Sorry about the changes, can you test the latest version?

@MaggotHATE
Copy link
Contributor

MaggotHATE commented Nov 13, 2025

Tested again, works with the same setup. I should, however, point out two things:

  1. Since I'm still using the old compilation of shaders (i.e. to a single .hpp file), I cannot use some changes in this PR, but they are minuscule. Most changes work without errors.
  2. Currently, shaders compilation is fast, definitely faster than the initial solution in this PR. While I liked how elegant it looked, increased compilation time was probably not intended.

Tested again on latest commit, all good, thank you!

@0cc4m 0cc4m merged commit a19bd6f into master Nov 13, 2025
71 of 72 checks passed
@0cc4m 0cc4m deleted the 0cc4m/vulkan-shader-gen-no-shell branch November 13, 2025 13:51
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.

5 participants