@evujumenuk, thanks for your PR! By analyzing the history of the files in this pull request, we identified @urkud, @Ericson2314 and @edolstra to be potential reviewers.

for what it's worth, I'm not sure if anything actually uses writeShellScriptBin in nixpkgs. A cursory grep reveals no references to it.

Right. AFAIU this functionality is not yet used because it necessitates a mass rebuild (#19050).

because it necessitates a mass rebuild

That's not a problem because then it just goes via staging.

So, should this be merged into staging instead?

Since nothing currently calls writeShellScriptBin in master, merging this to master should not trigger a mass rebuild, right? If we were to make stdenv use it or something, that would be the part to go to staging first.

Yeah, as a stdenv/platform developer, my life would be significantly less happy if I had to build GHC from scratch a few times every time I changed something. I do however think it would be nice to have an additional layer of validation we could opt into during development somehow, but I'm not sure what that would look like.

Maybe I'm misunderstanding a thing or two about the whole 'mass rebuild' thingamajig, but #19050 is linked to from the Pending Mass Rebuilds page because it modifies writeTextFile. This PR does not, thus a mass rebuild should not be necessary.

The aforementioned PR was, in the end, merged to master, not staging.

I could introduce another builder (e.g. writeShellScriptBinShellchecked) that includes shellcheck in its checkPhase, and have writeShellScriptBin continue using sh -n. With that, we'd be able to use writeShellScriptBin where it's sensible to do so, and still provide a ready-made solution for stricter checking during development. With a "soft disallow" on writeShellScriptBinShellchecked in PRs, build times should not suffer (because evaluation of ${shellcheck} would never be forced outside of development). Correct?

Would this be an acceptable way forward?

That's not really my point here, it's currently not used anywhere so this is indeed not a mass rebuild. However if people start using it (that's the point 😄) this will trigger rebuilds each time something in the haskell infrastructure changes. And if somebody doesn't know this it could even end up in the stdenv, etc.
The current check isn't as nice, but it can be used safely everywhere.

Forgive my ignorance, but wouldn't a separate builder with shellcheck solve this? It could be used during development, then switched to the sh -n one, and it would not influence stdenv in any way. It'd only be a convenience for development work.

1. You hack on your fork of Nixpkgs and use writeShellScriptBinShellchecked to, uh, write shell scripts.
2. You're done hacking, and confident that your shell scripts are reasonably free of errors. You now switch your shell script builder to writeShellScriptBin.
3. Push, PR, merge, everyone's happy. Because writeShellScriptBinShellchecked is never actually called anywhere, ${shellcheck} is never evaluated and this week's Haskell changes never result in rebuilds.

Or am I missing something?

Oh! If it's just intended for development, what about using impureEnvVars. That would allow maintainers to enable the checks on their system without depending on the hash of shellcheck directly.

Hm. Does that mean that one would have to supply an outputHash with their writeShellScriptBin call? That sounds a little unwieldy.

Hi there :)
Regardless of using shellcheck or ${stdenv.shell} -n, the current checkPhase of writeShellScriptBin works on $out but should work on $out/bin/$name to take destination = "/bin/${name}" into account.

The bug.nix below can be used to reproduce the problem:

{ pkgs ? import <nixpkgs> {} }:
let inherit (pkgs) stdenv;
    builders = pkgs.callPackage pkgs/build-support/trivial-builders.nix {};
in stdenv.mkDerivation rec {
  name             = "bug";
  preferLocalBuild = true;
  script           = builders.writeShellScriptBin "error.sh" ''echo ERROR;)'';
  builder          = pkgs.writeText "builder.sh" ''echo ${script}/bin/error.sh >$out'';
}

Then in nixpkgs' root:

nixpkgs % nix-build --no-out-link bug.nix
these derivations will be built:
  /nix/store/y170bj8w6dgcqd6cn25x90q6a0mxxc3f-error.sh.drv
  /nix/store/y106m8b46hdyw61yhn6plfvpwbx3abg6-builder.sh.drv
  /nix/store/pd9s4qimc91l8mmnl3w1pp93qnnhfm2l-bug.drv
these paths will be fetched (0.01 MiB download, 0.03 MiB unpacked):
  /nix/store/vy068lswf10rhx4spys7zmpy2fp6i3m0-stdenv
building path(s) ‘/nix/store/rn0609ifd0ar930l6bax21x1kcagga45-error.sh’
/nix/store/rn0609ifd0ar930l6bax21x1kcagga45-error.sh: /nix/store/rn0609ifd0ar930l6bax21x1kcagga45-error.sh: Is a directory
builder for ‘/nix/store/y170bj8w6dgcqd6cn25x90q6a0mxxc3f-error.sh.drv’ failed with exit code 126
cannot build derivation ‘/nix/store/pd9s4qimc91l8mmnl3w1pp93qnnhfm2l-bug.drv’: 1 dependencies couldn't be built
error: build of ‘/nix/store/pd9s4qimc91l8mmnl3w1pp93qnnhfm2l-bug.drv’ failed

This patch can fix it:

--- a/pkgs/build-support/trivial-builders.nix
+++ b/pkgs/build-support/trivial-builders.nix
@@ -68,7 +68,7 @@ rec {
         ${text}
         '';
       checkPhase = ''
-        ${stdenv.shell} -n $out
+        ${stdenv.shell} -n $out/bin/$name
       '';
     };

The syntax error is detected:

nixpkgs 100% nix-build --no-out-link bug.nix
these derivations will be built:
  /nix/store/122rdnb5470c1jj9bdh44hzbn8nm1d1j-error.sh.drv
  /nix/store/wds5dcqqmfyj95b9dh7bhd74iz01icm7-builder.sh.drv
  /nix/store/1dmp6zi5dp6fncnmbvsq3yy0nll6kf30-bug.drv
these paths will be fetched (0.01 MiB download, 0.03 MiB unpacked):
  /nix/store/vy068lswf10rhx4spys7zmpy2fp6i3m0-stdenv
building path(s) ‘/nix/store/wbladhn0dgq9n0fhvk6s5dncgr9a26ic-error.sh’
/nix/store/wbladhn0dgq9n0fhvk6s5dncgr9a26ic-error.sh/bin/error.sh: line 2: syntax error near unexpected token `)'
/nix/store/wbladhn0dgq9n0fhvk6s5dncgr9a26ic-error.sh/bin/error.sh: line 2: `echo ERROR;)'
builder for ‘/nix/store/122rdnb5470c1jj9bdh44hzbn8nm1d1j-error.sh.drv’ failed with exit code 2
cannot build derivation ‘/nix/store/1dmp6zi5dp6fncnmbvsq3yy0nll6kf30-bug.drv’: 1 dependencies couldn't be built
error: build of ‘/nix/store/1dmp6zi5dp6fncnmbvsq3yy0nll6kf30-bug.drv’ failed

Regards.

Ping. What is the current status?

Are there any updates on this pull request, please?