fix(nix): make nix run build reliably on macOS#554
fix(nix): make nix run build reliably on macOS#5540xcaff wants to merge 1 commit intoDimillian:mainfrom
Conversation
xkonjin
left a comment
There was a problem hiding this comment.
Code Review
Summary: Fix for Nix build reliability on macOS by making libclang and perl available on all platforms and unconditionally setting LIBCLANG_PATH.
Strengths
- Addresses a legitimate cross-platform build issue
- The change is minimal and focused
- Good use of
pkgs.lib.optionalalsfor Linux-specific dependencies
Issues & Suggestions
-
Missing documentation: The PR title and description don't explain why this change is needed or what the symptoms were. Add details about:
- What error occurred before the fix
- Why macOS needs these dependencies now
- Any relevant issues this resolves
-
perldependency unexplained: AddingperltonativeBuildInputsis not mentioned in the PR. What is it used for? If it's required by cargo-tauri, document this dependency. -
LIBCLANG_PATHnow unconditional: The change removes the Linux-only conditional, settingLIBCLANG_PATHunconditionally. This might cause issues if the path doesn't exist on some systems. Consider:LIBCLANG_PATH = pkgs.lib.optionalString (pkgs.llvmPackages.libclang != null) "${pkgs.llvmPackages.libclang.lib}/lib";
Or document why this is safe.
-
npmDepsHash update: The npmDepsHash changed from
sha256-GWBewwZD6Q3wstZCvVYToiRHRPvrVQcPBJGJPvook6o=tosha256-YbHVvsYijeCw0FlTWx4yhvfNH+yHRAm7fcfYKG/SkU0=. This indicates a dependency change. What changed in package.json or lockfile? This should be mentioned in the PR description. -
No tests: There's no way to verify this fix works across different macOS versions or Nix configurations. Consider adding a CI check or at least manual testing notes.
Security
- ✅ No security concerns
Minor
- The new
apps.defaultdefinition is a nice quality-of-life improvement but wasn't mentioned in the PR description.
Recommendation: Request changes - Add documentation explaining the dependency changes, symptoms, and testing notes.
No description provided.