Skip to content

fix(nix): make nix run build reliably on macOS#554

Open
0xcaff wants to merge 1 commit intoDimillian:mainfrom
0xcaff:fix/nix-run-flake
Open

fix(nix): make nix run build reliably on macOS#554
0xcaff wants to merge 1 commit intoDimillian:mainfrom
0xcaff:fix/nix-run-flake

Conversation

@0xcaff
Copy link

@0xcaff 0xcaff commented Mar 14, 2026

No description provided.

Copy link

@xkonjin xkonjin left a comment

Choose a reason for hiding this comment

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

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.optionalals for Linux-specific dependencies

Issues & Suggestions

  1. 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
  2. perl dependency unexplained: Adding perl to nativeBuildInputs is not mentioned in the PR. What is it used for? If it's required by cargo-tauri, document this dependency.

  3. LIBCLANG_PATH now unconditional: The change removes the Linux-only conditional, setting LIBCLANG_PATH unconditionally. 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.

  4. npmDepsHash update: The npmDepsHash changed from sha256-GWBewwZD6Q3wstZCvVYToiRHRPvrVQcPBJGJPvook6o= to sha256-YbHVvsYijeCw0FlTWx4yhvfNH+yHRAm7fcfYKG/SkU0=. This indicates a dependency change. What changed in package.json or lockfile? This should be mentioned in the PR description.

  5. 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.default definition 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.

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