Crazy idea: run ShellCheck on scripts, mkDerivation, runCommand, etc.

[3noch]

What if we could easily run ShellCheck on scripts found in writeScript, mkDerivation's various steps, runCommand, etc. during instantiation? At the very least options could be added to these library tools that would do it. I think it would make developing nix derivations much easier.

[taktoa]

I have had this thought, and definitely agree that it would be good.

[grahamc]

I like the idea, and use shellcheck on all my builders. One difficulty (I think) is writeScript and mkDerivation and friends can't depend on shellcheck, because it is too complex and itself requires too many dependencies. Ie: a bootstrapping problem.

[abbradar]

@grahamc Do you have some kind of automated workflow for this? I'd like to run shellcheck much more often vs. I now do (i.e. always vs. almost never because I'm too lazy to copy *Phase {pre,post}* snippets somewhere; maybe I just need a macro for Emacs to run shellcheck on a region...).

[grahamc]

I sure do. I can post that in about 48 hours and will set myself a reminder. On Sun, Dec 25, 2016 at 4:49 PM Nikolay Amiantov notifications@github.com wrote:

@grahamc https://github.com/grahamc Do you have some kind of automated workflow for this? I'd like to run shellcheck much more often vs. I now do (i.e. always vs. almost never because I'm too lazy to copy Phase {pre,post} snippets somewhere; maybe I just need a macro for Emacs to run shellcheck on a region...).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/NixOS/nixpkgs/issues/21166#issuecomment-269140555, or mute the thread https://github.com/notifications/unsubscribe-auth/AAErrF6egSN4sLaOV1JRFzq9eV36xqc5ks5rLvMFgaJpZM4LNveG .

[grahamc]

and by 48 hours I mean 8 days... this is what I have:

rec {
mutatedScript = { stdenv, shellcheck }:
{ name, src, mutate, doCheck ? true }:
stdenv.mkDerivation {
    inherit name src doCheck;

    loc = "./${name}";

    unpackPhase = ''
      cp $src $loc
    '';

    buildPhase = mutate;

    postBuild = ''
     patchShebangs $loc
    '';

    checkPhase = ''
      ${shellcheck}/bin/shellcheck -x $loc
    '';

    installPhase = ''
     install -D -m755 $loc $out
    '';
};

  prenew = mutatedScript {
    name = "pre-new";
    src = ./pre-new.sh;
    mutate = ''
      substituteInPlace $loc \
        --replace "%isync%" "${isync}"
    '';
  };

}

and pre-new.sh:

#!/bin/bash

set -eux
set -o pipefail

%isync%/bin/mbsync tumblr

[3noch]

@grahamc This is awesome! Thanks. Regarding the "bootstrapping problem", how does nixpkgs currently handle things like coreutils? I'd imagine a similar problem exists for bash, gcc, etc.

[taktoa]

I believe some of those are fixed-output derivations. I suspect adding a fixed-output derivation containing a statically-linked ShellCheck binary to the Nix stdenv will massively enlarge the minimum closure size for Nix, which is quite undesirable.

A much better idea, I think, is to have a derivation called shellCheckDerivation that accepts a derivation as its only argument, and then uses runCommand to run ShellCheck on the builder (assuming the relevant values and environment variables are available as attributes, which should be possible). There are some confusing issues here regarding what parts of the script are available statically in the Nix environment, and which need to be computed at derivation-build-time, but I think they are resolvable with some careful thought.

[3noch]

Great idea!

[nh2]

I would also love to have a possibility to run shellcheck on everything.

Lost so much time to it already. Consider this from yesterday:

set -e
echo 'Trying to net-destroy mynet, this will fail if it doesn't exist'
nonexistent/bin/virsh net-destroy mynet || echo 'net-destroying mynet failed, probably it didn't exist'

shellcheck would point out what's wrong with that.

[baracoder]

(triage) Any updates on this?

[bignaux]

Would be fine to have checkPhase in pkgs/build-support/substitute/substitute-all.nix ? for the moment i need to use postInstall() ( see #82266 ).

[Fuuzetsu]

In case anyone is interested in user-land tool, I just uploaded https://github.com/Fuuzetsu/shellcheck-nix-attributes that I developed internally very recently. See README but roughly, it lets you run shellcheck on chosen string attributes of derivation at start of the build.

[stale[bot]]

Hello, I'm a bot and I thank you in the name of the community for opening this issue.

To help our human contributors focus on the most-relevant reports, I check up on old issues to see if they're still relevant. This issue has had no activity for 180 days, and so I marked it as stale, but you can rest assured it will never be closed by a non-human.

The community would appreciate your effort in checking if the issue is still valid. If it isn't, please close it.

If the issue persists, and you'd like to remove the stale label, you simply need to leave a comment. Your comment can be as simple as "still important to me". If you'd like it to get more attention, you can ask for help by searching for maintainers and people that previously touched related code and @ mention them in a comment. You can use Git blame or GitHub's web interface on the relevant files to find them.

Lastly, you can always ask for help at our Discourse Forum or at #nixos' IRC channel.

[nh2]

The idea and desire remain legitimate

[stale[bot]]

I marked this as stale due to inactivity. → More info

[nh2]

The idea and desire remain legitimate

[stale[bot]]

I marked this as stale due to inactivity. → More info

[ShamrockLee]

Maybe we could start from checking all the setup hooks with ShellCheck?

[nh2]

using an explicit allow list of shellcheck rules

I agree, if shellcheck doesn't already have an option to disable all stylistic rules and use only shell correctness rules, we should use an allowlist.

Given the recent controversy around liblzma it should be evident that running shellcheck on everything requires trust and it would be prudent to be able to not trust shellcheck.

Indeed it would be nice to be able to disable it without mass rebuilds. I don't know if that's possible.

Regarding trust, it has pros and cons. One could equally argue that without shellcheck it is even easier to add e.g. underhanded behaviour into nixpkgs, e.g. with lack of correct shell quoting, which shellcheck does point out. E.g. chown $mypath vs chown "$mypath", where the latter also allows $mypath to be ; arbitrary other code.

[3noch]

Indeed it would be nice to be able to disable it without mass rebuilds. I don't know if that's possible.

Content-addressed derivations should help a lot for the subset of Nixpkgs that's actually reproducible. For FODs, we could rely on impure environment variables to enable/disable.