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.

Sign up for GitHub

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: usage of potentially wrong melos version during script execution #783

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

fix: usage of potentially wrong melos version during script execution #783

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2d07d2c
Executes scripts with same dart/melos combination as currently running
bspinner Oct 29, 2024
6cd9e7a
Fixes analysis issues
bspinner Oct 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 50 additions & 1 deletion packages/melos/lib/src/scripts.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import 'package:collection/collection.dart';
import 'package:meta/meta.dart';

import 'common/platform.dart';
import 'common/utils.dart';
import 'common/validation.dart';
import 'package.dart';
Expand Down Expand Up @@ -318,7 +319,8 @@
if (exec == null) {
return scriptCommand;
} else {
final execCommand = ['melos', 'exec'];
/// `dart melos.dart exec` or `melos exec` with absolute paths
final execCommand = [..._determineMelosExecutablePaths(), 'exec'];

if (exec.concurrency != null) {
execCommand.addAll(['--concurrency', '${exec.concurrency}']);
Expand All @@ -338,6 +340,53 @@
}
}

/// Identifies path of currently running melos and reuses it
/// for nested script execution.
///
/// Starting melos by just calling `melos` via shell
/// would use the first found binary found in `$PATH`,
/// 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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, thanks for the quick review.

Is this a future enhancement request or already implemented? 🙂

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :(

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

/// So the initially started melos binary/script and the one used for melos script execution
/// could differ.
///
/// Besides ensuring starting the same melos as currently running,
/// this furthermore enables use of melos in environments where
/// `.pub-cache/bin` should/must not be put in `$PATH`.
List<String> _determineMelosExecutablePaths() {
final currentExecutablePathString = currentPlatform.resolvedExecutable;
final currentExecutablePathUri = Uri.file(currentExecutablePathString);
if (!currentExecutablePathUri.isAbsolute) {
throw FormatException(
'Got invalid, unsupported or relative path to running executable.'

Check notice on line 363 in packages/melos/lib/src/scripts.dart

View workflow job for this annotation

GitHub Actions / analyze

Missing whitespace between adjacent strings. 

Try adding whitespace between the strings.
See https://dart.dev/lints/missing_whitespace_between_adjacent_strings to learn more about this problem.
'Expected absolute path.');

Check notice on line 364 in packages/melos/lib/src/scripts.dart

View workflow job for this annotation

GitHub Actions / analyze

Use 'const' with the constructor to improve performance. 

Try adding the 'const' keyword to the constructor invocation.
See https://dart.dev/diagnostics/prefer_const_constructors to learn more about this problem.
}

final currentScriptPathUri = currentPlatform.script;
final currentScriptPathString = currentScriptPathUri.toFilePath();
if (currentScriptPathString.isNotEmpty &&
!currentScriptPathUri.isAbsolute) {
throw FormatException(
'Got invalid, unsupported or relative path to melos.'

Check notice on line 372 in packages/melos/lib/src/scripts.dart

View workflow job for this annotation

GitHub Actions / analyze

Missing whitespace between adjacent strings. 

Try adding whitespace between the strings.
See https://dart.dev/lints/missing_whitespace_between_adjacent_strings to learn more about this problem.
'Expected absolute path.');

Check notice on line 373 in packages/melos/lib/src/scripts.dart

View workflow job for this annotation

GitHub Actions / analyze

Use 'const' with the constructor to improve performance. 

Try adding the 'const' keyword to the constructor invocation.
See https://dart.dev/diagnostics/prefer_const_constructors to learn more about this problem.
}

// Determine if melos is running as script or compiled binary
final isScriptPathAvailable = currentScriptPathString.isNotEmpty;
final isCompiledMelosRunning =
currentExecutablePathUri == currentScriptPathUri;

if (!isScriptPathAvailable || isCompiledMelosRunning) {
return [currentExecutablePathString];
}

// currentExecutablePathString => path to dart binary
// currentScriptPathUri => path to melos.dart
return [currentExecutablePathString, currentScriptPathString];
}

/// Validates the script. Throws a [MelosConfigException] if the script is
/// invalid.
void validate() {
Expand Down
Loading