melos icon indicating copy to clipboard operation
melos copied to clipboard

[bug] `melos run` args only work in single line scripts

Open jeroen-meijer opened this issue 3 years ago • 9 comments
trafficstars

Hi there! 👋🏻

@Salakar graciously added the functionality to run melos run scripts with additional arguments in #231. 😄

Description

Looking at the tests and trying it out myself, it works fine in single line scripts.

// melos.yaml

name: test_args

packages:
  - packages/**

scripts:
  testargs:
    run: echo "$0" "$1" "$2" "$3"

Output:

$ melos run testargs first second third
melos run testargs
   └> echo "$0" "$1" "$2" "$3"
       └> RUNNING

/bin/sh    first second third

melos run testargs
   └> echo "$0" "$1" "$2" "$3"
       └> SUCCESS

Actual behavior

However, using a multi-line string in YAML to define a script breaks this behavior.

Example 1

// melos.yaml

name: test_args

packages:
  - packages/**

scripts:
  testargs:
    run: |
      echo "$0" "$1" "$2" "$3"

Output:

$ melos run testargs first second third
melos run testargs
   └> echo "$0" "$1" "$2" "$3"
       └> RUNNING

/bin/sh   
/bin/sh: line 1: first: command not found


melos run testargs
   └> echo "$0" "$1" "$2" "$3"
       └> FAILED
ScriptException: The script testargs failed to execute

Example 2

// melos.yaml

name: test_args

packages:
  - packages/**

scripts:
  testargs:
    run: |
      echo "Shell: $0"
      echo "First: $1"
      echo "Second: $2"
      echo "Third: $3"

Output:

$ melos run testargs first second third
melos run testargs
   └> echo "Shell: $0"echo "First: $1"echo "Second: $2"echo "Third: $3"
       └> RUNNING

Shell: /bin/sh
First: 
Second: 
Third: 
/bin/sh: line 4: first: command not found


melos run testargs
   └> echo "Shell: $0"echo "First: $1"echo "Second: $2"echo "Third: $3"
       └> FAILED
ScriptException: The script testargs failed to execute

Expected behavior

I believe arguments should be handled independently of the format of the script payload provided to Melos, just like when executing a some_script.sh first second third would parse $1, $2, etc. correctly.


If you need any additional info or help, feel free to reach out! 😄 Great job y'all 👍🏻

jeroen-meijer avatar Jan 26 '22 23:01 jeroen-meijer

Doh, need to tinker with this one since the current implementation just passes them along as additional args at the end of the script, maybe I need to explicitly define positional args as envs myself $0, $1, etc - but then this probably is going to be a nightmare cross-platform 🤔

Salakar avatar Jan 27 '22 18:01 Salakar

@Salakar Yeah, I've tried it for a little bit myself, and it's trickier than it seems 😜

In bash, functions aren't first class citizens, so you can't simply define an anonymous one and call it immediately.

However, one idea I had was, wrap any script you get in the following:

"_run_melos_script() { $scriptPayload ; }"

Then simply run the script and append the arguments

final scriptFunction = "_run_melos_script() { $scriptPayload ; }"
await _startProcess("$scriptFunction && _run_melos_script ${restArgs.map((arg) => '"$arg"').join()}")

It's a bit rough, but I think it might do the job. I'll do some experimenting. Lmkwyt 😄

jeroen-meijer avatar Jan 27 '22 18:01 jeroen-meijer

I'm fine with it being a bit rough if it works, but it would also need to work on Windows too 😭

Salakar avatar Jan 27 '22 19:01 Salakar

image

Good point. I'll have a think about it...

In the current implementation, all provided scripts are processed as-is in bash on POSIX platforms and in CMD on Windows, is that right? 👀

jeroen-meijer avatar Jan 27 '22 19:01 jeroen-meijer

I wonder if this would also fix an issue I'm having trying to pass in a variable:

I'm trying to use this command in a melos script:

melos exec --since=<commit hash> -- flutter test

where the commit hash could be passed in dynamically like:

melos exec --since=$1 -- flutter test

I can't see any way to do this currently from the documentation. I've tried running (in various forms, using the --dir-exists flag as a test):

Melos script:

args:
    description: args test
    run: melos exec --dir-exists=$1 flutter test

Melos command:

melos args test

which produces:

$ melos exec
  └> flutter test test
     └> RUNNING (in 5 packages)

windybranch avatar Jan 19 '23 13:01 windybranch

I have implemented a solution to this issue and submitted it as a pull request. #525 Please take a look when you have a chance.

KoheiKanagu avatar May 16 '23 03:05 KoheiKanagu

When will this feature be released finally? I really need it.

ara-mentz avatar Jun 28 '23 12:06 ara-mentz

Extra script args never quite worked as described in this issue. The test for extra args was a bit misleading, which has been fixed in #540. You were never able to access the extra args with $1, $2, etc. They were always just appended to the script. #540 also makes this more obvious by logging the full script command, including extra args.

I think the current behavior is actually what we want, though. It is simple and works on all platforms. Adding support for real placeholders that can be used anywhere in the script in a way that works on all platforms leads us down a path where we create our own little shell language for scripts.

Generally, I feel like we should not encourage people to use multi-line scripts because they routinely forget to use && in every line to actually get the behavior they expect (that the script exits after the first failed command/line).

It's typically better to put the implementation of more complex scripts into a separate file (e.g. a shell script, or better yet, a Dart script). Then you can do all the parsing and processing of extra args there.

blaugold avatar Jul 04 '23 07:07 blaugold

Hm, before running, can't we write the run script to a temp folder and execute it from the file? something like bash $TMPDIR/my_script.sh $?

pastre avatar Apr 15 '24 10:04 pastre