Autoformatting PRs

[dustypomerleau]

While I was preparing #1053, I noticed that nixfmt makes significant changes to the current formatting (I am currently running nixfmt nixpkgs-unstable-2024-08-16). Is nix-darwin interested in adopting the RFC version of nixfmt? Should contributors use it (or explicitly avoid it)? It would be nice to have some sort of agreed approach to autoformatting.

[Enzime]

I've been meaning to get around to adding an autoformatter for this codebase, I would definitely merge a PR that adds it to the codebase using https://github.com/cachix/git-hooks.nix or something similar

[emilazy]

Yes, we should format with nixfmt-rfc-style and put the commit in .git-blame-ignore-revs and enforce it on all files with CI going forward. I don’t think we need an elaborate transition like Nixpkgs had. Hooks cannot be relied on for correctness since they are entirely opt‐in on the user machine, and I personally strongly dislike pre‐commit hooks as opposed to pre‐push hooks, which sadly there is less available automation around. So I would prefer to punt on hooks for now unless anyone feels like writing a pre‐push one.

[Enzime]

The stage at which the hook runs at can be customized with the hooks option for the above framework, however testing will be need to check if the hooks support pre-push out of the box as they may not be able to handle multiple commits.

I don't agree that we should punt on hooks even if we could only have a pre-commit hook, because they're opt in and the code style will inevitably be enforced by CI, so my preference would still be to include the hook.

[emilazy]

Part of the problem is that hooks/development shells are a security can of worms, because they come from the currently checked‐out commit but run outside of Nix builds. That means that they’re a very easy takeover vector for anyone who checks a PR out to review it, especially if they’re installed by default in an .envrc. See e.g. the discussion in NixOS/nixpkgs#325793.

[Enzime]

That sounds reasonable to me to not include .envrc and make it explicitly opt in to use the devShell

[antoineco]

IMO the easiest path forward is to leverage nix fmt by adding a formatter attribute to flake.nix. It is as easy as adding a line similar to this to the outputs attribute set:

{
  outputs = {
      formatter = forAllSystems ({ pkgs }: pkgs.nixfmt-rfc-style);
  };
}

Mention in a CONTRIBUTING.md file that PRs should be nix fmt'd, add a CI step which fails early on nix fmt resulting in changes, and you're good to go. People are then free to adopt whichever flow, devenvs, hooks, ... they like.

[emilazy]

Since nixfmt deprecated their recursive mode, we’d probably want to rely on Treefmt. I hope to work on this soon as part of release branches (since not doing it beforehand would be annoying for backports). One complication is that I am also trying to work on keeping modules rebased against their upstream NixOS equivalents where applicable, and NixOS has not yet done their own tree‐wide reformat…

[antoineco]

About recursive mode, there seems to be some confusion: nixfmt did deprecate it, but nix fmt did not.

nix fmt is a wrapper around nixfmt in this scenario, equivalent to treefmt but without the extra customization.

Note that nix fmt could be set to pkgs.treefmt too!

[emilazy]

AFAIK nix fmt just does <formatter> . and doesn’t implement its own recursive logic, cf. NixOS/nix#11438.

[antoineco]

It doesn't match my experience. nix fmt formats my whole nix config repo without issues. It even tries to format .direnv/ which I wish I could disable natively.

edit: I should have mentionned that I'm still using nixfmt v0.6.0. You are right, newer versions will no longer work that way, especially with that change in Nix coming.