Skip to content

Conversation

@yimsk
Copy link
Contributor

@yimsk yimsk commented Feb 3, 2026

Summary

  • Set env.GOROOT in flake.nix so nix develop always uses Nix-provided Go toolchain instead of system GOROOT

Prevents compile: version "go1.25.4" does not match go tool version "go1.25.5" errors when system Go differs from Nix Go.

@claude
Copy link

claude bot commented Feb 3, 2026

Code Review: PR #160 - Set GOROOT in flake.nix

Summary

This PR adds env.GOROOT configuration to the Nix development shell to prevent version mismatch errors between system Go and Nix-provided Go toolchain.

✅ Strengths

  1. Solves a Real Problem: Addresses the common issue where developers have a different system Go version than the Nix-provided version, preventing compilation errors like compile: version "go1.25.4" does not match go tool version "go1.25.5".

  2. Minimal Change: Very focused two-line addition that doesn't affect any other functionality.

  3. Good Commit Message: Clear explanation of the problem being solved.

⚠️ Potential Issues

1. GOROOT Path Verification Needed

The path ${pkgs.go_1_25}/share/go may not be correct for all Nix installations. In nixpkgs, the Go package typically exposes its root at different paths depending on the package structure.

Recommendation: Verify that this path is correct. Common alternatives in Nix are:

  • ${pkgs.go_1_25} (the package root itself)
  • ${pkgs.go_1_25}/lib/go

You can verify the correct path by running:

nix develop --command bash -c 'echo $GOROOT'

2. Consider Using go.env Instead

In newer nixpkgs and Nix flake patterns, it's more idiomatic to let Go determine its own GOROOT:

shellHook = ''
  export GOROOT="$(go env GOROOT)"
  echo "claws dev env - Go $(go version | cut -d' ' -f3)"
'';

This approach is more portable and doesn't hardcode assumptions about package structure.

3. Missing Test/Verification

While this is a small change, it would be valuable to verify that:

  • The path actually exists in the Nix environment
  • Go commands work correctly after this change
  • The version mismatch error is actually resolved

🔍 Code Quality Assessment

  • Style: Follows existing flake.nix formatting ✅
  • Syntax: Valid Nix syntax ✅
  • Documentation: PR description explains the change well ✅

🛡️ Security & Performance

  • Security: No security concerns - environment variable only ✅
  • Performance: No performance impact ✅
  • Backwards Compatibility: Should not affect existing users ✅

📝 Recommendations

  1. Test the change: Verify that nix develop works correctly with this GOROOT setting
  2. Validate the path: Confirm ${pkgs.go_1_25}/share/go exists and contains a valid Go installation
  3. Alternative approach: Consider using $(go env GOROOT) in shellHook instead for better portability
  4. Documentation: Consider adding a comment explaining why GOROOT is set explicitly

Suggested Alternative Implementation

devShells.default = pkgs.mkShell {
  packages = with pkgs; [
    go_1_25
    go-task
    golangci-lint
  ];

  shellHook = ''
    # Use Nix-provided Go instead of system Go to avoid version mismatches
    export GOROOT="$(go env GOROOT)"
    echo "claws dev env - Go $(go version | cut -d' ' -f3)"
  '';
};

Verdict

Conditional Approval ⚠️ - The intent is good and the change is minimal, but please verify the GOROOT path is correct before merging. If the path is incorrect, the development environment may not work properly for users who don't have the directory at that location.

Next Steps:

  1. Verify the path works: nix develop --command bash -c 'ls -la $GOROOT'
  2. Test go commands work: nix develop --command go version
  3. Consider the alternative shellHook approach for better portability

@yimsk yimsk merged commit 7781a5c into main Feb 3, 2026
11 checks passed
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.

1 participant