terminal-emulators/kitty: Fix ShellCheck lints

[l0b0]

Motivation for this change

#133088, @Artturin.

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)
• 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/)
• 22.05 Release Notes (or backporting 21.11 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
  • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
• Fits CONTRIBUTING.md.

[Artturin]

This would be better as a hook
https://nixos.org/manual/nixpkgs/stable/#ssec-setup-hooks

Some examples
https://github.com/NixOS/nixpkgs/blob/master/pkgs/build-support/install-shell-files/default.nix

https://github.com/NixOS/nixpkgs/tree/master/pkgs/build-support/setup-hooks

https://github.com/NixOS/nixpkgs/search?q=makeSetupHook&type=

[l0b0]

@Artturin I've created #149934 and others to introduce ShellCheck in a bunch of places. Hopefully at least some of them can be merged without fixing a bajillion build scripts.

[zowoq]

This would be better as a hook
https://nixos.org/manual/nixpkgs/stable/#ssec-setup-hooks

Agreed.

shellcheck also needs to be pinned somehow (and consistently treewide, not per package), see #148845 where the package broke after we bumped shellcheck.

[l0b0]

shellcheck also needs to be pinned somehow (and consistently treewide, not per package), see #148845 where the package broke after we bumped shellcheck.

Shouldn't whoever bumps ShellCheck also fix the lints as part of the PR? That way the code is simpler. It just couples the ShellCheck update to the linting, which seems natural.

[l0b0]

This would be better as a hook https://nixos.org/manual/nixpkgs/stable/#ssec-setup-hooks

Some examples https://github.com/NixOS/nixpkgs/blob/master/pkgs/build-support/install-shell-files/default.nix

https://github.com/NixOS/nixpkgs/tree/master/pkgs/build-support/setup-hooks

https://github.com/NixOS/nixpkgs/search?q=makeSetupHook&type=

I don't really understand that documentation. I'm only up to part 33 of How to Learn Nix, and sentences like "Packages adding a hook should not hard code a specific hook, but rather choose a variable relative to how they are included." are still just gobbledygook to me.

Based on the example code I'm guessing I need something like this (thrown together):

pkgs/build-support/setup-hooks/shell-check.bash:

shellCheck() {
    shellcheck -- "$@"
}

pkgs/build-support/shell-check/default.nix:

{ makeSetupHook, tests }:

let
  setupHook = makeSetupHook { name = "shell-check"; } ../setup-hooks/shell-check.sh;
in

setupHook.overrideAttrs (oldAttrs: {
  passthru = (oldAttrs.passthru or {}) // {
    tests = tests.shell-check;
  };
})

and then I need to use shellCheck "$out/bin/foo" in checkPhase? Is that better than just shellcheck "$out/bin/foo" in checkPhase? Or, more likely, am I missing something important?

[zowoq]

Shouldn't whoever bumps ShellCheck also fix the lints as part of the PR? That way the code is simpler. It just couples the ShellCheck update to the linting, which seems natural.

In theory, sure. In practice, no. Depends entirely on how many fixes (and rebuilds) are needed. Likely this check will just end up being disabled selectively or outright unless it can be introduced in way that isn't going to cause potential problems in the future.

Also shellcheck isn't a isolated package, it's part of a package set (haskellPackages) that gets bumped every week or two as a separate workflow on the haskell-updates branch. Adding shellcheck as a dependency everywhere will eventually disrupt that workflow due to to many rebuilds, etc.

[Ma27]

If we start doing this everywhere, we'll effectively require the Haskell toolchain including GHC (which is btw the largest package in e.g. my system closure with ~1.5G) in the build-closure for each package. Also, I fully agree with @zowoq, this will cause even more rebuilds:

Adding shellcheck as a dependency everywhere will eventually disrupt that workflow due to to many rebuilds, etc.

I don't really understand why this shouldn't be part of a review/CI process. In theory, we'll only need to check this once when a PR is filed with the build scripts, right?

[l0b0]

Adding shellcheck as a dependency everywhere will eventually disrupt that workflow due to to many rebuilds, etc.

I don't really understand why this shouldn't be part of a review/CI process. In theory, we'll only need to check this once when a PR is filed with the build scripts, right?

That sounds like an excellent compromise. The vast majority of variable substitutions in embedded shell scripts are just paths or very simple strings, so there's probably a low chance of any reasonable configuration change to introduce a lint to a dependent script.

How would you get to the various phase scripts from the outside, though? Are they all part of the output?

[l0b0]

@Artturin @Ma27 @zowoq Thank you for the feedback! I've reduced the scope of this PR to just fix some ShellCheck lints.

[adamcstephens]

These changes are ok with me. I'm a bit concerned about tooling and editor support to maintain this moving forward, but otherwise should be safe to merge.

[l0b0]

@Artturin Would you be OK to review this?

[Artturin]

/nix/store/zx2jzw5j0lxigaq34wmyfkcia3n0cvxi-install-shell-files/nix-support/setup-hook: line 100: $1: unbound variable

I don't think this is worth setting like this because if someone changes one of the hooks but doesn't build kitty then the breakage will go undiscovered. Until the channels are updates (like what seems to have happened here)

a better place to start adding these would be pkgs/test/

example

diff --git a/pkgs/test/install-shell-files/default.nix b/pkgs/test/install-shell-files/default.nix
index aef5acc1de6b..27363175cfc3 100644
--- a/pkgs/test/install-shell-files/default.nix
+++ b/pkgs/test/install-shell-files/default.nix
@@ -5,7 +5,7 @@ let
     runCommandLocal "install-shell-files--${name}" ({
       nativeBuildInputs = [ installShellFiles ];
       meta.platforms = lib.platforms.all;
-    } // env) buildCommand;
+    } // env) ("set -o nounset\n" + buildCommand);
 in
 
 recurseIntoAttrs {

$ nix build ".#tests.install-shell-files.install-completion"
this derivation will be built:
  /nix/store/bblpr8v64b153v0p842hv7fp2dvppy59-install-shell-files--install-completion.drv
building '/nix/store/bblpr8v64b153v0p842hv7fp2dvppy59-install-shell-files--install-completion.drv'...
install-shell-files> /nix/store/zx2jzw5j0lxigaq34wmyfkcia3n0cvxi-install-shell-files/nix-support/setup-hook: line 100: $1: unbound variable

[l0b0]

I've removed the set -o nounset line - adding it to a more central test file should probably go into a separate PR. Relatedly:

• Isn't changing that line going to cause thousands of rebuilds?

• There are several other useful strictness directives; should we try to introduce several/all of the following?

  set -o errexit -o noclobber -o nounset -o pipefail
  shopt -s failglob inherit_errexit

[Artturin]

Changing the tests won't cause many rebuilds but fixing the issue will of course.

set -eu,set -o pipefail, and shopt -s inherit_errexit are already set setup.sh but for them to be set during the build i guess they should be set in default-builder.sh