Conversation
WalkthroughReplaces devFlake/flake-parts wiring with a new per-system Changes
Sequence DiagramsequenceDiagram
participant Flake as flake.nix
participant HooksFile as packaging/pre-commit-hook-settings.nix
participant DevShell as packaging/dev-shell.nix
participant Checks as flake checks
rect rgb(245,250,245)
Note over Flake,HooksFile: New per-system hook wiring
Flake->>HooksFile: import (pkgs, lib, src)
Flake->>Flake: expose preCommitHooksFor(system) -> { check, settings }
Flake->>DevShell: pass preCommitHooksFor into dev-shell
DevShell->>DevShell: set _NIX_PRE_COMMIT_HOOKS_CONFIG = modular.shellHook
Flake->>Checks: checks[system].pre-commit-check = (preCommitHooksFor system).check
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packaging/pre-commit-hook-settings.nix (1)
9-25: Consider consolidating the merge conflict checks.Both
check-merge-conflicts(line 9) andcheck-merge-conflicts-2(lines 10-25) detect merge conflict markers. This duplication may be unnecessary.If the built-in hook is sufficient, consider removing the custom script:
- check-merge-conflicts.enable = true; - check-merge-conflicts-2 = { - enable = true; - entry = "${pkgs.writeScript "check-merge-conflicts" '' - #!${pkgs.runtimeShell} - conflicts=false - for file in "$@"; do - if grep --with-filename --line-number -E '^>>>>>>> ' -- "$file"; then - conflicts=true - fi - done - if $conflicts; then - echo "ERROR: found merge/patch conflicts in files" - exit 1 - fi - ''}"; - }; + check-merge-conflicts.enable = true;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
flake.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
flake.nix(3 hunks)maintainers/flake-module.nix(0 hunks)packaging/dev-shell.nix(4 hunks)packaging/pre-commit-hook-settings.nix(1 hunks)
💤 Files with no reviewable changes (1)
- maintainers/flake-module.nix
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build_x86_64-linux / manual
- GitHub Check: build_x86_64-linux / vm_tests_smoke
- GitHub Check: build_aarch64-darwin / build
🔇 Additional comments (6)
packaging/pre-commit-hook-settings.nix (1)
49-77: LGTM!The hook configurations for
nixfmt-rfc-style,clang-format, andshellcheckare well-structured with appropriate exclusions for test files and vendored code.packaging/dev-shell.nix (3)
3-3: LGTM!The parameter change from
devFlaketopreCommitHooksForand its usage at line 14 correctly integrate with the new pre-commit hook architecture.Also applies to: 14-14
76-78: LGTM!The updated reference to
modular.settingscorrectly accesses the pre-commit configuration from the newpreCommitHooksForstructure.
118-118: LGTM!Adding
pkgs.buildPackages.pre-commitdirectly tonativeBuildInputsis a cleaner approach than the previous indirection through the modular settings.flake.nix (2)
76-90: LGTM!The
preCommitHooksForfunction is well-structured and provides clean per-system pre-commit hook configuration. The return value exposes both the check derivation and the settings for use in dev shells.
350-352: LGTM!The integration of
preCommitHooksForinto both the checks output (lines 350-352) and dev shell construction (line 501) is correct and maintains consistency across the flake outputs.Also applies to: 501-501
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packaging/pre-commit-hook-settings.nix (1)
26-48: Verify meson.format location and consider patch reliability.The path
${../meson.format}(line 45) should resolve to the repository root. Please verify this file exists at the expected location.Additionally, fetching a patch from GitHub (lines 32-35) can be fragile if the URL becomes unavailable or if the upstream commit is rebased/deleted.
Run the following script to verify the meson.format file location:
#!/bin/bash # Description: Verify meson.format exists at repository root if [ -f "meson.format" ]; then echo "✓ meson.format exists at repository root" head -n 10 meson.format else echo "✗ ERROR: meson.format not found at repository root" echo "Searching for meson.format elsewhere:" fd -t f "^meson.format$" fi
🧹 Nitpick comments (1)
packaging/pre-commit-hook-settings.nix (1)
67-75: Consider pinning llvmPackages version.Using
pkgs.llvmPackages_latest(line 69) means clang-format behavior could change unexpectedly with Nixpkgs updates. Consider pinning to a specific version (e.g.,llvmPackages_19) for reproducibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
flake.nix(3 hunks)packaging/dev-shell.nix(4 hunks)packaging/pre-commit-hook-settings.nix(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packaging/dev-shell.nix
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build_x86_64-linux / manual
- GitHub Check: build_x86_64-linux / vm_tests_smoke
- GitHub Check: build_x86_64-linux / test
- GitHub Check: build_aarch64-darwin / build
🔇 Additional comments (6)
packaging/pre-commit-hook-settings.nix (3)
1-8: LGTM!The function signature and structure are clean and follow Nix conventions for modular configuration.
49-66: LGTM!The nixfmt-rfc-style configuration appropriately excludes test files that may require non-standard formatting for validation purposes.
76-76: LGTM!Enabling shellcheck is excellent for catching common shell scripting issues.
flake.nix (3)
76-86: LGTM!The
preCommitHooksForfunction correctly wires the pre-commit hook settings from the new modular configuration file. Thesrc = selfon line 86 is correctly provided togit-hooks-nix.lib.runas required by that API.
346-348: LGTM!Exposing the pre-commit checks in the checks output is excellent for CI/CD integration, ensuring hooks run automatically in continuous integration.
497-497: LGTM!The dev shell integration with
preCommitHooksForsuccessfully replaces the flake-parts dependency, aligning with the PR objective.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
flake.nix(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build_aarch64-darwin / manual
- GitHub Check: build_x86_64-linux / vm_tests_smoke
- GitHub Check: build_x86_64-linux / manual
- GitHub Check: build_x86_64-linux / test
🔇 Additional comments (2)
flake.nix (2)
81-86: No changes needed—the code pattern is correct.The verification confirms the implementation follows the expected pattern. The
src = selfpassed to the import at line 83 and merged again at line 86 is correct behavior:packaging/pre-commit-hook-settings.nixreceivessrcas input but returns only thehooksconfiguration object without it, sosrcmust be re-added to the final configuration before passing toinputs.git-hooks-nix.lib.${system}.run, which requires it.
497-497: ✓ Signature compatibility verified.The
dev-shell.nixfunction signature correctly expectslibandpreCommitHooksForas parameters. Both are actively used:preCommitHooksForis called with the build platform system (line 14), andlibis used throughout for utility functions (lib.optionals,lib.optionalAttrs,lib.optional, etc.). The function call in flake.nix is correctly passing both parameters.
|
Closing this in the hopes that NixOS#15017 goes through and we can inherit this from upstream |
1 participant
Not an urgent thing but it does remove one bulky dependency and, from my perspective, simplifies the dev shell logic at least a little bit.
Summary by CodeRabbit
Refactor
Chores
Summary by CodeRabbit
New Features
Refactor
Chores