Fix Vulkan glslc invocation command lines #13289
Open
+36
−6
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
They were shoehorned through a Windows-style string representation, where the entire command line is just a space-separated string of arguments. If any arguments contain spaces, you have to escape them. This was implemented in ggml-org#8573, however, the issue remains on Unix. Usually, this would NOT be an issue on Unix, because it doesn't have the braindead Windows-style API that only takes a flat string as a command line. In Unix, you can pass the arguments as a legit array. However, Llama.cpp didn't make use of that; instead running `sh -c <the flattened argument string>`. Thereby inflicting the same problems onto itself as Windows has. Instead of bandaid-fixing this by also slapping quotes around the path arguments (which would also break apart as soon as the path contains quote characters), I decided to fix it properly. So, the code now doesn't wrap the command line in `sh -c`, but passes the arguments directly, circumventing the need to do any brittle escaping. This also necessitated replacing the `coopmat ? "" : "-O"` parameter with a proper conditional parameter. Because the empty string parameter (rightfully) confused glslc.
github-actions
bot
added
Vulkan
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
They were shoehorned through a Windows-style string representation, where the entire command line is just a space-separated string of arguments. If any arguments contain spaces, you have to escape them. This was implemented in #8573, however, the issue remains on Unix.
Usually, this would NOT be an issue on Unix, because it doesn't have the braindead Windows-style API that only takes a flat string as a command line. In Unix, you can pass the arguments as a legit array. However, Llama.cpp didn't make use of that; instead running
sh -c <the flattened argument string>. Thereby inflicting the same problems onto itself as Windows has.Instead of bandaid-fixing this by also slapping quotes around the path arguments (which would also break apart as soon as the path contains quote characters), I decided to fix it properly.
So, the code now doesn't wrap the command line in
sh -c, but passes the arguments directly, circumventing the need to do any brittle escaping.This also necessitated replacing the
coopmat ? "" : "-O"parameter with a proper conditional parameter. Because the empty string parameter (rightfully) confused glslc.Fixes #13288