treewide: remove nixfmt from update scripts' shebangs

[wolfgangwalther]

We expect all of these update scripts to be run inside a nixpkgs' nix-shell environment, which will provide the correct nixfmt including treefmt configuration.

We already did this as part of b68cc63, but more occurences sneaked in or were left over.

Things done

• Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

---

Add a 👍 reaction to pull requests you find important.

[MattSturgeon]

I'd still like to investigate having the update scripts load the nixpkgs shell themselves and/or detect whether they are in one, so that that can display a helpful error message. Expecting maintainers to just know they should already be in the shell is not ideal.

But for now, expecting maintainers to enter the shell themselves (or use direnv) is acceptable, and this PR brings consistency to the remaining update scripts.

LGTM

[wolfgangwalther]

Maybe it would be possible to replace the calls to nixfmt in these update scripts with nix fmt? This would not require the shell, right?

[MattSturgeon]

Maybe it would be possible to replace the calls to nixfmt in these update scripts with nix fmt? This would not require the shell, right?

That requires experimental features, so is probably a non-starter for some maintainers.

Additionally, (as with most flakes-commands) using nix fmt requires copying the current repo to store if the current repo's NAR hash is different (i.e. if the repo has changed, even if the formatter itself hasn't changed).

On the other hand, you can enter the shell once and continue to use the same formatter without re-evaluating it, regardless of whether your repo is dirty.

[wolfgangwalther]

That requires experimental features, so is probably a non-starter for some maintainers.

I don't think this part would matter, because these can be enabled one-off via CLI for that command. nix --extra-experimental-features nix-command fmt or so?

Additionally, (as with most flakes-commands) using nix fmt requires copying the current repo to store if the current repo's NAR hash is different (i.e. if the repo has changed, even if the formatter itself hasn't changed).

On the other hand, you can enter the shell once and continue to use the same formatter without re-evaluating it, regardless of whether your repo is dirty.

Once you know you have to do it, it's fine. But if you don't - you'll have literally no idea and will think this is broken. Copying the repo to the store is a small price to pay for that, I think.

It does have other benefits, too - it actually runs treefmt, not only nixfmt. This gives us more consistent results in case any other formatters are actually required for the change in question.

Although just replacing nixfmt with treefmt in these scripts will probably have the same effect. It's much easier to reason that just adding plain treefmt to this script as a dependency won't do much. And when maintainers read treefmt they hopefully draw the line to nix-shell quicker?

[MattSturgeon]

Unlike more user-interactive workflows and/or git hooks where copying nixpkgs to store would be noticeable and frustrating, an update script could probably get away it as it does other slow actions anyway. But the perfectionist in me would still like to explore other options before resorting to an unnecessary copy-to-store.

That said, nix --extra-experimental-features 'flakes nix-command' fmt could turn out to be the best simple solution to making the correct formatter available to update scripts.

I believe it is also compatible with #425551, so long as the CWD is somewhere within the nixpkgs working tree? It wouldn't work if the user was trying to run update scripts from outside their nixpkgs repo, of course.

Include an additional shebang to load the nixpkgs shell is another option, but isn't really compatible with #425551 because it'd have to reference the CWD.

---

For me I think the ideal solution looks like some combination of -E 'with import ../../../.. {}; mkShell { packages = [ <list of packages> ]; }' (proposed in #425551), together with exposing some CI-pinned stuff in the main pkgs tree, so that something like ci.treefmt could be included in such a shell.

That said, -E 'with import ../../../.. {}; mkShell { packages = [ <list of packages> ]; }' is much more verbose than -p <list of packages>, so I think we may struggle to get update-script authors & maintainers on board...

Or maybe we could expose the nixpkgs shell itself in the pkgs tree (pkgs.dev-shell)? This could then be overridden by update-scripts to add additional input packages.

[corngood]

Copying the repo to the store is a small price to pay for that, I think.

To me this doesn't seem like a small price.

@MattSturgeon all of your suggestions sound good to me.

That said, -E 'with import ../../../.. {}; mkShell { packages = [ ]; }' is much more verbose than -p , so I think we may struggle to get update-script authors & maintainers on board...

I personally wouldn't have a problem with this. I'd rather the script be explicit about its dependencies.

Removing nixfmt from dotnet/update.sh actually breaks it in some cases, because it's run indirectly from dotnet/update.nix which uses a pure shell:

  #! nix-shell -i ${runtimeShell} --pure ${drv} --keep UPDATE_NIX_ATTR_PATH

In fact, I would prefer if update.sh also used a pure shell. I just tested this, and it works:

diff --git a/pkgs/development/compilers/dotnet/update.sh b/pkgs/development/compilers/dotnet/update.sh
index 46486f9e309a..c5c3a007aae2 100755
--- a/pkgs/development/compilers/dotnet/update.sh
+++ b/pkgs/development/compilers/dotnet/update.sh
@@ -1,5 +1,5 @@
 #!/usr/bin/env nix-shell
-#!nix-shell -I nixpkgs=./. -i bash -p curl jq nix gnused nixfmt
+#!nix-shell --pure -I nixpkgs=./. -i bash -p curl jq nix gnused nixfmt cacert
 # shellcheck shell=bash
 
 set -Eeuo pipefail

[wolfgangwalther]

In fact, I would prefer if update.sh also used a pure shell. I just tested this, and it works:

+#!nix-shell --pure -I nixpkgs=./. -i bash -p curl jq nix gnused nixfmt cacert

This works by accident, because we don't use any configuration for nixfmt. Using nixfmt directly via nix-shell's shebang is not going to work in general. Formatters are configured via treefmt-nix and any formatters you get directly via their package lose that configuration.

We should not think about nixfmt anymore, but about "how can we run treefmt in update scripts?". This makes this a bit clearer, I think.

[corngood]

We should not think about nixfmt anymore, but about "how can we run treefmt in update scripts?". This makes this a bit clearer, I think.

I have no problem switching it to use treefmt, but we still have the same problem of getting the properly configured treefmt into the environment, right?

[wolfgangwalther]

Correct.

[corngood]

Not cleaned up yet, but this does seem to work:

diff --git a/pkgs/development/compilers/dotnet/update.sh b/pkgs/development/compilers/dotnet/update.sh
index c5c3a007aae2..ac7c639b79d0 100755
--- a/pkgs/development/compilers/dotnet/update.sh
+++ b/pkgs/development/compilers/dotnet/update.sh
@@ -1,5 +1,5 @@
 #!/usr/bin/env nix-shell
-#!nix-shell --pure -I nixpkgs=./. -i bash -p curl jq nix gnused nixfmt cacert
+#!nix-shell --pure -I nixpkgs=./. -i bash -E 'with import <nixpkgs> {}; mkShell { inputsFrom = [ (import <nixpkgs/ci> { nixpkgs = <nixpkgs>; }).fmt.shell ]; packages = [ curl jq nix gnused cacert  ]; }'
 # shellcheck shell=bash
 
 set -Eeuo pipefail

It'll use the same nixfmt as in nix-shell, but I believe that still won't be configured correctly?

treefmt also works in the shell, so I can switch it to that, but currently it's running nixfmt on a /tmp/ path and then copying the output. treefmt obviously doesn't like this.

[corngood]

This will break dotnet updates because this is called from a pure shell.

[corngood]

Suggested change

	#!nix-shell -I nixpkgs=./. -i bash -p curl jq nix gnused
	#!nix-shell --pure -I nixpkgs=./. -i bash -E 'with import <nixpkgs> {}; mkShell { inputsFrom = [ (import <nixpkgs/ci> { nixpkgs = <nixpkgs>; }).fmt.shell ]; packages = [ curl jq nix gnused cacert ]; }'

[wolfgangwalther]

This will break dotnet updates because this is called from a pure shell.

Would an alternative be to remove --pure from the other shell?

[corngood]

It seems better to be explicit about the dependencies. If the dependency is missing, it'll do a bunch of time consuming work and then fail without telling the user what they did wrong. Also if they happen to have the wrong treefmt/nixfmt in PATH already, it'll do something bad without failing.

[philiptaron]

Having a consistent shebang across all the update scripts is a meaningful increase in consistency and quality. Since it relies on an ambient nix-shell (or some other mechanism) all of the issues from #425551 apply.

In that issue, @infinisil suggests something like the following (with an appropriate number of ../'s):

#!/usr/bin/env nix-shell
#!nix-shell --expr 'with import ../../../../ci {}; with pkgs; mkShellNoCC { packages = [ <list of packages> ]; }

That accomplishes the following:

1. Pinned nixpkgs that gets updated regularly
2. Consistent config imports (no aliases)
3. Tools like treefmt and nixpkgs-vet that are configured for Nixpkgs use (only somewhat relevant for update scripts, but more can be added)

This all sums up, in my mind, to a greater chance that the update script will work.

[wolfgangwalther]

I'm not going to follow up on this, realistically. The underlying issue is tracked in #425551.