Remove flake-parts dependency #249
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.
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