Add pre-commit hook and CI check, excluding currently unformatted files #7745

roberth · 2023-02-03T18:48:00Z

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.

Scoped out

Currently the formatters only run manually. Pre-commit invocation depends on an upstream PR because we want it to be opt-in:

Make hook installation optional cachix/git-hooks.nix#261

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

agreed on idea
agreed on implementation strategy
tests, as appropriate
- functional tests - tests/**.sh
- unit tests - src/*/tests
- integration tests - tests/nixos/*
documentation in the manual
code and comments are self-explanatory
commit message explains why the change was made
new feature or bug fix: updated release notes

fricklerhandwerk

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.

doc/manual/src/contributing/hacking.md

SuperSandro2000 · 2023-02-08T10:30:56Z

doc/manual/src/contributing/hacking.md

@@ -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.


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.

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.

SuperSandro2000 · 2023-02-08T10:32:17Z

maintainers/flake-module.nix

+ # 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"


I wouldn't mind if that would be reusable and also working for editorconfig but thats probably out of scope.

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.

edolstra · 2023-02-08T11:33:34Z

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.

nixos-discourse · 2023-02-17T11:51:36Z

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

nixos-discourse · 2023-02-20T14:58:39Z

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

roberth · 2023-03-17T16:40:00Z

flake.nix

@@ -705,6 +734,8 @@

 # Make bash completion work.
 XDG_DATA_DIRS+=:$out/share
+
+ ${(devFlake.getSystem stdenv.hostPlatform.system).pre-commit.settings.installationScript}


Installation is neutered by only specifying manual hooks.

fricklerhandwerk · 2023-03-22T10:28:46Z

Discussed in the Nix team meeting 2023-03-17:

The pre-commit hook shouldn't be enabled by default (at least not for starters).
Create phony make target to manually run the formatter
Also try to unify the different nixpkgs dependencies in the Flake
Small issue around the use of flake-parts which shouldn't be blessed as the default way of doing things. But fairly minor, we can ignore it
Idea-approved as an experiment

nixos-discourse · 2023-03-22T20:18:36Z

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

roberth · 2023-04-14T12:17:43Z

Currently blocked on upstream changes to make hook installation more flexible.

Make hook installation optional cachix/git-hooks.nix#261 (comment)

nixos-discourse · 2023-04-17T09:45:21Z

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

thufschmitt · 2024-01-26T15:35:54Z

Discussed during the Nix maintainers meeting on 2024-01-26.
Agreement on doing the CI+manual tooling part first.

Blocked on Make hook installation optional cachix/git-hooks.nix#261
@thufschmitt: Can we skip the pre-commit-hook part at first?
- @roberth: Would be very annoying
- @Ericson2314: Probably acceptable though
  - Would help building the case for the upstream PR
What if we integrate it, but make it optional using different means
@roberth: sgtm

nixos-discourse · 2024-01-26T16:21:46Z

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

9999years · 2024-01-26T17:14:15Z

Why not just apply the pre-commit patch in our environment? Nix is very good at providing a patched dependency.

roberth · 2024-04-18T21:54:33Z

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).

L-as · 2024-04-19T12:16:37Z

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.

roberth · 2024-04-19T12:30:33Z

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.
A better way to achieve a reduction in dependency footprint is to give cachix/git-hooks.nix a lightweight backend. That's compatible with pre-commit configuration items such as the exclude regex.

Yet another solution is to use a different solution just for make format.
I don't think this is a good idea, because a git hook is very useful, and we shouldn't have to maintain two configs.

fricklerhandwerk

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.

fricklerhandwerk · 2024-04-19T13:24:06Z

doc/manual/src/contributing/hacking.md

@@ -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


Suggested change

## Formatting and pre-commit

## Formatting and pre-commit hooks

fricklerhandwerk · 2024-04-19T13:25:26Z

doc/manual/src/contributing/hacking.md

+
+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.


Stop and start what?

fricklerhandwerk · 2024-04-19T13:25:42Z

doc/manual/src/contributing/hacking.md

+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:


Which config? The dev shell?

fricklerhandwerk · 2024-04-19T13:25:59Z

doc/manual/src/contributing/hacking.md

+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.


Suggested change

- 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`.

fricklerhandwerk · 2024-04-19T13:26:38Z

flake.nix

@@ -186,6 +209,12 @@
 inherit fileset stdenv;
 };

+ # https://github.com/NixOS/nixpkgs/pull/214409


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.

roberth requested review from fricklerhandwerk, edolstra and thufschmitt as code owners February 3, 2023 18:48

roberth mentioned this pull request Feb 3, 2023

pre-commit: Disable check on i686-linux NixOS/nixpkgs#214409

Merged

14 tasks

roberth force-pushed the pre-commit branch 2 times, most recently from 40c3d90 to 2073772 Compare February 3, 2023 20:46

roberth changed the title ~~Add pre-commit hook and CI check, excluding currently unformatted~~ Add pre-commit hook and CI check, excluding currently unformatted files Feb 7, 2023

fricklerhandwerk approved these changes Feb 8, 2023

View reviewed changes

doc/manual/src/contributing/hacking.md Outdated Show resolved Hide resolved

SuperSandro2000 reviewed Feb 8, 2023

View reviewed changes

infinisil mentioned this pull request Feb 17, 2023

Experimental python bindings #7735

Draft

14 tasks

roberth mentioned this pull request Feb 26, 2023

Reorganize this repo #7876

Open

thufschmitt added the contributor-experience Developer experience for Nix contributors label Feb 27, 2023

thufschmitt added the idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. label Mar 17, 2023

roberth force-pushed the pre-commit branch from 95b8918 to 77b35af Compare March 17, 2023 16:23

roberth commented Mar 17, 2023

View reviewed changes

roberth force-pushed the pre-commit branch 2 times, most recently from af29522 to a9b3ea3 Compare March 17, 2023 17:13

roberth force-pushed the pre-commit branch from a9b3ea3 to d86c7d5 Compare April 7, 2023 15:31

thufschmitt assigned Ericson2314 Apr 14, 2023

roberth marked this pull request as draft April 14, 2023 12:16

jlesquembre mentioned this pull request Jan 8, 2024

Add clang format configuration #9719

Merged

roberth mentioned this pull request Apr 18, 2024

C API: Add nix_init_apply #10537

Merged

roberth force-pushed the pre-commit branch 2 times, most recently from a242b43 to 70cf09e Compare April 18, 2024 21:36

roberth marked this pull request as ready for review April 18, 2024 21:38

roberth force-pushed the pre-commit branch from 70cf09e to 58bdc99 Compare April 18, 2024 21:46

fricklerhandwerk approved these changes Apr 19, 2024

View reviewed changes

roberth added 4 commits April 21, 2024 13:54

Add pre-commit hook and make format target

5b36ee4

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.

pre-commit: Remove nixpkgs-fmt

8f042a5

devShells: Prefix shell-for-

96c8a9a

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.

devShells: null out src to avoid nix develop rebuild

a3ff75f

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.

roberth force-pushed the pre-commit branch from 5271b15 to 90ccd40 Compare April 21, 2024 11:55

roberth enabled auto-merge April 21, 2024 11:55

Edit docs

b5f1d4c

roberth force-pushed the pre-commit branch from 90ccd40 to b5f1d4c Compare April 21, 2024 12:16

roberth merged commit 6a5d222 into NixOS:master Apr 21, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pre-commit hook and CI check, excluding currently unformatted files #7745

Add pre-commit hook and CI check, excluding currently unformatted files #7745

roberth commented Feb 3, 2023 •

edited

Loading

fricklerhandwerk left a comment

SuperSandro2000 Feb 8, 2023

9999years Jan 13, 2024

SuperSandro2000 Feb 8, 2023

roberth Mar 17, 2023

edolstra commented Feb 8, 2023

nixos-discourse commented Feb 17, 2023

nixos-discourse commented Feb 20, 2023

roberth Mar 17, 2023

fricklerhandwerk commented Mar 22, 2023 •

edited

Loading

nixos-discourse commented Mar 22, 2023

roberth commented Apr 14, 2023 •

edited

Loading

nixos-discourse commented Apr 17, 2023

thufschmitt commented Jan 26, 2024 •

edited by fricklerhandwerk

Loading

nixos-discourse commented Jan 26, 2024

9999years commented Jan 26, 2024

roberth commented Apr 18, 2024

L-as commented Apr 19, 2024

roberth commented Apr 19, 2024

fricklerhandwerk left a comment

fricklerhandwerk Apr 19, 2024

fricklerhandwerk Apr 19, 2024

fricklerhandwerk Apr 19, 2024

fricklerhandwerk Apr 19, 2024

fricklerhandwerk Apr 19, 2024

	## Formatting and pre-commit
	## Formatting and pre-commit hooks

	- 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`.

Add pre-commit hook and CI check, excluding currently unformatted files #7745

Add pre-commit hook and CI check, excluding currently unformatted files #7745

Conversation

roberth commented Feb 3, 2023 • edited Loading

Motivation

Context

Scoped out

Checklist for maintainers

fricklerhandwerk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edolstra commented Feb 8, 2023

nixos-discourse commented Feb 17, 2023

nixos-discourse commented Feb 20, 2023

Choose a reason for hiding this comment

fricklerhandwerk commented Mar 22, 2023 • edited Loading

nixos-discourse commented Mar 22, 2023

roberth commented Apr 14, 2023 • edited Loading

nixos-discourse commented Apr 17, 2023

thufschmitt commented Jan 26, 2024 • edited by fricklerhandwerk Loading

nixos-discourse commented Jan 26, 2024

9999years commented Jan 26, 2024

roberth commented Apr 18, 2024

L-as commented Apr 19, 2024

roberth commented Apr 19, 2024

fricklerhandwerk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roberth commented Feb 3, 2023 •

edited

Loading

fricklerhandwerk commented Mar 22, 2023 •

edited

Loading

roberth commented Apr 14, 2023 •

edited

Loading

thufschmitt commented Jan 26, 2024 •

edited by fricklerhandwerk

Loading