Skip to content

Commit

Permalink
fix: Don't deadlock on cycle exec with order dependents (#761)
Browse files Browse the repository at this point in the history
<!--
  Thanks for contributing!

Provide a description of your changes below and a general summary in the
title

Please look at the following checklist to ensure that your PR can be
accepted quickly:
-->

## Description

This change sets the concurrency to 1 when there are cyclic dependencies
and you are running `exec` with `--order-dependents`.

Fixes: #610 

## Type of Change

<!--- Put an `x` in all the boxes that apply: -->

- [ ] ✨ `feat` -- New feature (non-breaking change which adds
functionality)
- [x] 🛠️ `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
  • Loading branch information
spydon authored Oct 7, 2024
1 parent b432749 commit cec45d7
Show file tree
Hide file tree
Showing 2 changed files with 158 additions and 10 deletions.
24 changes: 14 additions & 10 deletions packages/melos/lib/src/commands/exec.dart
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,20 @@ mixin _ExecMixin on _Melos {
required bool orderDependents,
Map<String, String> additionalEnvironment = const {},
}) async {
final sortedPackages = packages.toList(growable: false);
var hasImpactfulCycles = false;

if (orderDependents) {
// TODO: This is not really the right way to do this. Cyclic dependencies
// are handled in a way that is specific for publishing.
sortPackagesForPublishing(sortedPackages);
hasImpactfulCycles =
findCyclicDependenciesInWorkspace(sortedPackages).isNotEmpty;
}

final calculatedConcurrency = hasImpactfulCycles ? 1 : concurrency;
final failures = <String, int?>{};
final pool = Pool(concurrency);
final pool = Pool(calculatedConcurrency);
final execArgsString = execArgs.join(' ');
final prefixLogs = concurrency != 1 && packages.length != 1;

Expand All @@ -107,14 +119,6 @@ mixin _ExecMixin on _Melos {
logger.horizontalLine();
}

final sortedPackages = packages.toList(growable: false);

if (orderDependents) {
// TODO: This is not really the right way to do this. Cyclic dependencies
// are handled in a way that is specific for publishing.
sortPackagesForPublishing(sortedPackages);
}

final packageResults = Map.fromEntries(
packages.map((package) => MapEntry(package.name, Completer<int?>())),
);
Expand All @@ -125,7 +129,7 @@ mixin _ExecMixin on _Melos {
pool.forEach<Package, void>(sortedPackages, (package) async {
assert(!(failFast && failures.isNotEmpty));

if (orderDependents) {
if (orderDependents && !hasImpactfulCycles) {
final dependenciesResults = await Future.wait(
package.allDependenciesInWorkspace.values
.map((package) => packageResults[package.name]?.future)
Expand Down
144 changes: 144 additions & 0 deletions packages/melos/test/commands/exec_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,150 @@ ${'-' * terminalWidth}
);
});

test(
'sorts execution order topologically with cyclic dependencies',
() async {
final workspaceDir = await createTemporaryWorkspace();

await createProject(
workspaceDir,
PubSpec(
name: 'a',
dependencies: {'b': HostedReference(VersionConstraint.any)},
),
);

await createProject(
workspaceDir,
PubSpec(
name: 'b',
dependencies: {'a': HostedReference(VersionConstraint.any)},
),
);

await createProject(
workspaceDir,
const PubSpec(name: 'c'),
);

final logger = TestLogger();
final config =
await MelosWorkspaceConfig.fromWorkspaceRoot(workspaceDir);
final melos = Melos(
logger: logger,
config: config,
);

await melos.exec(
['echo', 'hello', 'world'],
concurrency: 2,
orderDependents: true,
);

expect(
logger.output.normalizeNewLines(),
ignoringAnsii(
'''
\$ melos exec
└> echo hello world
└> RUNNING (in 3 packages)
${'-' * terminalWidth}
[c]: hello world
[a]: hello world
[b]: hello world
${'-' * terminalWidth}
\$ melos exec
└> echo hello world
└> SUCCESS
''',
),
);
},
);

test(
'sorts execution order topologically with larger cyclic dependencies',
() async {
final workspaceDir = await createTemporaryWorkspace();

await createProject(
workspaceDir,
PubSpec(
name: 'a',
dependencies: {'b': HostedReference(VersionConstraint.any)},
),
);

await createProject(
workspaceDir,
PubSpec(
name: 'b',
dependencies: {'c': HostedReference(VersionConstraint.any)},
),
);

await createProject(
workspaceDir,
PubSpec(
name: 'c',
dependencies: {'d': HostedReference(VersionConstraint.any)},
),
);

await createProject(
workspaceDir,
PubSpec(
name: 'd',
dependencies: {'a': HostedReference(VersionConstraint.any)},
),
);

await createProject(
workspaceDir,
const PubSpec(name: 'e'),
);

final logger = TestLogger();
final config =
await MelosWorkspaceConfig.fromWorkspaceRoot(workspaceDir);
final melos = Melos(
logger: logger,
config: config,
);

await melos.exec(
['echo', 'hello', 'world'],
concurrency: 2,
orderDependents: true,
);

expect(
logger.output.normalizeNewLines(),
ignoringAnsii(
'''
\$ melos exec
└> echo hello world
└> RUNNING (in 5 packages)
${'-' * terminalWidth}
[e]: hello world
[c]: hello world
[b]: hello world
[a]: hello world
[d]: hello world
${'-' * terminalWidth}
\$ melos exec
└> echo hello world
└> SUCCESS
''',
),
);
},
);

test('fails fast if dependencies fail', () async {
final workspaceDir = await createTemporaryWorkspace();

Expand Down

0 comments on commit cec45d7

Please sign in to comment.