-
-
Notifications
You must be signed in to change notification settings - Fork 158
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
[RFC 0101] Nix formatting #101
Conversation
May I note that we can not reformat the whole codebase just yet because it will just result in conflicts between 21.05 and unstable branches? I believe the preferable time to do so is right before 21.11 branch off. Food for thought:
|
[alternatives]: #alternatives | ||
|
||
- Alternative formatter [nixfmt](https://github.com/serokell/nixfmt) | ||
- 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). |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit torn on this one. On the one hand, I feel like having a formatter is just something we'll have to get over with, but at the same time I'm sceptical it'll solve the problems we are having and may have some risks:
- Formatting is not the only point of contention. There's also endless discussion about other aspects of nix expressions that have little to no actual consequence, e. g.
meta = { maintainers = [ lib.maintainers.xyz ]; }
vs.meta = with lib; { maintainers = with maintainers; [ xyz ]; }
. To fix these issues once and for all we would need to have a full blown coding style guide. - Depending on the implementation it may become more cumbersome to contribute (for the first time), especially if you have to install extra software. A PoC of a GtiHub action that does formatting would probably be useful here.
I also want to echo @7c6f434c here: We should also consider establishing an explicitly liberal formatting and code style policy: nixpkgs is a project of many occasional contributors who we can't realistically expect to familiarize themselves with a lengthy contributing guideline (already first time contributors often miss the commit format section).
0101-nix formatting.md
Outdated
[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 ? |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
||
[drawbacks]: #drawbacks | ||
|
||
- Having a commit that changes the formatting, can make git blame harder to use. It will need `--ignore-rev` flag. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
0101-nix formatting.md
Outdated
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. | ||
- If adopted, agree on a schedule to format nixpkgs codebase. Should it be done at once, or on a per package basis. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
0101-nix formatting.md
Outdated
[drawbacks]: #drawbacks | ||
|
||
- Having a commit that changes the formatting, can make git blame harder to use. It will need `--ignore-rev` flag. | ||
- Every formatter will have bugs. An example of a bug for nixpkgs-fmt can be found [here](https://github.com/NixOS/nixpkgs/pull/129392) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
0101-nix formatting.md
Outdated
|
||
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. |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
I actually think that style guide with «please fix any violations in freshly-changed parts if asked, please do not ask to change formatting without a guideline» could be efficient at shutting down the improperly placed discussions about style regardless of whether the guide is short or excessively long or in-between. (I probably prefer a short one). One thing that has a high risk of relying on semantically described guidelines even with a formatter is vertical whitespace — I think there is a preference to group things like version/pname/source — build parameters — meta, with empty lines in between, and I have some suspicion some prefer this tradition to stay but an automated procedure won't enforce ensure this well enough. |
For
|
Yep, |
0101-nix formatting.md
Outdated
|
||
[alternatives]: #alternatives | ||
|
||
- Alternative formatter [nixfmt](https://github.com/serokell/nixfmt) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
Co-authored-by: Sandro <sandro.jaeckel@gmail.com>
+1 from me on just applying
Agreed, but best not to let the best be the enemy of the good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm glad somebody is trying to take nixpkgs-fmt to the finish line. Thanks!
0101-nix formatting.md
Outdated
|
||
- Agree on an automated formatter. [nixpkgs-fmt](https://github.com/nix-community/nixpkgs-fmt) is proposed here. | ||
- If adopted, agree on a schedule to format nixpkgs codebase. Should it be done at once, or on a per package basis. | ||
- Potentially agree on a hook to enforce formatting |
There was a problem hiding this comment.
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 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0101-nix formatting.md
Outdated
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. | ||
- If adopted, agree on a schedule to format nixpkgs codebase. Should it be done at once, or on a per package basis. |
There was a problem hiding this comment.
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.
Who wants to shepherd for this RFC? |
I would be up for it. I am biased as one of the nixpkgs-fmt author of course. |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
I can shepherd as well if you still need a few more. |
Ok. We still need at least one more shepherd for this. |
I nominate myself as a shepherd to fill in the blank. |
While I support a standard formatter in nixpkgs, I have some gripes with nixpkgs-fmt. It does not print syntax errors when formatting, instead it goes on and removes any syntax after the line where the error is made, making it very confusing and frustrating to use. |
The sheperds for this RFC are zimbatm, nrdxp, 0x4a6f, with @zimbatm as the shepherd leader. |
There's been quite extensive work on the rules and tooling since the shepherd team took over the RFC, if you look at the (excessively detailed IMO) appendices in https://github.com/nix-rfc-101/rfcs/blob/master/rfcs/0101-nix-formatting.md. |
I'm aware, but
suggests that it is still in flux. My concern isn't ‘nobody will ever invest person-hours into working on rules’. My concern is if the team can't put together a rule set and say, ’Here it is, ready for your review; it isn't perfect and we expect to evolve it in the future but here's a complete specification of what correctly-formatted means,’ then we may be granting authority to a team that is more invested in the journey than the destination. And no, I don't consider ‘whatever the current implementation happens to do right now’ an adequate specification for this. |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/nixcon-governance-workshop/32705/3 |
I find this gatekeeping mildly offensive, and I am not even someone who has spent a lot of effort working on this. It's pretty clear that those working on the rules do not share this view, or they would have pushed for the use of one of the existing formatters instead of dedicating a non-trivial amount of their time updating and implementing rules into nixfmt. Personally, I am of the opinion that any formatter is better than no formatter. nixpkgs maintainers are already stretched too thin, and the status quo requires extra effort to maintain. Files must be manually formatted, pull requests go through extra reviews because of formatting, and there is no consistency across the code base. In short, we are wasting people's time needlessly without one. @rhendric I'm sure there would be plenty of space for you to help contribute to this effort, and I'd encourage you to do so if you feel so strongly about this topic. If you're not interested, or don't have the time to do so, instead I'd ask for you to make space for those who will be working on the implementation to make the decisions. This process, and the resulting formatter, as you point out will never be perfect, but putting up roadblocks is no more productive than rubber stamping a formatter. |
couldn't have said it better |
I apologize for any offense I caused. I thought the RFC process was intended to gather the opinions of the community. As a community member, I didn't think that expressing my opinion on an RFC would constitute ‘gatekeeping’. Also, I find it pretty odd to be accused of gatekeeping in a message that then proceeds to tell me to leave the conversation to the more active members. To your argument, bad code formatting standards impacts readability, which would also waste people's time when they are trying to understand what is going on in a large and complex project like Nixpkgs. The two time wastes, along with alternate mitigations for each, should be weighed against each other—I disagree that there is a no-brainer argument for implementing a formatter, any formatter. I'd go into more detail but you seem pretty upset that I would dare to share my thoughts here so I'll leave it at that. |
So you think the status quo, of inconsistent formatting across the codebase (along with the other negatives I listed above) is potentially lower than the cognitive load of an imperfect formatting? Seems like a good research track for an academic to study, but where I sit as a maintainer I fail to see how that helps here. I know that I spend time manually aligning and rearranging code, when a computer could do this much more efficiently. I've also reviewed and been subject to review of changes that alter formatting, requiring extra cycles through the review process. We have a lot of smart people who have worked on this, and when I encouraged you to join in I was hoping you could lend your expertise. But if you don't want to lend anything but negative roadblocks to what I consider a crucial need in the Nix ecosystem, I'm going to push back on that negativity.
We need positive contributions to make this happen, not dismissive statements about those with different opinions. If we can't move this RFC forward because of all the bike shedding over specific rules, then yes I want to delegate this responsibility (with accountability!) to a smaller more focused group. I definitely didn't tell you to leave it to others, but it's clear we're at odds here so maybe you missed my invitation for you to be more involved. |
I didn't intend to be dismissive of anyone. I'm sincerely sorry that I caused you to feel that way. Can we start over? piegames said:
I took that as an implicit question: is the community willing to let the implementation team cut this corner? If that question is addressed, in part, to me, as a community member, then my answer is, respectfully, no. That doesn't mean I think the people who say ‘yes’ are to be dismissed. Nor does it mean that I feel so strongly about this that I would throw myself into the machine to stop the gears of progress. I was asked a question and I gave an honest answer that happens to disagree with yours. That's a ‘positive contribution’, surely? I probably wouldn't have spoken up at all if this hadn't been prefaced in my inbox with a small flood of messages from people expressing the sentiment that any formatter is better than no formatter. I intended to communicate that I don't want the team to adopt that sentiment themselves. I wouldn't have expected them to prior to this flood of messages, but given the context—hey community, should we be less diligent so we can finish faster?—I thought that if all of the responses were ‘yes’, there was a good chance that they would take that response seriously and declare mission accomplished prematurely, even though prior to now that isn't what I would have expected from them. If you still think that's a ‘negative roadblock’, then I really don't understand how you expect community feedback processes to work. The team asks the community more or less a yes-no question, but only one answer is permitted because the other answer constitutes being negative? I don't get it. As for the object-level question, we might have to agree to disagree. I would certainly prefer a world in which a good-enough formatter is applied consistently to all of Nixpkgs to the world in which committer-maintainers have free reign within their domains. I only expect a small number of things out of a formatter for it to qualify as good-enough. I'm skeptical, though, that if you took the union of every maintainer's small-number-of-things, you would still have a small number of things—or even a consistent set of things. The best way, IMO, to demonstrate that this is possible is with an existence proof, and I would like to see that proof before I agree that it would be good for the project to apply a uniform format to Nixpkgs. I respect your right to disagree and voice your disagreement, as I would hope you do mine. |
(I do want to clarify that although you can probably fairly reasonably gloss my bottom-line opinion as "any formatter is better than none", I wasn't intending to express that with any of my comments here, as opposed to more tightly-scoped sentiments like "formatting standards that are technologically possible to use with the Nix of today are preferable to 5 more years of upstream prerequisite work" and "community consensus is important for high-level matters like whether to format at all and the principles of the formatting rules, but the minutiae of how to apply those to every single detailed case is better handled by a focused group of experts than indefinite public bikeshedding of a necessarily-incomplete natural-language specification". That said, I expect we do still have fundamental disagreements here all the same.) |
And for the record, I fully cosign the first, and mostly cosign the second with my only caveat being that I think the experts should come back to the public with those minutiae—but only so that the public can see the principles in action, ask questions that might not have occurred to them absent the minutiae, and give a general approval vote, not to start a new round of bikeshedding. |
I am going to throw my weight in on the side of "formatting rules don't matter". The biggest issue with Nixpkgs today is too many unmerged pull-requests. Automated formatting will streamline pull-requests, regardless of specific formatting rules. To that end, our focus when selecting a formatting tool should be stability and compatibility, not specific formatting rules. From my personal investigations, the most compatible and stable formatter is Alejandra. |
Nah, the most compatible and stable formatter is
Using If you think this is a good idea, then I think you are probably in favor of a different RFC than this one, unless this one basically gets a lobotomy. If you think this is a bad idea, then you do care (or, perhaps more likely, you're concerned that certain others care) about the set of formatting rules after all, because that's the only way |
If We should evaluate options in context of the purpose of this RFC. As of writing, the motivation section says:
Only the last bullet-point touches on specifics of formatting rules. I suggest, to move this RFC along, we split the specifics of formatting rules into a separate RFC for the creation of an official Nix style-guide. We can debate details of code-style there. This RFC can focus on automated formatting for Nixpkgs. When the official style-guide is ready, the formatter we choose will either implement it, or we will migrate to a formatter that does. |
How do you think any other formatter would streamline the pull request process? What do you think makes As I see it, the only thing that contributes to streamlining the pull request process is this step:
If the formatter is It might be a harder sell, you might object? You might have a harder time getting committers to agree not to argue with |
Well, there are people like me who agree that In the meantime, Nixpkgs has some formatting conventions that are bad for reading the code (the goal being to improve reading the diffs). Most formatters can probably be sold to uniformists (sadly |
2023-09-12 MeetingAttendees: @infinisil, @piegamesde, @0x4A6F (lurking)
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/proposal-require-non-committer-review-for-prs/33307/6 |
2023-09-26 MeetingAttendees: @infinisil @piegamesde @0x4A6F @tomberek Sergey (Serokell) Roman (Serokell) Agenda:
|
2023-10-10 MeetingAttendees: @infinisil @0x4A6F @Lucus16 Sergey @tomberek @piegamesde
|
2023-10-31 MeetingAttendees: @piegamesde @infinisil Sergey
|
2023-11-14 MeetingAttendees: @infinisil @piegames @tomberek @0x4A6F Sergey
|
Reopened as #166 |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/enforcing-nix-formatting-in-nixpkgs/49506/3 |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/satisfaction-survey-from-the-new-rfc-166-formatting/49758/54 |
automated formatting of nix files in nixpkgs
rendered