rust: expose toolchainPackage and use it correctly override git hooks

[sandydoo]

Unlike fenix, rust-overlay does it's patching when building the
toolchain profile from a set of resolved components. For that reason we
can't use the unpatched toolchain components directly to fixup the git
hooks packages.

[claude]

Claude finished @sandydoo's task —— View job

---

Code Review Complete

Todo List:

• Read and analyze the modified rust.nix file
• Review the toolchain handling changes
• Check for potential bugs or issues
• Evaluate code quality and best practices
• Provide specific feedback with file references

Analysis Summary

The PR introduces a new toolchainPackage option and changes how git-hooks tools are configured to use the aggregated toolchain package instead of individual components. This addresses a fundamental difference between fenix and rust-overlay where rust-overlay patches components during profile aggregation.

Key Changes:

1. Added toolchainPackage option (lines 98-105)
2. Modified git-hooks tools to use cfg.toolchainPackage (lines 174-176)
3. Added toolchainPackage implementation for both nixpkgs and rust-overlay channels
4. Added proper naming for the nixpkgs toolchain package

Code Review Feedback

Potential Issues & Improvements

1. Missing readOnly attribute on toolchainPackage option (lines 98-105)

   • The toolchainPackage option is automatically computed based on channel/components configuration
   • Should be marked as readOnly = true to prevent user modification
   • This would make the intent clearer and prevent configuration errors

2. Inconsistent option pattern (lines 182-186)

   • For nixpkgs channel, toolchainPackage uses symlinkJoin without inheriting the name attribute pattern
   • Consider using a more descriptive name like rust-toolchain-nixpkgs-${lib.concatStringsSep "-" cfg.components}
   • Current implementation could result in unclear store paths

3. Error handling could be improved (lines 204-205, 226-230)

   • The error messages are helpful but could include more context
   • Consider adding validation earlier in the pipeline rather than during evaluation
   • The throw statements could benefit from standardized error message format

4. Hardcoded native target handling (line 217)

   • pkgs.stdenv.hostPlatform.rust.rustcTargetSpec assumes rustc target is available
   • Should validate that the target exists before adding it to allTargets
   • Could fail silently on platforms without rust support

5. Private API usage not future-proof (lines 194-201)

   • The comment warns about using rust-overlay's private API
   • No version pinning or compatibility checks for the private API
   • Could break with future rust-overlay updates

6. Test coverage gap

   • No tests validate the new toolchainPackage functionality
   • The existing test (tests/rust/devenv.nix) only tests RUSTFLAGS behavior
   • Should add tests for both nixpkgs and rust-overlay toolchain packages

Security Considerations

7. Input validation (lines 204-205)
   • Channel and version validation happens at evaluation time
   • Consider adding assertions to validate inputs earlier
   • Current approach could lead to obscure error messages

Performance Considerations

8. Repeated computation (lines 217-218)

   • nativeTarget is computed but allTargets recalculates uniqueness
   • Consider caching the native target computation
   • The lib.unique call on every evaluation could be optimized

9. Component resolution efficiency (lines 236-246)

   • The component resolution logic iterates through availability checks
   • Could be optimized by building a lookup table once
   • Current approach is O(n×m) where n=components, m=available components

Code Quality Improvements

10. Documentation clarity (lines 100-103)

    • The description could be more specific about when this option should be used
    • Should mention the difference between nixpkgs and rust-overlay behavior
    • Consider adding examples of typical usage patterns

11. Magic string usage (line 208)

    • The "rust" profile exclusion is hardcoded
    • Could be defined as a constant with explanatory comment
    • Makes the code more maintainable

Recommendations

1. Add comprehensive tests for both nixpkgs and rust-overlay toolchain packages
2. Consider making toolchainPackage read-only to prevent misuse
3. Add validation for native target availability before inclusion
4. Document the private API risk and consider adding version guards
5. Optimize component resolution with lookup tables for better performance

The core functionality is sound and addresses the rust-overlay patching issue correctly. The changes maintain backward compatibility while fixing the git-hooks integration problem.

[cloudflare-workers-and-pages]

Deploying devenv with    Cloudflare Pages

Latest commit:	9e8d80b
Status:	✅  Deploy successful!
Preview URL:	https://ed7dcb0b.devenv.pages.dev
Branch Preview URL:	https://fix-rust-hooks-with-custom-t.devenv.pages.dev

View logs