-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add pre-commit hook and CI check, excluding currently unformatted files #7745
Conversation
40c3d90
to
2073772
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about including flake-parts, but I really like the incremental strategy which allows for deliberate changes that can be timed not to break conflicting PRs.
@@ -86,6 +86,16 @@ by: | |||
$ nix develop | |||
``` | |||
|
|||
## Pre-commit and formatting | |||
|
|||
`nix develop` installs Nix-[integrated](https://github.com/cachix/pre-commit-hooks.nix) [pre-commit](https://pre-commit.com) hooks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be turned off by default. git hooks can be incredible annoying when they are not fully working and you are in the middle of a more complicated git action like a rebase or you are doing a commit which is broken on purpose and will be later fixed. Also when switching to an old branch when still inside the nix develop shell, this will probably blow up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's OK. It's easy enough to use git commit -n
to skip the Git hooks when needed.
It could be nice to have an environment variable to configure this behavior, like NIX_DEVELOP_PRE_COMMIT
.
maintainers/flake-module.nix
Outdated
# We haven't applied formatting to pre-existing files yet | ||
"doc/manual/generate-manpage.nix" | ||
"doc/manual/generate-options.nix" | ||
"doc/manual/redirects.js" | ||
"doc/manual/theme/highlight.js" | ||
"doc/manual/utils.nix" | ||
"docker.nix" | ||
"flake.nix" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't mind if that would be reusable and also working for editorconfig but thats probably out of scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Editorconfig doesn't suffer from large diffs as much, I would think, so editorconfig won't really need this list. We could just apply its suggestions immediately.
I'm not in favor of doing this until #6721 is merged. I don't see a big advantage in applying formatting only to a couple of new files. |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/tweag-nix-dev-update-44/25546/1 |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/2023-02-17-nix-team-meeting-minutes-33/25624/1 |
flake.nix
Outdated
@@ -705,6 +734,8 @@ | |||
|
|||
# Make bash completion work. | |||
XDG_DATA_DIRS+=:$out/share | |||
|
|||
${(devFlake.getSystem stdenv.hostPlatform.system).pre-commit.settings.installationScript} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Installation is neutered by only specifying manual
hooks.
af29522
to
a9b3ea3
Compare
Discussed in the Nix team meeting 2023-03-17:
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/2023-03-17-nix-team-meeting-minutes-41/26614/1 |
Currently blocked on upstream changes to make hook installation more flexible. |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/2023-04-14-nix-team-meeting-minutes-48/27358/1 |
Discussed during the Nix maintainers meeting on 2024-01-26.
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/2024-01-26-nix-team-meeting-minutes-118/38851/1 |
Why not just apply the |
a242b43
to
70cf09e
Compare
I've simplified the setup by making hook installation/update a manually invoked command. While I would like for the config to be updated automatically when entering a dev shell, that's currently too complicated to pull off while also making it opt-in. It's not essential and could be added later. Let's keep it simple for now and start automating away suggestions like #10537 (review). |
Have you considered not using pre-commit? I don't see why such a large dependency is needed to put a script in .git/hooks/pre-commit. |
I have. Implementing a pre-commit hook correctly is actually non-trivial. It is a rather low level interface. I don't know all the intricacies, but iirc, one of the problems is that you will accidentally touch unstaged changes. Pre-commit takes care of that, and Nix and Nixpkgs handle dependencies quite well. By duplicating this logic, we could indeed reduce our dependency footprint, but that would add undue maintenance cost that negates the benefit. Yet another solution is to use a different solution just for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in general, but needs more clarity for contributor documentation. I also suggest to add in-code comments that instruct contributors what to do and also say why that's important.
Generally, everyone: Please don't leave rationale only in the GitHub threads. Always put them into commit messages, contributor documentation, and in-code comments.
@@ -273,6 +273,29 @@ Configure your editor to use the `clangd` from the `.#native-clangStdenvPackages | |||
> Some other editors (e.g. Emacs, Vim) need a plugin to support LSP servers in general (e.g. [lsp-mode](https://github.com/emacs-lsp/lsp-mode) for Emacs and [vim-lsp](https://github.com/prabirshrestha/vim-lsp) for vim). | |||
> Editor-specific setup is typically opinionated, so we will not cover it here in more detail. | |||
|
|||
## Formatting and pre-commit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
## Formatting and pre-commit | |
## Formatting and pre-commit hooks |
|
||
To refresh the config, do the following: | ||
- if you use `make format`: stop and start your `nix develop` shell. | ||
- if you use the pre-commit hook: stop and start, and run `pre-commit-hooks-install` again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stop and start what?
When making a commit, pay attention to the console output. | ||
If it fails, run `git add --patch` to approve the suggestions _and commit again_. | ||
|
||
To refresh the config, do the following: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which config? The dev shell?
If it fails, run `git add --patch` to approve the suggestions _and commit again_. | ||
|
||
To refresh the config, do the following: | ||
- if you use `make format`: stop and start your `nix develop` shell. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- if you use `make format`: stop and start your `nix develop` shell. | |
- if you use `make format`: exit the development shell and start it again by running `nix develop`. |
flake.nix
Outdated
@@ -186,6 +209,12 @@ | |||
inherit fileset stdenv; | |||
}; | |||
|
|||
# https://github.com/NixOS/nixpkgs/pull/214409 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does that mean? The PR top post doesn't state the problem it's supposedly solving
I've added the new local.mk to the package sources. While this should not be needed for the build, it is the simplest solution, and won't cause many extra rebuilds, because the file won't change very often.
Without this, it's not clear from an error trace that it's the shell that's evaluated. It would look like evaluating the nix package.
Whenever src changed, nix develop would internally create a fresh derivation, which it has to try and substitute and then build. Let's not do that.
Motivation
Eliminate discussions about formatting and make the code consistent, eventually. Make progress.
This will enforce formatting for ~30 files currently, and any new files added.
EDIT: 6 weeks later, I've had to add 14 new files to the ignore list.
Context
Implementation strategy: minimally invasive addition; first turn off the faucet, then start drying.
Inclusion of flake-parts: the other entrypoints into pre-commit-hooks.nix are either non-flake (and a little odd), or don't support the system attributes we need.
Closes pre-commit, editorconfig, nixpkgs-fmt? #7634
Automatically format with clang-tools #6721
Scoped out
Currently the formatters only run manually. Pre-commit invocation depends on an upstream PR because we want it to be opt-in:
Checklist for maintainers
Maintainers: tick if completed or explain if not relevant
tests/**.sh
src/*/tests
tests/nixos/*