Automatically formatting nixpkgs

domenkozar commented 3 years ago

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 https://github.com/NixOS/nixpkgs/issues/118661#issuecomment-827403677

FRidh commented 3 years ago

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

Synthetica9 commented 3 years ago

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 commented 3 years ago

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:

have a strict linter
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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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

Ma27 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

FRidh commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

domenkozar commented 3 years ago

Nice! Feedback:

not sure if github token has permissions to write to that branch for contributors that don't have write permission: https://github.com/Synthetica9/nixpkgs-format-testbed/pull/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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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

Wrong link, should be https://github.com/NixOS/nixpkgs/pull/121306

domenkozar commented 3 years ago

Love the progress so far, keep it coming :raised_hands:

Synthetica9 commented 3 years ago

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

Shown here:

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 commented 3 years ago

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 commented 3 years ago

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 (https://github.com/NixOS/nixpkgs/issues/120832#issuecomment-830272796)

nixos-discourse commented 3 years ago

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 commented 3 years ago

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:

asymmetric commented 3 years ago

@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 commented 3 years ago

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 commented 1 year ago

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 commented 1 year ago

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

tomodachi94 commented 3 months ago

Related: https://github.com/NixOS/rfcs/pull/166 and (perhaps this is superseded by) https://github.com/NixOS/nixfmt/issues/153

infinisil commented 2 months ago

This is being worked on, check out https://github.com/NixOS/nixpkgs/issues/322520 for future tracking :)

NixOS / nixpkgs

Automatically formatting nixpkgs #120832