invertase / melos

๐ŸŒ‹ A tool for managing Dart projects with multiple packages. With IntelliJ and Vscode IDE support. Supports automated versioning, changelogs & publishing via Conventional Commits.
https://melos.invertase.dev/~melos-latest
Apache License 2.0
1.13k stars 201 forks source link

fix: multiline run commands were failing #694

Closed pastre closed 5 months ago

pastre commented 5 months ago

Description

Multiline scripts were not properly being parsed in order to run. Trimming them before creation ensures no empty spaces interfere with the script's execution

Type of Change

CLAassistant commented 5 months ago

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

spydon commented 5 months ago

Can you give an example of a script that would fail? All our multi-line scripts work as intended as far as I know. I think this might even break some scripts, since it would remove the newline characters and if people have escaped the newline with a backslash it would create a trailing \ that wouldn't escape the right thing.

On another note, most of the time the new steps feature can be used instead of multi-line scripts: https://melos.invertase.dev/configuration/scripts#steps

pastre commented 5 months ago

@spydon Sorry, I could've been a little more descriptive on the bug - and also thanks for the quick feedback! The bug happens when passing additional arguments to a multiline YAML string - it apparently takes those arguments in as new commands and attempts to execute them With this script:

scripts:
  build:
    run: |
      echo $1 $2 $3

I got the following output

$ melos build with extra arguments
melos run build
  โ””> echo $1 $2 $3
      with extra arguments
     โ””> RUNNING

ERROR: /bin/sh: line 1: with: command not found

melos run build
  โ””> echo $1 $2 $3
      with extra arguments
     โ””> FAILED
ScriptException: The script build failed to execute.
spydon commented 5 months ago

@pastre ah, I see, that is filed as bug #232, and there is a good description about why these types of scripts are problematic here.

spydon commented 5 months ago

I'll close this meanwhile, since this fix will break other scripts. We can continue the discussion in the issue instead.

pastre commented 5 months ago

Cool, thanks for pointing towards the right direction @spydon :)