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

Combining scripts #653

Closed nielsenko closed 6 months ago

nielsenko commented 7 months ago

Is there an existing feature request for this?

Command

No response

Description

Currently, if you want one script to execute a set of other scripts, you can do something like:

  test:
    description: Run all tests.
    run: >-
      melos run test:unit &&
      melos run test:widget &&
      melos run test:lints &&
      melos run test:integration

However, since these are simply invoked in a subprocess (without a tty) the color coding you get when invoking the scripts separately is lost.

Also, only && is really safe to use, so the construct is implicitly serialised, and fail-fast in nature.

I suggest some way of running a set of other scripts, allowing melos

As above, this would also mitigate some use of multiline scripts, which come with its own set of problems.

Reasoning

I find the melos run x && melos run y useful, but cumbersome, and ruining the nice output. It is definitely a nice-to-have.

Additional context and comments

No response

spydon commented 7 months ago

I like the idea, any suggestion for how the API might look like?

nielsenko commented 7 months ago

I have a controversial one πŸ˜„

What if run could only be used to invoke other scripts. To call out of melos, always use exec.

  test:
    run: 
      - test:unit
      - test:widget
      - test:lints
      - test:integration

 test:unit:
    exec:
      command: dart test --concurrency=1 --coverage=coverage/ --file-reporter=json:test-results.json --reporter=github
      concurrency: 1 # only one project at a time to keep output sane
    packageFilters:
      dependsOn: test
      dirExists: test/
      flutter: false

Notice I introduced a command: tag under exec.

spydon commented 7 months ago

I don't think we'd want to make a breaking change of that magnitude (or ideally we'd not want to make a breaking change at all) to introduce this feature, but maybe this new "run" could just be called something else?

nielsenko commented 7 months ago

Yes. I just think it would be consistent with melos run calling scripts, and also address https://github.com/invertase/melos/issues/566.

Should be simple enough to make a tool to do rudimentary migration.

But obviously a new command would work as well - group perhaps?

nielsenko commented 7 months ago

A few extra thoughts.

nielsenko commented 7 months ago

You might also want to include other commands, such as clean and bootstrap in a a group πŸ€”

Maybe groups should contain commands, not scripts, so

ci:
  group:
    - clean
    - bootstrap
    - run test
    - run analyze
spydon commented 7 months ago

Looks like a good structure, would you be interested in implementing it?

nielsenko commented 7 months ago

@spydon Scratch my own itch so to say..

I assume that would require a fair bit of familiarity with the codebase that I don't currently possess. If I find time, I would like to, but it won't be this side of summer for sure.

If someone familiar with the code wants to run with it, feel free :smile:

spydon commented 7 months ago

Alright, maybe I'll have time to look at it at some point between the million different things I want to build. πŸ˜…

spydon commented 6 months ago

Having a look at it again, this would probably be a better structure:

scripts:
  groups:
    group-name:
      - clean
      - bootstrap
      - other-group
      - run test
      - run analyze

Or possibly have some symbol to indicate a group, like:

scripts:
  group-name: $
    - clean
    - bootstrap
    - other-group
    - run test
    - run analyze
jessicatarra commented 6 months ago

I really like how bitrise.yml works as the configuration file for bitrise.com. It uses the steps key to define a series of actions in the script. The structure would be something like the following:

scripts:
  group-name:
    description: this is the group description
    steps:
      - clean
      - bootstrap
      - other-group
      - run test
      - run analyze

It would be similar to a normal script, but instead of using run it uses steps and obviously these two keys are mutually exclusive. What do you think of this approach?

spydon commented 6 months ago

That sounds great! That's similar to the GitHub actions config too and the more familiar people will already be with the syntax the better.

spydon commented 6 months ago

Should I assign you to the issue @jessicatarra?

jessicatarra commented 6 months ago

Yes, sure!

nielsenko commented 6 months ago

@jessicatarra and @spydon I really appreciate you taking the time to implement and review #664, and I'm happy with the steps feature you added, but perhaps it is still a bit early to close this issue completely.

Again, don't get me wrong - I'm truly grateful for your awesome contribution, but the reason I suggest we keep this issue open, is that:

The individual scripts are still just invoked in a subshell, causing fx the color coding to be off.

Example:

image

Perhaps the better way is to fix this for all sub processes, but my idea with the combining script feature was to allow melos to be smarter about this, and only shell out in the actual exec leaves, instead of shelling out to melos run for each subscript. In the above example, only dev build actually needs to be invoked in a separate process.

Also, I know it was not at the top, but in the comment https://github.com/invertase/melos/issues/653#issuecomment-1981783422 I suggested that it should hold for commands as well, which it currently doesn't.

I do appreciate that calling melos run recursively is a lot easier, and my apologies if I misinterpreted how melos works internally and all of this doesn't make sense.

spydon commented 6 months ago

Thanks for the feedback @nielsenko, the issue was automatically closed when the PR closed, I'll open it up again. Both your suggestions about including commands and fix so that colors work sound like good improvements.

Would you be interested in continuing working on this @jessicatarra? :)

jessicatarra commented 6 months ago

Sure, I'll take a look. And, just to make sure I'm on the same page, the idea is to also include support for melos commands and fix the log outputs, so instead of using melos run, I should directly call the script execution and the output would be -taking @nielsenko's example- the following:

$ melos exec
  β””> dev build
     β””> RUNNING (in 1 packages)
spydon commented 6 months ago

Sure, I'll take a look. And, just to make sure I'm on the same page, the idea is to also include support for melos commands

Yes, I think that could be the first step.

and fix the log outputs, so instead of using melos run, I should directly call the script execution and the output would be -taking

I'm not sure what the best solution is here, it would be good to solve the whole subshelling problem so that normal concurrent scripts also get color etc. I think there is a separate issue for this somewhere.

xsahil03x commented 6 months ago

I guess we can close this issue as completed with #683

MarkOSullivan94 commented 5 months ago

Recently I was wondering if this was possible and to my delight I found this issue and seen the feature available in the latest release πŸ™Œ

Thanks to all involved in the process of getting this feature added to Melos: @nielsenko @jessicatarra @Salakar @spydon