Skip to content

Fix shellcheck SC2148 and SC1008#146601

Draft
l0b0 wants to merge 360 commits intoNixOS:stagingfrom
l0b0:fix-shellcheck-sc2148
Draft

Fix shellcheck SC2148 and SC1008#146601
l0b0 wants to merge 360 commits intoNixOS:stagingfrom
l0b0:fix-shellcheck-sc2148

Conversation

@l0b0
Copy link
Contributor

@l0b0 l0b0 commented Nov 19, 2021

Motivation for this change

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

This change only prepares for running ShellCheck by indicating which shell ShellCheck should interpret the code (Bash). The same can also be achieved by renaming the scripts to .bash, but that would be much more involved and risky, possibly including changes to other repos which reference scripts by name.

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:
  • 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/) 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.

@l0b0 l0b0 requested a review from a user November 19, 2021 09:45
@github-actions github-actions bot added 6.topic: bsd Running or building packages on BSD 6.topic: emacs Text editor 6.topic: fetch Fetchers (e.g. fetchgit, fetchsvn, ...) 6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: golang Go is a high-level general purpose programming language that is statically typed and compiled. 6.topic: haskell General-purpose, statically typed, purely functional programming language 6.topic: kernel The Linux kernel 6.topic: lua Lua is a powerful, efficient, lightweight, embeddable scripting language. 6.topic: nim Nim programing language 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS labels Nov 19, 2021
l0b0 added 28 commits November 23, 2021 20:34
@l0b0
Copy link
Contributor Author

l0b0 commented Dec 4, 2021

I've set this as a draft, for several reasons:

  • Most importantly, a better solution would be to rename the files from .sh to .bash, which would clearly indicate to anyone reading it (including tools like ShellCheck) that the shell dialect matters, and that programmers should feel free to use bashisms to improve maintainability, speed, etc.
  • Someone could fairly easily create one PR per package, which would be much easier to get merged individually.
  • There seems to be no clear standard for shebang lines in this project. Some scripts seem to use #!/bin/sh when they shouldn't, because they contain bashisms. Others use #!/usr/bin/env bash, #!/usr/bin/env nix-shell plus #!nix-shell -i bash -p …, others again use #!@shell@ or #!@runtimeShell@. I don't know whether all of these forms are helpful, and if so, when they should each be used.

@Et7f3
Copy link
Contributor

Et7f3 commented Oct 17, 2022

#!@runtimeShell@ Allow to resolve bash at build time. So for wrapper I would prefer using that. For independant script that will not be rebuild /usr/bin/env bash seem better because it choose bash at runtime. #!/usr/bin/env nix-shell with --pure is even better because it show explicitly dependencies of a script but require more work to use this style so just usign env is fine if you only need tools in stdenv like sed/find/cat/cd/mv/...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2.status: merge conflict This PR has merge conflicts with the target branch 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: bsd Running or building packages on BSD 6.topic: emacs Text editor 6.topic: erlang General-purpose, concurrent, functional high-level programming language 6.topic: fetch Fetchers (e.g. fetchgit, fetchsvn, ...) 6.topic: golang Go is a high-level general purpose programming language that is statically typed and compiled. 6.topic: haskell General-purpose, statically typed, purely functional programming language 6.topic: kernel The Linux kernel 6.topic: lua Lua is a powerful, efficient, lightweight, embeddable scripting language. 6.topic: nim Nim programing language 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: ocaml OCaml is a general-purpose, high-level, multi-paradigm programming language. 6.topic: printing Drivers, CUPS & Co. 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: qt/kde Object-oriented framework for GUI creation 6.topic: rust General-purpose programming language emphasizing performance, type safety, and concurrency. 6.topic: stdenv Standard environment 6.topic: steam Steam game store/launcher (store.steampowered.com) 6.topic: TeX Issues regarding texlive and TeX in general 6.topic: vim Advanced text editor 6.topic: xfce The Xfce Desktop Environment 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants