shellcheck icon indicating copy to clipboard operation
shellcheck copied to clipboard

Add support for #!/usr/bin/env nix-shell

Open coretemp opened this issue 7 years ago • 20 comments

For new checks and feature suggestions

  • [x] shellcheck.net (i.e. the latest commit) currently gives no useful warnings about this
  • [x] I searched through https://github.com/koalaman/shellcheck/issues and found https://github.com/koalaman/shellcheck/issues/977#issuecomment-373405520, which shows that there are at least two persons who want this.

Here's a snippet or screenshot that shows the problem:

#!/usr/bin/env nix-shell
true

Here's what shellcheck currently says:

^-- SC1008: This shebang was unrecognized. Note that ShellCheck only handles sh/bash/dash/ksh.

Here's what I wanted or expected to see:

Empty output, because it's a standard way of using nix-shell.

coretemp avatar May 09 '18 13:05 coretemp

I don't know much about nix-shell, from [0], it seems like it downloads nix deps and opens a bash shell, so will it be okay to treat the file as bash?

[0] https://nixos.org/nix/manual/#idm140737318501472

ngzhian avatar May 13 '18 18:05 ngzhian

What the file is supposed to contain depends on the -i argument.

This is an example of correct usage:

#!/usr/bin/env nix-shell
#!nix-shell -i bash -p qemu jq ec2_api_tools awscli

The -i can also be sh or zsh, etc.

coretemp avatar May 13 '18 21:05 coretemp

-i <interpreter> so it can be bash/ruby/python etc. shellcheck could disable itself if != sh/bash.

teto avatar Aug 17 '18 15:08 teto

Can we get some feedback from whoever maintains this that this is a feature that would be accepted?

coretemp avatar Nov 23 '18 09:11 coretemp

I am very hesitant to add first-class support for new shells, because it's easy to add but hard to maintain and remove.

I'd prefer a simple third party wrapper script nix-shellcheck that invokes shellcheck -s bash on such files, similar to how hadolint and bats testing works.

koalaman avatar Nov 25 '18 04:11 koalaman

@koalaman Thanks for your reply.

I don't know what you mean by first-class support for new shells in this particular case.

The only thing that needs to happen is a conditional to check for the first line being equal to #!/usr/bin/env nix-shell

and in that case, it should look for the -i option. If the -i option is equal to sh it should check the file ignoring the first two lines. If the -i option is equal to zsh (or bash), it should do something similar, etc.

I don't understand your concerns.

coretemp avatar Nov 26 '18 12:11 coretemp

@coretemp Would the shell directive be useful in this case?

baodrate avatar Nov 27 '18 16:11 baodrate

The shell directive does solve this issue for me! Thanks @qubidt. I just wanted to share an example of how to use it here for the next person stumbling upon this issue:

$ cat test-shell.sh
#!/usr/bin/env nix-shell
#!nix-shell -i bash -p hello
#shellcheck shell=bash

hello
$ ./test-shell.sh
Hello, world!
$ shellcheck test-shell.sh
$ echo $?
0

basile-henry avatar Jun 29 '19 10:06 basile-henry

I would really love to see shellcheck gain native support for this, so I don't end up with errors in VS Code every time I edit a shell script that uses nix-shell. In my own scripts I can add shell directives, but I'm often opening scripts I don't control.

lilyball avatar Jan 10 '20 21:01 lilyball

Agree with @lilyball, if a PR implementing support for this (identifying the -i parameter in the second line) be accepted?

expipiplus1 avatar Nov 11 '20 13:11 expipiplus1

@expipiplus1 Sure, it makes sense for autoidentifying purpose

koalaman avatar Nov 11 '20 17:11 koalaman

Great, it looks like it would require a slight change to the parser to not ignore additional "shebang"s at the top of the file, at the moment the pretend #!nix-shell -i foo line is ignored as a comment.

expipiplus1 avatar Nov 12 '20 01:11 expipiplus1

This is not a nix-shell specific bug. env is part of the GNU Coreutils package that is used in most *Nix based systems. Any interpreter (indeed any executable) can be passed the current environment using the GNU 'env' command. It is very common to write #!/usr/bin/env sh or #!/usr/bin/env bash, etc. As the shebang line. It is also used for non shell interpreters, like perl, python, and many others.

Effectively it does the same as something like: envvar1="a" envvar2="b" myprog but for every current environment variable.

Uses:

  • Ensure the sub-process adopts the same environment, not just 'exported' parameters
  • Alter the environment for the new interpreter
  • Pass switches to the interpreter (using -S otherwise exec() would try and run an interpreter with a filename, e.g. bash --posix)

Ref:

the-moog avatar Sep 06 '23 12:09 the-moog

@the-moo to expand on your point, env isn't just part of the gnu coreutils. env is part of POSIX. Thus it is safe to assume it is available on all plattforms of interest.

port19x avatar Sep 07 '23 09:09 port19x

Shellcheck already supports /usr/bin/env in general, so that's a red herring. This issue is indeed specific to nix-shell.

lilyball avatar Sep 11 '23 00:09 lilyball

Shellcheck already supports /usr/bin/env in general, so that's a red herring. This issue is indeed specific to nix-shell.

Hmm, well when running in vscode it seems to disagree, at least for me....

image

Could it be that the vscode plugin that uses this project is somehow disabling this feature? (Shellcheck/VSCode wrapper v0.34.0) Note the debug console states that it is using the latest 0.9.0 shellcheck binary.

the-moog avatar Sep 12 '23 14:09 the-moog

That's because you're using /usr/bin/env wrong. The correct line here would be #!/usr/bin/env bash

port19x avatar Sep 12 '23 17:09 port19x

That's because you're using /usr/bin/env wrong. The correct line here would be #!/usr/bin/env bash

Interesting, you are right, the error goes away if I remove the explicit path to the interpreter.

(BTW as I am not allowed to post my actual scripts here I made a cut and paste error in the empty file that I snipped the error from, in doing that I omitted the leading /)

As we all know, despite age, there are still things to learn.... And so I've probably been using this wrong for as long as I can remember! (~20+ years ?? though my memory is not that good thse days). I am fairly sure I always wrote /path/interpreter for bash (or sh or dash) and now I need to know why I am 'doing it wrong' by being explicit with the path to the exe?

It has always been my take that without an explicit path I am at the mercy of whatever path the requested interpreter is found at (note, I am on a huge system with ~10k+ users and make use of gnu environemt modules), so without being explicit, surely it could be dangerous? e.g. a user could accidently (or deliberately) insert line into their own path and use an interpreter that behaved differently to the intended one? I just did a quick test to examine this and it seems to behave as I would expect. Gist

the-moog avatar Sep 13 '23 13:09 the-moog

Well wrong might be a strong word here, considering it still works.

Let's check man 1p env which goes into this in some detail. I'll quote parts I believe are relevant, (...) signifying omission.

NAME
       env — set the environment for command invocation

SYNOPSIS
       env [‐i] [name=value]... [utility [argument...]]
(...)

OPERANDS
       The following operands shall be supported:
(...)

       utility   The name of the utility to be invoked. If the utility operand names any of the special  built‐in  utilities  in  Section
                 2.14, Special Built‐In Utilities, the results are undefined.
(...)

ENVIRONMENT VARIABLES
       The following environment variables shall affect the execution of env:
(...)
       
       PATH      Determine  the location of the utility, as described in the Base Definitions volume of POSIX.1‐2017, Chapter 8, Environ‐
                 ment Variables.  If PATH is specified as a name=value operand to env, the value given shall be used in  the  search  for
                 utility.

You are correct that by ommiting the path you're at the mercy of PATH. You can make a semantic argument that if the user is able to alter their PATH, they can alter behaviour. However this argument leads us astray, as we already have an alternative if we do not want the operating system to search the PATH flexibly. In such cases we should specify the path directly to the shebang, as env provides no additional value in this case.

#!/bin/bash > #!/usr/bin/env /bin/bash (goal: consistent path) #!/usr/bin/env bash > #!/usr/bin/env /bin/bash (goal: dynamic lookup)

Mixing env and a static path combines the subtle disadvantages of both. Does that make sense?

port19x avatar Sep 13 '23 14:09 port19x

The fact that #!/usr/bin/env /bin/bash does not work should be filed as a separate bug. It's a rather useless construct, as the only reason to invoke /usr/bin/env with an absolute path is if you want to modify the environment (e.g. #!/usr/bin/env FOO=bar /bin/bash) or wish to make use of flags (e.g. on many systems, the shebang does not split options and so you may need #!/usr/bin/env -S /bin/bash --login --posix to instruct env to split the arguments), neither of which is being used in your example. But shellcheck should probably still recognize it, so you should file an issue on it, but it's unrelated to the nix-shell issue.

lilyball avatar Sep 13 '23 21:09 lilyball