fix(windows): link memset_shim object directly and suppress dead_code warning #120

sarman · 2026-06-19T15:57:34Z

sarman commented

2026-06-19 15:57:34 +00:00

Use rustc-link-arg to directly link memset_shim.o object file
instead of -l flag, ensuring symbol is available regardless of
link order with libsodium_sys
Add #[allow(dead_code)] to find_ollama_binary() which is only
used inside cfg(target_os = "macos") and cfg(target_os = "linux")
blocks, causing false positive warning when cross-compiling for Windows

- Use rustc-link-arg to directly link memset_shim.o object file instead of -l flag, ensuring symbol is available regardless of link order with libsodium_sys - Add #[allow(dead_code)] to find_ollama_binary() which is only used inside cfg(target_os = "macos") and cfg(target_os = "linux") blocks, causing false positive warning when cross-compiling for Windows

sarman added 1 commit 2026-06-19 15:57:35 +00:00

fix(windows): link memset_shim object directly and suppress dead_code warning

Test / frontend-tests (pull_request) Successful in 1m39s

Details

Test / frontend-typecheck (pull_request) Successful in 1m48s

Details

PR Review Automation / review (pull_request) Successful in 4m19s

Details

Test / rust-fmt-check (pull_request) Failing after 12m39s

Details

Test / rust-clippy (pull_request) Successful in 13m40s

Details

Test / rust-tests (pull_request) Successful in 15m31s

Details

6e890aa05f

- Use rustc-link-arg to directly link memset_shim.o object file
  instead of -l flag, ensuring symbol is available regardless of
  link order with libsodium_sys

- Add #[allow(dead_code)] to find_ollama_binary() which is only
  used inside cfg(target_os = "macos") and cfg(target_os = "linux")
  blocks, causing false positive warning when cross-compiling for Windows

sarman reviewed 2026-06-19 16:01:55 +00:00

sarman left a comment

Automated PR Review (qwen3.5-122b-think via liteLLM):\n\n

CODE REVIEW

Summary
This PR addresses two Windows build issues: (1) linking memset_shim object directly to resolve memset_explicit symbol errors with libsodium_sys on MinGW, and (2) suppressing dead_code warning for find_ollama_binary() which is only used in cfg-gated macOS/Linux blocks. The changes are targeted and appropriate for the stated problems.

Findings

[WARNING] src-tauri/build.rs:19 - Missing error handling for C compilation
Evidence: cc::Build::new().file("memset_s_shim.c")...compile("memset_shim");
Fix: The cc::Build::new() call can panic or return errors if memset_s_shim.c doesn't exist or compilation fails. Consider checking file existence first or wrapping in proper error handling.
[SUGGESTION] src-tauri/src/ollama/installer.rs:140 - Alternative approach for dead_code suppression
Evidence: #[allow(dead_code)] fn find_ollama_binary() -> Option<std::path::PathBuf> {
Fix: Consider using #[cfg(any(target_os = "macos", target_os = "linux"))] on the function itself instead of #[allow(dead_code)], which would prevent the function from even being compiled on Windows. This is cleaner than suppressing the warning.
[WARNING] src-tauri/build.rs:15 - Hardcoded file path assumption
Evidence: .file("memset_s_shim.c")
Fix: The build script assumes memset_s_shim.c exists in src-tauri/ directory. If this file is missing or moved, the build will fail with a cryptic error. Consider adding a file existence check with a clearer error message.

Verdict: APPROVE WITH COMMENTS

DETAILED ANALYSIS

Agent 1 & 2 (CLAUDE.md Compliance)

No CLAUDE.md files found in the changed file paths (src-tauri/). No violations detected.

Agent 3 (Bug Detection - Diff Only)

The rustc-link-arg approach is correct for direct object file linking
The #[allow(dead_code)] placement is syntactically correct
No syntax errors or type mismatches detected

Agent 4 (Bug Detection - Full Context)

build.rs: The memset_shim compilation path is relative to src-tauri/ directory where build.rs resides. This is standard Cargo behavior.
installer.rs: The find_ollama_binary() function is indeed only called within #[cfg(target_os = "macos")] and #[cfg(target_os = "linux")] blocks (lines 168 and 203), confirming the dead_code warning is legitimate for Windows cross-compilation.

Validation of Findings

Finding 1 (build.rs error handling): The cc::Build::new() call uses .compile() which will panic on failure. For a critical Windows build fix, this should be acceptable as build failures should be visible. However, a clearer error message would help debugging.

Finding 2 (dead_code suppression): The #[allow(dead_code)] is a valid workaround. The suggested cfg attribute approach is cleaner but both are acceptable. This is a minor improvement, not a bug.

Finding 3 (file path assumption): The file path is standard for Cargo build scripts. If memset_s_shim.c doesn't exist, the build will fail with a clear error from cc-rs. This is not a critical issue.

All findings are low-severity improvements rather than critical bugs. The PR effectively addresses the stated Windows build issues.

Automated PR Review (qwen3.5-122b-think via liteLLM):\n\n ## CODE REVIEW **Summary** This PR addresses two Windows build issues: (1) linking memset_shim object directly to resolve memset_explicit symbol errors with libsodium_sys on MinGW, and (2) suppressing dead_code warning for find_ollama_binary() which is only used in cfg-gated macOS/Linux blocks. The changes are targeted and appropriate for the stated problems. **Findings** - [WARNING] src-tauri/build.rs:19 - Missing error handling for C compilation Evidence: `cc::Build::new().file("memset_s_shim.c")...compile("memset_shim");` Fix: The `cc::Build::new()` call can panic or return errors if memset_s_shim.c doesn't exist or compilation fails. Consider checking file existence first or wrapping in proper error handling. - [SUGGESTION] src-tauri/src/ollama/installer.rs:140 - Alternative approach for dead_code suppression Evidence: `#[allow(dead_code)] fn find_ollama_binary() -> Option<std::path::PathBuf> {` Fix: Consider using `#[cfg(any(target_os = "macos", target_os = "linux"))]` on the function itself instead of #[allow(dead_code)], which would prevent the function from even being compiled on Windows. This is cleaner than suppressing the warning. - [WARNING] src-tauri/build.rs:15 - Hardcoded file path assumption Evidence: `.file("memset_s_shim.c")` Fix: The build script assumes memset_s_shim.c exists in src-tauri/ directory. If this file is missing or moved, the build will fail with a cryptic error. Consider adding a file existence check with a clearer error message. **Verdict**: APPROVE WITH COMMENTS --- ## DETAILED ANALYSIS ### Agent 1 & 2 (CLAUDE.md Compliance) No CLAUDE.md files found in the changed file paths (src-tauri/). No violations detected. ### Agent 3 (Bug Detection - Diff Only) - The `rustc-link-arg` approach is correct for direct object file linking - The `#[allow(dead_code)]` placement is syntactically correct - No syntax errors or type mismatches detected ### Agent 4 (Bug Detection - Full Context) - **build.rs**: The memset_shim compilation path is relative to src-tauri/ directory where build.rs resides. This is standard Cargo behavior. - **installer.rs**: The `find_ollama_binary()` function is indeed only called within `#[cfg(target_os = "macos")]` and `#[cfg(target_os = "linux")]` blocks (lines 168 and 203), confirming the dead_code warning is legitimate for Windows cross-compilation. ### Validation of Findings **Finding 1 (build.rs error handling)**: The `cc::Build::new()` call uses `.compile()` which will panic on failure. For a critical Windows build fix, this should be acceptable as build failures should be visible. However, a clearer error message would help debugging. **Finding 2 (dead_code suppression)**: The #[allow(dead_code)] is a valid workaround. The suggested cfg attribute approach is cleaner but both are acceptable. This is a minor improvement, not a bug. **Finding 3 (file path assumption)**: The file path is standard for Cargo build scripts. If memset_s_shim.c doesn't exist, the build will fail with a clear error from cc-rs. This is not a critical issue. All findings are low-severity improvements rather than critical bugs. The PR effectively addresses the stated Windows build issues.

sarman force-pushed fix/cargo-config-sodium from 6e890aa05f to e2e7b02a0e

2026-06-19 16:23:23 +00:00

Compare

sarman merged commit 0c5d06c4e9 into beta

2026-06-19 16:52:57 +00:00

sarman deleted branch fix/cargo-config-sodium

2026-06-19 16:52:57 +00:00

sarman referenced this issue from a commit

2026-06-19 16:52:59 +00:00

Merge pull request 'fix(windows): link memset_shim object directly and suppress dead_code warning' (#120) from fix/cargo-config-sodium into beta

Sign in to join this conversation.

No reviewers