Skip to content
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

Improving flake formatting experience #273

Open
GaetanLepage opened this issue Dec 23, 2024 · 14 comments · May be fixed by NixOS/nixpkgs#384857
Open

Improving flake formatting experience #273

GaetanLepage opened this issue Dec 23, 2024 · 14 comments · May be fixed by NixOS/nixpkgs#384857

Comments

@GaetanLepage
Copy link

GaetanLepage commented Dec 23, 2024

In order to have your flake formatted with nixfmt-rfc-style, you have two options:

  1. Setting formatter = pkgs.nixfmt-rfc-style;. This solution is concise (ignoring the fact that the name "nixfmt-rfc-style" is not really intuitive compared to simply "nixfmt").
    However, this is not expected to be supported for long as: Passing directories or non-Nix files (such as ".") is deprecated and will be unsupported soon, please use https://treefmt.com/ instead, e.g. via https://github.com/numtide/treefmt-nix.
  2. Using treefmt-nix. While it is for sure a great, very flexible and powerful tool, it is quite overkilled if you simply want to format a small nix-only code base. It's major con is having to add both several lines of code and an extra dependency, simply to achieve what the first solution is doing.

I think it should be great to support the existing formatter = pkgs.nixfmt[-rfc-style]; syntax as it gives a very efficient and user-friendly way to format a flake while keeping it lean code and input wise.

In the long term, I could see the benefit of further "officializing" the nixfmt formatter and having a format = true; flag to automatically format the flake. This would have to be thought more.

cc @infinisil

@MattSturgeon
Copy link

That one is kinda awkward, since it was already decided that recursively finding nix files to be formatted is a non-trivial issue that is kinda out-of-scope for a tool that just wants to be able to format a nix file.

IIRC, the main complexities come from ignoring .git and respecting .gitignore.

Maybe this could be worked around by renaming pkgs.nixfmt-rfc-style something like pkgs.nixfmt-unwrapped and having a wrapper package that handles the logic required to format a directory recursively?

The wrapper package could of course use an existing lib/tool to implement that. And treefmt users should probably be able to use either the wrapped or unwrapped package.

@eclairevoyant
Copy link

It seems quite trivial, actually; this decision comes off more like it's just intentionally making it harder to use flakes. Using a random third-party tool just to be able to use the official formatter with official nix is a nonstarter IMO. And there's some irony here that the nix docs still give the example of setting formatter.<system> to nixfmt... plus if we're talking about scope, nix even having a fmt subcommand is a strange scope creep, though it's certainly offtopic here to go further into that rabbithole.

In any case, as amusing as whining about nix* community dysfunction is, since I'd rather just get things done, fd is my tool of choice for this sort of thing:

fd -t f -e nix -x nixfmt '{}'

I specifically chose fd over find, as fd ignores hidden files/dirs and respects .gitignore the way one would hope nix fmt to operate. Also note this only includes regular files. And I do expect fd to be better-maintained, better-documented, and stabler in behaviour and API than treefmt or other third-party nix ecosystem tools.

To be able to effectively use nix fmt rather than hardcoding the formatter in the command, of course, one can substitute nixfmt with nix fmt in the command above; alternatively, since I'd rather the search-and-format details be properly encapsulated within the formatter package, I prefer to use a wrapper script around nixfmt via writeShellApplication:

  formatter.x86_64-linux =
    let
      pkgs = nixpkgs.legacyPackages.x86_64-linux;
    in
    pkgs.writeShellApplication {
      name = "nixfmt-wrapper";

      runtimeInputs = [
        pkgs.fd
        pkgs.nixfmt-rfc-style
      ];

      text = ''
        fd "$@" -t f -e nix -x nixfmt '{}'
      '';
    };

The args that are passed (to the formatter package's executable) appear to be the path(s) that are (im/ex)plicitly passed to nix fmt; if no args are explicitly passed, the CWD is implicitly passed. (Arguably it should be the flake enclosing the CWD, since that's the implicit flake that is passed to any other nix3 command by default, but...)

@GaetanLepage
Copy link
Author

Yes, the observation behind this issue was that is should be dead simple to format a flake the "official way".
It should not require to rely on external tool.
Again, in an ideal world, I would say that running nix fmt should work OOTB on any flake and would run nixfmt on the code base.
Of course, users would still be free to override the formatters flake output (so as to use treefmt-nix, alejandra...).

@eclairevoyant
Copy link

eclairevoyant commented Dec 28, 2024

Sure, I'm not entirely disagreeing that having nixfmt behave differently from the other formatters is surprising; I just wanted to share (with anyone watching this issue) a straightforward workaround that works today and will continue to work even if directory support is removed from nixfmt entirely.

@infinisil
Copy link
Member

infinisil commented Jan 7, 2025

Discussed in todays meeting (though we didn't fully read the previous discussion here):

It's possible to create a binary that runs treefmt with a default config, two options:

  • A separate binary $out/bin/nixfmt-tree in the same derivation that's a small wrapper around treefmt with nixfmt that supports all the same arguments as nixfmt but also supports directories. nixfmt-tree would be the meta.mainProgram.
    • Rough POC by @dasJ
      #!/usr/bin/env bash
      
      set -euo pipefail
      
      # Argument handling
      nixfmtArgs=()
      treefmtArgs=()
      noMoreNixfmt=0
      for arg in "${@}"; do
          if [[ "${noMoreNixfmt}" == 0 ]] && [[ "${arg}" == '-'* ]]; then
              nixfmtArgs+=("${arg}")
          elif [[ "${arg}" == '--' ]]; then
              noMoreNixfmt=1
          else
              treefmtArgs+=("${arg}")
          fi
      done
      
      # Build config
      if [[ "${#nixfmtArgs}" != 0 ]]; then
          nixfmtArgsStr="$(printf '", "%s' "${nixfmtArgs[@]}")\""
          nixfmtArgsStr="${nixfmtArgsStr:3}"
      else
          nixfmtArgsStr=
      fi
      set +e
      read -r -d '' cfg <<EOF
      [formatter.nix]
      command = "nixfmt"
      includes = ["*.nix"]
      options = [${nixfmtArgsStr}]
      EOF
      set -e
      
      exec treefmt --config-file <(echo "${cfg}") "${treefmtArgs[@]}"
    • Disadvantage: Will be a bit hacky (see above), and would not encourage people to switch to treefmt for customisations (such as passing extra flags, changing the included files or having formatters for other files)

  • A separate derivation pkgs.nixfmt-tree which just runs treefmt with a minimal treefmt.toml file that uses nixfmt on all Nix files, and includes nixfmt as a dependency. Flake users can set it as the formatter to get a good default experience. The meta.description can be along the lines of "treefmt/nixfmt wrapper as a stepping stone towards a custom treefmt.toml config"
    • Disadvantage: Separate derivation, adds mental overhead

See also NixOS/nix#11252 which is related

  • Alternative: Have Nix do the formatting filtering logic, already doing that as part of the Git input type. nixfmt needs to be in PATH
  • Alternative: nix fmt calls treefmt if there's no formatter set. If treefmt.toml doesn't exist, it uses a default one formatting Nix files with nixfmt. treefmt and nixfmt need to be in PATH

@nixos-discourse
Copy link

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/formatting-team-meeting-2024-12-10-and-2025-01-07/58466/1

@MattSturgeon
Copy link

I think I agree with @eclairevoyant that we should avoid tightly coupling nixfmt with treefmt. I prefer their proposal of a wrapper that uses something more generic like fd.

I'd propose having:

  • pkgs.nixfmt: a wrapper that calls nixfmt using fd
  • pkgs.nixfmt-unwrapped: the current nixfmt-rfc-style package.

The wrapper script could either be maintained in nixpkgs, or here in nixfmt. I don't think that's too important.

If the wrapper script is maintained here, then perhaps there's no need for separate wrapper packages?

I also think we should avoid going down the rabbit hole of changing how nix fmt behaves, at least for now, because that could open up a tonne of bike shedding...

@infinisil
Copy link
Member

The ultimate test is whether it works for Nixpkgs development, which the simple fd solution wouldn't because it takes too long (iirc multiple minutes). Treefmt is super valuable for this because it avoids running the formatter on files that weren't changed, runs the formatter in parallel, and also allows multiplexing to different formatters.

@jfly
Copy link

jfly commented Jan 7, 2025

+1 to not having nixfmt have the directory recursion logic. Giving people a convenient wrapper makes sense, and if we do it right, the implementation (fd vs treefmt) would just be a detail that users don't have to think about.

One minor thing to keep in mind: treefmt/treefmt-nix make an effort to have nix fmt do sane things:

  • nix fmt: format the whole flake, regardless of the current directory (this requires being able to discover the root of the flake)
  • nix fmt ./path/to/file: format ./path/to/file relative to the current directory

I believe neither @eclairevoyant's fd solution above nor @dasJ's POC treefmt wrapper handle this.

@infinisil
Copy link
Member

infinisil commented Jan 7, 2025

(All credit of the POC goes to @dasJ! Updated the meeting notes to point that out 😄 )

@MattSturgeon
Copy link

format the whole flake, regardless of the current directory (this requires being able to discover the root of the flake)

Correct me if I'm wrong, but with the current nix3 and flake implementation doesn't this require some pretty nasty hacks and/or assumptions?

No matter what treefmt-nix attempts, there isn't a deterministic way to accurately & reliably find the flake root. Right?

I assume using treefmt without treefmt-nix doesn't bring this baggage, though?

@jfly
Copy link

jfly commented Jan 7, 2025

Correct me if I'm wrong, but with the current nix3 and flake implementation doesn't this require some pretty nasty hacks and/or assumptions?

Yeah, that's what treefmt-nix's projectRootFile setting is all about: so it can discover and then tell treefmt where the tree-root is. Sure would be nice if nix did this for us.

And now I realize I've steered us towards that rabbit hole of bike shedding you mentioned above...

@nixos-discourse
Copy link

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/formatting-team-meeting-2025-01-21/59183/1

@infinisil infinisil linked a pull request Feb 24, 2025 that will close this issue
2 tasks
@infinisil
Copy link
Member

infinisil commented Feb 24, 2025

I've just opened NixOS/nixpkgs#384857 to mostly address this. Also #287 to make it more discoverable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Todo
Development

Successfully merging a pull request may close this issue.

6 participants