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

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

Open
jeroen-meijer opened this issue Jan 26, 2022 · 9 comments
Open

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

jeroen-meijer opened this issue Jan 26, 2022 · 9 comments
Assignees
Labels
bug Something isn't working

Comments

@jeroen-meijer
Copy link
Contributor

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 👍🏻

@Salakar Salakar self-assigned this Jan 27, 2022
@Salakar
Copy link
Contributor

Salakar commented Jan 27, 2022

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 🤔

@jeroen-meijer
Copy link
Contributor Author

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

@Salakar
Copy link
Contributor

Salakar commented Jan 27, 2022

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

@jeroen-meijer
Copy link
Contributor Author

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? 👀

@blaugold blaugold added the bug Something isn't working label May 27, 2022
@windybranch
Copy link

windybranch commented Jan 19, 2023

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)

@KoheiKanagu
Copy link

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.

@ara-mentz
Copy link

ara-mentz commented Jun 28, 2023

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

@blaugold
Copy link
Collaborator

blaugold commented Jul 4, 2023

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.

@pastre
Copy link

pastre commented Apr 15, 2024

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 $?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants