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.

Sign up for GitHub

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

Jump to bottom

[RFC 0101] Nix formatting #101

Closed
wants to merge 7 commits into from
Closed

[RFC 0101] Nix formatting #101

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8acc6e2
[101] nix formatting
happysalada Aug 17, 2021
f4c25e7
Update 0101-nix formatting.md
happysalada Aug 21, 2021
ac8274b
rfc101: add shepherd team and leader
happysalada Sep 23, 2021
564f9e8
[rfc101]: add infinisil as shepherd
happysalada Mar 10, 2022
e66eb4c
Update from hedgedoc
infinisil Apr 6, 2023
c040ff4
Update after today's meeting
infinisil Apr 18, 2023
afaa285
Updates from the meeting
infinisil May 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 65 additions & 0 deletions 0101-nix formatting.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
---
feature: nix_formatting
start-date: 2021-08-17
author: Raphael Megzari
co-authors: (find a buddy later to help out with the RFC)
shepherd-team: (names, to be nominated and accepted by RFC steering committee)
shepherd-leader: (name to be appointed by RFC steering committee)
happysalada marked this conversation as resolved.
Show resolved Hide resolved
related-issues: (will contain links to implementation PRs)
---

# Summary

[summary]: #summary

Decide on a recommended automated formatter for nix files in nixpkgs.

# Motivation

[motivation]: #motivation

Prevent debate around how things should be formatted.

# Detailed design

[design]: #detailed-design

The implementation of this RFC should include several parts

- Agree on an automated formatter. [nixpkgs-fmt](https://github.com/nix-community/nixpkgs-fmt) is proposed here.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That formatter would need to become a channel blocker for the nixpkgs-* channels (like e. g. nixpkgs-review is).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to propose Alejandra to be taken into consideration as well.

- If adopted, agree on a schedule to format nixpkgs codebase. Should it be done at once, or on a per package basis.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One good point made by Domen in NixOS/nixpkgs#121490 is that it should be done once, on a treewide basis, just before branch-off of a stable release: Otherwise backporting changes from unstable will almost always run into merge conflicts which have to be manually resolved.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If nixpkgs-fmt changes the content of values, I would consider this as a bug. Unfortunately due to how the internal data structures that rnix-parser is using, deal with multi-line strings and comments has been particularly challenging.

If somebody could extract the repro and post an issue to nixpkgs-fmt that would be great.

- Potentially agree on a hook to enforce formatting
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be implemented as a github action pretty easily. $(nix-build -A nixpgks-fmt)/bin/nixpkgs-fmt .

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is also treefmt, which might be used to exclude specific path to address problematic files.
Here is a test run of treefmt on nixpkgs.

List to consider for exclusion:


Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a few formatting choices still need to be discussed. One of the largest issues is probably where the trailing commands in attrsets should be nix-community/nixpkgs-fmt#248. The main arguments have probably already been said in that issue - I don't know if we should continue the discussion here or there.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a comment in the issue

# Examples and Interactions

[examples-and-interactions]: #examples-and-interactions

This section is not needed for this RFC.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if adding an exact specification of how files are formatted is too much work a few representative samples should at least be added here. I think that would be a fair compromise between effort and at least having some reference. Other than that the formatter at a specified commit is probably the ground truth.


# Drawbacks

[drawbacks]: #drawbacks

- Having a commit that changes the formatting, can make git blame harder to use. It will need `--ignore-rev` flag.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignore-rev can be set up convenient-ish-ly by contributors by running one to two git config commands. However it needs to be stated that GitHub doesn't support this feature at all. I'm guessing the web blame feature is quite frequently used.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made a pr for a .git-blame-ignore-revs file
NixOS/nixpkgs#162430

According to a github employee they're currently working on the web blame

Update: implementation of this feature is in progress
6 days ago

community/community#5033 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feature is now live on GitHub: https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view

- Every formatter will have bugs. An example of a bug for nixpkgs-fmt can be found [here](https://github.com/NixOS/nixpkgs/pull/129392)
Copy link
Member

@sternenseemann sternenseemann Aug 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another drawback is that, as it stands nixpkgs-fmt is unable to reformat indented strings in a way that doesn't change their content. As a result, a reformat of nixpkgs will trigger a mass rebuild (see NixOS/nixpkgs#121490). Sending the reformat through staging will be quite painful as we'll have to deal with constant merge conflicts in the staging-next phase.

This implies that the “ideal” solution I've outlined in my other comment may not actually be doable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alejandra gets multi-line strings done perfectly, plus only 94 out of 34079 derivations would need rebuilding (and for reasons that can be happily ignored, see footnote on the readme)


# Alternatives

[alternatives]: #alternatives

- Alternative formatter [nixfmt](https://github.com/serokell/nixfmt)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enforces short line lengths which is not ideal with src in fetchers.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If single string literal is too long, it is kept, so I see little problem with it. Can you provide example when it generates outright ugly results?

- Keep the status quo of not having an official formatter. The danger is that this creates discord within the nix community. On top of fragmented the community it can generate lengthy discussions (which do not advance the eco-system).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is also an alternative of explicitly discouraging any comments on formatting not directly based on coding style recommendations spelled out in the Nixpkgs manual.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say that it would be step in wrong direction. As far as I remember, nothing in nixpkgs manual says that your expression should contain newlines at all.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, discouraging is not the same as strictly forbidding, if you in good faith consider something an exceptionally unfortunate formatting decision, you could comment about that… with a link to a PR to the manual, obviously.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where would you draw the line? It should be explicit, otherwise one reviewer thinks one part is exceptionally unfortunate and the next thinks another part is exceptionally unfortunate.


# Unresolved questions

[unresolved]: #unresolved-questions

- Not sure how much work there is left on nixpkgs-fmt before most people would consider it ok to use. Not even sure how much it is actually used.
- Are there situation where automated formatting is worse than manual formatting ? Do we want to have exceptions to automated formatting ?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One major question mark for this RFC is the question of automatically generated source files. We have multiple tools currently used to automatically generate files in nixpkgs: hackage2nix, node2nix, quicklisp-to-nix, go2nix, bundix to name a few.

I think none of these generate code that any formatter is happy with at the moment. If they were to be reformatted, the changes would be rolled back the next time the files are regenerated.

I think the RFC needs to commit to one of the following solutions:

  • Update code generators to produce conforming code. Then the question becomes: Who is gonna do that? Is it feasible? Has it any advantage? The burden to update the code generators may be significant as you can cut quite a lot of corners when generating nix because whitespace is not syntactically significant.
  • Make exceptions for generated files. The question is how this could be implemented in a hassle-free way since some generated files (think *deps.nix) don't have a well-known location and may be moved around as well.
  • Forbid code generation. This could solve this problem as well, but would require a separate RFC which proves that it's doable. On additional incentive for such a change would be that it likely improves performance: We could use JSON files to encode the same information as before and Nix's JSON parser is must faster than the Nix parser itself.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to admit that updating code generation by putting the generator call inside a script and calling nixpkgs-fmt afterwards sounds like a nonzero but manageable amount of work.

On the other hand, maintaining generators to always be formatter-clean would create unncessary pushback to fixing some weird corner cases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO just fixup the generated files after they have been generated. Either the tool itself can do that, your pre-commit hooks, or the update script. Either of these works. If we enforce formatting of files then almost no file should be excluded. Especially not generated files as being able to read them is important. There is no excuse to treat those a black boxes that shouldn't be looked at.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original tool should be fixed to always comply, to avoid churn. Or we marke the file "no format". Or we do #109

happysalada marked this conversation as resolved.
Show resolved Hide resolved

# Future work

[future]: #future-work

What future work, if any, would be implied or impacted by this feature
without being directly part of the work?