fix: usage of potentially wrong melos version during script execution

[bspinner]

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 refactor
• ✅ ci -- Build configuration change
• 📝 docs -- Documentation
• 🗑️ chore -- Chore

[docs-page]

To view this pull requests documentation preview, visit the following URL:

docs.page/invertase/melos~783

Documentation is deployed and generated using docs.page.

[CLAassistant]

All committers have signed the CLA.

[spydon]

From a quick glance it looks like a sound idea!

[spydon]

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.

[bspinner]

Oh, thanks for the quick review.

Is this a future enhancement request or already implemented? 🙂

[bspinner]

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_dependencies might only define a minimum version, which wouldn’t fully address this, as you likely know as well. :)

[spydon]

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.

[bspinner]

@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. :)

[spydon]

❌ test/commands/run_test.dart: script supports passing additional arguments to scripts (exec) (failed)
ScriptException: The script hello failed to execute.
package:melos/src/commands/run.dart 70:7  _RunMixin.run
===== asynchronous gap ===========================
test/commands/run_test.dart 247:7         main.<fn>.<fn>
❎ test/commands/format_test.dart: Melos Format should run format with --output none and --set-exit-if-changed flag (skipped)
❌ test/commands/run_test.dart: script supports running "melos exec" script with "exec" options (failed)
ScriptException: The script test_script failed to execute.
package:melos/src/commands/run.dart 70:7  _RunMixin.run
===== asynchronous gap ===========================
test/commands/run_test.dart 318:7         main.<fn>.<fn>

Do the tests not fail locally for you?

There are also some analyze issues.

[bspinner]

I'm referring to the output created by the scripts used for testing. All it says is "test_script failed to execute" – but what output did it generate? I.e. why did it fail?

The tests fail locally for me.

[spydon]

Hmm curious, if you comment out these lines: https://github.com/invertase/melos/blob/main/packages/melos/lib/src/commands/run.dart#L68-L71
you'll get an output with lots of these:

              '  Exception: Failed to run pub get:\n'
              '  Resolving dependencies...\n'
              '  \n'
              '  Error on line 1, column 7 of ../melos_test_THCNVC/pubspec.yaml: "name" field doesn\'t match expected name "melos".\n'
              '    ╷\n'
              '  1 │ name: workspace\n'
              '    │       ^^^^^^^^^\n'
              '    ╵\n'
              '  \n'
              '\n'
              '  /home/spydon/repos/melos/packages/melos/test/utils.dart 96:5              runPubGet\n'

[bspinner]

I had a look at the failing tests of run_tests.dart and if I'm not mistaken 2 of the tests fail, because during test execution the running script isn't melos, but the test implementation.
So _determineMelosExecutablePaths() resolves to something like /path/to/dart /path/to/test.dill which fails of course.

Unfortunately I no longer have spare time to work on this. :(

[spydon]

I had a look at the failing tests of run_tests.dart and if I'm not mistaken 2 of the tests fail, because during test execution the running script isn't melos, but the test implementation.
So _determineMelosExecutablePaths() resolves to something like /path/to/dart /path/to/test.dill which fails of course.

Ah, that makes sense... Hmm, I don't know what the best way to solve that would be. Maybe set an environment flag in the affected tests and look at that flag in _determineMelosExecutablePaths() and fall back to the old way if the flag is set?

Unfortunately I no longer have spare time to work on this. :(

Do you think you will at some point in the future? Like within a month of two?

[bspinner]

@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.

[spydon]

I'll close this for now since I don't think it will properly take the version in dev_depenendencies into consideration, if it does we can re-open it.