Automatically formatting nixpkgs

[domenkozar]

Is there an existing PR/issue regarding adding some sort of linting system to the CI to help avoid nitpicky manual reviews?

IIRC this was brought up a few times on IRC, but I don't know of any existing issue. However:

• IMHO it doesn't matter anyways whether we do e.g. callPackage ./. {} vs callPackage ./. { }.
• The last time this was discussed in #nixos-dev (IIRC) it was agreed that if we establish a formatter, it should be primarily used on new packages. Apart from the fact that formatting makes backports usually harder (because you'll run into way more merge conflicts), my experience is that most formatters (to be fair, I have to take a way closer look at both nixfmt and nixpkgs-fmt, but from what I've seen we'll have the problem here as well) make it harder to git blame a file. Despite the IMHO problematic name I generally use this to understand why a line of code looks the way it does when I don't understand it entirely.
  So what I want to say is, if we start using a formatter, I personally think that we should be careful with applying it to existing code.

Originally posted by @Ma27 in #118661 (comment)

[FRidh]

Discourse topic https://discourse.nixos.org/t/formatting-rules-for-nixpkgs/2233.

[Synthetica9]

There is also definitely something to be said for re-formatting all at once. It would make it easy to have an .git-blame-ignore-revs file to look past that commit when needed rather than having either inconsistent standards across nixpkgs or having a million billion tiny shard commits that format one file each

EDIT: The commit message for the reformat commit should probably explain "hey, if you're seeing this commit pop up in blame, run git config blame.ignoreRevsFile .git-blame-ignore-revs to hide it in the future". There should definitely be a lot of scrutiny on this commit message, since it's impossible to change in the future. Of course, this still breaks blame in github's web interface, but it should work with all local tools.

[sumnerevans]

I think the original reason for bringing this up was due to nitpicky formatting requests pushing away new contributors. Personally, I see there being two options:

1. have a strict linter
2. nitpicky formatting requests should be eliminated

One challenge with the latter option is that the definition of "nitpick" is not very clear.

Personally, I'm a fan of the "explicit over implicit" rule, so I would be in favor of a linter, but I understand the difficulties that would create with things like git blame.

[Synthetica9]

nitpicky formatting requests should be eliminated

I think that could be done by adding a GitHub workflow /format that formats all touched files with the blessed formatting tool. Maybe even do a filter-branch?

[grahamc]

I'm in favor of ripping off the bandaid, formatting everything, and starting a ignore revs file. We'd need to simultaneously reformat all of stable if we wanted to backport things. I've written a tool rebase a branch on top of a branch where the target branch was automatically formatted. It needs work, but is available as an answer for how to deal with existing PRs: https://github.com/grahamc/git-rebase-format

[domenkozar]

A good time to reformat would then be just before 21.05 branch off. It seems we agree with a single point in history to format things.

Does someone have time to test & prepare for formatting to happen on May 21st?

cc @jonringer

[jonringer]

I'm all in favor of this. I would prefer to do this before zhf and cause issues for contributors, which is a week away.

[domenkozar]

@jonringer the problem with doing it before is that it would make existing PRs conflict, wouldn't it be better to do it just before a new branch is formed?

Why do you think that's better?

[Synthetica9]

Do we even know which tool we're going to bless yet?

[Ma27]

Does that mean we consider the current state of nixpkgs-fmt as "single source of truth" for autoformatting (in nixpkgs at least)? Or in other words, what exactly do we want to achieve? I mean, we could also run a subset which satisifies most of us and causes less conflicts.

[domenkozar]

I'd like to get us to commit to doing it first, then we (take a limited amount of time to) pick the tool. I think details can be figured out, the tooling will evolve, we can exclude things that break, etc.

[jonringer]

I guess if the strategy is to format all files, waiting until branchoff is also fine. There's just more risk of delaying a release if a regression gets introduced. And changes of that scale make it difficult to capture regressions. Which is why I had a preference to do it before zhf.

But if the refactoring is done on another branch, and we're confident about it, then I'm okay with that as well.

[domenkozar]

I'm willing to take that risk by volunteering to fix issues if they happen between branch-off and release, related to formatting.

[FRidh]

nixpkgs-fmt (I haven't checked nixfmt) design choices and test case examples seem good to me. In that case, I suggest opening a PR asap describing ("copying") the design choices and formatting rules in the Nixpkgs manual contributing section, get that accepted and then do the formatting straight after, hopefully before 21.05.

[sumnerevans]

nixfmt is much more opinionated than nixpkgs-fmt. I think it may be too aggressive to be useful in nixpkgs considering the speed at which this repo is updated.

[Synthetica9]

I created a proof-of-concept to automatically format pr's, feel free to use and modify as needed:

• Example PR
• Workflow file

[domenkozar]

Nice! Feedback:

• not sure if github token has permissions to write to that branch for contributors that don't have write permission: test non-write contribution Synthetica9/nixpkgs-format-testbed#13
• also needs a way to format changed files locally, easiest to start with a script, but that's simple to add once we settle on it
• if nixpkgs-fmt changes the way something is formatted, it will auto-update - we probably want to pin it

[Synthetica9]

Nice! Feedback:

* not sure if github token has permissions to write to that branch for contributors that don't have write permission: [Synthetica9/nixpkgs-format-testbed#13](https://github.com/Synthetica9/nixpkgs-format-testbed/pull/13)

I do get a prompt to manually approve this in the webinterface.

* also needs a way to format changed files locally, easiest to start with a script, but that's simple to add once we settle on it

Wouldn't that just be nixpkgs-fmt?

* if nixpkgs-fmt changes the way something is formatted, it will auto-update - we probably want to pin it

Agreed!

[sumnerevans]

What are your opinions on something like https://github.com/reviewdog/reviewdog? It looks like it can create an actual GitHub review from the lint output complete with suggested fixes. It can work with diff, so you can just run nixpkgs-fmt on all of the modified files and it will give automated feedback: https://github.com/reviewdog/reviewdog#diff

I think this is less intrusive than actually committing to the PR.

[Synthetica9]

I don't know, ideally this shouldn't actually do anything on most PRs because people have nixpkgs-fmt setup to automatically trigger before they submit the PR. It's definitely worth considering tho!

[sumnerevans]

I just know that a lot of times I'll accidentally push an unformatted commit and quickly follow it up with a correctly formatted one. I don't want my second push to be a conflict with the automated bot.

[asymmetric]

@FRidh I started adding nixpkgs-fmt rules in this PR. I'm basing it on personal use of the tool and this suite of tests.

I'm not sure if I'll be able to catch them all though, and feedback is very welcome, before I spend too much time on it.

[Synthetica9]

I started adding nixpkgs-fmt rules in this draft PR.

Wrong link, should be #121306

[domenkozar]

Love the progress so far, keep it coming 🙌

[Synthetica9]

Status update: I got a /format command to work based on https://github.com/NixOS/nixpkgs/blob/master/.github/workflows/rebase.yml

Shown here:

• In action
• Action code

This uses git filter-branch, which is full of footguns, so this should definitely be checked thoroughly! However, this is a relatively simple use, so it should be okay. Very interested in feedback on this.

[jonringer]

If we decide on sticking with nixpkgs-fmt, then we should add nixpkgs-fmt as a release blocker to prevent potential PR gates failing
https://github.com/Synthetica9/nixpkgs-format-testbed/blob/f710e22ffd997c90ea168d98d89e5563e95122d9/.github/workflows/format.yml#L23

[Synthetica9]

If we decide on sticking with nixpkgs-fmt, then we should add nixpkgs-fmt as a release blocker to prevent potential PR gates failing
Synthetica9/nixpkgs-format-testbed@f710e22/.github/workflows/format.yml#L23

I have pinned nixpkgs to a specific revisions due to concerns expressed by @domenkozar (#120832 (comment))

[nixos-discourse]

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

https://discourse.nixos.org/t/formatting-rules-for-nixpkgs/2233/8

[abathur]

This may be something that is resolved by now, or doesn't apply in this context, but I recalled seeing an assertion in IRC semi-recently that nixpkgs-fmt may perpetually reformat some files.

It looks like the ~proof was https://github.com/Kiwi/lol-nix-formatter/commits/master/apache.nix but you can see the conversation as well:

• https://logs.nix.samueldr.com/nixos/2021-01-11#1610390743-1610390798;
• https://logs.nix.samueldr.com/nixos/2021-01-11#1610394873-1610395233;

[asymmetric]

@abathur the apache file formats nicely for me on 1.2.0.

This file has some syntax problems (flake = value lacks a ; and I think threre's a dangling }), so I'm not sure how useful it can be as a test.

Would be really good to follow-up on it though, to ensure nixpkgs-fmt is behaving correctly.

[jonringer]

I have pinned nixpkgs to a specific revisions due to concerns expressed by @domenkozar (#120832 (comment))

That's another solution. I'm just not as big of a fan when it comes to pinning. Generally this introduces additional friction when trying to update processes.

[Stunkymonkey]

I think this deserves a lot more attention. Maybe we could gradually migrate the packages by doing one folder a time and merge it in smaller batches.

[0x4A6F]

There is an ongoing RFC for this at NixOS/rfcs#101.

[tomodachi94]

Related: NixOS/rfcs#166 and (perhaps this is superseded by) NixOS/nixfmt#153

[infinisil]

This is being worked on, check out #322520 for future tracking :)