Skip to content

Commit c3efd6e

Browse files
authored
feat(rush-lib): early cycle detection for workspace packages in rush install/update (#5904)
* feat: add workspace cycle detection to rush install/update * fix(rush-lib): update cycle error message to recommend refactoring over decoupledLocalDependencies * fix(rush-lib): narrow decoupledLocalDependencies guidance to bootstrapping problem only * refactor(rush-lib): remove redundant path array in _findWorkspaceCycle --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
1 parent 0fb78ed commit c3efd6e

13 files changed

Lines changed: 260 additions & 0 deletions

File tree

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
{
2+
"changes": [
3+
{
4+
"packageName": "@microsoft/rush",
5+
"comment": "Add early validation to `rush install`/`rush update` that immediately fails with a meaningful error message if an undeclared cycle is detected among workspace packages, specifying the cycle path.",
6+
"type": "minor"
7+
}
8+
]
9+
}
Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,109 @@
1+
// Copyright (c) Microsoft Corporation. All rights reserved. Licensed under the MIT license.
2+
// See LICENSE in the project root for license information.
3+
4+
import { AlreadyReportedError } from '@rushstack/node-core-library';
5+
import { Colorize, type ITerminal } from '@rushstack/terminal';
6+
7+
import type { RushConfiguration } from '../api/RushConfiguration';
8+
import type { RushConfigurationProject } from '../api/RushConfigurationProject';
9+
import { RushConstants } from './RushConstants';
10+
11+
/**
12+
* Detects cycles in the workspace package dependency graph (i.e., cycles that are not
13+
* broken by `decoupledLocalDependencies`) and reports them as errors.
14+
*
15+
* @remarks
16+
* A cycle means that pnpm would be unable to install the workspace, so it is better to
17+
* fail fast with a clear message rather than let pnpm produce a cryptic error.
18+
*
19+
* The fix is to refactor the code to eliminate the cycle, for example by extracting shared
20+
* code into a new package that both projects can depend on, or by moving code from one project
21+
* to another. `decoupledLocalDependencies` is intended only for the bootstrapping problem
22+
* (e.g. the version of a compiler used to compile itself) and should not be used as a
23+
* general escape hatch for cycles.
24+
*/
25+
export function detectAndReportWorkspaceCycles(
26+
rushConfiguration: RushConfiguration,
27+
terminal: ITerminal
28+
): void {
29+
const cycle: ReadonlyArray<string> | undefined = _findWorkspaceCycle(rushConfiguration.projects);
30+
31+
if (cycle !== undefined) {
32+
terminal.writeLine();
33+
terminal.writeLine(
34+
Colorize.red(
35+
'A cyclic dependency was detected among workspace packages:\n' +
36+
` ${cycle.join(' -> ')}\n\n` +
37+
`To fix this, refactor the code to eliminate the cycle. For example, extract the shared ` +
38+
`code into a new package that both projects can depend on, or move code from one project ` +
39+
`to another so the dependency only goes in one direction.\n\n` +
40+
`NOTE: The "decoupledLocalDependencies" setting in ${RushConstants.rushJsonFilename} is ` +
41+
`intended only for the bootstrapping problem (for example, the version of a compiler used ` +
42+
`to compile itself). It is not a general solution for cyclic dependencies.`
43+
)
44+
);
45+
throw new AlreadyReportedError();
46+
}
47+
}
48+
49+
/**
50+
* Finds one cycle in the workspace dependency graph, or returns `undefined` if there are none.
51+
*
52+
* Uses depth-first search with a "currently visiting" set for O(V + E) detection.
53+
* The `visiting` set doubles as the ordered path: ES6 Sets preserve insertion order,
54+
* so iterating it from the cycle-start node yields the cycle without a separate array.
55+
*/
56+
export function _findWorkspaceCycle(
57+
projects: ReadonlyArray<RushConfigurationProject>
58+
): ReadonlyArray<string> | undefined {
59+
// Nodes that have been fully explored (no cycles reachable from them)
60+
const visited: Set<RushConfigurationProject> = new Set();
61+
// Nodes currently on the DFS recursion stack, in insertion order
62+
const visiting: Set<RushConfigurationProject> = new Set();
63+
64+
function dfs(node: RushConfigurationProject): ReadonlyArray<string> | undefined {
65+
if (visited.has(node)) {
66+
return undefined;
67+
}
68+
if (visiting.has(node)) {
69+
// Back-edge found — iterate `visiting` (insertion order) and collect from
70+
// the cycle-start node onward, then close the loop.
71+
const cycleNames: string[] = [];
72+
let found: boolean = false;
73+
for (const n of visiting) {
74+
if (n === node) {
75+
found = true;
76+
}
77+
if (found) {
78+
cycleNames.push(n.packageName);
79+
}
80+
}
81+
cycleNames.push(node.packageName); // close the cycle
82+
return cycleNames;
83+
}
84+
85+
visiting.add(node);
86+
87+
for (const dep of node.dependencyProjects) {
88+
const cycle: ReadonlyArray<string> | undefined = dfs(dep);
89+
if (cycle !== undefined) {
90+
return cycle;
91+
}
92+
}
93+
94+
visiting.delete(node);
95+
visited.add(node);
96+
return undefined;
97+
}
98+
99+
for (const project of projects) {
100+
if (!visited.has(project)) {
101+
const cycle: ReadonlyArray<string> | undefined = dfs(project);
102+
if (cycle !== undefined) {
103+
return cycle;
104+
}
105+
}
106+
}
107+
108+
return undefined;
109+
}

libraries/rush-lib/src/logic/base/BaseInstallManager.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ import { ProjectImpactGraphGenerator } from '../ProjectImpactGraphGenerator';
6060
import { FlagFile } from '../../api/FlagFile';
6161
import { PnpmSyncUtilities } from '../../utilities/PnpmSyncUtilities';
6262
import { HotlinkManager } from '../../utilities/HotlinkManager';
63+
import { detectAndReportWorkspaceCycles } from '../WorkspaceCycleDetector';
6364

6465
/**
6566
* Pnpm don't support --ignore-compatibility-db, so use --config.ignoreCompatibilityDb for now.
@@ -442,6 +443,11 @@ export abstract class BaseInstallManager {
442443
// Check the policies
443444
await PolicyValidator.validatePolicyAsync(this.rushConfiguration, subspace, variant, this.options);
444445

446+
// Fail fast if there are undeclared cycles in the workspace package dependency graph.
447+
// Pnpm cannot install a workspace with cycles, so this gives a clearer error message
448+
// than whatever pnpm would emit.
449+
detectAndReportWorkspaceCycles(this.rushConfiguration, terminal);
450+
445451
await this._installGitHooksAsync();
446452

447453
const approvedPackagesChecker: ApprovedPackagesChecker = new ApprovedPackagesChecker(
Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
// Copyright (c) Microsoft Corporation. All rights reserved. Licensed under the MIT license.
2+
// See LICENSE in the project root for license information.
3+
4+
import * as path from 'node:path';
5+
6+
import { RushConfiguration } from '../../api/RushConfiguration';
7+
import { _findWorkspaceCycle } from '../WorkspaceCycleDetector';
8+
9+
describe(_findWorkspaceCycle.name, () => {
10+
function loadProjectsFromRepo(repoName: string): RushConfiguration['projects'] {
11+
const rushConfiguration: RushConfiguration = RushConfiguration.loadFromConfigurationFile(
12+
path.resolve(__dirname, `workspaceCycleDetector/${repoName}/rush.json`)
13+
);
14+
return rushConfiguration.projects;
15+
}
16+
17+
it('returns undefined when there are no cycles', () => {
18+
const projects: RushConfiguration['projects'] = loadProjectsFromRepo('no-cycle');
19+
const result: ReadonlyArray<string> | undefined = _findWorkspaceCycle(projects);
20+
expect(result).toBeUndefined();
21+
});
22+
23+
it('returns the cycle path when an undeclared cycle is present', () => {
24+
const projects: RushConfiguration['projects'] = loadProjectsFromRepo('with-cycle');
25+
const result: ReadonlyArray<string> | undefined = _findWorkspaceCycle(projects);
26+
expect(result).toBeDefined();
27+
// The cycle should form a closed loop: [..., pkg-a] or [..., pkg-b] depending on
28+
// iteration order. Either way the first and last element must be the same package,
29+
// and both pkg-a and pkg-b must appear in the cycle.
30+
expect(result!.length).toBeGreaterThanOrEqual(2);
31+
expect(result![0]).toBe(result![result!.length - 1]);
32+
const uniqueNames: Set<string> = new Set(result);
33+
expect(uniqueNames.has('pkg-a')).toBe(true);
34+
expect(uniqueNames.has('pkg-b')).toBe(true);
35+
});
36+
37+
it('returns undefined when the cycle is intentionally broken with decoupledLocalDependencies', () => {
38+
const projects: RushConfiguration['projects'] = loadProjectsFromRepo('decoupled-cycle');
39+
const result: ReadonlyArray<string> | undefined = _findWorkspaceCycle(projects);
40+
expect(result).toBeUndefined();
41+
});
42+
43+
it('returns the cycle path when using the workspacePackages test repo (cyclic-dep-1 <-> cyclic-dep-2)', () => {
44+
const rushConfiguration: RushConfiguration = RushConfiguration.loadFromConfigurationFile(
45+
path.resolve(__dirname, 'workspacePackages/rush.json')
46+
);
47+
const result: ReadonlyArray<string> | undefined = _findWorkspaceCycle(rushConfiguration.projects);
48+
expect(result).toBeDefined();
49+
expect(result![0]).toBe(result![result!.length - 1]);
50+
const uniqueNames: Set<string> = new Set(result);
51+
expect(uniqueNames.has('cyclic-dep-1')).toBe(true);
52+
expect(uniqueNames.has('cyclic-dep-2')).toBe(true);
53+
});
54+
});
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
{
2+
"name": "pkg-a",
3+
"version": "1.0.0",
4+
"dependencies": {
5+
"pkg-b": "workspace:*"
6+
}
7+
}
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
{
2+
"name": "pkg-b",
3+
"version": "1.0.0",
4+
"dependencies": {
5+
"pkg-a": "workspace:*"
6+
}
7+
}
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
{
2+
"rushVersion": "0.0.0",
3+
"pnpmVersion": "8.0.0",
4+
"projects": [
5+
{
6+
"packageName": "pkg-a",
7+
"projectFolder": "pkg-a"
8+
},
9+
{
10+
"packageName": "pkg-b",
11+
"projectFolder": "pkg-b",
12+
"decoupledLocalDependencies": ["pkg-a"]
13+
}
14+
]
15+
}
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
{
2+
"name": "pkg-a",
3+
"version": "1.0.0"
4+
}
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
{
2+
"name": "pkg-b",
3+
"version": "1.0.0",
4+
"dependencies": {
5+
"pkg-a": "workspace:*"
6+
}
7+
}
Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
{
2+
"rushVersion": "0.0.0",
3+
"pnpmVersion": "8.0.0",
4+
"projects": [
5+
{
6+
"packageName": "pkg-a",
7+
"projectFolder": "pkg-a"
8+
},
9+
{
10+
"packageName": "pkg-b",
11+
"projectFolder": "pkg-b"
12+
}
13+
]
14+
}

0 commit comments

Comments
 (0)