Skip to content

nixfmt-tree: refactor impl to use treefmt.withConfig #391319

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

Merged
merged 1 commit into from
Apr 1, 2025

Conversation

MattSturgeon
Copy link
Contributor

@MattSturgeon MattSturgeon commented Mar 19, 2025

Follow up to #390147 and #384857, this PR refactors the new nixfmt-tree package to make use of the new treefmt.withConfig wrapper.

This is technically a breaking change:

  1. Overriding settings will no longer entirely replace all settings, instead settings are now merged as modules.
  2. Adding the package to a nix shell will no longer make runtime inputs (such as nixfmt) available on the PATH, only the wrapped treefmt will be added to the shell's PATH.

There are also non-breaking changes:

  1. Introduced runtimeInputs, for consistency with treefmt.withConfig and other parts of nixpkgs, such as writeShellApplication.
  2. Introduced nixfmtPackage to allow specifying a different nixfmt package, without having to know to override nixfmt-rfc-style.
  3. Deprecated runtimePackages with a warning. Use the new args instead.

We could have it so that runtime inputs still end up being installed to $out/bin/, but this would negate much of the advantage of delegating most impl-details to treefmt.withConfig. In hindsight, it also feels like a bit of a leaky abstraction?

Fixes #395045

cc @infinisil @NixOS/nix-formatting

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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Mar 19, 2025
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.

LGTM!

@@ -62,7 +80,7 @@ buildEnv {
```

You can then also use `treefmt` in a pre-commit/pre-push [Git hook](https://git-scm.com/docs/githooks)
and `nixfmt` with your editors format-on-save feature.
and with your editor's format-on-save feature.
Copy link
Member

Choose a reason for hiding this comment

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

Yeah so this was the original motivation for including the formatter binaries as well, but as explained in the last formatting team meeting, people should be using treefmt even for format-on-save, because treefmt knows about the options you format the codebase with :)

@wegank wegank added 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages. 12.approvals: 1 This PR was reviewed and approved by one person. labels Mar 20, 2025
@MattSturgeon
Copy link
Contributor Author

Any objections to getting this merged @NixOS/nix-formatting ?

Copy link
Contributor

@jfly jfly left a comment

Choose a reason for hiding this comment

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

One thought for you inline. I trust your decision either way.

@MattSturgeon MattSturgeon force-pushed the nixfmt-tree branch 2 times, most recently from e4741ac to afedaef Compare April 1, 2025 11:32
@MattSturgeon MattSturgeon removed 12.approvals: 1 This PR was reviewed and approved by one person. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages. labels Apr 1, 2025
Refactor the new `nixfmt-tree` package, to make use of the new
`treefmt.withConfig` wrapper.

Breaking changes:
1. Overriding `settings` will no longer entirely replace all settings,
   instead settings are now merged as modules.
2. Adding the package to a nix shell will no longer make runtime inputs
   (such as `nixfmt`) available on the PATH, only the wrapped `treefmt`
   will be added to the shell's PATH.

Non-breaking changes:
1. Introduced `runtimeInputs`, for consistency with `treefmt.withConfig`
   and other parts of nixpkgs, such as `writeShellApplication`.
2. Introduced `nixfmtPackage` to allow specifying a different nixfmt
   package, without having to know to override `nixfmt-rfc-style`.
3. Deprecated `runtimePackages` with a warning. Use the new args
   instead.
@MattSturgeon
Copy link
Contributor Author

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 391319


x86_64-linux

✅ 1 package built:
  • nixfmt-tree

@MattSturgeon
Copy link
Contributor Author

Based on @jfly's feedback, I've reworked the runtimePackages -> runtimeInputs migrations a little:

I introduced a nixfmtPackage argument. This allows users to modify or replace the nixfmt package without having to explicitly override nixfmt-rfc-style. I'm open to alternative names or approaches here.

The (old) runtimePackages argument defaults to [ nixfmtPackage ] and the (new) runtimeInputs argument defaults to [ ].

The final runtimeInputs supplied to treefmt.withConfig is runtimePackages ++ runtimeInputs. Once runtimePackages is removed, that'll just be [ nixfmtPackage ] ++ runtimeInputs.

If a user explicitly overrides runtimePackages, they will see a deprecation warning:

nix-repl> pkgs.nixfmt-tree.override { runtimePackages = []; }
evaluation warning: nixfmt-tree: overriding `runtimePackages` is deprecated, use `runtimeInputs` instead.
                    Note: you do not need to supply a nixfmt package when using `runtimeInputs`, however you can override `nixfmtPackage` to a different nixfmt package.
                    For additional flexibility, or to configure treefmt without nixfmt, consider using `treefmt.withConfig` instead of `nixfmt-tree`.
«derivation /nix/store/l4xmpvzzwrmhxn3bmmicw0wpa2rh4ngh-nixfmt-tree.drv»

@MattSturgeon MattSturgeon requested review from jfly and infinisil April 1, 2025 12:09
Copy link
Contributor

@jfly jfly left a comment

Choose a reason for hiding this comment

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

LGTM! I was surprised to see the binary name "nixmt" still hardcoded in this PR, rather than using lib.getExe, but IIUC, it has to be this way for backwards compatibility. I believe this can get cleaner in the future when we drop support for backwards compatiblity.

Thanks for iterating on this!

@MattSturgeon
Copy link
Contributor Author

I was surprised to see the binary name "nixmt" still hardcoded in this PR, rather than using lib.getExe

Currently we're running nixfmt from the PATH; having added it to the PATH via runtimeInputs and makeWrapper.

Not adding nixfmt to the PATH and instead configuring its absolute path (via lib.getExe) is also an option. I think the migration for runtimePackagesruntimeInputs may be more work, or handle less edge-cases, but I'm open to trying it out if you prefer.

@jfly
Copy link
Contributor

jfly commented Apr 1, 2025

but I'm open to trying it out if you prefer.

No, let's merge this. We can revisit this in the future when we drop the deprecated codepath.

@wegank wegank added 12.approvals: 2 This PR was reviewed and approved by two persons. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages. labels Apr 1, 2025
@dasJ dasJ merged commit e812a70 into NixOS:master Apr 1, 2025
29 of 31 checks passed
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/formatting-team-meeting-2025-04-01/62524/1

@MattSturgeon MattSturgeon deleted the nixfmt-tree branch April 2, 2025 10:14
@awwpotato
Copy link
Contributor

awwpotato commented May 14, 2025

@MattSturgeon could this be backported to 24.11? EDIT: oops I meant #390147, but this would also be good. Also thinking about it more the branch off of 25.05 is in only a few days so it probably doesn't make sense to do a significant backport like this. Basically what I'm saying is ignore this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 12.approvals: 2 This PR was reviewed and approved by two persons. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nixfmt-tree: fails to format after overriding
8 participants