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

flake: configure nix fmt #325575

Closed
wants to merge 1 commit into from
Closed

Conversation

lucasew
Copy link
Contributor

@lucasew lucasew commented Jul 8, 2024

Description of changes

Sets up the flake attribute that Nix uses in the nix fmt subcommand to use the RFC 166 Nix formatter.

Nixpkgs has a handful of formatters, this PR basically just make it explicit which is the right one for the project.

cc NixOS/rfcs#166

Signed-off-by: lucasew [email protected]

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added the 6.topic: flakes The experimental Nix feature label Jul 8, 2024
@lucasew lucasew marked this pull request as ready for review July 8, 2024 16:02
@superherointj
Copy link
Contributor

superherointj commented Jul 8, 2024

While this makes sense. I wonder if this is going to be automatically invoked somewhere?

Remaining files to be formatted:
image

This can't be used effectively until nixpkgs gets formatted first.

My naive wish: A trusted contributor would nixfmt format nixpkgs fully (in a single PR). We would re-check nixfmt execution. Test in a separate branch in Hydra. If nothing is breaking, as method is working, apply it to all branches. Then, we are done with this annoyance of formatter in reviews. [Worst case it's a single PR revert.] Picture that.

@superherointj superherointj requested a review from Mic92 July 8, 2024 16:46
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Yeah as @superherointj explains, it doesn't really make sense to do this right now. The next step is to format (pretty much) all of Nixpkgs and enforce it for all new files with CI, like I'm doing in #322537. Even after that we still can't have nix fmt, because there's going to be PRs that get merged with unformatted files for a while still (because CI doesn't run unless you update a PR). This should fizzle out over the following weeks/months though, and at some point there won't be any PRs that haven't been running with CI that enforces formatting, at which point we can fully enforce it and use nix fmt :)

Beyond that, there's other minor problems with this:

@lucasew
Copy link
Contributor Author

lucasew commented Jul 8, 2024

Right now there is PR reviews asking the contributors to format the code using the formatter. My intention for this PR is just to make it easier to run this formatting step.

The whole repo is not fully formatted today because, as you said, there are some stuff still to be decided. I think until there this can be a little more informal, like, format new stuff now.

BTW about ops, I'd suggest doing it like how it's done with EditorConfig, a CI step that then can take it's 40s without issues just to warn the contributor that stuff is not formatted. No precommit hooks. It would be simpler, and can be out of scope of this PR.

@infinisil
Copy link
Member

#322537 is not far from being done, at which point there will be CI that takes some time until it tells the user about what files need to be formatted :)

Btw would be great for anybody that wants to help out with this to join the Nix Formatting Matrix room. And for such PRs, please ping the @NixOS/nix-formatting team :D

@infinisil infinisil mentioned this pull request Jul 11, 2024
7 tasks
@lucasew
Copy link
Contributor Author

lucasew commented Sep 11, 2024

I guess this should be closed then

@lucasew lucasew closed this Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants