ci: run shellcheck and shfmt on shell scripts

[wolfgangwalther]

There have been various efforts for single files to keep them shellcheck'ed and nicely formatted. But none of that will persist without CI.

So let's do that. Here's a proposal to start discussion.

Assuming that the shellcheck part is less controversional than the shfmt part, I added the latter as a second commit on top.

CC @emilazy @ShamrockLee with whom this came up in discussions lately.
CC @NixOS/nix-formatting even though this is not about nix files, you might still have an opinion?

Note for how to look at the changed files:

• The changes in stdenv/generic/setup.sh are all how shfmt works (except the very top lines about shellcheck).
• All the other setup hooks show how using shellcheck with "dependency declarations via source=" would look like.

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:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• 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.

[emilazy]

Just to make sure we don’t have a formatting PR without any bikeshedding going on, I would be really happy if we could standardize on 2‐space indentation for our shell files, since our shell in Nix is usually 2‐space indented to match Nix indentation (anything else would look very strange with nested interpolation), and it shouldn’t be harder than necessary to move shell from Nix into separate files; I see no downsides to consistency there.

I haven’t read the implementation here in great detail, but I am highly in favour of the principle. One thing I did notice on a quick skim is that *.bash files are getting ignored right now, and we do have some of them. We should either standardize on one extension (and lint against the other in CI?), or process them both.

Also, it’s nice to be able to run CI checks locally without relying on pushing to GitHub. It would be good if the logic could be implemented in a script in the ci directory that the GitHub Actions workflow calls out to. Things like nixpkgs-vet already work like this.

[wolfgangwalther]

Just to make sure we don’t have a formatting PR without any bikeshedding going on, I would be really happy if we could standardize on 2‐space indentation for our shell files, since our shell in Nix is usually 2‐space indented to match Nix indentation (anything else would look very strange with nested interpolation), and it shouldn’t be harder than necessary to move shell from Nix into separate files; I see no downsides to consistency there.

No objection against 2 spaces from my side, for now I went with 4, because that's what .editorconfig already has:

nixpkgs/.editorconfig

Lines 38 to 40 in f946d35

	# Match perl/python/shell scripts, set indent width of four
	[*.{pl,pm,py,sh}]
	indent_size = 4

I haven’t read the implementation here in great detail, but I am highly in favour of the principle. One thing I did notice on a quick skim is that *.bash files are getting ignored right now, and we do have some of them. We should either standardize on one extension (and lint against the other in CI?), or process them both.

I guess adding *.bash should not be a problem. I wonder about the files without extension, though. Could it be that we have any of those, marked as executable with a bash shebang?

Also, it’s nice to be able to run CI checks locally without relying on pushing to GitHub. It would be good if the logic could be implemented in a script in the ci directory that the GitHub Actions workflow calls out to. Things like nixpkgs-vet already work like this.

I tried to leave all logic out of it - and it worked quite well. All settings are in config files. Similar to nixfmt-check, the CI job prints the command needed to fix this inside nix-shell. The commands are either shfmt -w <path> ... or shellcheck <path> .... So it's really simple to call right now, not sure whether we do need a wrapper for that.

[emilazy]

No objection against 2 spaces from my side, for now I went with 4, because that's what .editorconfig already has:

nixpkgs/.editorconfig

Lines 38 to 40 in f946d35

	# Match perl/python/shell scripts, set indent width of four
	[*.{pl,pm,py,sh}]
	indent_size = 4

Yeah, I just figure if we’re doing a big reformatting commit that needs to go in .git-blame-ignore-revs anyway we might as well fix this inconsistency :)

I guess adding *.bash should not be a problem. I wonder about the files without extension, though. Could it be that we have any of those, marked as executable with a bash shebang?

Yeah, rg -g '!*.{sh,bash,md}' '^#!.*\b(ba)?sh\b' turns up some candidates.

I tried to leave all logic out of it - and it worked quite well. All settings are in config files. Similar to nixfmt-check, the CI job prints the command needed to fix this inside nix-shell. The commands are either shfmt -w <path> ... or shellcheck <path> .... So it's really simple to call right now, not sure whether we do need a wrapper for that.

Fixing might be easy, but I think there’s still no easy command you can run locally to determine “will CI complain about my PR”, right? That’s what a script in ci would be nice for, as it would let people check their work before running CI, set up pre-push hooks, and so on.

[l0b0]

One thing I did notice on a quick skim is that *.bash files are getting ignored right now, and we do have some of them. We should either standardize on one extension (and lint against the other in CI?), or process them both.

Bash is a specific shell, just like Korn, Zsh, csh, etc. Using the "sh" extension for scripts which use a specific syntax is at best confusing, and at worst inviting bugs. Another (minor) advantage of using the "bash" extension is that we won't need # shellcheck shell=bash comments to tell ShellCheck what the script is.

[emilazy]

I agree that standardizing on .bash makes sense.

[wolfgangwalther]

I agree that standardizing on .bash makes sense.

I'd say:

• All shell scripts referenced via nix use .bash (there are no other shells used inside nixpkgs, right?)
• All stand-alone scripts remove the extension. Essentially all those with +x. Not sure whether there are more than maintainers/scripts/... and some in ci/....
• Thus, no .sh should remain - we could even add a simple ci check for that.

I think we should apply the no-extension-for-executables to all scripts (perl, python, ...) in maintainers, too. Everything that is executable, should "identify" via shebang anyway.

[emilazy]

I think some editors don’t work properly with shebangs and no file extensions, which is a factor we should probably consider.

[wolfgangwalther]

I think some editors don’t work properly with shebangs and no file extensions, which is a factor we should probably consider.

True. Especially with the more complex nix-shell shebang, which only references bash on the second line. Actually.. happens for me, too :).

So.. all bash = .bash then, but still a "no .sh" check?

[emilazy]

That sounds good to me, although I can imagine edge cases where you would want a bona fide .sh (e.g. I was working on some kexec stuff where I restricted the boot script to POSIX shell for portability).

[wolfgangwalther]

That sounds good to me, although I can imagine edge cases where you would want a bona fide .sh (e.g. I was working on some kexec stuff where I restricted the boot script to POSIX shell for portability).

Ok, so maybe the check needs to be slightly more elaborate: "shebang must match file extension"?

[emilazy]

SGTM. But I’m also okay just mandating .bash and dealing with .sh when we come to it.

[risicle]

Wait, another pair of tools that we've all got to integrate into our workflows to continue contributing to nixpkgs? Fun. The weight of all this nonsense is making me too tired to want to continue.

[emilazy]

Shell is really error‐prone and problems caused by Bash foot‐guns in Nixpkgs code are not uncommon; especially for stdenv code and hooks it’s pretty important to have it checked given the state of the language we have to use. It’s the same checks that are applied with writeShellApplication.

[ShamrockLee]

shellcheck-minimal is the ShellCheck derivation writeShellApplication uses, which is supposedly easier to bootstrap.

Suggested change

	shellcheck
	shellcheck-minimal

[ShamrockLee]

shfmt seldom complains, which reduces the integration effort. However, it has some known limitations about variable names and assignment-like expressions (mvdan/sh#858 (comment)). For example the following would fail:

• export {a,b}=c (alternative: export a=c b=c)
• (($FOO = BAR)) (alternative: (($FOO == bar)) or ((FOO = BAR)))
• @SUB@=VAL, PREF_@SUB@_SUFF=VAL

The @SUB@ one is especially tricky. pkgs/development/interpreters/python/hooks/setuptools-rust-hook.sh is a living example of such limitation. It can be worked around using sed 's/@\([A-Za-z0-9]*\)@/_\1_/g' at the cost of losing the file name in the error message.