-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Fix: engine unable to find dlls on when running on Windows #5863
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed everything up to 4909f1b in 4 minutes and 6 seconds. Click for details.
- Reviewed
61lines of code in2files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. extensions/llamacpp-extension/src/backend.ts:139
- Draft comment:
Consider handling OS types beyond Windows. Currently, non-Windows defaults to 'linux', which may be incorrect if macOS or other OSes are supported. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The comment initially seems valid since macOS exists in the codebase. However, looking deeper: 1. The platformName is only used for CUDA downloads 2. CUDA is only supported on Windows and Linux 3. Therefore defaulting to 'linux' for non-Windows is actually correct here 4. The comment is suggesting a fix for a non-existent problem I could be wrong about CUDA support - maybe future versions will support other platforms? Also, the code could be more explicit about the Windows/Linux limitation. CUDA is an NVIDIA technology that only runs on Windows and Linux. This is a fundamental limitation, not a temporary one. The code correctly handles the only two possible platforms for CUDA. The comment should be deleted because it suggests handling additional OS types for CUDA downloads, but CUDA only runs on Windows and Linux, so the current code is correct.
2. src-tauri/src/core/utils/extensions/inference_llamacpp_extension/server.rs:139
- Draft comment:
The raw string literal used in trim_start_matches (r"?") appears invalid because it ends with a backslash. Consider using a hash-delimited raw string (e.g. r#"?"#) to fix this. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. src-tauri/src/core/utils/extensions/inference_llamacpp_extension/server.rs:139
- Draft comment:
Typographical issue: The raw string literal on this line (r"\\?\") appears to be incorrectly terminated due to the trailing backslash escaping the closing quote. Consider adjusting the literal (e.g. using a raw string with additional#delimiters or by restructuring the string) so that it correctly represents the intended UNC prefix. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The comment is incorrect. In Rust, r"\?" is a valid raw string literal that correctly represents the Windows UNC prefix \?. The trailing backslash is not escaping the quote as the comment claims - that's not how raw strings work in Rust. The suggested change to r#"\?"# would produce the same result but is unnecessarily more verbose. Could there be some edge case where the extra # delimiters would be helpful? Could the current syntax be confusing to other developers? No, the current syntax is the standard way to write this in Rust. The extra # delimiters are only needed when the string itself contains quote marks, which this one does not. The current code is both correct and idiomatic. The comment should be deleted as it suggests an unnecessary change based on an incorrect understanding of Rust raw string literals.
Workflow ID: wflow_jIMYBVwV96Ctr8hn
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Barecheck - Code coverage reportTotal: 35.15%Your code coverage diff: -0.01% ▾ ✅ All code changes are covered |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Changes requested ❌
Reviewed 065993d in 1 minute and 53 seconds. Click for details.
- Reviewed
23lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. extensions/llamacpp-extension/src/index.ts:168
- Draft comment:
Throwing an error instead of logging a warning changes control flow. Ensure that all callers are ready to handle this exception. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to ensure that all callers are ready to handle the exception, which is a form of asking them to double-check their work. This violates the rule against asking the author to ensure behavior is intended or tested.
2. extensions/llamacpp-extension/src/index.ts:175
- Draft comment:
Typo: In the error message template literal,{error}should be${error}to correctly interpolate the error variable. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_Gvq4Z1u1qEGQ0aO2
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Changes requested ❌
Reviewed 173554c in 1 minute and 33 seconds. Click for details.
- Reviewed
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_Dq6txhoMG4Eskxk2
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
3c10e21 to
a99331f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed 3c10e21 in 1 minute and 14 seconds. Click for details.
- Reviewed
308lines of code in2files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. extensions/llamacpp-extension/package.json:32
- Draft comment:
Added dependency '@tauri-apps/plugin-log' looks good. Double-check version compatibility with other Tauri plugins. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. extensions/llamacpp-extension/src/index.ts:99
- Draft comment:
The custom logger provided logs to both console and the plugin. Consider if duplicating logs is intentional or if only one logging destination is desired. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
3. extensions/llamacpp-extension/src/index.ts:363
- Draft comment:
The added delay for Windows in handleAutoUpdate (setTimeout for 1000ms) helps with file operation timing. Consider making this delay configurable in case different Windows environments require a different wait time. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
4. extensions/llamacpp-extension/src/index.ts:1039
- Draft comment:
In the streaming response handler, logger.error is used to report JSON parsing issues. Adding additional context (like the raw chunk) might improve debugging if errors arise during stream processing. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_PgrHQG23MahxLKgv
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Changes requested ❌
Reviewed a99331f in 2 minutes and 24 seconds. Click for details.
- Reviewed
321lines of code in2files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. extensions/llamacpp-extension/package.json:32
- Draft comment:
Ensure that '@tauri-apps/plugin-log' (^2.6.0) is compatible with the other Tauri dependencies and overall project requirements. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. extensions/llamacpp-extension/src/index.ts:99
- Draft comment:
The custom logger wraps both console output and the plugin-log calls, resulting in duplicate log entries. Confirm if double logging is intended or consider using one method. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The comment identifies a real issue - each log message will appear twice, once in the console and once via plugin-log. However, this could be intentional behavior to ensure logs appear both in the browser console for development and in the application's logging system. The comment asks for confirmation rather than making a clear recommendation, which violates our rules about not asking for confirmation. The double logging could be intentional for development purposes. The comment doesn't provide a clear recommendation for improvement. While double logging may be intentional, the comment could be rewritten to be more actionable by suggesting a specific improvement rather than asking for confirmation. The comment should be deleted because it asks the author to confirm their intention rather than making a specific suggestion for improvement. If there's a concern about duplicate logging, it should be raised with a concrete proposal for change.
3. extensions/llamacpp-extension/src/index.ts:367
- Draft comment:
The Windows-specific fixed delays (using setTimeout) in the auto-update flow may not reliably cover all file system timing issues. Consider making these delays configurable or using dynamic readiness checks. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_m1XhWnT2UVvSDGMv
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed 740e5d6 in 3 minutes and 39 seconds. Click for details.
- Reviewed
70lines of code in4files - Skipped
0files when reviewing. - Skipped posting
6draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. extensions/llamacpp-extension/rolldown.config.mjs:16
- Draft comment:
Good use of JSON.stringify to inject platform booleans. Consider centralizing platform detection if these are used in many places. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. extensions/llamacpp-extension/src/backend.ts:97
- Draft comment:
Replacing runtime call with IS_WINDOWS constant simplifies logic. Ensure the build‐time platform value matches the runtime environment. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. extensions/llamacpp-extension/src/backend.ts:134
- Draft comment:
The platformName is now determined using IS_WINDOWS. Note that non‐Windows systems default to 'linux'; if macOS is supported, consider an explicit branch. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
4. extensions/llamacpp-extension/src/env.d.ts:1
- Draft comment:
Added declarations for IS_WINDOWS, IS_MAC, and IS_LINUX; looks appropriate for ensuring type safety with injected globals. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
5. extensions/llamacpp-extension/src/index.ts:367
- Draft comment:
Updated platform check to use IS_WINDOWS. This enhances consistency, though be sure the build-time constant reflects the actual runtime OS. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
6. extensions/llamacpp-extension/src/index.ts:390
- Draft comment:
Similarly, using IS_WINDOWS for delay before cleaning up old backends improves readability and consistency. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_1EykKmiX4hnRhHY6
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ALL GOOD NOW!
🧩 Summary
This PR introduces two key improvements to ensure robust backend handling across platforms and better compatibility with Windows paths:
🟩 1. Platform-Aware CUDA Download URLs
File:
extensions/llamacpp-extension/src/backend.tswindow.core.api.getSystemInfo().platformNamevariable ('win'or'linux') for consistent platform identification."linux"suffixes.Before:
After:
This allows Windows users to correctly download matching backend binaries.
🟦 2. Normalize and Validate Library Path on Windows
File:
src-tauri/src/core/utils/extensions/inference_llamacpp_extension/server.rs\\?\) and normalize paths.current_dir.This ensures that the server:
✅ Result
These changes:
Fixes: #5815
Important
Fixes platform-specific issues by updating CUDA URLs and normalizing Windows paths, with improved logging and error handling.
downloadBackend()inbackend.tsto use platform-aware CUDA URLs, replacing hardcoded "linux" with dynamic platform names.load_llama_model()inserver.rs, removing UNC prefixes and checking path validity.consolelogging withloggerinindex.tsfor consistent logging.server.rs.@tauri-apps/plugin-logdependency inpackage.json.rolldown.config.mjsfor build-time platform checks.This description was created by
for 740e5d6. You can customize this summary. It will automatically update as commits are pushed.