fix: usage of potentially wrong melos version during script execution #783
Conversation
|
To view this pull requests documentation preview, visit the following URL: Documentation is deployed and generated using docs.page. |
|
|
bspinner
force-pushed
the
fix/executable_path_handling
branch
from
daa7230 to
c040977
Compare
bspinner
changed the title
bspinner
changed the title
| /// i.e. usually the globally activated version. If there is one. | ||
| /// | ||
| /// This can cause issues when melos is started with `dart run melos`, | ||
| /// which will prefer the melos version specified via (dev_)dependencies. |
There was a problem hiding this comment.
Melos should always run scripts etc with the version that is defined in dev_dependencies, even if it is started directly from the version on path.
There was a problem hiding this comment.
Oh, thanks for the quick review.
Is this a future enhancement request or already implemented? 🙂
There was a problem hiding this comment.
The overarching goal of this PR is to eliminate ambiguity and surprises in the build process of Dart/Flutter applications. In line with that, I believe it would be ideal for Melos to behave in a consistent and predictable way.
Here’s what I think could be considered "unexpected" behavior:
- If Melos uses different versions of itself for script execution (i.e. for subprocesses of itself)
- This will be addressed with this PR!? :)
- If Melos silently switches to a different version at startup (i.e., in the main process)
- This could be especially challenging when intentionally testing a specific version.
- Perhaps an optional strict mode (e.g., for CI environments) could help, where Melos would verify that its version matches the one specified in
pubspec.lock.- The version in
dev_dependenciesmight only define a minimum version, which wouldn’t fully address this, as you likely know as well. :)
- The version in
There was a problem hiding this comment.
Is this a future enhancement request or already implemented? 🙂
It's already implemented, try pinning you melos version to an older version an see what happens. :)
The version in dev_dependencies might only define a minimum version, which wouldn’t fully address this, as you likely know as well. :)
If one wants a specific version of melos, why not just pin the version in pubspec? I don't see the need for any separate feature for this.
There was a problem hiding this comment.
@spydon Am I correct in understanding that you're unhappy with the comment in the code? If so, could you suggest a better or more accurate phrasing? :)
Additionally, if the only remaining blocker for merging is the failing tests, I’d be happy to look into the breaking cod – given it's likely the changes will be merged. :)
There was a problem hiding this comment.
I'm unsure if this is really needed/wanted, if I haven't misunderstood what this PR is trying to achieve.
If you run dart run melos --version you'll get the version specified in pubspec.yaml, which is the intentional behavior, no matter what melos binary that started it, the scripts should run with the melos version specified in the pubspec by the constraints.
There was a problem hiding this comment.
@bspinner any updated on this? Am I correct in that this PR would change so that it doesn't run with the melos version specified in the root pubspec?
There was a problem hiding this comment.
@spydon Sorry for the unexpected delay. Life is full of surprises... :|
According to my very basic knowledge of this codebase, this change does not interfere with anything happening during melos' start.
All it does is using the absolute path to the melos binary already running. So when melos starts itself another time for a script/command, the same melos binary will be used.
So this PR's changes apply to starting a subprocess, not melos' startup and checks. :)
Doing this differently could lead to heisenbugs, as behaviour could differ between directly executing a command and executing a command nested in another melos command.
Because which binary is used for the nested command execution currently depends on the user's shell environment.
Furthermore, it enables usage of melos in situations in which you don't have melos in your environmental PATH (which is the case in one of our CI environments).
There was a problem hiding this comment.
Life is full of surprises...
It is indeed, I hope you're all good!
Alright, that sounds like it makes sense, could you just add a test that runs melos --version inside of a script and checks that it is the same melos version as specified in the pubspec, if you pin a version in there.
|
@spydon I'm unsure if this could/should/must be applied to other places as well, since this is the first time I had a look at the code. |
|
I'll close this for now since I don't think it will properly take the version in |
Description
See long comment at https://github.com/invertase/melos/pull/783/files#diff-35f09e1c67ac8d5bd4889489c6ce0f6dc320b1491828444f2eca21c0f8ff765cR343 :)
Type of Change
feat-- New feature (non-breaking change which adds functionality)fix-- Bug fix (non-breaking change which fixes an issue)!-- Breaking change (fix or feature that would cause existing functionality to change)refactor-- Code refactorci-- Build configuration changedocs-- Documentationchore-- Chore