root: Add ShellCheck pre-commit configuration

[l0b0]

Motivation for this change

Existing issues requesting ShellCheck compliance: #133088, #21166.

pre-commit is a handy way to configure linters and formatters with a single configuration file. It can easily be set up (pre-commit install) to run them for you as part of a Git pre-commit handler, and then only runs on the files you are about to commit.

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual) N/A
• 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 N/A
• Tested basic functionality of all binary files (usually in ./result/bin/) N/A
• 21.11 Release Notes (or backporting 21.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.

[SuperSandro2000]

Using docker to lint your shell files is overkill. There are way simpler possibilities for probably most editors. I personally use shellcheck linting in vim and vscode through a plugin and extension. This also does not solve the problem that the end user still needs to install something, namely docker. I'd rather would like to see a shell.nix or a CI action instead of this.

[l0b0]

I didn't think pre-commit used or required Docker, though. Where do you see that?

[roberth]

pre-commit-hooks.nix may be a good option. It's a shell hook that configures pre-commit with a Nix-generated config. Also provides a derivation that runs the checks, for CI.

[mohe2015]

I didn't think pre-commit used or required Docker, though. Where do you see that?

https://github.com/koalaman/shellcheck-precommit/blob/master/.pre-commit-hooks.yaml I suppose

[l0b0]

I didn't think pre-commit used or required Docker, though. Where do you see that?

https://github.com/koalaman/shellcheck-precommit/blob/master/.pre-commit-hooks.yaml I suppose

Oh. I'll just change it to use language: system in that case. The pre-commit configuration is a bit unintuitive, but that just means it'll call the command in the entry value. This means the user has to install ShellCheck themselves to run the tests.

Update: Fixed. Thanks for the heads up @SuperSandro2000 and the details @mohe2015!

[l0b0]

@Artturin Is this something you're interested in reviewing?

[l0b0]

Ping @domenkozar @grahamc.

[l0b0]

Ping? This still seems relevant.

[roberth]

I don't have an opinion on adding impure pre-commit, so the following is just for context.

pre-commit-hooks.nix may be a good option.

Installation of hooks via shellHook should be opt in, so I would only recommend that project after

• Make hook installation optional cachix/git-hooks.nix#261

[roberth]

Still worth having. We now have a shell.nix that can serve as a substrate for such a hook.
This idea / PR could be of interest to @NixOS/nixpkgs-ci perhaps?

[MattSturgeon]

IIUC, nixpkgs doesn't want any automatically installed git hooks, e.g. via shellHook.

We could add manually installed hooks, but I'm unsure how useful that'd be. How would we promote and encourage their use, for example? What role would they play? Are they for running existing CI rules earlier (before push) or for introducing new rules?

I assume the hook's rules would also be enforced separately by CI? If so, what'd be the workflow for someone who wants to run the failing rules locally, without installing any git hooks?

I may sound skeptical, but I do think there is value in having pre-commit and/or pre-push hooks. I just think we need to consider rough edges that could add friction for contributors. Especially since the goal of such hooks should be to reduce friction.

[l0b0]

Closed because the majority of the feedback was negative. Which I don't quite understand, since at the core this is an opt-in feature with a single configuration file.

I work around this by running nix-shell -p deadnix nixfmt-rfc-style shellcheck statix and run each of them manually when I do nixpkgs development.

[roberth]

I believe certainly optional hooks should become part of the developer experience at some point.

I feel like a simple config with formatting only would already provide a net benefit.
Presumably treefmt can run quickly when pre-commit passes the list of changed files. Relying on nix-shell to "link" the tools should be fine; don't need the added complexity of git-hooks.nix for that.

Regardless of that, I think it's possible to configure pre-commit to use a config file in, say, .git/hooks/.pre-commit.yaml, where it doesn't get in the way, but we shouldn't have to rely on that. It's a double waste of potential that way.

What if the file came with a top comment that manages expectations and requirements based on the points brought up here, and lists @l0b0 as the file's primary maintainer?
That way we get the benefit of opting in to these hooks, without handing off more responsibility to the CI team than they're comfortable with.

[MattSturgeon]

I believe certainly optional hooks should become part of the developer experience at some point.

I feel like a simple config with formatting only would already provide a net benefit.

Agreed

Presumably treefmt can run quickly when pre-commit passes the list of changed files.

This is how treefmt works by default most of the time. After its first run, it caches the list of formatted files and only checks modified files in subsequent runs.

It can also be configured to ask git for the modified files instead of managing its own cache, iirc.

Relying on nix-shell to "link" the tools should be fine; don't need the added complexity of git-hooks.nix for that.

Agreed, I'd prefer to avoid a dependency on git-hooks.nix if possible.

Regardless of that, I think it's possible to configure pre-commit to use a config file in, say, .git/hooks/.pre-commit.yaml, where it doesn't get in the way, but we shouldn't have to rely on that. It's a double waste of potential that way.

Note that this can cause issues when working with git workftres. I often have to manually reinstall hooks when working in a worktree that is out of sync with the last worktree I installed hooks from.

This may be a fundamental issue with git hooks, though.

What if the file came with a top comment that manages expectations and requirements based on the points brought up here, and lists @l0b0 as the file's primary maintainer?
That way we get the benefit of opting in to these hooks, without handing off more responsibility to the CI team than they're comfortable with.

If we're starting off with a simple hook that only runs treefmt, then I suspect the CI team will be happy to help maintain it. At least, I don't object. But if @l0b0 is happy to be listed as a git hooks maintainer that's even better.

[l0b0]

If we're starting off with a simple hook that only runs treefmt, then I suspect the CI team will be happy to help maintain it. At least, I don't object. But if @l0b0 is happy to be listed as a git hooks maintainer that's even better.

I'd be happy to maintain .pre-commit-config.yaml, which I've already used in 10+ projects. (If we go with pre-commit, I want to avoid spending any time bikeshedding this. If people demand supporting many different workflows, or any features pre-commit doesn't provide by default, then I'm out.)

I've only started using treefmt this week, so I'm not qualified to configure or maintain it to the standard I would expect from nixpkgs. To be clear, I have no opinion on which one is more appropriate for nixpkgs, and I'd be happy using anything which improves on the current situation.

[MattSturgeon]

Nixpkgs is already using and configuring treefmt. It is made available in the default shell for the root directory of nixpkgs.

[roberth]

It can also be configured to ask git for the modified files instead of managing its own cache, iirc.

I expect this to be significantly faster. No need to re-stat everything when git has already done that work.

[wolfgangwalther]

My opinion on the matter:

• We need to separate the "shellcheck issue" from the "pre-commit hook" issue. I have looked into adding shellcheck into CI and treefmt and there is tons of work that needs to be solved first.
• We can look at a pre-commit hook considering only the current treefmt configuration.
• I really dislike pre-commit.com (and thus git-hooks.nix which is based on it). It doesn't support auto-formatting the staged changes - it only throws errors and then I have to do something about it. That's so annoying. Also, IIRC, it's based on all that stupid "pushing and popping stashes" logic, which is really.. annoying.

My goal for Nixpkgs is an opt-in pre-commit hook that will just do the right thing: Autoformat every commit with treefmt, fail on those things that can't be auto-fixed and work seamlessly across rebases, for example when the base branch had a huge reformat after a nixfmt update or so. Or put differently: Nixpkgs developers should never have to think about manually running treefmt again.

All of that can be done, and I have done all of that in different pieces for different (internal) projects already, it just needs to happen.

[domenkozar]

I really dislike pre-commit.com (and thus git-hooks.nix which is based on it). It doesn't support auto-formatting the staged changes - it only throws errors and then I have to do something about it. That's so annoying. Also, IIRC, it's based on all that stupid "pushing and popping stashes" logic, which is really.. annoying.

It does auto-format, but it throws an error so that git commit fails and you review the changes. If you blindly commit you're risking committing something where a check failed. If you decide to implement that, you'll have to know which tool behaves with 100% guarante of only formatting when it fails, which they don't.

My goal for Nixpkgs is an opt-in pre-commit hook that will just do the right thing

Yeah, if you only stick to only Nix formatting that's doable :)

[wolfgangwalther]

If you blindly commit you're risking committing something where a check failed. If you decide to implement that, you'll have to know which tool behaves with 100% guarante of only formatting when it fails, which they don't.

Not sure I follow. When I run treefmt, it will fix everything it can and only fail with exit code != 0 if any problem remains that is not auto-fixable.

The same thing will happen to the pre-commit hook: It will fix everything it can automatically, but if there's something that needs to be taken care of manually, it will fail, thus stopping the commit. No risk to commit something that would fail treefmt.

Yes, tools need to behave in a certain sane way, but the requirements are pretty simple, see https://treefmt.com/v2.3/reference/formatter-spec/

[MattSturgeon]

Not sure I follow

I think the issue isn't about whether something is auto-fixable or not, but about whether anything occurred at all.

If the pre-commit hook changed something, then the author should review the new diff and manually stage the new changes before committing. Just because something was changed automatically, doesn't necessarily mean it should be part of the current commit.

I don't have a solid example to hand, but it feels like committing with a partially staged diff (git add --patch) is one example that could result in potentially incorrect (or premature) treefmt results?

Another example my be cases where there are more than one acceptable formats. treefmt may default to one, but the author should review and consider if they prefer another. E.g. single-line vs multi-line lists.

---

As for pre-commit and git-hooks.nix, I'm also not a huge fan. Although it seems to be somewhat of a defacto standard.

If we could implement our own hooks with minimal effort and maintenance, that would be my preference. But I won't block using pre-commit as an initial starting point.

[wolfgangwalther]

If the pre-commit hook changed something, then the author should review the new diff and manually stage the new changes before committing. Just because something was changed automatically, doesn't necessarily mean it should be part of the current commit.

That's precisely where I disagree. Do you really check the results of every treefmt run manually? I don't. I trust treefmt and the underlying formatters to do the right thing. If a formatter can't be trusted this way, then it should not be included in our treefmt configuration to begin with.

I don't have a solid example to hand, but it feels like committing with a partially staged diff (git add --patch) is one example that could result in potentially incorrect (or premature) treefmt results?

Yeah, that's precisely the case where the "pushing and popping stashes" approach is really bad. git-format-staged uses a very different approach, described in this blog post. git-format-staged itself doesn't scale well, it's terribly slow because it doesn't parallelize at all. But the same approach can be taken and implemented efficiently, too.

[MattSturgeon]

Do you really check the results of every treefmt run manually? I don't. I trust treefmt and the underlying formatters to do the right thing. If a formatter can't be trusted this way, then it should not be included in our treefmt configuration to begin with.

I always look at the output "number of files formatted" as a sanity check. Usually I expect it to be a handful of files checked, and 0-1 modified.

Depending on the kind of changes I've been doing, I may check how they were formatted and use that to influence potential refactoring or tweaks, such as whitespace, comment positioning, single/multi-line lists, etc. Especially when using a heuristic formatter, like nixfmt, that can be influenced by the author's "intent".

I recall in 0f3f710 (#427437) you moving some auto-formatted comments 😉

If I'm doing a treewide reformat because the formatter itself has changed, then I'd be spot-checking the result just to feel confident in the formatter itself.

If I'm fixing someone elses badly formatted code, then I would only really care whether CI passes, so no real need to check the result in any detail.

---

TL;DR: if any tool has modified something as a result of my local changes, I'd like to know about it before committing. I don't ever want to blindly commit anything, unless maybe I'm intentionally doing a large treewide-style commit.

[wolfgangwalther]

Clearly, we need to support both ways to run this hook :D

[domenkozar]

I'd personally trust pre-commit here, because the tool went through many iterations over the years, so the experiernce you get isn't some random guys opinion :)

In general it's better to be explicit about squashing things into a commit, but if you want to roll your own solutions that's okay.

I'd still like if someone makes the hook installation optional as Robert attempted in cachix/git-hooks.nix#261

[wolfgangwalther]

I'd personally trust pre-commit here, because the tool went through many iterations over the years, so the experiernce you get isn't some random guys opinion :)

I do understand that, sure. At the same time, I have used an internal tool based on git-format-staged with auto-commit for 5+ years more than successfully.

[domenkozar]

I've made the PR so you can individually opt out of git hooks, for those that want to run things manually.