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.11k stars 198 forks source link

fix: `melos exec --order-dependents` does not run in all packages #610

Open kankaristo opened 9 months ago

kankaristo commented 9 months ago

Is there an existing issue for this?

Version

3.3.0

Description

Running some simple command like melos exec echo with or without --order-dependents gives a different list of packages. Naturally, the order changes, but --order-dependents also filters the list, apparently only to the dependent packages.

Based on the parameter name and the description in melos exec --help, I would expect --order-dependents to only order the list of packages, so that the command is run in dependent packages first, but it also filters the list to only the dependent packages.

I'm not a 100 % sure if this is a bug or intended behavior, but at the very least the documentation could be clarified about this.

Steps to reproduce

  1. In a project with both dependent and non-dependent packages, run melos exec with or without --order-dependents
  2. melos exec echo runs in all packages
  3. melos exec --order-dependents echo orders the dependent packages first, but does not run in all packages

Expected behavior

melos exec --order-dependents should only order packages, not also filter them.

Screenshots

No response

Additional context and comments

No response

spydon commented 8 months ago

@mspb1g12 you wrote that feature right? Was the intention that it should filter too? It sounds reasonable like @kankaristo say that it shouldn't filter the packages I think.

mspb1g12 commented 8 months ago

I didn't add any extra filtering that wouldn't be normally part of exec, only if the dependency would've been filtered anyway would I expect it to be omitted, e.g. melos exec --depends-on=build_runner --order-dependents dart run build_runner build then I wouldn't expect any dependencies that don't have build runner to be added. Or if something is globally ignored in the melos.yaml.

If that's not what you're seeing then it sounds like a bug, have you got an example package structure that causes this? I've tried with a very basic 3 package repo and echo runs in all of them with or without order-dependents like I'd expect.

kankaristo commented 8 months ago

Looks like there's something else weird happening here, it's not filtering the packages, but the command is not run in all packages.

If I run melos exec echo, the output shows:

$ melos exec
  â””> echo
     â””> RUNNING (in 37 packages)

(list of 37 packages, one on each line)

$ melos exec
  â””> echo
     â””> SUCCESS

If I run melos exec --order-dependents echo, the beginning of the output still shows the same number of packages, but the command is not actually run in all packages:

$ melos exec
  â””> echo
     â””> RUNNING (in 37 packages)

(list of only 19 packages, not the full 37)

(no "SUCCESS" message, but also no errors, and exit code is 0)

With -c1, the above command only runs in 11 out of 37 packages. Without --order-dependents it always runs in all 37 packages.

I didn't notice it was behaving weirdly, because there were no errors (just a quiet exit), and the command I was originally running wasn't a simple "echo" (although it happens with that too).

Unfortunately, I can't share the project, but I'll try to debug what's happening and create a reproducible case.

Any hints on how to debug Melos? --verbose doesn't produce any extra interesting output for this, it only adds the duration of the execution.

spydon commented 8 months ago

Looks like there's something else weird happening here, it's not filtering the packages, but the command is not run in all packages.

Good find!

Any hints on how to debug Melos? --verbose doesn't produce any extra interesting output for this, it only adds the duration of the execution.

I don't think melos produces any logs or something that would be helpful here, I think the only way to really get to know what is happening is to run a modified version of melos with extra logging or stepping through it with the debugger.

It's probably something around here that is wrong.

kankaristo commented 8 months ago

I debugged this a little bit, and figured out why it's failing. We have a bit of a strange dependency thing going on with some of our packages, where:

So, one of the tests in package b depends on a, but otherwise there isn't a circular dependency. This breaks melos exec --order-dependents echo in a way where Melos just quits quietly. It happens exactly where @spydon suspected, right on this line. A simple logger.log() call right after that map() call is never reached.


I created a simple Dart project, and packages a, b and c inside it, under a directory called packages. Then:

With the above dependency structure, melos exec -c1 echo runs fine, but melos exec -c1 --order-dependents echo runs successfully in package c. Then it gets to package a, and Melos quits quietly.

I can understand if you don't want to support this kind of dependency structure, it is a little bit weird. But it would be nice to at least have some sort of error message (and non-zero exit code) here, now this went unnoticed for us for a while.

spydon commented 8 months ago

Well investigated!

I created a simple Dart project, and packages a, b and c inside it, under a directory called packages.

Does it still happen if you don't use path dependencies? You don't need path dependencies for packages within the same monorepo, since that is what melos bootstrap sets up for you.

kankaristo commented 8 months ago

We actually don't use Melos for dependency management at all, so we're not running melos bootstrap either. We only use Melos for exec/run in custom scripts.

Our current (somewhat custom) dependency management pre-dates our use of Melos. We might transition to using Melos for dependency management as well at some point, but we currently don't have plans for that. We would have to make sure the dependencies work with Renovate, etc., so it would be a bit of a project.

I'll check in the test project if it happens without path dependencies.

spydon commented 8 months ago

We would have to make sure the dependencies work with Renovate, etc., so it would be a bit of a project.

Renovate doesn't yet support melos common dependencies, but you don't have to use common dependencies for linking local packages so it doesn't affect this issue.

One thing you have to make sure though is that you also run bootstrap in the pipeline, with for example the melos-action.

kankaristo commented 8 months ago

Does it still happen if you don't use path dependencies?

Just checked this in the test project, and it also happens without path dependencies (using melos bootstrap instead).

spydon commented 8 months ago

Just checked this in the test project, and it also happens without path dependencies (using melos bootstrap instead).

I was afraid it would, thanks for checking!

md-weber commented 6 months ago

We ran today in a similar issue which was a classic circular dependency, it would be great if we could log the package that was not able to build or get any other error message.

spydon commented 6 months ago

We ran today in a similar issue which was a classic circular dependency, it would be great if we could log the package that was not able to build or get any other error message.

Hi Max 👋 That sounds like it deserves its own issue, it doesn't sound like it's related to the core issue here at least?