Conversation
|
Looks like you had a painful journey here, sorry about that. LGTM when squash-merging. Real root cause: dev inputs shouldn't matter to flake dependents |
|
@roberth No, not too painful at all, just a few careless naming oversights and some failures to copy/paste properly on my part. Totally reasonable shortcut on your part and reintroducing flake-parts would potentially make sense if there were ever a need to bring in multiple flake-parts modules. Also well said re: the root cause. Would love to see those changes down the road. Finally, a "dev subflake" is a best practice I hadn't considered but makes complete sense. |
|
Ok, glad to hear. I guess the 9 commits are really small yeah. Reintroducing might make sense when combining pre-commit with treefmt, if that's what people want, or maybe it'd be something else, but we'll get the when we get there. |
|
SGTM, but could this be squashed? |
flake.nix
Outdated
| # git-hooks-nix doesn't support systems like `i686-linux` so we can't have a pre-commit check for it | ||
| // (lib.optionalAttrs (system != "i686-linux")) { | ||
| pre-commit = preCommitHooksFor system; | ||
| } |
There was a problem hiding this comment.
Doesn't this mean that the devshell for i686 will be borked?
There was a problem hiding this comment.
We currently don't have proper eval checks, so it might not be caught. I got started at some point adding a nix-eval-jobs check to actually evaluate everything in CI, but that was hard without #14897.
There was a problem hiding this comment.
Please correct me if I'm wrong, but as far as I can tell, git-hooks.nix doesn't support i686-linux at the flake.lock revision that's currently in master: https://github.com/cachix/git-hooks.nix/blob/aa9f40c906904ebd83da78e7f328cd8aeaeae785/flake.nix#L18-L23.
That means, in turn, that there's currently no pre-commit checks for i686-linux because this...
devFlake.checks.${system} or { }...is evaluating to an empty attribute set. So the changes in this PR aren't changing that behavior.
But I suspect that I might be missing something here, as I'm not all that familiar with flake-parts.
There was a problem hiding this comment.
But with a flakeModule there's no need to hardcode systems in the flake though. dev-shell.nix does have the hooks and I distinctly remember the long build times of it on my machine :)
903cd2f to
bc9a33f
Compare
a594ac1 to
18b1fd3
Compare
|
Considering that the system argument handling of flakes is totally inadequate I'm inclined to think that we might want to keep flake-parts around. We do have to at some point untangle our overcomplicated packaging expressions after all. |
18b1fd3 to
f32d9a6
Compare
f32d9a6 to
bd4fc8e
Compare
|
Closing this. The reasoning of @xokdvium is correct: without flake-parts, pre-commit hooks can't be provided for all supported systems. There are a few things that could change to move the needle on that (like adding system support in |
Motivation
This makes the input tree a bit leaner and removes some unnecessary complexity.
Context
flake-parts can be useful in highly complex flakes. In this case, though, the flake is relatively straightforward and flake-parts is only used to configure git-hooks-nix, which doesn't strictly require it. Better to refactor it out and prevent an unnecessary flake input from propagating to downstream lockfiles.
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.