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

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

Open
1 task done
kankaristo opened this issue Dec 4, 2023 · 12 comments
Open
1 task done

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

kankaristo opened this issue Dec 4, 2023 · 12 comments
Labels
bug Something isn't working

Comments

@kankaristo
Copy link

Is there an existing issue for this?

  • I have searched the existing issues.

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

@kankaristo kankaristo added the bug Something isn't working label Dec 4, 2023
@spydon
Copy link
Collaborator

spydon commented Dec 15, 2023

@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
Copy link
Contributor

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
Copy link
Author

kankaristo commented Dec 19, 2023

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.

@kankaristo kankaristo changed the title fix: --order-dependents does not only order packages, but also filters fix: melos exec --order-dependents does not run in all packages Dec 19, 2023
@spydon
Copy link
Collaborator

spydon commented Dec 19, 2023

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
Copy link
Author

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:

  • package a has a dependency on package b
  • package b has a dev_dependency on package a

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:

  • melos.yaml in the root project has packages/* for the package list
  • package a has a dependency on package b (using path: ../b)
  • package b has a dev_dependency on package a (using path: ../a)
  • package c is probably not necessary, it's just for testing

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
Copy link
Collaborator

spydon commented Dec 20, 2023

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
Copy link
Author

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
Copy link
Collaborator

spydon commented Dec 20, 2023

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
Copy link
Author

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
Copy link
Collaborator

spydon commented Dec 20, 2023

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

This comment was marked as off-topic.

@spydon
Copy link
Collaborator

spydon commented Feb 14, 2024

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?

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

No branches or pull requests

4 participants