build-support: ShellCheck trivial builder output

[l0b0]

Motivation for this change

#133088

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

[KAction]

Looks reasonable, but I highly doubt that it will just work. shellcheck(1) doesn't understand that $out without quotes is okay, and I expect that to be used quite a bit.

[l0b0]

@KAction There's been a lot of discussion about variable quoting online, especially on Stack Overflow and the sister sites. Based on that it seems pretty clear that the expert consensus is that it's better to just always quote variables.

[SuperSandro2000]

@KAction There's been a lot of discussion about variable quoting online, especially on Stack Overflow and the sister sites. Based on that it seems pretty clear that the expert consensus is that it's better to just always quote variables.

For general scripts that is also right and correct but in nix we control the contents of certain variables like $out and it is ensured that they never contain spaces.

[l0b0]

@KAction There's been a lot of discussion about variable quoting online, especially on Stack Overflow and the sister sites. Based on that it seems pretty clear that the expert consensus is that it's better to just always quote variables.

For general scripts that is also right and correct but in nix we control the contents of certain variables like $out and it is ensured that they never contain spaces.

It looks like these are the choices:

1. Abandon QA: The used scripts in nixpkgs are not sane #133088, accepting a lower bar for the quality of the scripts here.
2. Quote $out, fixing the issue.
3. Add a # shellcheck disable=SC2046 line above every bareword $out, cluttering the source, but making sure this and other checks are performed everywhere else.
4. Run shellcheck --exclude=SC2046 or add disable=SC2046 to .shellcheckrc to disable this check globally, which means at least every other check will run.
5. Ignore the exit code of shellcheck, making the output informative.
6. Start slowly, first disabling all checks except for one, then introducing checks one by one to improve the quality gradually.
7. ?

What do you suggest? I'm sort of partial to introducing rules one by one, and I'm willing to help de-linting files en masse if there's a decent chance the result will be merged.

[zowoq]

I don't see how any of those address the rebuild and closure concerns?

#148947 (comment)
#148947 (comment)

[l0b0]

I don't see how any of those address the rebuild and closure concerns?

I've asked follow-up questions about that there, but didn't receive any answers. I'm just trying to move this forward. If anyone has suggestions for options 7 onwards (like adding a step in the PR template to please ShellCheck your scripts), feel free to suggest them.

[zowoq]

I already suggested one option which would also avoid any accidental breakage when shellcheck is bumped. #148947 (comment)

Sorry, misread you comment.

[zowoq]

TBH I just don't see this this approach moving forward.

Adding pkgs.shellcheck all over the place in nixpkgs isn't a good idea. Closure, rebuilds, breakage makes it too problematic.

It needs to be "external" check that we can run in PRs only e.g. add it to https://github.com/jtojnar/nixpkgs-hammering.

Actually already mentioned in jtojnar/nixpkgs-hammering#1