Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
2 tasks done
coretemp opened this issue May 9, 2018 · 20 comments
Open
2 tasks done

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

coretemp opened this issue May 9, 2018 · 20 comments

Comments

@coretemp
Copy link

coretemp commented May 9, 2018

For new checks and feature suggestions

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.

@ngzhian
Copy link
Contributor

ngzhian commented May 13, 2018

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

@coretemp
Copy link
Author

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.

@teto
Copy link

teto commented Aug 17, 2018

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

@coretemp
Copy link
Author

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

@koalaman
Copy link
Owner

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.

@coretemp
Copy link
Author

@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.

@baodrate
Copy link

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

@basile-henry
Copy link

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

@lilyball
Copy link

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.

@expipiplus1
Copy link

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

@koalaman
Copy link
Owner

@expipiplus1 Sure, it makes sense for autoidentifying purpose

@expipiplus1
Copy link

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.

unbel13ver pushed a commit to unbel13ver/spectrum that referenced this issue Dec 1, 2022
There's an open issue about teaching shellcheck to automatically
recognize nix-shell shebangs:
koalaman/shellcheck#1210

Signed-off-by: Alyssa Ross <alyssa.ross@unikie.com>
Message-Id: <20221118100947.33597-1-alyssa.ross@unikie.com>
@the-moog
Copy link

the-moog commented Sep 6, 2023

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:

@port19x
Copy link

port19x commented Sep 7, 2023

@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.

@lilyball
Copy link

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

@the-moog
Copy link

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.

@port19x
Copy link

port19x commented Sep 12, 2023

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

@the-moog
Copy link

the-moog commented Sep 13, 2023

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

@port19x
Copy link

port19x commented Sep 13, 2023

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?

@lilyball
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants