nat-n / poethepoet

A task runner that works well with poetry.
https://poethepoet.natn.io/
MIT License
1.46k stars 59 forks source link

Error: Shell task does not accept arguments #17

Closed amotl closed 3 years ago

amotl commented 3 years ago

Dear Nat,

thanks a stack for conceiving and maintaining Poe the Poet. I wish you a happy new year.

We are about to use it within wireviz-web and currently observe the error Shell task does not accept arguments when trying to make a "shell" task obtain extra arguments from the command line.

A minimal reproducible example would be

[tool.poe.tasks]
hello = { shell = "echo $1" }

which croaks with

$ poe hello world
Poe the Poet - A task runner that works well with poetry.
version 0.9.0

Error: Shell task 'hello' does not accept arguments

Is there anything you can do about it?

With kind regards, Andreas.

nat-n commented 3 years ago

Hi @amotl,

As I discuss a bit in #18, I intend to fix this. I think making shell tasks accept positional args and expose them like that would make sense. I recall thinking there was a reason not to do this back when implementing the shell task type, though now I don't see it 🤷‍♂️

amotl commented 3 years ago

Hi Nat,

As I discuss a bit in #18, I intend to fix this.

Thanks. Specifically your edit at https://github.com/nat-n/poethepoet/issues/18#issuecomment-756801498

EDIT: It just occurred to me that a more quick win approach in terms of new functionality might be to simply support positional arguments on shell tasks which are then available a $1, $2, $@ etc within the shell task content. I guess this might also suffice for your use case?

would make sense. I believe that should be sufficient and a very nice feature for poe in order to better support shell tasks with parameters. shell has only positional args anyway and might parse $@ how ever it is convenient, right?

By doing it that way, people will be able to hook existing shell scripts into poe without further ado as poe would not interfere with argument parsing when invoking shell scripts in any way.

With kind regards, Andreas.

nat-n commented 3 years ago

Turns out this is trickier than I thought, in that I'm not sure whether it's possible to set/pass the special environment variables on the child shell process. So unless I'm missing something there I guess it could either

  1. preprocess the script to replace the $1, $2, $@ etc references before passing it to the child shell process – which could get messy
  2. decide on an alternative convention for naming the variables (e.g. $ARG1, $ARG2, $ARGS)
  3. wait to implement named arguments

We'll see.

amotl commented 3 years ago

Hi Nat,

thank you for looking into this.

https://github.com/nat-n/poethepoet/blob/76a92602267e215a2f82944a01e2e51ded860137/poethepoet/task/shell.py#L32-L39

Why not just tack the extra_args after the shell command without replacing them beforehand at all? Wouldn't this be the most natural way to execute shell scripts without interfering in any way?

if self._is_windows:
    shell = self._find_posix_shell_on_windows()
else:
    # Prefer to use configured shell, otherwise look for bash
    shell = [os.environ.get("SHELL", shutil.which("bash") or "/bin/bash")]

extra_args = [arg.strip() for arg in extra_args]
shell += extra_args

There's definitively a chance I am missing something here. Nevertheless I figured that it wouldn't be a bad idea to at least write it down. What do you think?

With kind regards, Andreas.

nat-n commented 3 years ago

@amotl That was the second thing I tried. Unfortunately that has the effect that the shell command interprets the arguments as shell scripts to run.

amotl commented 3 years ago

Ah, sure! So, we would have to modify the execution flow to yield something along the lines of....?

bash -c 'echo hello $@' -o world

However, after diving more into the code base, the paradigm seems to be that the program to execute will be passed to the executor through STDIN, right? So, it will currently get executed like

echo 'echo hello $@' | bash

Hm. With Stack Overflow to the rescue [1], I found there is a way to make bash both accept the program on STDIN and arguments to it on the command line itself, like

echo 'echo hello $@' | bash -s -- world

So, this might work already?

shell += ["-s", "--"] + extra_args

However, I recognize this would only work when really using bash. For other shells, the procedure might have to be adjusted. But at least, there would be a way doing it like this I wanted to share.

[1] https://stackoverflow.com/questions/38580423/use-bash-with-script-text-from-stdin-and-options-from-command-line

nat-n commented 3 years ago

@amotl thanks for the suggestions. This might work, but it's getting a bit complicated, and my initial testing leads me to believe it's not well standardised across shell types and versions, so adopting this might complicate resolving #15 :/

Therefore I'm starting to think the best solution would be just to support named arguments on shell tasks. I hope to have something to show for this soon.

nat-n commented 3 years ago

Closing in favour of #6, @amotl you might want to check the progress there.

gangofnuns commented 2 years ago

Curious of this issue was ever resolved. I'm looking for a way to launch a bash script with a single, positional argument. The docs aren't completely clear, but I'm beginning to think it's not possible. I've tried several permutations, and I'm not able to make it happen.

nat-n commented 2 years ago

@gangofnuns That sounds like something that should be possible now... do you mean something like this?

[tool.poe.delete-stuff]
shell = """
cd $PLACE
echo "Gonna delete all this stuff!"
ls
rm -rf ./*
"""
args = [{name = "PLACE", positional = true}]  

To be used like:

poe delete-stuff ./trash
gangofnuns commented 2 years ago

Hey Nat-N

Yes - this is exactly what I was looking for.

Using this simple example I was able to set up a working scenario for my own script.
Perhaps this might be included in the docs?

Many thanks for the clue!