Skip to content

Resolve Metal shader sources relative to the executable#575

Open
rinaldofesta wants to merge 1 commit into
antirez:mainfrom
rinaldofesta:fix/metal-shader-exe-relative
Open

Resolve Metal shader sources relative to the executable#575
rinaldofesta wants to merge 1 commit into
antirez:mainfrom
rinaldofesta:fix/metal-shader-exe-relative

Conversation

@rinaldofesta

Copy link
Copy Markdown
Contributor

Launching any ds4 binary from outside the repo silently falls back to the CPU backend: the Metal kernel sources only resolve as metal/*.metal against the current directory, so from another working directory the library build fails and the engine starts with backend=cpu and CPU-speed prefill, with no error pointing at the cause. The README's --chdir note covers it, but that requires knowing the trap exists.

The fix adds one more candidate path per kernel source: the directory containing the executable, obtained via _NSGetExecutablePath with symlinks resolved (so a symlink from ~/bin or /usr/local/bin also works). Lookup order is unchanged otherwise: per-file env override first, then the two cwd-relative forms, then the executable-relative path. In-repo launches behave exactly as before; the file is Darwin-only so CUDA/ROCm paths are untouched.

Validation: make on top of current main, zero warnings (-Wall -Wextra). On an M5 Max with the patch applied, launching ds4-agent from an unrelated directory now starts with backend=metal in the context-buffers banner (previously backend=cpu), and the same holds when launched through a PATH symlink. Not run here: CUDA/ROCm builds (no change to those paths).

Implemented with the help of a coding agent (Claude Code) and reviewed locally by the author.

Launching ds4 binaries from outside the repo silently falls back to the
CPU backend because metal/*.metal only resolves against the current
directory. Try the executable's own directory (symlinks followed) as a
last candidate, so the binaries work from any working directory or a
PATH symlink without --chdir.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@OPS-NeoRetro OPS-NeoRetro left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

APPROVED!!!!! 👍👍👍

Comment thread ds4_metal.m
* from any working directory (e.g. inside another project). */
NSString *exe_dir = nil;
{
char exe_path[4096];

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
char exe_path[4096];
char exe_path[32768];

You have to do that because APFS can have 32768-char and longer paths, and unlimited folders, and APFS can only have filenames and folder names up to 255 characters long. APFS is the filesystem for Apple Silicon Macs.

Comment thread ds4_metal.m
Comment on lines +3076 to +3082
uint32_t exe_len = sizeof(exe_path);
if (_NSGetExecutablePath(exe_path, &exe_len) == 0) {
NSString *resolved = [[NSString stringWithUTF8String:exe_path]
stringByResolvingSymlinksInPath];
exe_dir = [resolved stringByDeletingLastPathComponent];
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

That's the fix

Comment thread ds4_metal.m
Comment on lines +3093 to +3094
if (exe_dir)
[paths addObject:[exe_dir stringByAppendingPathComponent:spec[1]]];

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Second part of the fix

Comment thread ds4_metal.m
@@ -1,5 +1,6 @@
#import <Foundation/Foundation.h>
#import <Metal/Metal.h>
#include <mach-o/dyld.h>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You used _NSGetExecutablePath, which is a private dyld function, so you correctly included the right header

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants